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 ?
>
> 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
