| Message ID | 1409572385-6169-1-git-send-email-petri.savolainen@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/01/2014 03:53 PM, Petri Savolainen wrote: > Add flags parameter to reserve call. > * SW_ONLY: optimize memory usage when HW accelerators are not > involved. > * PROC: for communication with other (non-ODP) processes in the > system > * KERNEL: for communication with a kernel (OS) driver Hi Petri, this patch is about the same which I sent. 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is there is specific need for that? What will happen if app will set this flag but flag is not supported by platform. 2. If public API changed then you need to fix all existence platforms, to not break their compilation/work. 3. What I don't like in current odp_shm_reserve() is that it's difficult to understand what is the reason of the error. I think it's reasonable to change that function from: void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, odp_shm_e flag) to int odp_shm_reserve(void **addr, const char *name, uint64_t size, uint64_t align, odp_shm_e flag) where return codes are: 1) chunk with that name already exist. addr is set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. I.e. not just fail with ODP_ERR() but return some reasonable value. Thanks, Maxim. > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > .../linux-generic/include/api/odp_shared_memory.h | 13 ++++++- > platform/linux-generic/odp_shared_memory.c | 45 +++++++++++++++++----- > 2 files changed, 48 insertions(+), 10 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h > index 8ac8847..814acdc 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -24,6 +24,15 @@ extern "C" { > /** Maximum shared memory block name lenght in chars */ > #define ODP_SHM_NAME_LEN 32 > > +/* > + * Shared memory flags > + */ > + > +/* Share level */ > +#define ODP_SHM_SW_ONLY 0x1 /**< Internal to application SW, no HW access */ > +#define ODP_SHM_PROC 0x2 /**< Share with external processes */ > +#define ODP_SHM_KERNEL 0x4 /**< Share with kernel */ > + > > /** > * Reserve a block of shared memory > @@ -31,10 +40,12 @@ extern "C" { > * @param name Name of the block (maximum ODP_SHM_NAME_LEN - 1 chars) > * @param size Block size in bytes > * @param align Block alignment in bytes > + * @param flags Block parameter flags > * > * @return Pointer to the reserved block, or NULL > */ > -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align); > +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > + uint32_t flags); > > /** > * Lookup for a block of shared memory > diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c > index 784f42b..4cb82b5 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -11,6 +11,7 @@ > #include <odp_system_info.h> > #include <odp_debug.h> > > +#include <unistd.h> > #include <sys/mman.h> > #include <asm/mman.h> > #include <fcntl.h> > @@ -44,8 +45,6 @@ typedef struct { > #define MAP_ANONYMOUS MAP_ANON > #endif > > -#define SHM_FLAGS (MAP_SHARED | MAP_ANONYMOUS) > - > > /* Global shared memory table */ > static odp_shm_table_t *odp_shm_tbl; > @@ -60,7 +59,7 @@ int odp_shm_init_global(void) > #endif > > addr = mmap(NULL, sizeof(odp_shm_table_t), > - PROT_READ | PROT_WRITE, SHM_FLAGS, -1, 0); > + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); > > if (addr == MAP_FAILED) > return -1; > @@ -95,11 +94,17 @@ static int find_block(const char *name) > } > > > -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > + uint32_t flags) > { > int i; > odp_shm_block_t *block; > void *addr; > + int fd = -1; > + int map_flag = MAP_SHARED; > + /* O_EXCL: error, O_TRUNC: truncate to zero, if already exists */ > + int oflag = O_RDWR | O_CREAT | O_TRUNC; > + uint64_t alloc_size = size + align; > #ifdef MAP_HUGETLB > uint64_t huge_sz, page_sz; > > @@ -107,11 +112,31 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > page_sz = odp_sys_page_size(); > #endif > > + if (flags & ODP_SHM_PROC) { > + /* Creates a file to /dev/shm */ > + fd = shm_open(name, oflag, > + S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); > + > + if (fd == -1) { > + ODP_DBG("odp_shm_reserve: shm_open failed\n"); > + return NULL; > + } > + > + if (ftruncate(fd, alloc_size) == -1) { > + ODP_DBG("odp_shm_reserve: ftruncate failed\n"); > + return NULL; > + } > + > + } else { > + map_flag |= MAP_ANONYMOUS; > + } > + > odp_spinlock_lock(&odp_shm_tbl->lock); > > if (find_block(name) >= 0) { > /* Found a block with the same name */ > odp_spinlock_unlock(&odp_shm_tbl->lock); > + ODP_DBG("odp_shm_reserve: name already used\n"); > return NULL; > } > > @@ -125,6 +150,7 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > if (i > ODP_SHM_NUM_BLOCKS - 1) { > /* Table full */ > odp_spinlock_unlock(&odp_shm_tbl->lock); > + ODP_DBG("odp_shm_reserve: no more blocks\n"); > return NULL; > } > > @@ -135,16 +161,16 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > > #ifdef MAP_HUGETLB > /* Try first huge pages */ > - if (huge_sz && (size + align) > page_sz) { > - addr = mmap(NULL, size + align, PROT_READ | PROT_WRITE, > - SHM_FLAGS | MAP_HUGETLB, -1, 0); > + if (huge_sz && alloc_size > page_sz) { > + addr = mmap(NULL, alloc_size, PROT_READ | PROT_WRITE, > + map_flag | MAP_HUGETLB, fd, 0); > } > #endif > > /* Use normal pages for small or failed huge page allocations */ > if (addr == MAP_FAILED) { > - addr = mmap(NULL, size + align, PROT_READ | PROT_WRITE, > - SHM_FLAGS, -1, 0); > + addr = mmap(NULL, alloc_size, PROT_READ | PROT_WRITE, > + map_flag, fd, 0); > > } else { > block->huge = 1; > @@ -153,6 +179,7 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > if (addr == MAP_FAILED) { > /* Alloc failed */ > odp_spinlock_unlock(&odp_shm_tbl->lock); > + ODP_DBG("odp_shm_reserve: mmap failed\n"); > return NULL; > } >
> -----Original Message----- > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov > Sent: Tuesday, September 02, 2014 6:43 PM > To: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > > > > On 09/01/2014 03:53 PM, Petri Savolainen wrote: > > Add flags parameter to reserve call. > > * SW_ONLY: optimize memory usage when HW accelerators are not > > involved. > > * PROC: for communication with other (non-ODP) processes in the > > system > > * KERNEL: for communication with a kernel (OS) driver > Hi Petri, > > this patch is about the same which I sent. Didn't realize that. This is not only for IPC, but the shm flag was on TODO list from last sprint to help Taras to optimize shared memory allocation (flag SW only vs. SW+HW accelerator access). > > 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is > there is specific need for that? What will happen if > app will set this flag but flag is not supported by platform. I was thinking to use that to flag memory that will be shared with a kernel driver (but not with external processes). It can be dropped now if not needed. Flags are additional information to the implementation what's the intended memory usage, so that they can optimize how HW resource are used. > > 2. If public API changed then you need to fix all existence platforms, > to not break their compilation/work. I changed the reference API and implementation (== linux-generic). Maintainers of other implementations are free to decide when (and more importantly who) to follow. We cannot keep all implementations in 100% sync. It's better to break a build (if they are reusing linux-generic code), than try to fix all implementations at once (with potentially untested, buggy code). > > 3. What I don't like in current odp_shm_reserve() is that it's > difficult to understand what is the reason of the error. > I think it's reasonable to change that function from: > void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > odp_shm_e flag) > to > int odp_shm_reserve(void **addr, const char *name, uint64_t size, > uint64_t align, > odp_shm_e flag) > > where return codes are: 1) chunk with that name already exist. addr is > set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. > I.e. not just fail with ODP_ERR() but return some reasonable value. This goes back to errno discussion... I think it should return NULL and set errno. Errno/abort()/etc changes will be done in sync with all other APIs. -Petri
On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote: > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov >> Sent: Tuesday, September 02, 2014 6:43 PM >> To: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >> >> >> >> On 09/01/2014 03:53 PM, Petri Savolainen wrote: >>> Add flags parameter to reserve call. >>> * SW_ONLY: optimize memory usage when HW accelerators are not >>> involved. >>> * PROC: for communication with other (non-ODP) processes in the >>> system >>> * KERNEL: for communication with a kernel (OS) driver >> Hi Petri, >> >> this patch is about the same which I sent. > Didn't realize that. This is not only for IPC, but the shm flag was on TODO list from last sprint to help Taras to optimize shared memory allocation (flag SW only vs. SW+HW accelerator access). Yes, it's patch for reserving memory. Because it's general api I need the same for IPC. My patch was here: https://patches.linaro.org/35037/ > >> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is >> there is specific need for that? What will happen if >> app will set this flag but flag is not supported by platform. > I was thinking to use that to flag memory that will be shared with a kernel driver (but not with external processes). It can be dropped now if not needed. Flags are additional information to the implementation what's the intended memory usage, so that they can optimize how HW resource are used. Lets do that. Drop this flag for now. And if Taras needs this flag he will add it with KS2 implementation. >> 2. If public API changed then you need to fix all existence platforms, >> to not break their compilation/work. > I changed the reference API and implementation (== linux-generic). Maintainers of other implementations are free to decide when (and more importantly who) to follow. We cannot keep all implementations in 100% sync. It's better to break a build (if they are reusing linux-generic code), than try to fix all implementations at once (with potentially untested, buggy code). You should not break other platforms when changing api for linux-generic. You added new flag to odp_shm_reserve() for example to example/generator/odp_generator.c but ks2 and dpdk still have on one argument less. So their compilation will fail. And this things have to be avoided. We also decided that git history have to be 'git bisect' friendly. That rule is also acceptable on other arches. >> 3. What I don't like in current odp_shm_reserve() is that it's >> difficult to understand what is the reason of the error. >> I think it's reasonable to change that function from: >> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, >> odp_shm_e flag) >> to >> int odp_shm_reserve(void **addr, const char *name, uint64_t size, >> uint64_t align, >> odp_shm_e flag) >> >> where return codes are: 1) chunk with that name already exist. addr is >> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. >> I.e. not just fail with ODP_ERR() but return some reasonable value. > This goes back to errno discussion... I think it should return NULL and set errno. Errno/abort()/etc changes will be done in sync with all other APIs. errno is ok for me. Maxim. > -Petri > >
> -----Original Message----- > From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org] > Sent: Wednesday, September 03, 2014 1:37 PM > To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > > On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote: > > > >> -----Original Message----- > >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > >> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov > >> Sent: Tuesday, September 02, 2014 6:43 PM > >> To: lng-odp@lists.linaro.org > >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > >> > >> > >> > >> On 09/01/2014 03:53 PM, Petri Savolainen wrote: > >>> Add flags parameter to reserve call. > >>> * SW_ONLY: optimize memory usage when HW accelerators are not > >>> involved. > >>> * PROC: for communication with other (non-ODP) processes in the > >>> system > >>> * KERNEL: for communication with a kernel (OS) driver > >> Hi Petri, > >> > >> this patch is about the same which I sent. > > Didn't realize that. This is not only for IPC, but the shm flag was on > TODO list from last sprint to help Taras to optimize shared memory > allocation (flag SW only vs. SW+HW accelerator access). > > Yes, it's patch for reserving memory. Because it's general api I need > the same for IPC. > My patch was here: > https://patches.linaro.org/35037/ > > > > >> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is > >> there is specific need for that? What will happen if > >> app will set this flag but flag is not supported by platform. > > I was thinking to use that to flag memory that will be shared with a > kernel driver (but not with external processes). It can be dropped now if > not needed. Flags are additional information to the implementation what's > the intended memory usage, so that they can optimize how HW resource are > used. > > Lets do that. Drop this flag for now. And if Taras needs this flag he > will add it with KS2 implementation. > >> 2. If public API changed then you need to fix all existence platforms, > >> to not break their compilation/work. > > I changed the reference API and implementation (== linux-generic). > Maintainers of other implementations are free to decide when (and more > importantly who) to follow. We cannot keep all implementations in 100% > sync. It's better to break a build (if they are reusing linux-generic > code), than try to fix all implementations at once (with potentially > untested, buggy code). > > You should not break other platforms when changing api for > linux-generic. You added new flag to odp_shm_reserve() for example to > example/generator/odp_generator.c but ks2 and dpdk still have on one > argument less. So their compilation will fail. And this things have to > be avoided. We also decided that git history > have to be 'git bisect' friendly. That rule is also acceptable on > other arches. Maybe we need to start versioning the API and test apps, so that it's possible to have different implementations out-of-sync. Otherwise after couple of more platforms, it's pretty impossible to modify the (reference) API if all implementations must be modified at once. Who can master HW/SDK/implementation details of all the platforms? Also it would require pretty tight lock-step development (not very agile). A new argument is trivial to add... but if implementation is missing, test apps would fail still... if you implement you should test it... does everybody have access to all HW platforms... -Petri > > >> 3. What I don't like in current odp_shm_reserve() is that it's > >> difficult to understand what is the reason of the error. > >> I think it's reasonable to change that function from: > >> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > >> odp_shm_e flag) > >> to > >> int odp_shm_reserve(void **addr, const char *name, uint64_t size, > >> uint64_t align, > >> odp_shm_e flag) > >> > >> where return codes are: 1) chunk with that name already exist. addr is > >> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. > >> I.e. not just fail with ODP_ERR() but return some reasonable value. > > This goes back to errno discussion... I think it should return NULL and > set errno. Errno/abort()/etc changes will be done in sync with all other > APIs. > errno is ok for me. > > Maxim. > > -Petri > > > >
Perti, can you please take a look at "linux-generic: odp ipc implementation v4" patch? We need to define how to pass flags to odp_shm_reserve. Or we use mine variant or yours. Thank you, Maxim. On 09/03/2014 03:56 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > >> -----Original Message----- >> From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Wednesday, September 03, 2014 1:37 PM >> To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >> >> On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote: >>>> -----Original Message----- >>>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >>>> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov >>>> Sent: Tuesday, September 02, 2014 6:43 PM >>>> To: lng-odp@lists.linaro.org >>>> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >>>> >>>> >>>> >>>> On 09/01/2014 03:53 PM, Petri Savolainen wrote: >>>>> Add flags parameter to reserve call. >>>>> * SW_ONLY: optimize memory usage when HW accelerators are not >>>>> involved. >>>>> * PROC: for communication with other (non-ODP) processes in the >>>>> system >>>>> * KERNEL: for communication with a kernel (OS) driver >>>> Hi Petri, >>>> >>>> this patch is about the same which I sent. >>> Didn't realize that. This is not only for IPC, but the shm flag was on >> TODO list from last sprint to help Taras to optimize shared memory >> allocation (flag SW only vs. SW+HW accelerator access). >> >> Yes, it's patch for reserving memory. Because it's general api I need >> the same for IPC. >> My patch was here: >> https://patches.linaro.org/35037/ >> >>>> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is >>>> there is specific need for that? What will happen if >>>> app will set this flag but flag is not supported by platform. >>> I was thinking to use that to flag memory that will be shared with a >> kernel driver (but not with external processes). It can be dropped now if >> not needed. Flags are additional information to the implementation what's >> the intended memory usage, so that they can optimize how HW resource are >> used. >> >> Lets do that. Drop this flag for now. And if Taras needs this flag he >> will add it with KS2 implementation. >>>> 2. If public API changed then you need to fix all existence platforms, >>>> to not break their compilation/work. >>> I changed the reference API and implementation (== linux-generic). >> Maintainers of other implementations are free to decide when (and more >> importantly who) to follow. We cannot keep all implementations in 100% >> sync. It's better to break a build (if they are reusing linux-generic >> code), than try to fix all implementations at once (with potentially >> untested, buggy code). >> >> You should not break other platforms when changing api for >> linux-generic. You added new flag to odp_shm_reserve() for example to >> example/generator/odp_generator.c but ks2 and dpdk still have on one >> argument less. So their compilation will fail. And this things have to >> be avoided. We also decided that git history >> have to be 'git bisect' friendly. That rule is also acceptable on >> other arches. > Maybe we need to start versioning the API and test apps, so that it's possible to have different implementations out-of-sync. Otherwise after couple of more platforms, it's pretty impossible to modify the (reference) API if all implementations must be modified at once. Who can master HW/SDK/implementation details of all the platforms? Also it would require pretty tight lock-step development (not very agile). > > A new argument is trivial to add... but if implementation is missing, test apps would fail still... if you implement you should test it... does everybody have access to all HW platforms... > > -Petri > >>>> 3. What I don't like in current odp_shm_reserve() is that it's >>>> difficult to understand what is the reason of the error. >>>> I think it's reasonable to change that function from: >>>> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, >>>> odp_shm_e flag) >>>> to >>>> int odp_shm_reserve(void **addr, const char *name, uint64_t size, >>>> uint64_t align, >>>> odp_shm_e flag) >>>> >>>> where return codes are: 1) chunk with that name already exist. addr is >>>> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. >>>> I.e. not just fail with ODP_ERR() but return some reasonable value. >>> This goes back to errno discussion... I think it should return NULL and >> set errno. Errno/abort()/etc changes will be done in sync with all other >> APIs. >> errno is ok for me. >> >> Maxim. >>> -Petri >>> >>>
On 3 September 2014 07:56, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > > > > -----Original Message----- > > From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org] > > Sent: Wednesday, September 03, 2014 1:37 PM > > To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org > > Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > > > > On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote: > > > > > >> -----Original Message----- > > >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > >> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov > > >> Sent: Tuesday, September 02, 2014 6:43 PM > > >> To: lng-odp@lists.linaro.org > > >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > > >> > > >> > > >> > > >> On 09/01/2014 03:53 PM, Petri Savolainen wrote: > > >>> Add flags parameter to reserve call. > > >>> * SW_ONLY: optimize memory usage when HW accelerators are not > > >>> involved. > > >>> * PROC: for communication with other (non-ODP) processes in the > > >>> system > > >>> * KERNEL: for communication with a kernel (OS) driver > > >> Hi Petri, > > >> > > >> this patch is about the same which I sent. > > > Didn't realize that. This is not only for IPC, but the shm flag was on > > TODO list from last sprint to help Taras to optimize shared memory > > allocation (flag SW only vs. SW+HW accelerator access). > > > > Yes, it's patch for reserving memory. Because it's general api I need > > the same for IPC. > > My patch was here: > > https://patches.linaro.org/35037/ > > > > > > > >> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is > > >> there is specific need for that? What will happen if > > >> app will set this flag but flag is not supported by platform. > > > I was thinking to use that to flag memory that will be shared with a > > kernel driver (but not with external processes). It can be dropped now if > > not needed. Flags are additional information to the implementation what's > > the intended memory usage, so that they can optimize how HW resource are > > used. > > > > Lets do that. Drop this flag for now. And if Taras needs this flag he > > will add it with KS2 implementation. > > >> 2. If public API changed then you need to fix all existence platforms, > > >> to not break their compilation/work. > > > I changed the reference API and implementation (== linux-generic). > > Maintainers of other implementations are free to decide when (and more > > importantly who) to follow. We cannot keep all implementations in 100% > > sync. It's better to break a build (if they are reusing linux-generic > > code), than try to fix all implementations at once (with potentially > > untested, buggy code). > > > > You should not break other platforms when changing api for > > linux-generic. You added new flag to odp_shm_reserve() for example to > > example/generator/odp_generator.c but ks2 and dpdk still have on one > > argument less. So their compilation will fail. And this things have to > > be avoided. We also decided that git history > > have to be 'git bisect' friendly. That rule is also acceptable on > > other arches. > > Maybe we need to start versioning the API and test apps, so that it's > possible to have different implementations out-of-sync. I agree but won't that be for official releases ? If not we need to schedule a point each week/month to get a working set - monthly fits Linaros schedule. Otherwise after couple of more platforms, it's pretty impossible to modify > the (reference) API if all implementations must be modified at once. Who > can master HW/SDK/implementation details of all the platforms? Also it > would require pretty tight lock-step development (not very agile). > If an API change is made, I think linux-generic should always be fixed in the first commit even if the implementation returns not implemented, this is true when the change was driven by another platform. > > A new argument is trivial to add... but if implementation is missing, test > apps would fail still... if you implement you should test it... does > everybody have access to all HW platforms... > > We only have to worry about those that are part of the repo, and we do have access to all of them in LAVA. We also have scripts to build and run the tests fairly easily across the board - anyone can set up a CI job to take their own repo and nightly build all the platforms and run all the tests, Maxim has one of these setup he uses as a staging test in addition to the real repo test. As for maintainers I see it this way. Venki is the mainter for DPDK, Taras for KS2, linux generic everyone has a hand in it, but we should pick a maintainer for that - possibly not Maxim if he is looking after the whole thing and odp-apps And I agree 100% that if you implement it you need to add a test for it, it should almost be a requirement that there is a basic unit test > -Petri > > > > > >> 3. What I don't like in current odp_shm_reserve() is that it's > > >> difficult to understand what is the reason of the error. > > >> I think it's reasonable to change that function from: > > >> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > > >> odp_shm_e flag) > > >> to > > >> int odp_shm_reserve(void **addr, const char *name, uint64_t size, > > >> uint64_t align, > > >> odp_shm_e flag) > > >> > > >> where return codes are: 1) chunk with that name already exist. addr is > > >> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. > > >> I.e. not just fail with ODP_ERR() but return some reasonable value. > > > This goes back to errno discussion... I think it should return NULL and > > set errno. Errno/abort()/etc changes will be done in sync with all other > > APIs. > > errno is ok for me. > > > > Maxim. > > > -Petri > > > > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
> >> 2. If public API changed then you need to fix all existence platforms, > >> to not break their compilation/work. > > I changed the reference API and implementation (== linux-generic). > Maintainers of other implementations are free to decide when (and more > importantly who) to follow. We cannot keep all implementations in 100% > sync. It's better to break a build (if they are reusing linux-generic > code), than try to fix all implementations at once (with potentially > untested, buggy code). > > You should not break other platforms when changing api for > linux-generic. You added new flag to odp_shm_reserve() for example to > example/generator/odp_generator.c but ks2 and dpdk still have on one > argument less. So their compilation will fail. And this things have to > be avoided. We also decided that git history > have to be 'git bisect' friendly. That rule is also acceptable on > other arches. Maybe we need to start versioning the API and test apps, so that it's possible to have different implementations out-of-sync. I agree but won't that be for official releases ? If not we need to schedule a point each week/month to get a working set - monthly fits Linaros schedule. [Petri] Even weekly freeze cycle is too slow. I was thinking that: - every API change increment API version number - we have only one reference API/implementation/test app branch and that's linux-generic - latest development is always in linux-generic (for example API v0.8.12) - other implementations (in the repo) would branch(some version of) the reference, for example - linux-dpdk could be at 0.8.11 (including API and test cases) - linux-k2 could be at 0.7.17 (including API and test cases) Otherwise we have three references implementations (if all must at sync with latest API) and we don't afford that. E.g. if we change the queue API, should we all wait that TI K2 (HW specific) implementation is updated, before the change can be merged? It'd hold back API development and all other guys (also those outside of the repo). Shouldn't this all be easy with git :) -Petri
Let's use mine. Added shm features are not IPC specific. Also...
+typedef enum {
+ ODP_SHM_THREAD = 1, /**< Memory accessible by threads. */
+ ODP_SHM_PROC = 2, /**< Memory accessible by processes.
+ Will be created if not exist. */
+ ODP_SHM_PROC_NOCREAT = 3, /**< Memory accessible by processes.
+ Has to be created before usage.*/
+} odp_shm_e;
... this exposes too much (OS) details to the application. Application wants to share data with an entity outside of ODP - no need to specify if the other entity id a thread or process.
NOCREAT not needed, it's a look up.
-Petri
> -----Original Message-----
> From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
> Sent: Wednesday, September 03, 2014 4:59 PM
> To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag
>
> Perti, can you please take a look at "linux-generic: odp ipc
> implementation v4" patch?
>
> We need to define how to pass flags to odp_shm_reserve. Or we use mine
> variant or yours.
>
> Thank you,
> Maxim.
>
>
> On 09/03/2014 03:56 PM, Savolainen, Petri (NSN - FI/Espoo) wrote:
> >
> >> -----Original Message-----
> >> From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
> >> Sent: Wednesday, September 03, 2014 1:37 PM
> >> To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org
> >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag
> >>
> >> On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote:
> >>>> -----Original Message-----
> >>>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> >>>> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
> >>>> Sent: Tuesday, September 02, 2014 6:43 PM
> >>>> To: lng-odp@lists.linaro.org
> >>>> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag
> >>>>
> >>>>
> >>>>
> >>>> On 09/01/2014 03:53 PM, Petri Savolainen wrote:
> >>>>> Add flags parameter to reserve call.
> >>>>> * SW_ONLY: optimize memory usage when HW accelerators are not
> >>>>> involved.
> >>>>> * PROC: for communication with other (non-ODP) processes in the
> >>>>> system
> >>>>> * KERNEL: for communication with a kernel (OS) driver
> >>>> Hi Petri,
> >>>>
> >>>> this patch is about the same which I sent.
> >>> Didn't realize that. This is not only for IPC, but the shm flag was on
> >> TODO list from last sprint to help Taras to optimize shared memory
> >> allocation (flag SW only vs. SW+HW accelerator access).
> >>
> >> Yes, it's patch for reserving memory. Because it's general api I need
> >> the same for IPC.
> >> My patch was here:
> >> https://patches.linaro.org/35037/
> >>
> >>>> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is
> >>>> there is specific need for that? What will happen if
> >>>> app will set this flag but flag is not supported by platform.
> >>> I was thinking to use that to flag memory that will be shared with a
> >> kernel driver (but not with external processes). It can be dropped now
> if
> >> not needed. Flags are additional information to the implementation
> what's
> >> the intended memory usage, so that they can optimize how HW resource
> are
> >> used.
> >>
> >> Lets do that. Drop this flag for now. And if Taras needs this flag he
> >> will add it with KS2 implementation.
> >>>> 2. If public API changed then you need to fix all existence
> platforms,
> >>>> to not break their compilation/work.
> >>> I changed the reference API and implementation (== linux-generic).
> >> Maintainers of other implementations are free to decide when (and more
> >> importantly who) to follow. We cannot keep all implementations in 100%
> >> sync. It's better to break a build (if they are reusing linux-generic
> >> code), than try to fix all implementations at once (with potentially
> >> untested, buggy code).
> >>
> >> You should not break other platforms when changing api for
> >> linux-generic. You added new flag to odp_shm_reserve() for example to
> >> example/generator/odp_generator.c but ks2 and dpdk still have on one
> >> argument less. So their compilation will fail. And this things have to
> >> be avoided. We also decided that git history
> >> have to be 'git bisect' friendly. That rule is also acceptable on
> >> other arches.
> > Maybe we need to start versioning the API and test apps, so that it's
> possible to have different implementations out-of-sync. Otherwise after
> couple of more platforms, it's pretty impossible to modify the (reference)
> API if all implementations must be modified at once. Who can master
> HW/SDK/implementation details of all the platforms? Also it would require
> pretty tight lock-step development (not very agile).
> >
> > A new argument is trivial to add... but if implementation is missing,
> test apps would fail still... if you implement you should test it... does
> everybody have access to all HW platforms...
> >
> > -Petri
> >
> >>>> 3. What I don't like in current odp_shm_reserve() is that it's
> >>>> difficult to understand what is the reason of the error.
> >>>> I think it's reasonable to change that function from:
> >>>> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t
> align,
> >>>> odp_shm_e flag)
> >>>> to
> >>>> int odp_shm_reserve(void **addr, const char *name, uint64_t size,
> >>>> uint64_t align,
> >>>> odp_shm_e flag)
> >>>>
> >>>> where return codes are: 1) chunk with that name already exist. addr
> is
> >>>> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc.
> >>>> I.e. not just fail with ODP_ERR() but return some reasonable value.
> >>> This goes back to errno discussion... I think it should return NULL
> and
> >> set errno. Errno/abort()/etc changes will be done in sync with all
> other
> >> APIs.
> >> errno is ok for me.
> >>
> >> Maxim.
> >>> -Petri
> >>>
> >>>
On Mon, Sep 01, 2014 at 12:53:04PM +0100, Petri Savolainen wrote: > Add flags parameter to reserve call. > * SW_ONLY: optimize memory usage when HW accelerators are not > involved. > * PROC: for communication with other (non-ODP) processes in the > system The non-ODP part isn't really a limitation as suggested here. > * KERNEL: for communication with a kernel (OS) driver > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > .../linux-generic/include/api/odp_shared_memory.h | 13 ++++++- > platform/linux-generic/odp_shared_memory.c | 45 +++++++++++++++++----- > 2 files changed, 48 insertions(+), 10 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h > index 8ac8847..814acdc 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -24,6 +24,15 @@ extern "C" { > /** Maximum shared memory block name lenght in chars */ typo - length (I know it wasn't changed in this patch, but if you're doing a v2 anyway..) > #define ODP_SHM_NAME_LEN 32 > > +/* > + * Shared memory flags > + */ > + > +/* Share level */ > +#define ODP_SHM_SW_ONLY 0x1 /**< Internal to application SW, no HW access */ > +#define ODP_SHM_PROC 0x2 /**< Share with external processes */ > +#define ODP_SHM_KERNEL 0x4 /**< Share with kernel */ This seems a bit confused as to whether it's a level or a flag. If you set only ODP_SHM_PROC the suggestion is that this means it wouldn't be shared internal to the application.. but ODP_SHM_SW_ONLY|ODP_SHM_PROC isn't logical either. > + > > /** > * Reserve a block of shared memory > @@ -31,10 +40,12 @@ extern "C" { > * @param name Name of the block (maximum ODP_SHM_NAME_LEN - 1 chars) > * @param size Block size in bytes > * @param align Block alignment in bytes > + * @param flags Block parameter flags > * > * @return Pointer to the reserved block, or NULL > */ > -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align); > +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > + uint32_t flags); > > /** > * Lookup for a block of shared memory > diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c > index 784f42b..4cb82b5 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -11,6 +11,7 @@ > #include <odp_system_info.h> > #include <odp_debug.h> > > +#include <unistd.h> > #include <sys/mman.h> > #include <asm/mman.h> > #include <fcntl.h> > @@ -44,8 +45,6 @@ typedef struct { > #define MAP_ANONYMOUS MAP_ANON > #endif > > -#define SHM_FLAGS (MAP_SHARED | MAP_ANONYMOUS) > - > > /* Global shared memory table */ > static odp_shm_table_t *odp_shm_tbl; > @@ -60,7 +59,7 @@ int odp_shm_init_global(void) > #endif > > addr = mmap(NULL, sizeof(odp_shm_table_t), > - PROT_READ | PROT_WRITE, SHM_FLAGS, -1, 0); > + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); > > if (addr == MAP_FAILED) > return -1; > @@ -95,11 +94,17 @@ static int find_block(const char *name) > } > > > -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > + uint32_t flags) > { > int i; > odp_shm_block_t *block; > void *addr; > + int fd = -1; > + int map_flag = MAP_SHARED; > + /* O_EXCL: error, O_TRUNC: truncate to zero, if already exists */ > + int oflag = O_RDWR | O_CREAT | O_TRUNC; Not sure why the comment mentions O_EXCL as it isn't being set. I suppose the expected behaviour is that if the shm already exists it's just opened, so this is just a comment error.
On 09/04/2014 02:18 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > Let's use mine. Added shm features are not IPC specific. Also... > > +typedef enum { > + ODP_SHM_THREAD = 1, /**< Memory accessible by threads. */ > + ODP_SHM_PROC = 2, /**< Memory accessible by processes. > + Will be created if not exist. */ > + ODP_SHM_PROC_NOCREAT = 3, /**< Memory accessible by processes. > + Has to be created before usage.*/ > +} odp_shm_e; > > ... this exposes too much (OS) details to the application. Application wants to share data with an entity outside of ODP - no need to specify if the other entity id a thread or process. > > NOCREAT not needed, it's a look up. It's not standard look up. It also needs to create entry in odp_shm_tbl. But this thing in platform implementation. Will think how to move it there. Maxim. > > -Petri > >> -----Original Message----- >> From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Wednesday, September 03, 2014 4:59 PM >> To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >> >> Perti, can you please take a look at "linux-generic: odp ipc >> implementation v4" patch? >> >> We need to define how to pass flags to odp_shm_reserve. Or we use mine >> variant or yours. >> >> Thank you, >> Maxim. >> >> >> On 09/03/2014 03:56 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: >>>> -----Original Message----- >>>> From: ext Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >>>> Sent: Wednesday, September 03, 2014 1:37 PM >>>> To: Savolainen, Petri (NSN - FI/Espoo); lng-odp@lists.linaro.org >>>> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >>>> >>>> On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote: >>>>>> -----Original Message----- >>>>>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >>>>>> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov >>>>>> Sent: Tuesday, September 02, 2014 6:43 PM >>>>>> To: lng-odp@lists.linaro.org >>>>>> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >>>>>> >>>>>> >>>>>> >>>>>> On 09/01/2014 03:53 PM, Petri Savolainen wrote: >>>>>>> Add flags parameter to reserve call. >>>>>>> * SW_ONLY: optimize memory usage when HW accelerators are not >>>>>>> involved. >>>>>>> * PROC: for communication with other (non-ODP) processes in the >>>>>>> system >>>>>>> * KERNEL: for communication with a kernel (OS) driver >>>>>> Hi Petri, >>>>>> >>>>>> this patch is about the same which I sent. >>>>> Didn't realize that. This is not only for IPC, but the shm flag was on >>>> TODO list from last sprint to help Taras to optimize shared memory >>>> allocation (flag SW only vs. SW+HW accelerator access). >>>> >>>> Yes, it's patch for reserving memory. Because it's general api I need >>>> the same for IPC. >>>> My patch was here: >>>> https://patches.linaro.org/35037/ >>>> >>>>>> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is >>>>>> there is specific need for that? What will happen if >>>>>> app will set this flag but flag is not supported by platform. >>>>> I was thinking to use that to flag memory that will be shared with a >>>> kernel driver (but not with external processes). It can be dropped now >> if >>>> not needed. Flags are additional information to the implementation >> what's >>>> the intended memory usage, so that they can optimize how HW resource >> are >>>> used. >>>> >>>> Lets do that. Drop this flag for now. And if Taras needs this flag he >>>> will add it with KS2 implementation. >>>>>> 2. If public API changed then you need to fix all existence >> platforms, >>>>>> to not break their compilation/work. >>>>> I changed the reference API and implementation (== linux-generic). >>>> Maintainers of other implementations are free to decide when (and more >>>> importantly who) to follow. We cannot keep all implementations in 100% >>>> sync. It's better to break a build (if they are reusing linux-generic >>>> code), than try to fix all implementations at once (with potentially >>>> untested, buggy code). >>>> >>>> You should not break other platforms when changing api for >>>> linux-generic. You added new flag to odp_shm_reserve() for example to >>>> example/generator/odp_generator.c but ks2 and dpdk still have on one >>>> argument less. So their compilation will fail. And this things have to >>>> be avoided. We also decided that git history >>>> have to be 'git bisect' friendly. That rule is also acceptable on >>>> other arches. >>> Maybe we need to start versioning the API and test apps, so that it's >> possible to have different implementations out-of-sync. Otherwise after >> couple of more platforms, it's pretty impossible to modify the (reference) >> API if all implementations must be modified at once. Who can master >> HW/SDK/implementation details of all the platforms? Also it would require >> pretty tight lock-step development (not very agile). >>> A new argument is trivial to add... but if implementation is missing, >> test apps would fail still... if you implement you should test it... does >> everybody have access to all HW platforms... >>> -Petri >>> >>>>>> 3. What I don't like in current odp_shm_reserve() is that it's >>>>>> difficult to understand what is the reason of the error. >>>>>> I think it's reasonable to change that function from: >>>>>> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t >> align, >>>>>> odp_shm_e flag) >>>>>> to >>>>>> int odp_shm_reserve(void **addr, const char *name, uint64_t size, >>>>>> uint64_t align, >>>>>> odp_shm_e flag) >>>>>> >>>>>> where return codes are: 1) chunk with that name already exist. addr >> is >>>>>> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. >>>>>> I.e. not just fail with ODP_ERR() but return some reasonable value. >>>>> This goes back to errno discussion... I think it should return NULL >> and >>>> set errno. Errno/abort()/etc changes will be done in sync with all >> other >>>> APIs. >>>> errno is ok for me. >>>> >>>> Maxim. >>>>> -Petri >>>>> >>>>>
> -----Original Message----- > From: ext Stuart Haslam [mailto:stuart.haslam@arm.com] > Sent: Thursday, September 04, 2014 2:19 PM > To: Petri Savolainen > Cc: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > > On Mon, Sep 01, 2014 at 12:53:04PM +0100, Petri Savolainen wrote: > > Add flags parameter to reserve call. > > * SW_ONLY: optimize memory usage when HW accelerators are not > > involved. > > * PROC: for communication with other (non-ODP) processes in the > > system > > The non-ODP part isn't really a limitation as suggested here. I think it is, since ODP API calls should be the same regardless of the application is running as threads/processes/bare metal. That's supported by default. Here the PROC flag says that external processes should see the memory also. > > > * KERNEL: for communication with a kernel (OS) driver > > > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > > --- > > .../linux-generic/include/api/odp_shared_memory.h | 13 ++++++- > > platform/linux-generic/odp_shared_memory.c | 45 > +++++++++++++++++----- > > 2 files changed, 48 insertions(+), 10 deletions(-) > > > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h > b/platform/linux-generic/include/api/odp_shared_memory.h > > index 8ac8847..814acdc 100644 > > --- a/platform/linux-generic/include/api/odp_shared_memory.h > > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > > @@ -24,6 +24,15 @@ extern "C" { > > /** Maximum shared memory block name lenght in chars */ > > typo - length (I know it wasn't changed in this patch, but if you're > doing a v2 anyway..) > > > #define ODP_SHM_NAME_LEN 32 > > > > +/* > > + * Shared memory flags > > + */ > > + > > +/* Share level */ > > +#define ODP_SHM_SW_ONLY 0x1 /**< Internal to application SW, no HW > access */ > > +#define ODP_SHM_PROC 0x2 /**< Share with external processes */ > > +#define ODP_SHM_KERNEL 0x4 /**< Share with kernel */ > > This seems a bit confused as to whether it's a level or a flag. If you > set only ODP_SHM_PROC the suggestion is that this means it wouldn't be > shared internal to the application.. but ODP_SHM_SW_ONLY|ODP_SHM_PROC > isn't logical either. ODP_SHM_SW_ONLY == HW accelerators will _not_ access this memory define ODP_SHM_PROC == external (user space) processes access this memory For a SW only struct, ODP_SHM_SW_ONLY|ODP_SHM_PROC would be logical choice. > > > + > > > > /** > > * Reserve a block of shared memory > > @@ -31,10 +40,12 @@ extern "C" { > > * @param name Name of the block (maximum ODP_SHM_NAME_LEN - 1 chars) > > * @param size Block size in bytes > > * @param align Block alignment in bytes > > + * @param flags Block parameter flags > > * > > * @return Pointer to the reserved block, or NULL > > */ > > -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align); > > +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > > + uint32_t flags); > > > > /** > > * Lookup for a block of shared memory > > diff --git a/platform/linux-generic/odp_shared_memory.c > b/platform/linux-generic/odp_shared_memory.c > > index 784f42b..4cb82b5 100644 > > --- a/platform/linux-generic/odp_shared_memory.c > > +++ b/platform/linux-generic/odp_shared_memory.c > > @@ -11,6 +11,7 @@ > > #include <odp_system_info.h> > > #include <odp_debug.h> > > > > +#include <unistd.h> > > #include <sys/mman.h> > > #include <asm/mman.h> > > #include <fcntl.h> > > @@ -44,8 +45,6 @@ typedef struct { > > #define MAP_ANONYMOUS MAP_ANON > > #endif > > > > -#define SHM_FLAGS (MAP_SHARED | MAP_ANONYMOUS) > > - > > > > /* Global shared memory table */ > > static odp_shm_table_t *odp_shm_tbl; > > @@ -60,7 +59,7 @@ int odp_shm_init_global(void) > > #endif > > > > addr = mmap(NULL, sizeof(odp_shm_table_t), > > - PROT_READ | PROT_WRITE, SHM_FLAGS, -1, 0); > > + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, > 0); > > > > if (addr == MAP_FAILED) > > return -1; > > @@ -95,11 +94,17 @@ static int find_block(const char *name) > > } > > > > > > -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) > > +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > > + uint32_t flags) > > { > > int i; > > odp_shm_block_t *block; > > void *addr; > > + int fd = -1; > > + int map_flag = MAP_SHARED; > > + /* O_EXCL: error, O_TRUNC: truncate to zero, if already exists */ > > + int oflag = O_RDWR | O_CREAT | O_TRUNC; > > Not sure why the comment mentions O_EXCL as it isn't being set. I suppose > the expected behaviour is that if the shm already exists it's just opened, > so this is just a comment error. O_EXCL is there to remain, that it should be used after we have proper clean up implemented. I choose to O_TRUNC now so that user do not have to delete the the /dev/shm/foobar file between application runs. -Petri
On 09/04/2014 04:27 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > >> -----Original Message----- >> From: ext Stuart Haslam [mailto:stuart.haslam@arm.com] >> Sent: Thursday, September 04, 2014 2:19 PM >> To: Petri Savolainen >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag >> >> On Mon, Sep 01, 2014 at 12:53:04PM +0100, Petri Savolainen wrote: >>> Add flags parameter to reserve call. >>> * SW_ONLY: optimize memory usage when HW accelerators are not >>> involved. >>> * PROC: for communication with other (non-ODP) processes in the >>> system >> The non-ODP part isn't really a limitation as suggested here. > I think it is, since ODP API calls should be the same regardless of the application is running as threads/processes/bare metal. That's supported by default. Here the PROC flag says that external processes should see the memory also. Why not put this to enum? I have: typedef enum { ODP_SHM_THREAD = 1, /**< Memory accessible by threads. */ ODP_SHM_PROC = 2, } It might be ODP_SHM_THREAD is not the good name. And we can change it to ODP_SHM_DEFAULT. Which is more readable then 0 flag. Or we can go with enum and bind default to 0. Maxim. > >>> * KERNEL: for communication with a kernel (OS) driver >>> >>> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >>> --- >>> .../linux-generic/include/api/odp_shared_memory.h | 13 ++++++- >>> platform/linux-generic/odp_shared_memory.c | 45 >> +++++++++++++++++----- >>> 2 files changed, 48 insertions(+), 10 deletions(-) >>> >>> diff --git a/platform/linux-generic/include/api/odp_shared_memory.h >> b/platform/linux-generic/include/api/odp_shared_memory.h >>> index 8ac8847..814acdc 100644 >>> --- a/platform/linux-generic/include/api/odp_shared_memory.h >>> +++ b/platform/linux-generic/include/api/odp_shared_memory.h >>> @@ -24,6 +24,15 @@ extern "C" { >>> /** Maximum shared memory block name lenght in chars */ >> typo - length (I know it wasn't changed in this patch, but if you're >> doing a v2 anyway..) >> >>> #define ODP_SHM_NAME_LEN 32 >>> >>> +/* >>> + * Shared memory flags >>> + */ >>> + >>> +/* Share level */ >>> +#define ODP_SHM_SW_ONLY 0x1 /**< Internal to application SW, no HW >> access */ >>> +#define ODP_SHM_PROC 0x2 /**< Share with external processes */ >>> +#define ODP_SHM_KERNEL 0x4 /**< Share with kernel */ >> This seems a bit confused as to whether it's a level or a flag. If you >> set only ODP_SHM_PROC the suggestion is that this means it wouldn't be >> shared internal to the application.. but ODP_SHM_SW_ONLY|ODP_SHM_PROC >> isn't logical either. > ODP_SHM_SW_ONLY == HW accelerators will _not_ access this memory > define ODP_SHM_PROC == external (user space) processes access this memory > > For a SW only struct, ODP_SHM_SW_ONLY|ODP_SHM_PROC would be logical choice. > >>> + >>> >>> /** >>> * Reserve a block of shared memory >>> @@ -31,10 +40,12 @@ extern "C" { >>> * @param name Name of the block (maximum ODP_SHM_NAME_LEN - 1 chars) >>> * @param size Block size in bytes >>> * @param align Block alignment in bytes >>> + * @param flags Block parameter flags >>> * >>> * @return Pointer to the reserved block, or NULL >>> */ >>> -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align); >>> +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, >>> + uint32_t flags); >>> >>> /** >>> * Lookup for a block of shared memory >>> diff --git a/platform/linux-generic/odp_shared_memory.c >> b/platform/linux-generic/odp_shared_memory.c >>> index 784f42b..4cb82b5 100644 >>> --- a/platform/linux-generic/odp_shared_memory.c >>> +++ b/platform/linux-generic/odp_shared_memory.c >>> @@ -11,6 +11,7 @@ >>> #include <odp_system_info.h> >>> #include <odp_debug.h> >>> >>> +#include <unistd.h> >>> #include <sys/mman.h> >>> #include <asm/mman.h> >>> #include <fcntl.h> >>> @@ -44,8 +45,6 @@ typedef struct { >>> #define MAP_ANONYMOUS MAP_ANON >>> #endif >>> >>> -#define SHM_FLAGS (MAP_SHARED | MAP_ANONYMOUS) >>> - >>> >>> /* Global shared memory table */ >>> static odp_shm_table_t *odp_shm_tbl; >>> @@ -60,7 +59,7 @@ int odp_shm_init_global(void) >>> #endif >>> >>> addr = mmap(NULL, sizeof(odp_shm_table_t), >>> - PROT_READ | PROT_WRITE, SHM_FLAGS, -1, 0); >>> + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, >> 0); >>> if (addr == MAP_FAILED) >>> return -1; >>> @@ -95,11 +94,17 @@ static int find_block(const char *name) >>> } >>> >>> >>> -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) >>> +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, >>> + uint32_t flags) >>> { >>> int i; >>> odp_shm_block_t *block; >>> void *addr; >>> + int fd = -1; >>> + int map_flag = MAP_SHARED; >>> + /* O_EXCL: error, O_TRUNC: truncate to zero, if already exists */ >>> + int oflag = O_RDWR | O_CREAT | O_TRUNC; >> Not sure why the comment mentions O_EXCL as it isn't being set. I suppose >> the expected behaviour is that if the shm already exists it's just opened, >> so this is just a comment error. > O_EXCL is there to remain, that it should be used after we have proper clean up implemented. I choose to O_TRUNC now so that user do not have to delete the the /dev/shm/foobar file between application runs. > > -Petri > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
> > Why not put this to enum? I have: > > typedef enum { > ODP_SHM_THREAD = 1, /**< Memory accessible by threads. */ > ODP_SHM_PROC = 2, > } > > It might be ODP_SHM_THREAD is not the good name. And we can change it to > ODP_SHM_DEFAULT. Which is more readable then 0 flag. > Or we can go with enum and bind default to 0. > > Maxim. I used uint32_t since there can be multiple flags you can enable simultaneously. One example is: ODP_SHM_SW_ONLY|ODP_SHM_PROC, and we may come up with more such flags in the future. Enum is int, so you cannot tell how many flags are supported in total. -Petri
On 09/05/2014 12:30 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: >> Why not put this to enum? I have: >> >> typedef enum { >> ODP_SHM_THREAD = 1, /**< Memory accessible by threads. */ >> ODP_SHM_PROC = 2, >> } >> >> It might be ODP_SHM_THREAD is not the good name. And we can change it to >> ODP_SHM_DEFAULT. Which is more readable then 0 flag. >> Or we can go with enum and bind default to 0. >> >> Maxim. > I used uint32_t since there can be multiple flags you can enable simultaneously. One example is: ODP_SHM_SW_ONLY|ODP_SHM_PROC, and we may come up with more such flags in the future. > > Enum is int, so you cannot tell how many flags are supported in total. > > -Petri > Ok, if everybody agree, I'm ok to have it int. Please send v2, I will update IPC path on top of yours. Thanks, Maxim.
diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h index 8ac8847..814acdc 100644 --- a/platform/linux-generic/include/api/odp_shared_memory.h +++ b/platform/linux-generic/include/api/odp_shared_memory.h @@ -24,6 +24,15 @@ extern "C" { /** Maximum shared memory block name lenght in chars */ #define ODP_SHM_NAME_LEN 32 +/* + * Shared memory flags + */ + +/* Share level */ +#define ODP_SHM_SW_ONLY 0x1 /**< Internal to application SW, no HW access */ +#define ODP_SHM_PROC 0x2 /**< Share with external processes */ +#define ODP_SHM_KERNEL 0x4 /**< Share with kernel */ + /** * Reserve a block of shared memory @@ -31,10 +40,12 @@ extern "C" { * @param name Name of the block (maximum ODP_SHM_NAME_LEN - 1 chars) * @param size Block size in bytes * @param align Block alignment in bytes + * @param flags Block parameter flags * * @return Pointer to the reserved block, or NULL */ -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align); +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, + uint32_t flags); /** * Lookup for a block of shared memory diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c index 784f42b..4cb82b5 100644 --- a/platform/linux-generic/odp_shared_memory.c +++ b/platform/linux-generic/odp_shared_memory.c @@ -11,6 +11,7 @@ #include <odp_system_info.h> #include <odp_debug.h> +#include <unistd.h> #include <sys/mman.h> #include <asm/mman.h> #include <fcntl.h> @@ -44,8 +45,6 @@ typedef struct { #define MAP_ANONYMOUS MAP_ANON #endif -#define SHM_FLAGS (MAP_SHARED | MAP_ANONYMOUS) - /* Global shared memory table */ static odp_shm_table_t *odp_shm_tbl; @@ -60,7 +59,7 @@ int odp_shm_init_global(void) #endif addr = mmap(NULL, sizeof(odp_shm_table_t), - PROT_READ | PROT_WRITE, SHM_FLAGS, -1, 0); + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); if (addr == MAP_FAILED) return -1; @@ -95,11 +94,17 @@ static int find_block(const char *name) } -void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) +void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, + uint32_t flags) { int i; odp_shm_block_t *block; void *addr; + int fd = -1; + int map_flag = MAP_SHARED; + /* O_EXCL: error, O_TRUNC: truncate to zero, if already exists */ + int oflag = O_RDWR | O_CREAT | O_TRUNC; + uint64_t alloc_size = size + align; #ifdef MAP_HUGETLB uint64_t huge_sz, page_sz; @@ -107,11 +112,31 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) page_sz = odp_sys_page_size(); #endif + if (flags & ODP_SHM_PROC) { + /* Creates a file to /dev/shm */ + fd = shm_open(name, oflag, + S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); + + if (fd == -1) { + ODP_DBG("odp_shm_reserve: shm_open failed\n"); + return NULL; + } + + if (ftruncate(fd, alloc_size) == -1) { + ODP_DBG("odp_shm_reserve: ftruncate failed\n"); + return NULL; + } + + } else { + map_flag |= MAP_ANONYMOUS; + } + odp_spinlock_lock(&odp_shm_tbl->lock); if (find_block(name) >= 0) { /* Found a block with the same name */ odp_spinlock_unlock(&odp_shm_tbl->lock); + ODP_DBG("odp_shm_reserve: name already used\n"); return NULL; } @@ -125,6 +150,7 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) if (i > ODP_SHM_NUM_BLOCKS - 1) { /* Table full */ odp_spinlock_unlock(&odp_shm_tbl->lock); + ODP_DBG("odp_shm_reserve: no more blocks\n"); return NULL; } @@ -135,16 +161,16 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) #ifdef MAP_HUGETLB /* Try first huge pages */ - if (huge_sz && (size + align) > page_sz) { - addr = mmap(NULL, size + align, PROT_READ | PROT_WRITE, - SHM_FLAGS | MAP_HUGETLB, -1, 0); + if (huge_sz && alloc_size > page_sz) { + addr = mmap(NULL, alloc_size, PROT_READ | PROT_WRITE, + map_flag | MAP_HUGETLB, fd, 0); } #endif /* Use normal pages for small or failed huge page allocations */ if (addr == MAP_FAILED) { - addr = mmap(NULL, size + align, PROT_READ | PROT_WRITE, - SHM_FLAGS, -1, 0); + addr = mmap(NULL, alloc_size, PROT_READ | PROT_WRITE, + map_flag, fd, 0); } else { block->huge = 1; @@ -153,6 +179,7 @@ void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align) if (addr == MAP_FAILED) { /* Alloc failed */ odp_spinlock_unlock(&odp_shm_tbl->lock); + ODP_DBG("odp_shm_reserve: mmap failed\n"); return NULL; }
Add flags parameter to reserve call. * SW_ONLY: optimize memory usage when HW accelerators are not involved. * PROC: for communication with other (non-ODP) processes in the system * KERNEL: for communication with a kernel (OS) driver Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- .../linux-generic/include/api/odp_shared_memory.h | 13 ++++++- platform/linux-generic/odp_shared_memory.c | 45 +++++++++++++++++----- 2 files changed, 48 insertions(+), 10 deletions(-)