[API-NEXT] linux-generic: ishm: set up len for block correctly

Message ID 1481743993-20672-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Dec. 14, 2016, 7:33 p.m.
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

Comments

Christophe Milard Dec. 15, 2016, 7:47 a.m. | #1
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

>

>
Maxim Uvarov Dec. 15, 2016, 12:54 p.m. | #2
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

> 

>
Christophe Milard Dec. 15, 2016, 1:05 p.m. | #3
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

> >

> >

>

>
Christophe Milard Dec. 15, 2016, 1:07 p.m. | #4
...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

>> >

>> >

>>

>>

>
Maxim Uvarov Dec. 15, 2016, 7:15 p.m. | #5
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

>         >

>         >

> 

> 

>

Patch

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);