On 12 June 2015 at 14:58, Mike Holmes <[email protected]> wrote:
> > > On 12 June 2015 at 04:43, Maxim Uvarov <[email protected]> wrote: > >> Egress also uses odp_ring. >> >> For odp_ring I do not see anything bad to have it in helper. So >> applications can reuse that software ring if needed. Which might be very >> useful in number of cases. If we put in inside implementation then >> obviously it will be hard to reuse it. >> > > It is bad becasue it forces the implementation to define internal > functions, the implementation should only need to support the public API > > As above " The idea of a helper is that it does (or may do) a series of > ODP API calls exactly the same way the application would do those." So we > have clarified that public calls are not allowed to be called before the > API is initialized, and that internal calls are not allowed. > > Thus in the ring spin case it is a violation since it calls internal code, > from the definition in the file odp_spin_internal.h :- > > /** > * Spin loop for ODP internal use > */ > static inline void odp_spin(void) > > > Do we want to copy the spin (pause ) code, make the pause a public helper > or move ring into linux-generic ? > In this specific instance, this is such a small uncomplicated piece of code so we can copy it into the helper code. > > > >> >> Maxim. >> >> On 06/12/15 11:26, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >>> >>> Yes, helper API and implementation should be cleaned from all internals. >>> >>> E.g. these must be removed from helper/linux.c: >>> >>> #include <odp_internal.h> >>> >>> #include <odp/thread.h> >>> >>> #include <odp/init.h> >>> >>> #include <odp/system_info.h> >>> >>> #include <odp_debug_internal.h> >>> >>> “This is compounded by the notion that helpers should generally be >>> callable before an odp_init call, …” >>> >>> It’s actually the opposite, helpers can be called _/only after/_ >>> odp_init_global() + odp_init_local(). The idea of a helper is that it does >>> (or may do) a series of ODP API calls exactly the same way the application >>> would do those. For example, odph_linux_pthread_create helps the user to >>> create and pin a number of threads to cpus, while doing that it does couple >>> of ODP API calls, which require that ODP has been successfully initialized. >>> >>> -Petri >>> >>> int odph_linux_pthread_create(odph_linux_pthread_t *thread_tbl, >>> >>> const odp_cpumask_t *mask_in, >>> >>> void *(*start_routine) (void *), void *arg) >>> >>> { >>> >>> int i; >>> >>> int num; >>> >>> odp_cpumask_t mask; >>> >>> int cpu_count; >>> >>> int cpu; >>> >>> int ret; >>> >>> *odp_cpumask_copy*(&mask, mask_in); >>> >>> num = odp_cpumask_count(&mask); >>> >>> memset(thread_tbl, 0, num * sizeof(odph_linux_pthread_t)); >>> >>> cpu_count = *odp_cpu_count*(); >>> >>> if (num < 1 || num > cpu_count) { >>> >>> ODP_ERR("Invalid number of threads: %d (%d cores available)\n", >>> >>> num, cpu_count); >>> >>> return 0; >>> >>> } >>> >>> cpu = *odp_cpumask_first*(&mask); >>> >>> for (i = 0; i < num; i++) { >>> >>> odp_cpumask_t thd_mask; >>> >>> *odp_cpumask_zero*(&thd_mask); >>> >>> *odp_cpumask_set*(&thd_mask, cpu); >>> >>> pthread_attr_init(&thread_tbl[i].attr); >>> >>> thread_tbl[i].cpu = cpu; >>> >>> pthread_attr_setaffinity_np(&thread_tbl[i].attr, >>> >>> sizeof(cpu_set_t), &thd_mask.set); >>> >>> … >>> >>> *From:*ext Mike Holmes [mailto:[email protected]] >>> *Sent:* Thursday, June 11, 2015 11:31 PM >>> *To:* Maxim Uvarov; Petri Savolainen >>> *Cc:* lng-odp >>> *Subject:* RFC move the ring implementation into linux-generic >>> >>> ODP helper should be cleaned up so that it only depends on the ODP >>> public API and does not require implementations to provide internal >>> platform support for the helpers. This is compounded by the notion that >>> helpers should generally be callable before an odp_init call, indeed you >>> might expect to be able to use odph_rings without calling any ODP_API. >>> Neither of these goals if met currently. >>> >>> Assuming that helpers should not use implementation internals we need to >>> remove the following internal includes from the helpers :- >>> >>> odp_spin_internal.h >>> >>> odp_internal.h >>> >>> odp_debug_internal.h >>> >>> odp_align_internal.h >>> >>> I tried to clean this up and was successful with minimal change except >>> that the ring helper implementation calls odp_spin() which is a more >>> complex internal function of linux-generic. >>> >>> Given that Maxim wants to implement linux-generic PKTIO- IPC on the ring >>> implementation that makes a mess:- >>> >>> Linux-generic will now depend on an odp helper that depends on the >>> linux-generic internal implementation which is a bit of a loop. >>> >>> In the case of linux-generic it would make sense to make the ring >>> functionality part of the internal implementation and drop it as a helper. >>> Currently no test or application uses the ring helper so there would be no >>> impact at all to this move. >>> >>> Since the ring is not a HW abstraction its API does not belong in the >>> ODP API in my opinion, so I would keep the ring as an internal API of the >>> linux-generic implementation. Derived works such as ODP-DPDK, ODP-NETMAP >>> and ODP-KS2 can of course use the existing mechanisms to reuse the >>> internal ring code for their implementations. >>> >>> Summary: >>> >>> As part of the decoupling I propose moving helper/ring.c -> >>> platform/linux-generic/ring.c >>> >>> -- >>> >>> 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 > [email protected] > https://lists.linaro.org/mailman/listinfo/lng-odp > >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
