[v6,00/10] Register non-EAL threads as lcore

  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,
  

Hi David,
 
> 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>

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) ... 

+
+       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.

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

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

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.
  

On Mon, Jul 6, 2020 at 10:53 PM David Marchand
<david.marchand@redhat.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/

Series applied with last comments from Konstantin.