Re: [dpdk-dev] [PATCH v6 00/10] Register non-EAL threads as lcore

[David Marchand <david.marchand@redhat.com>]

On Tue, Jul 7, 2020 at 1:23 AM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
> > OVS and some other applications have been hacking into DPDK internals to
> > fake EAL threads and avoid performance penalty of only having non-EAL
> > threads.
> >
> > This series proposes to add a new type of lcores and maps those threads
> > to such lcores.
> > non-EAL threads won't run the DPDK eal mainloop.
> > As a consequence, part of the EAL threads API cannot work.
> >
> > Having new lcores appearing during the process lifetime is not expected
> > by some DPDK components. This is addressed by introducing init/uninit
> > callacks invoked when hotplugging of such lcore.
> >
> > There is still some work/discussion:
> > - refuse new lcore role in incompatible EAL threads API (or document it
> >   only as those API were already incompatible?),
> > - think about deprecation notices for existing RTE_FOREACH_LCORE macros
> >   and consorts, it is probably worth discussing on how to iterate over
> >   lcores,
> >
> > For the interested parties, I have a patch [1] against dpdk-latest OVS
> > branch that makes use of this series (this patch probably won't work with
> > v5, it will be rebased once dpdk side is ready).
> >
> > 1: https://patchwork.ozlabs.org/project/openvswitch/patch/20200626123017.28555-1-david.marchand@redhat.com/
> >
> > Changes since v5:
> > - fixed windows build,
> >
> > Changes since v4:
> > - added separate API to control mp feature activation,
> > - addressed Konstantin and Olivier comments,
> >
> > Changes since v3:
> > - added init failure when trying to use in conjunction with multiprocess,
> > - addressed Andrew comments,
> >
> > Changes since v2:
> > - fixed windows build error due to missing trace stub,
> > - fixed bug when rolling back on lcore register,
> >
> > Changes since v1:
> > - rebased on master (conflicts on merged Windows series),
> > - separated lcore role code cleanup in a patch,
> > - tried to use a single naming, so kept non-EAL threads as the main
> >   notion. non-EAL threads are then distinguished between registered and
> >   unregistered non-EAL threads,
> > - added unit tests (still missing some coverage, marked with a FIXME),
> > - reworked callbacks call under a common rwlock lock which protects
> >   lcores allocations and callbacks registration,
> > - introduced lcore iterators and converted the bucket mempool driver,
> >
>
> LGTM, just 2 nits see below.
> Apart from that:
> Series Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Thanks for the review.


>
> 1.
> +void
> +rte_lcore_callback_unregister(void *handle)
> +{
> +       struct rte_config *cfg = rte_eal_get_configuration();
> +       struct lcore_callback *callback = handle;
> +       unsigned int lcore_id;
>
> Seems like forgot to add formal parameter check here:
> if (callback == NULL) ...

Indeed, fixed.


>
> +
> +       rte_rwlock_write_lock(&lcore_lock);
> +       if (callback->uninit == NULL)
>
> 2.
>
> +bool
> +rte_mp_disable(void)
> +{
> +       return set_mp_status(MP_STATUS_DISABLED);
> +}
>
> Probably name it rte_eal_multiprocess_enable (or so)
> to  make it clear from naming and follow
> more closely our own name convention.
>

Apis for mp have the rte_mp_ prefix.

lib/librte_eal/rte_eal_version.map:     rte_mp_action_register;
lib/librte_eal/rte_eal_version.map:     rte_mp_action_unregister;
lib/librte_eal/rte_eal_version.map:     rte_mp_reply;
lib/librte_eal/rte_eal_version.map:     rte_mp_sendmsg;
lib/librte_eal/rte_eal_version.map:     rte_mp_request_async;
lib/librte_eal/rte_eal_version.map:     rte_mp_request_sync;

I prefer to stick to it.


> +
> +bool
> +eal_enable_multiprocess(void)
> +{
> +       return set_mp_status(MP_STATUS_ENABLED);
> +}

I will go with __rte_mp_ to indicate the private aspect.


>
> Might be worth to make that function public too.
> Then user will have a proper pair to use:
> rte_eal_multiprocess_(enable|disable).
>

I don't see the need for now.
If you feel strong about it, I can send a followup patch later.


Passed the checks again and pushed to the main branch.


-- 
David Marchand