| Message ID | 1431079069-9702-3-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
I dont think you can implement a mandatory ODP API based on an optional helper api. The ring implementation should be inside the linux-generic implementation if you want to use it, other implementations can copy it as they do now from linux-generic for other features such as timers. Helpers need to be supporting the tests, examples and applications and not the implementations. We have been walking a fuzzy line previously but I think the complexities and coupling get unwound when linux-generic does not rely on or compile in any helper code. Mike On 8 May 2015 at 05:57, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > helper/include/odp/helper/ring.h | 2 ++ > helper/ring.c | 9 ++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/helper/include/odp/helper/ring.h > b/helper/include/odp/helper/ring.h > index 65c32ad..5e640a7 100644 > --- a/helper/include/odp/helper/ring.h > +++ b/helper/include/odp/helper/ring.h > @@ -158,6 +158,8 @@ typedef struct odph_ring { > > #define ODPH_RING_F_SP_ENQ 0x0001 /* The default enqueue is > "single-producer".*/ > #define ODPH_RING_F_SC_DEQ 0x0002 /* The default dequeue is > "single-consumer".*/ > +#define ODPH_RING_SHM_PROC 0x0004 /* If set - ring is visible from > different > + processes. Default is thread visible. > */ > #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota exceed for burst ops */ > #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) /* Ring size mask */ > > diff --git a/helper/ring.c b/helper/ring.c > index a24a020..0927a6c 100644 > --- a/helper/ring.c > +++ b/helper/ring.c > @@ -159,8 +159,14 @@ odph_ring_create(const char *name, unsigned count, > unsigned flags) > char ring_name[ODPH_RING_NAMESIZE]; > odph_ring_t *r; > size_t ring_size; > + uint32_t shm_flag; > odp_shm_t shm; > > + if (flags & ODPH_RING_SHM_PROC) > + shm_flag = ODP_SHM_PROC; > + else > + shm_flag = 0; > + > /* count must be a power of 2 */ > if (!ODP_VAL_IS_POWER_2(count) || (count > ODPH_RING_SZ_MASK)) { > ODP_ERR("Requested size is invalid, must be power of 2, > and do not exceed the size limit %u\n", > @@ -173,7 +179,8 @@ odph_ring_create(const char *name, unsigned count, > unsigned flags) > > odp_rwlock_write_lock(&qlock); > /* reserve a memory zone for this ring.*/ > - shm = odp_shm_reserve(ring_name, ring_size, ODP_CACHE_LINE_SIZE, > 0); > + shm = odp_shm_reserve(ring_name, ring_size, ODP_CACHE_LINE_SIZE, > + shm_flag); > > r = odp_shm_addr(shm); > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 05/12/2015 20:52, Mike Holmes wrote: > I dont think you can implement a mandatory ODP API based on an > optional helper api. > > The ring implementation should be inside the linux-generic > implementation if you want to use it, other implementations can copy > it as they do now from linux-generic for other features such as timers. > > Helpers need to be supporting the tests, examples and applications and > not the implementations. > > We have been walking a fuzzy line previously but I think the > complexities and coupling get unwound when linux-generic does not rely > on or compile in any helper code. > > Mike I think it depends if someone else uses odp ring from helper or not. Ring itself might be useful as middle buffer between processes. Also I just added shm flag which is in our api to have smaller changes. I think later we can decided where it's better to move odp ring. Maxim. > > On 8 May 2015 at 05:57, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > helper/include/odp/helper/ring.h | 2 ++ > helper/ring.c | 9 ++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/helper/include/odp/helper/ring.h > b/helper/include/odp/helper/ring.h > index 65c32ad..5e640a7 100644 > --- a/helper/include/odp/helper/ring.h > +++ b/helper/include/odp/helper/ring.h > @@ -158,6 +158,8 @@ typedef struct odph_ring { > > #define ODPH_RING_F_SP_ENQ 0x0001 /* The default enqueue is > "single-producer".*/ > #define ODPH_RING_F_SC_DEQ 0x0002 /* The default dequeue is > "single-consumer".*/ > +#define ODPH_RING_SHM_PROC 0x0004 /* If set - ring is visible > from different > + processes. Default is thread > visible. */ > #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota exceed for > burst ops */ > #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) /* Ring size > mask */ > > diff --git a/helper/ring.c b/helper/ring.c > index a24a020..0927a6c 100644 > --- a/helper/ring.c > +++ b/helper/ring.c > @@ -159,8 +159,14 @@ odph_ring_create(const char *name, unsigned > count, unsigned flags) > char ring_name[ODPH_RING_NAMESIZE]; > odph_ring_t *r; > size_t ring_size; > + uint32_t shm_flag; > odp_shm_t shm; > > + if (flags & ODPH_RING_SHM_PROC) > + shm_flag = ODP_SHM_PROC; > + else > + shm_flag = 0; > + > /* count must be a power of 2 */ > if (!ODP_VAL_IS_POWER_2(count) || (count > > ODPH_RING_SZ_MASK)) { > ODP_ERR("Requested size is invalid, must be power > of 2, and do not exceed the size limit %u\n", > @@ -173,7 +179,8 @@ odph_ring_create(const char *name, unsigned > count, unsigned flags) > > odp_rwlock_write_lock(&qlock); > /* reserve a memory zone for this ring.*/ > - shm = odp_shm_reserve(ring_name, ring_size, > ODP_CACHE_LINE_SIZE, 0); > + shm = odp_shm_reserve(ring_name, ring_size, > ODP_CACHE_LINE_SIZE, > + shm_flag); > > r = odp_shm_addr(shm); > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs >
On 13 May 2015 at 05:03, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/12/2015 20:52, Mike Holmes wrote: > >> I dont think you can implement a mandatory ODP API based on an optional >> helper api. >> >> The ring implementation should be inside the linux-generic implementation >> if you want to use it, other implementations can copy it as they do now >> from linux-generic for other features such as timers. >> >> Helpers need to be supporting the tests, examples and applications and >> not the implementations. >> >> We have been walking a fuzzy line previously but I think the complexities >> and coupling get unwound when linux-generic does not rely on or compile in >> any helper code. >> >> Mike >> > > I think it depends if someone else uses odp ring from helper or not. Ring > itself might be useful as middle buffer between processes. Also I just > added shm flag which is in our api to have smaller changes. I think later > we can decided where it's better to move odp ring. > I dont agree that we can decide later. The test infrastructure that we decided to punt on to get 1.0 done is proving very difficult to fix now that it has established a pattern of use that is coupled to the platform is awkward ways, lets not do the same with helpers. Basing an implementation on optional code is not very robust, the makefile looks wrong with ../../ (below), we have made helpers not optional in this way. We compile in the now not optional ring.c and we dont test it or use it in any way at all! that can't be correct. __LIB__libodp_la_SOURCES = \ >------->------->------- odp_barrier.c \ >------->------->------- odp_buffer.c \ >------->------->------- odp_classification.c \ >------->------->------- odp_cpumask.c \ >------->------->------- odp_crypto.c \ >------->------->------- odp_errno.c \ >------->------->------- odp_event.c \ >------->------->------- odp_init.c \ >------->------->------- odp_impl.c \ >------->------->------- ../../helper/linux.c \ >------->------->------- odp_packet.c \ >------->------->------- odp_packet_flags.c \ >------->------->------- odp_packet_io.c \ >------->------->------- odp_packet_socket.c \ >------->------->------- odp_pool.c \ >------->------->------- odp_queue.c \ >------->------->------- ../../helper/ring.c \ To share implementations, thus far KS2 and DPDK have their make file include the src from linux-generic as appropriate, so they will reuse ring & IPC from there and all re-use is then uniformly handled. Let start by being clear what the helpers are before we start coupling them even more tightly with IPC. Previously stated and presumed still true is this description "Helpers are there to help tests, examples and applications, not implementations. Helpers are optional for applications" we need to establish this as fact before deciding much more. If helpers are not separate support code from the implementation then we need to state that all implementations should compile them all in so that customers of vendor implementations can run the validation tests. Mike > > Maxim. > > >> On 8 May 2015 at 05:57, Maxim Uvarov <maxim.uvarov@linaro.org <mailto: >> maxim.uvarov@linaro.org>> wrote: >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> >> --- >> helper/include/odp/helper/ring.h | 2 ++ >> helper/ring.c | 9 ++++++++- >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/helper/include/odp/helper/ring.h >> b/helper/include/odp/helper/ring.h >> index 65c32ad..5e640a7 100644 >> --- a/helper/include/odp/helper/ring.h >> +++ b/helper/include/odp/helper/ring.h >> @@ -158,6 +158,8 @@ typedef struct odph_ring { >> >> #define ODPH_RING_F_SP_ENQ 0x0001 /* The default enqueue is >> "single-producer".*/ >> #define ODPH_RING_F_SC_DEQ 0x0002 /* The default dequeue is >> "single-consumer".*/ >> +#define ODPH_RING_SHM_PROC 0x0004 /* If set - ring is visible >> from different >> + processes. Default is thread >> visible. */ >> #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota exceed for >> burst ops */ >> #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) /* Ring size >> mask */ >> >> diff --git a/helper/ring.c b/helper/ring.c >> index a24a020..0927a6c 100644 >> --- a/helper/ring.c >> +++ b/helper/ring.c >> @@ -159,8 +159,14 @@ odph_ring_create(const char *name, unsigned >> count, unsigned flags) >> char ring_name[ODPH_RING_NAMESIZE]; >> odph_ring_t *r; >> size_t ring_size; >> + uint32_t shm_flag; >> odp_shm_t shm; >> >> + if (flags & ODPH_RING_SHM_PROC) >> + shm_flag = ODP_SHM_PROC; >> + else >> + shm_flag = 0; >> + >> /* count must be a power of 2 */ >> if (!ODP_VAL_IS_POWER_2(count) || (count > >> ODPH_RING_SZ_MASK)) { >> ODP_ERR("Requested size is invalid, must be power >> of 2, and do not exceed the size limit %u\n", >> @@ -173,7 +179,8 @@ odph_ring_create(const char *name, unsigned >> count, unsigned flags) >> >> odp_rwlock_write_lock(&qlock); >> /* reserve a memory zone for this ring.*/ >> - shm = odp_shm_reserve(ring_name, ring_size, >> ODP_CACHE_LINE_SIZE, 0); >> + shm = odp_shm_reserve(ring_name, ring_size, >> ODP_CACHE_LINE_SIZE, >> + shm_flag); >> >> r = odp_shm_addr(shm); >> >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> >> >
On 05/14/2015 01:17, Mike Holmes wrote: > > > On 13 May 2015 at 05:03, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 05/12/2015 20:52, Mike Holmes wrote: > > I dont think you can implement a mandatory ODP API based on an > optional helper api. > > The ring implementation should be inside the linux-generic > implementation if you want to use it, other implementations > can copy it as they do now from linux-generic for other > features such as timers. > > Helpers need to be supporting the tests, examples and > applications and not the implementations. > > We have been walking a fuzzy line previously but I think the > complexities and coupling get unwound when linux-generic does > not rely on or compile in any helper code. > > Mike > > > I think it depends if someone else uses odp ring from helper or > not. Ring itself might be useful as middle buffer between > processes. Also I just added shm flag which is in our api to have > smaller changes. I think later we can decided where it's better to > move odp ring. > > > I dont agree that we can decide later. The test infrastructure that > we decided to punt on to get 1.0 done is proving very difficult to fix > now that it has established a pattern of use that is coupled to the > platform is awkward ways, lets not do the same with helpers. > > Basing an implementation on optional code is not very robust, the > makefile looks wrong with ../../ (below) we have the same for linux.c and it works. That looks good for me. Don't know why you don't like ../../ > , we have made helpers not optional in this way. We compile in the now > not optional ring.c and we dont test it or use it in any way at all! > that can't be correct. We compile it and test: ./test/api_test/odp_ring_test.c > > __LIB__libodp_la_SOURCES = \ > >------->------->------- odp_barrier.c \ > >------->------->------- odp_buffer.c \ > >------->------->------- odp_classification.c \ > >------->------->------- odp_cpumask.c \ > >------->------->------- odp_crypto.c \ > >------->------->------- odp_errno.c \ > >------->------->------- odp_event.c \ > >------->------->------- odp_init.c \ > >------->------->------- odp_impl.c \ > >------->------->------- ../../helper/linux.c \ > >------->------->------- odp_packet.c \ > >------->------->------- odp_packet_flags.c \ > >------->------->------- odp_packet_io.c \ > >------->------->------- odp_packet_socket.c \ > >------->------->------- odp_pool.c \ > >------->------->------- odp_queue.c \ > >------->------->------- ../../helper/ring.c \ > > To share implementations, thus far KS2 and DPDK have their make file > include the src from linux-generic as appropriate, so they will reuse > ring & IPC from there and all re-use is then uniformly handled. Both KS2 and DPDK do not need odph_ring and linux-generic IPC for IPC support. IPC there will be implemented in different way. But if needed they can use odph_ring for something else. > Let start by being clear what the helpers are before we start coupling > them even more tightly with IPC. > ok. > Previously stated and presumed still true is this description "Helpers > are there to help tests, examples and applications, not > implementations. Helpers are optional for applications" we need to > establish this as fact before deciding much more. > > If helpers are not separate support code from the implementation then > we need to state that all implementations should compile them all in > so that customers of vendor implementations can run the validation tests. > I think that implementation should not be limitation here. If any implementation wish to use helper it's free to do it. I would say that "Helpers are functions which are optional for applications. That function is very often used in ODP applications but they are not separate ODP API. These functions are not hided inside implementation so that apps can use them directly. " Maxim. > Mike > > > Maxim. > > > On 8 May 2015 at 05:57, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > > --- > helper/include/odp/helper/ring.h | 2 ++ > helper/ring.c | 9 ++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/helper/include/odp/helper/ring.h > b/helper/include/odp/helper/ring.h > index 65c32ad..5e640a7 100644 > --- a/helper/include/odp/helper/ring.h > +++ b/helper/include/odp/helper/ring.h > @@ -158,6 +158,8 @@ typedef struct odph_ring { > > #define ODPH_RING_F_SP_ENQ 0x0001 /* The default enqueue is > "single-producer".*/ > #define ODPH_RING_F_SC_DEQ 0x0002 /* The default dequeue is > "single-consumer".*/ > +#define ODPH_RING_SHM_PROC 0x0004 /* If set - ring is visible > from different > + processes. Default is > thread > visible. */ > #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota exceed for > burst ops */ > #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) /* Ring size > mask */ > > diff --git a/helper/ring.c b/helper/ring.c > index a24a020..0927a6c 100644 > --- a/helper/ring.c > +++ b/helper/ring.c > @@ -159,8 +159,14 @@ odph_ring_create(const char *name, > unsigned > count, unsigned flags) > char ring_name[ODPH_RING_NAMESIZE]; > odph_ring_t *r; > size_t ring_size; > + uint32_t shm_flag; > odp_shm_t shm; > > + if (flags & ODPH_RING_SHM_PROC) > + shm_flag = ODP_SHM_PROC; > + else > + shm_flag = 0; > + > /* count must be a power of 2 */ > if (!ODP_VAL_IS_POWER_2(count) || (count > > ODPH_RING_SZ_MASK)) { > ODP_ERR("Requested size is invalid, must > be power > of 2, and do not exceed the size limit %u\n", > @@ -173,7 +179,8 @@ odph_ring_create(const char *name, > unsigned > count, unsigned flags) > > odp_rwlock_write_lock(&qlock); > /* reserve a memory zone for this ring.*/ > - shm = odp_shm_reserve(ring_name, ring_size, > ODP_CACHE_LINE_SIZE, 0); > + shm = odp_shm_reserve(ring_name, ring_size, > ODP_CACHE_LINE_SIZE, > + shm_flag); > > r = odp_shm_addr(shm); > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software > for ARM SoCs > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs >
On 14 May 2015 at 06:31, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/14/2015 01:17, Mike Holmes wrote: > >> >> >> On 13 May 2015 at 05:03, Maxim Uvarov <maxim.uvarov@linaro.org <mailto: >> maxim.uvarov@linaro.org>> wrote: >> >> On 05/12/2015 20:52, Mike Holmes wrote: >> >> I dont think you can implement a mandatory ODP API based on an >> optional helper api. >> >> The ring implementation should be inside the linux-generic >> implementation if you want to use it, other implementations >> can copy it as they do now from linux-generic for other >> features such as timers. >> >> Helpers need to be supporting the tests, examples and >> applications and not the implementations. >> >> We have been walking a fuzzy line previously but I think the >> complexities and coupling get unwound when linux-generic does >> not rely on or compile in any helper code. >> >> Mike >> >> >> I think it depends if someone else uses odp ring from helper or >> not. Ring itself might be useful as middle buffer between >> processes. Also I just added shm flag which is in our api to have >> smaller changes. I think later we can decided where it's better to >> move odp ring. >> >> >> I dont agree that we can decide later. The test infrastructure that we >> decided to punt on to get 1.0 done is proving very difficult to fix now >> that it has established a pattern of use that is coupled to the platform is >> awkward ways, lets not do the same with helpers. >> >> Basing an implementation on optional code is not very robust, the >> makefile looks wrong with ../../ (below) >> > we have the same for linux.c and it works. That looks good for me. Don't > know why you don't like ../../ I object strongly to linux.c why is that built into the implementation when it is not an ODP API and is not in any way used by the implementation of linux-genric? It is optional support for tests, it makes no sense - this is exactly what I am trying to undo. > > , we have made helpers not optional in this way. We compile in the now >> not optional ring.c and we dont test it or use it in any way at all! that >> can't be correct. >> > > We compile it and test: > ./test/api_test/odp_ring_test.c > >> >> __LIB__libodp_la_SOURCES = \ >> >------->------->------- odp_barrier.c \ >> >------->------->------- odp_buffer.c \ >> >------->------->------- odp_classification.c \ >> >------->------->------- odp_cpumask.c \ >> >------->------->------- odp_crypto.c \ >> >------->------->------- odp_errno.c \ >> >------->------->------- odp_event.c \ >> >------->------->------- odp_init.c \ >> >------->------->------- odp_impl.c \ >> >------->------->------- ../../helper/linux.c \ >> >------->------->------- odp_packet.c \ >> >------->------->------- odp_packet_flags.c \ >> >------->------->------- odp_packet_io.c \ >> >------->------->------- odp_packet_socket.c \ >> >------->------->------- odp_pool.c \ >> >------->------->------- odp_queue.c \ >> >------->------->------- ../../helper/ring.c \ >> >> To share implementations, thus far KS2 and DPDK have their make file >> include the src from linux-generic as appropriate, so they will reuse ring >> & IPC from there and all re-use is then uniformly handled. >> > > Both KS2 and DPDK do not need odph_ring and linux-generic IPC for IPC > support. IPC there will be implemented in different way. But if needed they > can use odph_ring for something else Sure, I have no objection to reusing the code, but I dont agree to doing so by including it from the helper directory. > > > Let start by being clear what the helpers are before we start coupling >> them even more tightly with IPC. >> >> > ok. > > Previously stated and presumed still true is this description "Helpers >> are there to help tests, examples and applications, not implementations. >> Helpers are optional for applications" we need to establish this as fact >> before deciding much more. >> >> If helpers are not separate support code from the implementation then we >> need to state that all implementations should compile them all in so that >> customers of vendor implementations can run the validation tests. >> >> > I think that implementation should not be limitation here. If any > implementation wish to use helper it's free to do it. > I would say that "Helpers are functions which are optional for > applications. That function is very often used in ODP applications but they > are not separate ODP API. If they are not separate why are they not in the ODP API ? We say very clearly that there are no optional odp apis yet here we are coding exactly that so lets add them to the API or not. > These functions are not hided inside implementation so that apps > can use them directly. " > Maxim. > > Mike >> >> >> Maxim. >> >> >> On 8 May 2015 at 05:57, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> >> <mailto:maxim.uvarov@linaro.org>>> >> >> --- >> helper/include/odp/helper/ring.h | 2 ++ >> helper/ring.c | 9 ++++++++- >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/helper/include/odp/helper/ring.h >> b/helper/include/odp/helper/ring.h >> index 65c32ad..5e640a7 100644 >> --- a/helper/include/odp/helper/ring.h >> +++ b/helper/include/odp/helper/ring.h >> @@ -158,6 +158,8 @@ typedef struct odph_ring { >> >> #define ODPH_RING_F_SP_ENQ 0x0001 /* The default enqueue is >> "single-producer".*/ >> #define ODPH_RING_F_SC_DEQ 0x0002 /* The default dequeue is >> "single-consumer".*/ >> +#define ODPH_RING_SHM_PROC 0x0004 /* If set - ring is visible >> from different >> + processes. Default is >> thread >> visible. */ >> #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota exceed for >> burst ops */ >> #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) /* Ring size >> mask */ >> >> diff --git a/helper/ring.c b/helper/ring.c >> index a24a020..0927a6c 100644 >> --- a/helper/ring.c >> +++ b/helper/ring.c >> @@ -159,8 +159,14 @@ odph_ring_create(const char *name, >> unsigned >> count, unsigned flags) >> char ring_name[ODPH_RING_NAMESIZE]; >> odph_ring_t *r; >> size_t ring_size; >> + uint32_t shm_flag; >> odp_shm_t shm; >> >> + if (flags & ODPH_RING_SHM_PROC) >> + shm_flag = ODP_SHM_PROC; >> + else >> + shm_flag = 0; >> + >> /* count must be a power of 2 */ >> if (!ODP_VAL_IS_POWER_2(count) || (count > >> ODPH_RING_SZ_MASK)) { >> ODP_ERR("Requested size is invalid, must >> be power >> of 2, and do not exceed the size limit %u\n", >> @@ -173,7 +179,8 @@ odph_ring_create(const char *name, >> unsigned >> count, unsigned flags) >> >> odp_rwlock_write_lock(&qlock); >> /* reserve a memory zone for this ring.*/ >> - shm = odp_shm_reserve(ring_name, ring_size, >> ODP_CACHE_LINE_SIZE, 0); >> + shm = odp_shm_reserve(ring_name, ring_size, >> ODP_CACHE_LINE_SIZE, >> + shm_flag); >> >> r = odp_shm_addr(shm); >> >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software >> for ARM SoCs >> >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> >> >
On 05/14/2015 13:45, Mike Holmes wrote: > > > On 14 May 2015 at 06:31, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 05/14/2015 01:17, Mike Holmes wrote: > > > > On 13 May 2015 at 05:03, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > On 05/12/2015 20:52, Mike Holmes wrote: > > I dont think you can implement a mandatory ODP API > based on an > optional helper api. > > The ring implementation should be inside the linux-generic > implementation if you want to use it, other > implementations > can copy it as they do now from linux-generic for other > features such as timers. > > Helpers need to be supporting the tests, examples and > applications and not the implementations. > > We have been walking a fuzzy line previously but I > think the > complexities and coupling get unwound when > linux-generic does > not rely on or compile in any helper code. > > Mike > > > I think it depends if someone else uses odp ring from > helper or > not. Ring itself might be useful as middle buffer between > processes. Also I just added shm flag which is in our api > to have > smaller changes. I think later we can decided where it's > better to > move odp ring. > > > I dont agree that we can decide later. The test > infrastructure that we decided to punt on to get 1.0 done is > proving very difficult to fix now that it has established a > pattern of use that is coupled to the platform is awkward > ways, lets not do the same with helpers. > > Basing an implementation on optional code is not very robust, > the makefile looks wrong with ../../ (below) > > we have the same for linux.c and it works. That looks good for me. > Don't know why you don't like ../../ > > > I object strongly to linux.c why is that built into the implementation > when it is not an ODP API and is not in any way used by the > implementation of linux-genric? It is optional support for tests, it > makes no sense - this is exactly what I am trying to undo. > Ok, now looks like I understand. Looks like we wrongly compiled linux.c inside linux-generic implementation. linux-generic does not use any helper/linux.c functions. So that tests have to compile that helper on their build. Send a quick patch for that. > > , we have made helpers not optional in this way. We compile in > the now not optional ring.c and we dont test it or use it in > any way at all! that can't be correct. > > > We compile it and test: > ./test/api_test/odp_ring_test.c > > > __LIB__libodp_la_SOURCES = \ > >------->------->------- odp_barrier.c \ > >------->------->------- odp_buffer.c \ > >------->------->------- odp_classification.c \ > >------->------->------- odp_cpumask.c \ > >------->------->------- odp_crypto.c \ > >------->------->------- odp_errno.c \ > >------->------->------- odp_event.c \ > >------->------->------- odp_init.c \ > >------->------->------- odp_impl.c \ > >------->------->------- ../../helper/linux.c \ > >------->------->------- odp_packet.c \ > >------->------->------- odp_packet_flags.c \ > >------->------->------- odp_packet_io.c \ > >------->------->------- odp_packet_socket.c \ > >------->------->------- odp_pool.c \ > >------->------->------- odp_queue.c \ > >------->------->------- ../../helper/ring.c \ > > To share implementations, thus far KS2 and DPDK have their > make file include the src from linux-generic as appropriate, > so they will reuse ring & IPC from there and all re-use is > then uniformly handled. > > > Both KS2 and DPDK do not need odph_ring and linux-generic IPC for > IPC support. IPC there will be implemented in different way. But > if needed they can use odph_ring for something else > > > Sure, I have no objection to reusing the code, but I dont agree to > doing so by including it from the helper directory. I do not see nothing bad in including them. But that include has to be reasonable. In case of IPC it is reasonable (ring.c). In case of linux.c it's not reasonable due to implementation even do not use that functions. > > > Let start by being clear what the helpers are before we start > coupling them even more tightly with IPC. > > > ok. > > Previously stated and presumed still true is this description > "Helpers are there to help tests, examples and applications, > not implementations. Helpers are optional for applications" > we need to establish this as fact before deciding much more. > > If helpers are not separate support code from the > implementation then we need to state that all implementations > should compile them all in so that customers of vendor > implementations can run the validation tests. > > > I think that implementation should not be limitation here. If any > implementation wish to use helper it's free to do it. > > > I would say that "Helpers are functions which are optional for > applications. That function is very often used in ODP applications > but they are not separate ODP API. > > > If they are not separate why are they not in the ODP API ? We say very > clearly that there are no optional odp apis yet here we are coding > exactly that so lets add them to the API or not. Because they are optional and you can create application without that functions. I.e. use systems one or create your custom function. Most of that function can not be HW offloaded so no need to put them to separate API. > These functions are not hided inside implementation so that apps > can use them directly. " > > > Maxim. > > Mike > > > Maxim. > > > On 8 May 2015 at 05:57, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>>> wrote: > > Signed-off-by: Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>>> > > --- > helper/include/odp/helper/ring.h | 2 ++ > helper/ring.c | 9 ++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/helper/include/odp/helper/ring.h > b/helper/include/odp/helper/ring.h > index 65c32ad..5e640a7 100644 > --- a/helper/include/odp/helper/ring.h > +++ b/helper/include/odp/helper/ring.h > @@ -158,6 +158,8 @@ typedef struct odph_ring { > > #define ODPH_RING_F_SP_ENQ 0x0001 /* The default > enqueue is > "single-producer".*/ > #define ODPH_RING_F_SC_DEQ 0x0002 /* The default > dequeue is > "single-consumer".*/ > +#define ODPH_RING_SHM_PROC 0x0004 /* If set - > ring is visible > from different > + processes. Default is > thread > visible. */ > #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota > exceed for > burst ops */ > #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) > /* Ring size > mask */ > > diff --git a/helper/ring.c b/helper/ring.c > index a24a020..0927a6c 100644 > --- a/helper/ring.c > +++ b/helper/ring.c > @@ -159,8 +159,14 @@ odph_ring_create(const char > *name, > unsigned > count, unsigned flags) > char ring_name[ODPH_RING_NAMESIZE]; > odph_ring_t *r; > size_t ring_size; > + uint32_t shm_flag; > odp_shm_t shm; > > + if (flags & ODPH_RING_SHM_PROC) > + shm_flag = ODP_SHM_PROC; > + else > + shm_flag = 0; > + > /* count must be a power of 2 */ > if (!ODP_VAL_IS_POWER_2(count) || (count > > ODPH_RING_SZ_MASK)) { > ODP_ERR("Requested size is > invalid, must > be power > of 2, and do not exceed the size limit %u\n", > @@ -173,7 +179,8 @@ odph_ring_create(const char *name, > unsigned > count, unsigned flags) > > odp_rwlock_write_lock(&qlock); > /* reserve a memory zone for this ring.*/ > - shm = odp_shm_reserve(ring_name, ring_size, > ODP_CACHE_LINE_SIZE, 0); > + shm = odp_shm_reserve(ring_name, ring_size, > ODP_CACHE_LINE_SIZE, > + shm_flag); > > r = odp_shm_addr(shm); > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>>> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source > software > for ARM SoCs > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software > for ARM SoCs > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs >
On 14 May 2015 at 07:29, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/14/2015 13:45, Mike Holmes wrote: > >> >> >> On 14 May 2015 at 06:31, Maxim Uvarov <maxim.uvarov@linaro.org <mailto: >> maxim.uvarov@linaro.org>> wrote: >> >> On 05/14/2015 01:17, Mike Holmes wrote: >> >> >> >> On 13 May 2015 at 05:03, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> On 05/12/2015 20:52, Mike Holmes wrote: >> >> I dont think you can implement a mandatory ODP API >> based on an >> optional helper api. >> >> The ring implementation should be inside the linux-generic >> implementation if you want to use it, other >> implementations >> can copy it as they do now from linux-generic for other >> features such as timers. >> >> Helpers need to be supporting the tests, examples and >> applications and not the implementations. >> >> We have been walking a fuzzy line previously but I >> think the >> complexities and coupling get unwound when >> linux-generic does >> not rely on or compile in any helper code. >> >> Mike >> >> >> I think it depends if someone else uses odp ring from >> helper or >> not. Ring itself might be useful as middle buffer between >> processes. Also I just added shm flag which is in our api >> to have >> smaller changes. I think later we can decided where it's >> better to >> move odp ring. >> >> >> I dont agree that we can decide later. The test >> infrastructure that we decided to punt on to get 1.0 done is >> proving very difficult to fix now that it has established a >> pattern of use that is coupled to the platform is awkward >> ways, lets not do the same with helpers. >> >> Basing an implementation on optional code is not very robust, >> the makefile looks wrong with ../../ (below) >> >> we have the same for linux.c and it works. That looks good for me. >> Don't know why you don't like ../../ >> >> >> I object strongly to linux.c why is that built into the implementation >> when it is not an ODP API and is not in any way used by the implementation >> of linux-genric? It is optional support for tests, it makes no sense - this >> is exactly what I am trying to undo. >> >> Ok, now looks like I understand. Looks like we wrongly compiled linux.c > inside linux-generic implementation. linux-generic does not use any > helper/linux.c functions. So that tests have to compile that helper on > their build. > Send a quick patch for that. It is not so quick, but yes we need to decouple it. We need to make a lib that the ODP tests can link to, there is already a patch floating to actually test the helpers themselves if it were its own lib. > > > >> , we have made helpers not optional in this way. We compile in >> the now not optional ring.c and we dont test it or use it in >> any way at all! that can't be correct. >> >> >> We compile it and test: >> ./test/api_test/odp_ring_test.c >> >> >> __LIB__libodp_la_SOURCES = \ >> >------->------->------- odp_barrier.c \ >> >------->------->------- odp_buffer.c \ >> >------->------->------- odp_classification.c \ >> >------->------->------- odp_cpumask.c \ >> >------->------->------- odp_crypto.c \ >> >------->------->------- odp_errno.c \ >> >------->------->------- odp_event.c \ >> >------->------->------- odp_init.c \ >> >------->------->------- odp_impl.c \ >> >------->------->------- ../../helper/linux.c \ >> >------->------->------- odp_packet.c \ >> >------->------->------- odp_packet_flags.c \ >> >------->------->------- odp_packet_io.c \ >> >------->------->------- odp_packet_socket.c \ >> >------->------->------- odp_pool.c \ >> >------->------->------- odp_queue.c \ >> >------->------->------- ../../helper/ring.c \ >> >> To share implementations, thus far KS2 and DPDK have their >> make file include the src from linux-generic as appropriate, >> so they will reuse ring & IPC from there and all re-use is >> then uniformly handled. >> >> >> Both KS2 and DPDK do not need odph_ring and linux-generic IPC for >> IPC support. IPC there will be implemented in different way. But >> if needed they can use odph_ring for something else >> >> >> Sure, I have no objection to reusing the code, but I dont agree to doing >> so by including it from the helper directory. >> > > I do not see nothing bad in including them. I think I do - see my thought below. > But that include has to be reasonable. In case of IPC it is reasonable > (ring.c). In case of linux.c it's not reasonable due to implementation even > do not use that functions. I could agree on the dependency if helpers are a separate lib and linux-genric requires it for its IPC implementation but not when the code is directly included. I believe we need a clear distinction between helpers and implementation code so that we promote helpers being a reusable support library and not an ad hock source code sharing mechanism. If you build an app that uses rings and run it on platform x that does not build in ring.c for its IPC, then you have to add it at compile time. Now you move to linux-generic and it is built in, do you add compiler flags to compile it in optionally for the app when changing platforms ? You can hope the linker gets the version you want, but what happens if the app is built with one version of ring.c and the implementation used another - you may get the wrong one, I think it complicates platform independence. This problem will only get worse as more helpers are added to increase the level of abstraction ODP can offer. Helpers need to be a library, and I believe linux-generic should not depend on including the code directly in its implementation, but it could depend on that lib. > > > >> >> Let start by being clear what the helpers are before we start >> coupling them even more tightly with IPC. >> >> >> ok. >> >> Previously stated and presumed still true is this description >> "Helpers are there to help tests, examples and applications, >> not implementations. Helpers are optional for applications" >> we need to establish this as fact before deciding much more. >> >> If helpers are not separate support code from the >> implementation then we need to state that all implementations >> should compile them all in so that customers of vendor >> implementations can run the validation tests. >> >> >> I think that implementation should not be limitation here. If any >> implementation wish to use helper it's free to do it. >> >> I would say that "Helpers are functions which are optional for >> applications. That function is very often used in ODP applications >> but they are not separate ODP API. >> >> If they are not separate why are they not in the ODP API ? We say very >> clearly that there are no optional odp apis yet here we are coding exactly >> that so lets add them to the API or not. >> > Because they are optional and you can create application without that > functions. I.e. use systems one or create your custom function. Most of > that function can not be HW offloaded so no need > to put them to separate API. > > > > These functions are not hided inside implementation so that apps >> can use them directly. " >> >> >> Maxim. >> >> Mike >> >> >> Maxim. >> >> >> On 8 May 2015 at 05:57, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>>> wrote: >> >> Signed-off-by: Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>>> >> >> --- >> helper/include/odp/helper/ring.h | 2 ++ >> helper/ring.c | 9 ++++++++- >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/helper/include/odp/helper/ring.h >> b/helper/include/odp/helper/ring.h >> index 65c32ad..5e640a7 100644 >> --- a/helper/include/odp/helper/ring.h >> +++ b/helper/include/odp/helper/ring.h >> @@ -158,6 +158,8 @@ typedef struct odph_ring { >> >> #define ODPH_RING_F_SP_ENQ 0x0001 /* The default >> enqueue is >> "single-producer".*/ >> #define ODPH_RING_F_SC_DEQ 0x0002 /* The default >> dequeue is >> "single-consumer".*/ >> +#define ODPH_RING_SHM_PROC 0x0004 /* If set - >> ring is visible >> from different >> + processes. Default is >> thread >> visible. */ >> #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota >> exceed for >> burst ops */ >> #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) >> /* Ring size >> mask */ >> >> diff --git a/helper/ring.c b/helper/ring.c >> index a24a020..0927a6c 100644 >> --- a/helper/ring.c >> +++ b/helper/ring.c >> @@ -159,8 +159,14 @@ odph_ring_create(const char >> *name, >> unsigned >> count, unsigned flags) >> char ring_name[ODPH_RING_NAMESIZE]; >> odph_ring_t *r; >> size_t ring_size; >> + uint32_t shm_flag; >> odp_shm_t shm; >> >> + if (flags & ODPH_RING_SHM_PROC) >> + shm_flag = ODP_SHM_PROC; >> + else >> + shm_flag = 0; >> + >> /* count must be a power of 2 */ >> if (!ODP_VAL_IS_POWER_2(count) || (count > >> ODPH_RING_SZ_MASK)) { >> ODP_ERR("Requested size is >> invalid, must >> be power >> of 2, and do not exceed the size limit %u\n", >> @@ -173,7 +179,8 @@ odph_ring_create(const char *name, >> unsigned >> count, unsigned flags) >> >> odp_rwlock_write_lock(&qlock); >> /* reserve a memory zone for this ring.*/ >> - shm = odp_shm_reserve(ring_name, ring_size, >> ODP_CACHE_LINE_SIZE, 0); >> + shm = odp_shm_reserve(ring_name, ring_size, >> ODP_CACHE_LINE_SIZE, >> + shm_flag); >> >> r = odp_shm_addr(shm); >> >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>>> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source >> software >> for ARM SoCs >> >> >> >> >> >> -- Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software >> for ARM SoCs >> >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> >> >
diff --git a/helper/include/odp/helper/ring.h b/helper/include/odp/helper/ring.h index 65c32ad..5e640a7 100644 --- a/helper/include/odp/helper/ring.h +++ b/helper/include/odp/helper/ring.h @@ -158,6 +158,8 @@ typedef struct odph_ring { #define ODPH_RING_F_SP_ENQ 0x0001 /* The default enqueue is "single-producer".*/ #define ODPH_RING_F_SC_DEQ 0x0002 /* The default dequeue is "single-consumer".*/ +#define ODPH_RING_SHM_PROC 0x0004 /* If set - ring is visible from different + processes. Default is thread visible. */ #define ODPH_RING_QUOT_EXCEED (1 << 31) /* Quota exceed for burst ops */ #define ODPH_RING_SZ_MASK (unsigned)(0x0fffffff) /* Ring size mask */ diff --git a/helper/ring.c b/helper/ring.c index a24a020..0927a6c 100644 --- a/helper/ring.c +++ b/helper/ring.c @@ -159,8 +159,14 @@ odph_ring_create(const char *name, unsigned count, unsigned flags) char ring_name[ODPH_RING_NAMESIZE]; odph_ring_t *r; size_t ring_size; + uint32_t shm_flag; odp_shm_t shm; + if (flags & ODPH_RING_SHM_PROC) + shm_flag = ODP_SHM_PROC; + else + shm_flag = 0; + /* count must be a power of 2 */ if (!ODP_VAL_IS_POWER_2(count) || (count > ODPH_RING_SZ_MASK)) { ODP_ERR("Requested size is invalid, must be power of 2, and do not exceed the size limit %u\n", @@ -173,7 +179,8 @@ odph_ring_create(const char *name, unsigned count, unsigned flags) odp_rwlock_write_lock(&qlock); /* reserve a memory zone for this ring.*/ - shm = odp_shm_reserve(ring_name, ring_size, ODP_CACHE_LINE_SIZE, 0); + shm = odp_shm_reserve(ring_name, ring_size, ODP_CACHE_LINE_SIZE, + shm_flag); r = odp_shm_addr(shm);
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- helper/include/odp/helper/ring.h | 2 ++ helper/ring.c | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-)