On 14 May 2015 at 06:31, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 05/14/2015 01:17, Mike Holmes wrote: > >> >> >> On 13 May 2015 at 05:03, Maxim Uvarov <maxim.uva...@linaro.org <mailto: >> maxim.uva...@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.uva...@linaro.org >> <mailto:maxim.uva...@linaro.org> >> <mailto:maxim.uva...@linaro.org >> <mailto:maxim.uva...@linaro.org>>> wrote: >> >> Signed-off-by: Maxim Uvarov <maxim.uva...@linaro.org >> <mailto:maxim.uva...@linaro.org> >> <mailto:maxim.uva...@linaro.org >> >> <mailto:maxim.uva...@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 >> >> > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
