| Message ID | 1464678149-4551-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Accepted |
| Commit | d31384406eca620c1713253453085c3cdd8c86cb |
| Headers | show |
On 05/31/16 10:19, Savolainen, Petri (Nokia - FI/Espoo) wrote: > This is OK for a work around. > > Reviewed-by: Petri Savolainen <petri.savolainen@nokia.com> > > > But the current pool / IPC implementation should be corrected, so that API can be used to create pools with the same name on different instances. > > By definition, two instances must be able to call > > odp_pool_create("foo", ¶ms); > > without additional flags or worries about name space clash. > > > -Petri That is ok, I can add this difference with: 1) additional parameter to odp_pool_create(). That will require api change. 2) bind pool name to specific prefix, like odp_pool_create("ipc_my_name"). And add note somewhere in readme that app should name shared pools with that prefix to use ipc_pktio. Maxim. > > >> -----Original Message----- >> From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Tuesday, May 31, 2016 10:02 AM >> To: lng-odp@lists.linaro.org >> Cc: Maxim Uvarov <maxim.uvarov@linaro.org>; Savolainen, Petri (Nokia - >> FI/Espoo) <petri.savolainen@nokia.com> >> Subject: [PATCHv3] linux-generic: sched: do not allocate sheduler info in >> shm area >> >> Patch: >> 637a482 linux-generic: schedule: clean interface towards pktio >> Broke ipc pktio work. Restore original logic back. Idea here the following >> that 2 processes roll in the same name space instance and should not >> overlap each other shared memory files. There is no reason in putting >> sheduler internal structures to shared memory. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> Cc: Petri Savolainen <petri.savolainen@nokia.com> >> --- >> v3: 2 parametes without ifdefs (Petri) >> v2: add ifdefs. >> >> platform/linux-generic/odp_schedule.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux- >> generic/odp_schedule.c >> index 56f40a3..3ac32ef 100644 >> --- a/platform/linux-generic/odp_schedule.c >> +++ b/platform/linux-generic/odp_schedule.c >> @@ -22,6 +22,9 @@ >> #include <odp/api/thrmask.h> >> #include <odp_config_internal.h> >> #include <odp_schedule_internal.h> >> +#ifdef _ODP_PKTIO_IPC >> +#include <odp_pool_internal.h> >> +#endif >> >> /* Number of priority levels */ >> #define NUM_PRIO 8 >> @@ -153,7 +156,11 @@ int odp_schedule_init_global(void) >> params.buf.num = num_cmd; >> params.type = ODP_POOL_BUFFER; >> >> +#ifdef _ODP_PKTIO_IPC >> + pool = _pool_create("odp_sched_pool", ¶ms, 0); >> +#else >> pool = odp_pool_create("odp_sched_pool", ¶ms); >> +#endif >> if (pool == ODP_POOL_INVALID) { >> ODP_ERR("Schedule init: Pool create failed.\n"); >> return -1; >> -- >> 2.7.1.250.gff4ea60
Why does it have to be visible in the API, Maxim? (even introducing a specific prefix is really an API change, I think: Those app that happened to use the prefix before will not work as before). If IPC need its specific stuff, shouldn't it be using ODP internal functions, e.g. _odp_pool_create() wich can do whatever it wants? Christophe. On 31 May 2016 at 09:29, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/31/16 10:19, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> This is OK for a work around. >> >> Reviewed-by: Petri Savolainen <petri.savolainen@nokia.com> >> >> >> But the current pool / IPC implementation should be corrected, so that >> API can be used to create pools with the same name on different instances. >> >> By definition, two instances must be able to call >> >> odp_pool_create("foo", ¶ms); >> >> without additional flags or worries about name space clash. >> >> >> -Petri >> > > That is ok, I can add this difference with: > 1) additional parameter to odp_pool_create(). That will require api change. > 2) bind pool name to specific prefix, like odp_pool_create("ipc_my_name"). > And add note somewhere in readme that app should name shared pools > with that prefix to use ipc_pktio. > > Maxim. > > > > > >> >> -----Original Message----- >>> From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >>> Sent: Tuesday, May 31, 2016 10:02 AM >>> To: lng-odp@lists.linaro.org >>> Cc: Maxim Uvarov <maxim.uvarov@linaro.org>; Savolainen, Petri (Nokia - >>> FI/Espoo) <petri.savolainen@nokia.com> >>> Subject: [PATCHv3] linux-generic: sched: do not allocate sheduler info in >>> shm area >>> >>> Patch: >>> 637a482 linux-generic: schedule: clean interface towards pktio >>> Broke ipc pktio work. Restore original logic back. Idea here the >>> following >>> that 2 processes roll in the same name space instance and should not >>> overlap each other shared memory files. There is no reason in putting >>> sheduler internal structures to shared memory. >>> >>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >>> Cc: Petri Savolainen <petri.savolainen@nokia.com> >>> --- >>> v3: 2 parametes without ifdefs (Petri) >>> v2: add ifdefs. >>> >>> platform/linux-generic/odp_schedule.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux- >>> generic/odp_schedule.c >>> index 56f40a3..3ac32ef 100644 >>> --- a/platform/linux-generic/odp_schedule.c >>> +++ b/platform/linux-generic/odp_schedule.c >>> @@ -22,6 +22,9 @@ >>> #include <odp/api/thrmask.h> >>> #include <odp_config_internal.h> >>> #include <odp_schedule_internal.h> >>> +#ifdef _ODP_PKTIO_IPC >>> +#include <odp_pool_internal.h> >>> +#endif >>> >>> /* Number of priority levels */ >>> #define NUM_PRIO 8 >>> @@ -153,7 +156,11 @@ int odp_schedule_init_global(void) >>> params.buf.num = num_cmd; >>> params.type = ODP_POOL_BUFFER; >>> >>> +#ifdef _ODP_PKTIO_IPC >>> + pool = _pool_create("odp_sched_pool", ¶ms, 0); >>> +#else >>> pool = odp_pool_create("odp_sched_pool", ¶ms); >>> +#endif >>> if (pool == ODP_POOL_INVALID) { >>> ODP_ERR("Schedule init: Pool create failed.\n"); >>> return -1; >>> -- >>> 2.7.1.250.gff4ea60 >>> >> > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 05/31/16 10:38, Christophe Milard wrote: > Why does it have to be visible in the API, Maxim? (even introducing a > specific prefix is really an API change, I think: Those app that > happened to use the prefix before will not work as before). > If IPC need its specific stuff, shouldn't it be using ODP internal > functions, e.g. _odp_pool_create() wich can do whatever it wants? > > Christophe. That is long story like your patches. IPC pktio needs some shared pool between 2 odp instances to be able to do zero copy packet exchange. First version introduced api flag which said if pool can be shared or not. After that we decided to not do any packet change and make ipc pktio specific only for linux-generic. That is why there is prefix "ipc_" for odp_pktio_open. And it's linux-generic specific pktio. To not touch other external api I put call shm_open() for all pools. And to make it work I do not call shm_open() for scheduler pool on initialization (which is not clear why it has to be exported). so odp app should not call any internal functions. But we can add to README some information about pktio specific things or restrictions. Probably in that case we need to do the same for pool supposed to be shared. Maxim. > > > On 31 May 2016 at 09:29, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 05/31/16 10:19, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > This is OK for a work around. > > Reviewed-by: Petri Savolainen <petri.savolainen@nokia.com > <mailto:petri.savolainen@nokia.com>> > > > But the current pool / IPC implementation should be corrected, > so that API can be used to create pools with the same name on > different instances. > > By definition, two instances must be able to call > > odp_pool_create("foo", ¶ms); > > without additional flags or worries about name space clash. > > > -Petri > > > That is ok, I can add this difference with: > 1) additional parameter to odp_pool_create(). That will require > api change. > 2) bind pool name to specific prefix, like > odp_pool_create("ipc_my_name"). And add note somewhere in readme > that app should name shared pools > with that prefix to use ipc_pktio. > > Maxim. > > > > > > > -----Original Message----- > From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>] > Sent: Tuesday, May 31, 2016 10:02 AM > To: lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > Cc: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>; Savolainen, Petri (Nokia - > FI/Espoo) <petri.savolainen@nokia.com > <mailto:petri.savolainen@nokia.com>> > Subject: [PATCHv3] linux-generic: sched: do not allocate > sheduler info in > shm area > > Patch: > 637a482 linux-generic: schedule: clean interface towards pktio > Broke ipc pktio work. Restore original logic back. Idea > here the following > that 2 processes roll in the same name space instance and > should not > overlap each other shared memory files. There is no reason > in putting > sheduler internal structures to shared memory. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > Cc: Petri Savolainen <petri.savolainen@nokia.com > <mailto:petri.savolainen@nokia.com>> > --- > v3: 2 parametes without ifdefs (Petri) > v2: add ifdefs. > > platform/linux-generic/odp_schedule.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/platform/linux-generic/odp_schedule.c > b/platform/linux- > generic/odp_schedule.c > index 56f40a3..3ac32ef 100644 > --- a/platform/linux-generic/odp_schedule.c > +++ b/platform/linux-generic/odp_schedule.c > @@ -22,6 +22,9 @@ > #include <odp/api/thrmask.h> > #include <odp_config_internal.h> > #include <odp_schedule_internal.h> > +#ifdef _ODP_PKTIO_IPC > +#include <odp_pool_internal.h> > +#endif > > /* Number of priority levels */ > #define NUM_PRIO 8 > @@ -153,7 +156,11 @@ int odp_schedule_init_global(void) > params.buf.num = num_cmd; > params.type = ODP_POOL_BUFFER; > > +#ifdef _ODP_PKTIO_IPC > + pool = _pool_create("odp_sched_pool", ¶ms, 0); > +#else > pool = odp_pool_create("odp_sched_pool", ¶ms); > +#endif > if (pool == ODP_POOL_INVALID) { > ODP_ERR("Schedule init: Pool create > failed.\n"); > return -1; > -- > 2.7.1.250.gff4ea60 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On 31 May 2016 at 04:27, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/31/16 10:38, Christophe Milard wrote: >> >> Why does it have to be visible in the API, Maxim? (even introducing a >> specific prefix is really an API change, I think: Those app that happened to >> use the prefix before will not work as before). >> If IPC need its specific stuff, shouldn't it be using ODP internal >> functions, e.g. _odp_pool_create() wich can do whatever it wants? >> >> Christophe. > > > That is long story like your patches. IPC pktio needs some shared pool > between 2 odp instances to be able to do zero copy packet exchange. First > version introduced api flag which said if pool can be shared or not. After > that we decided to not do any packet change and make ipc pktio specific only > for linux-generic. That is why there is prefix "ipc_" for odp_pktio_open. > And it's linux-generic specific pktio. This is a problem, this feature is not going to be very portable unless this is widely adopted as the solution to IPC, and IPC is something needed presumably between guests as well as between processes. We did agree that we would merge this to see if it met a need, but I think we need to put a boundary on when we decide that it has not been taken up and it needs to be deleted again. Perhaps Zoltan, Nikhil, Forrest, Ivan and Bala you can comment on its use on your platforms ? Mike To not touch other external api I put > call shm_open() for all pools. And to make it work I do not call shm_open() > for scheduler pool on initialization (which is not clear why it has to be > exported). > > so odp app should not call any internal functions. But we can add to README > some information about pktio specific things or restrictions. Probably in > that > case we need to do the same for pool supposed to be shared. > > Maxim. > > >> >> >> On 31 May 2016 at 09:29, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 05/31/16 10:19, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> This is OK for a work around. >> >> Reviewed-by: Petri Savolainen <petri.savolainen@nokia.com >> <mailto:petri.savolainen@nokia.com>> >> >> >> But the current pool / IPC implementation should be corrected, >> so that API can be used to create pools with the same name on >> different instances. >> >> By definition, two instances must be able to call >> >> odp_pool_create("foo", ¶ms); >> >> without additional flags or worries about name space clash. >> >> >> -Petri >> >> >> That is ok, I can add this difference with: >> 1) additional parameter to odp_pool_create(). That will require >> api change. >> 2) bind pool name to specific prefix, like >> odp_pool_create("ipc_my_name"). And add note somewhere in readme >> that app should name shared pools >> with that prefix to use ipc_pktio. >> >> Maxim. >> >> >> >> >> >> >> -----Original Message----- >> From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>] >> Sent: Tuesday, May 31, 2016 10:02 AM >> To: lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> Cc: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>; Savolainen, Petri (Nokia - >> FI/Espoo) <petri.savolainen@nokia.com >> <mailto:petri.savolainen@nokia.com>> >> Subject: [PATCHv3] linux-generic: sched: do not allocate >> sheduler info in >> shm area >> >> Patch: >> 637a482 linux-generic: schedule: clean interface towards pktio >> Broke ipc pktio work. Restore original logic back. Idea >> here the following >> that 2 processes roll in the same name space instance and >> should not >> overlap each other shared memory files. There is no reason >> in putting >> sheduler internal structures to shared memory. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> Cc: Petri Savolainen <petri.savolainen@nokia.com >> <mailto:petri.savolainen@nokia.com>> >> >> --- >> v3: 2 parametes without ifdefs (Petri) >> v2: add ifdefs. >> >> platform/linux-generic/odp_schedule.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/platform/linux-generic/odp_schedule.c >> b/platform/linux- >> generic/odp_schedule.c >> index 56f40a3..3ac32ef 100644 >> --- a/platform/linux-generic/odp_schedule.c >> +++ b/platform/linux-generic/odp_schedule.c >> @@ -22,6 +22,9 @@ >> #include <odp/api/thrmask.h> >> #include <odp_config_internal.h> >> #include <odp_schedule_internal.h> >> +#ifdef _ODP_PKTIO_IPC >> +#include <odp_pool_internal.h> >> +#endif >> >> /* Number of priority levels */ >> #define NUM_PRIO 8 >> @@ -153,7 +156,11 @@ int odp_schedule_init_global(void) >> params.buf.num = num_cmd; >> params.type = ODP_POOL_BUFFER; >> >> +#ifdef _ODP_PKTIO_IPC >> + pool = _pool_create("odp_sched_pool", ¶ms, 0); >> +#else >> pool = odp_pool_create("odp_sched_pool", ¶ms); >> +#endif >> if (pool == ODP_POOL_INVALID) { >> ODP_ERR("Schedule init: Pool create >> failed.\n"); >> return -1; >> -- >> 2.7.1.250.gff4ea60 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c index 56f40a3..3ac32ef 100644 --- a/platform/linux-generic/odp_schedule.c +++ b/platform/linux-generic/odp_schedule.c @@ -22,6 +22,9 @@ #include <odp/api/thrmask.h> #include <odp_config_internal.h> #include <odp_schedule_internal.h> +#ifdef _ODP_PKTIO_IPC +#include <odp_pool_internal.h> +#endif /* Number of priority levels */ #define NUM_PRIO 8 @@ -153,7 +156,11 @@ int odp_schedule_init_global(void) params.buf.num = num_cmd; params.type = ODP_POOL_BUFFER; +#ifdef _ODP_PKTIO_IPC + pool = _pool_create("odp_sched_pool", ¶ms, 0); +#else pool = odp_pool_create("odp_sched_pool", ¶ms); +#endif if (pool == ODP_POOL_INVALID) { ODP_ERR("Schedule init: Pool create failed.\n"); return -1;
Patch: 637a482 linux-generic: schedule: clean interface towards pktio Broke ipc pktio work. Restore original logic back. Idea here the following that 2 processes roll in the same name space instance and should not overlap each other shared memory files. There is no reason in putting sheduler internal structures to shared memory. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> Cc: Petri Savolainen <petri.savolainen@nokia.com> --- v3: 2 parametes without ifdefs (Petri) v2: add ifdefs. platform/linux-generic/odp_schedule.c | 7 +++++++ 1 file changed, 7 insertions(+)