On 14 May 2015 at 07:29, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 05/14/2015 13:45, Mike Holmes wrote: > >> >> >> On 14 May 2015 at 06:31, Maxim Uvarov <maxim.uva...@linaro.org <mailto: >> 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> >> <mailto: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. >> >> 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.uva...@linaro.org <mailto:maxim.uva...@linaro.org> >> <mailto:maxim.uva...@linaro.org >> <mailto:maxim.uva...@linaro.org>> >> <mailto: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>> >> <mailto: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>> >> <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 >> >> > -- 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
