| Message ID | 1481743993-20672-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 14 December 2016 at 20:33, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Size might be not passed to that function if is is read > from file. But it has to be set to provide valid number > in odp_shm_info(). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/_ishm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/_ishm.c b/platform/linux-generic/_ > ishm.c > index 449e357..b34c70e 100644 > --- a/platform/linux-generic/_ishm.c > +++ b/platform/linux-generic/_ishm.c > @@ -803,7 +803,9 @@ int _odp_ishm_reserve(const char *name, uint64_t size, > int fd, > /* If a file descriptor is provided, get the real size and map: */ > if (fd >= 0) { > fstat(fd, &statbuf); > - len = statbuf.st_size; > + len = statbuf.st_size; > + size = statbuf.st_size; > + > That will return the real allocated size (aligned to page), not the user requested length (which may be shorter). is that ok? Alternatively, I think the block user size is known in the "find_exported" function, the info could be set there. Let me know if I am unclear, I feel some kind of responsability for this one :-)... so I am keen to help ! Christophe. /* note that the huge page flag is meningless here as huge > * page is determined by the provided file descriptor: */ > addr = do_map(new_index, len, align, flags, EXTERNAL, &fd); > -- > 2.7.1.250.gff4ea60 > >
On 12/15/16 10:47, Christophe Milard wrote: > > > On 14 December 2016 at 20:33, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Size might be not passed to that function if is is read > from file. But it has to be set to provide valid number > in odp_shm_info(). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/_ishm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/_ishm.c > b/platform/linux-generic/_ishm.c > index 449e357..b34c70e 100644 > --- a/platform/linux-generic/_ishm.c > +++ b/platform/linux-generic/_ishm.c > @@ -803,7 +803,9 @@ int _odp_ishm_reserve(const char *name, uint64_t > size, int fd, > /* If a file descriptor is provided, get the real size and > map: */ > if (fd >= 0) { > fstat(fd, &statbuf); > - len = statbuf.st_size; > + len = statbuf.st_size; > + size = statbuf.st_size; > + > > > That will return the real allocated size (aligned to page), not the user > requested length (which may be shorter). is that ok? api says: /** * Shared memory block info */ typedef struct odp_shm_info_t { const char *name; /**< Block name */ void *addr; /**< Block address */ uint64_t size; /**< Block size in bytes */ uint64_t page_size; /**< Memory page size */ uint32_t flags; /**< ODP_SHM_* flags */ } odp_shm_info_t; /** * Shared memory block info * @note This is the only shared memory API function which accepts invalid * shm handles (any bit value) without causing undefined behavior. * * @param[in] shm Block handle * @param[out] info Block info pointer for output * * @retval 0 on success * @retval <0 on failure */ int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info); That is not specified. I think there has to be real size, not requested size. > > Alternatively, I think the block user size is known in the > "find_exported" function, the info could be set there. Let me know if I > am unclear, I feel some kind of responsability for this one :-)... so I > am keen to help ! > > Christophe. Ok, I missed that. How about pass that parsed len to _odp_ishm_reserve() and add sanity check if len of file is the same as parsed value? Maxim. > > /* note that the huge page flag is meningless here > as huge > * page is determined by the provided file > descriptor: */ > addr = do_map(new_index, len, align, flags, > EXTERNAL, &fd); > -- > 2.7.1.250.gff4ea60 > >
I guess Petri is God when it comes to API: questions are: - should the size return as a result of the shm_get_info be the real size or just the requested size? On 15 December 2016 at 13:54, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/15/16 10:47, Christophe Milard wrote: > > > > > > On 14 December 2016 at 20:33, Maxim Uvarov <maxim.uvarov@linaro.org > > <mailto:maxim.uvarov@linaro.org>> wrote: > > > > Size might be not passed to that function if is is read > > from file. But it has to be set to provide valid number > > in odp_shm_info(). > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > > <mailto:maxim.uvarov@linaro.org>> > > --- > > platform/linux-generic/_ishm.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/platform/linux-generic/_ishm.c > > b/platform/linux-generic/_ishm.c > > index 449e357..b34c70e 100644 > > --- a/platform/linux-generic/_ishm.c > > +++ b/platform/linux-generic/_ishm.c > > @@ -803,7 +803,9 @@ int _odp_ishm_reserve(const char *name, uint64_t > > size, int fd, > > /* If a file descriptor is provided, get the real size and > > map: */ > > if (fd >= 0) { > > fstat(fd, &statbuf); > > - len = statbuf.st_size; > > + len = statbuf.st_size; > > + size = statbuf.st_size; > > + > > > > > > That will return the real allocated size (aligned to page), not the user > > requested length (which may be shorter). is that ok? > > api says: > /** > * Shared memory block info > */ > typedef struct odp_shm_info_t { > const char *name; /**< Block name */ > void *addr; /**< Block address */ > uint64_t size; /**< Block size in bytes */ > uint64_t page_size; /**< Memory page size */ > uint32_t flags; /**< ODP_SHM_* flags */ > } odp_shm_info_t; > /** > * Shared memory block info > * @note This is the only shared memory API function which accepts invalid > * shm handles (any bit value) without causing undefined behavior. > * > * @param[in] shm Block handle > * @param[out] info Block info pointer for output > * > * @retval 0 on success > * @retval <0 on failure > */ > int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info); > > That is not specified. I think there has to be real size, not requested > size. > > > > > > Alternatively, I think the block user size is known in the > > "find_exported" function, the info could be set there. Let me know if I > > am unclear, I feel some kind of responsability for this one :-)... so I > > am keen to help ! > > > > Christophe. > > > Ok, I missed that. How about pass that parsed len to _odp_ishm_reserve() > and add sanity check if len of file is the same as parsed value? > > Maxim. > > > > > /* note that the huge page flag is meningless here > > as huge > > * page is determined by the provided file > > descriptor: */ > > addr = do_map(new_index, len, align, flags, > > EXTERNAL, &fd); > > -- > > 2.7.1.250.gff4ea60 > > > > > >
...continued first part got sent for some reason... -should a size be given on import (for a check) On 15 December 2016 at 14:05, Christophe Milard < christophe.milard@linaro.org> wrote: > I guess Petri is God when it comes to API: > questions are: > - should the size return as a result of the shm_get_info be the real size > or just the requested size? > > > On 15 December 2016 at 13:54, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >> On 12/15/16 10:47, Christophe Milard wrote: >> > >> > >> > On 14 December 2016 at 20:33, Maxim Uvarov <maxim.uvarov@linaro.org >> > <mailto:maxim.uvarov@linaro.org>> wrote: >> > >> > Size might be not passed to that function if is is read >> > from file. But it has to be set to provide valid number >> > in odp_shm_info(). >> > >> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> > <mailto:maxim.uvarov@linaro.org>> >> > --- >> > platform/linux-generic/_ishm.c | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/platform/linux-generic/_ishm.c >> > b/platform/linux-generic/_ishm.c >> > index 449e357..b34c70e 100644 >> > --- a/platform/linux-generic/_ishm.c >> > +++ b/platform/linux-generic/_ishm.c >> > @@ -803,7 +803,9 @@ int _odp_ishm_reserve(const char *name, uint64_t >> > size, int fd, >> > /* If a file descriptor is provided, get the real size and >> > map: */ >> > if (fd >= 0) { >> > fstat(fd, &statbuf); >> > - len = statbuf.st_size; >> > + len = statbuf.st_size; >> > + size = statbuf.st_size; >> > + >> > >> > >> > That will return the real allocated size (aligned to page), not the user >> > requested length (which may be shorter). is that ok? >> >> api says: >> /** >> * Shared memory block info >> */ >> typedef struct odp_shm_info_t { >> const char *name; /**< Block name */ >> void *addr; /**< Block address */ >> uint64_t size; /**< Block size in bytes */ >> uint64_t page_size; /**< Memory page size */ >> uint32_t flags; /**< ODP_SHM_* flags */ >> } odp_shm_info_t; >> /** >> * Shared memory block info >> * @note This is the only shared memory API function which accepts invalid >> * shm handles (any bit value) without causing undefined behavior. >> * >> * @param[in] shm Block handle >> * @param[out] info Block info pointer for output >> * >> * @retval 0 on success >> * @retval <0 on failure >> */ >> int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info); >> >> That is not specified. I think there has to be real size, not requested >> size. >> >> >> > >> > Alternatively, I think the block user size is known in the >> > "find_exported" function, the info could be set there. Let me know if I >> > am unclear, I feel some kind of responsability for this one :-)... so I >> > am keen to help ! >> > >> > Christophe. >> >> >> Ok, I missed that. How about pass that parsed len to _odp_ishm_reserve() >> and add sanity check if len of file is the same as parsed value? >> >> Maxim. >> >> > >> > /* note that the huge page flag is meningless here >> > as huge >> > * page is determined by the provided file >> > descriptor: */ >> > addr = do_map(new_index, len, align, flags, >> > EXTERNAL, &fd); >> > -- >> > 2.7.1.250.gff4ea60 >> > >> > >> >> >
On 12/15/16 16:07, Christophe Milard wrote: > ...continued first part got sent for some reason... > > -should a size be given on import (for a check) > > here as it was in some previous version? odp_shm_t odp_shm_import(const char *remote_name, odp_instance_t odp_inst, const char *local_name) For me there is no big difference. Might be there are some case where you need map remote shm by name but you do not know size before you map. > > > > On 15 December 2016 at 14:05, Christophe Milard > <christophe.milard@linaro.org <mailto:christophe.milard@linaro.org>> wrote: > > I guess Petri is God when it comes to API: > questions are: > - should the size return as a result of the shm_get_info be the real > size or just the requested size? > the requested size you already know in application. Then it looks like real has more interest. For me it also more interesting real, because I see which memory I access by pointer and where it really points. Maxim. > > On 15 December 2016 at 13:54, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 12/15/16 10:47, Christophe Milard wrote: > > > > > > On 14 December 2016 at 20:33, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > > <mailto:maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>>> wrote: > > > > Size might be not passed to that function if is is read > > from file. But it has to be set to provide valid number > > in odp_shm_info(). > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > > --- > > platform/linux-generic/_ishm.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/platform/linux-generic/_ishm.c > > b/platform/linux-generic/_ishm.c > > index 449e357..b34c70e 100644 > > --- a/platform/linux-generic/_ishm.c > > +++ b/platform/linux-generic/_ishm.c > > @@ -803,7 +803,9 @@ int _odp_ishm_reserve(const char *name, uint64_t > > size, int fd, > > /* If a file descriptor is provided, get the real size and > > map: */ > > if (fd >= 0) { > > fstat(fd, &statbuf); > > - len = statbuf.st_size; > > + len = statbuf.st_size; > > + size = statbuf.st_size; > > + > > > > > > That will return the real allocated size (aligned to page), not the user > > requested length (which may be shorter). is that ok? > > api says: > /** > * Shared memory block info > */ > typedef struct odp_shm_info_t { > const char *name; /**< Block name */ > void *addr; /**< Block address */ > uint64_t size; /**< Block size in bytes */ > uint64_t page_size; /**< Memory page size */ > uint32_t flags; /**< ODP_SHM_* flags */ > } odp_shm_info_t; > /** > * Shared memory block info > * @note This is the only shared memory API function which > accepts invalid > * shm handles (any bit value) without causing undefined behavior. > * > * @param[in] shm Block handle > * @param[out] info Block info pointer for output > * > * @retval 0 on success > * @retval <0 on failure > */ > int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info); > > That is not specified. I think there has to be real size, not > requested > size. > > > > > > Alternatively, I think the block user size is known in the > > "find_exported" function, the info could be set there. Let me know if I > > am unclear, I feel some kind of responsability for this one :-)... so I > > am keen to help ! > > > > Christophe. > > > Ok, I missed that. How about pass that parsed len to > _odp_ishm_reserve() > and add sanity check if len of file is the same as parsed value? > > Maxim. > > > > > /* note that the huge page flag is > meningless here > > as huge > > * page is determined by the provided file > > descriptor: */ > > addr = do_map(new_index, len, align, flags, > > EXTERNAL, &fd); > > -- > > 2.7.1.250.gff4ea60 > > > > > > >
diff --git a/platform/linux-generic/_ishm.c b/platform/linux-generic/_ishm.c index 449e357..b34c70e 100644 --- a/platform/linux-generic/_ishm.c +++ b/platform/linux-generic/_ishm.c @@ -803,7 +803,9 @@ int _odp_ishm_reserve(const char *name, uint64_t size, int fd, /* If a file descriptor is provided, get the real size and map: */ if (fd >= 0) { fstat(fd, &statbuf); - len = statbuf.st_size; + len = statbuf.st_size; + size = statbuf.st_size; + /* note that the huge page flag is meningless here as huge * page is determined by the provided file descriptor: */ addr = do_map(new_index, len, align, flags, EXTERNAL, &fd);
Size might be not passed to that function if is is read from file. But it has to be set to provide valid number in odp_shm_info(). Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/_ishm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 2.7.1.250.gff4ea60