[ovs-dev,v2] dpdk: Use --in-memory by default.

From: Rosemarie O'Riorden <roriorde@redhat.com>

If anonymous memory mapping is supported by the kernel, it's better
to run OVS entirely in memory rather than creating shared data
structures. OVS doesn't work in multi-process mode, so there is no need
to litter a filesystem.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com>
---

Version 2:
 - Adjusted the commit message as per David's comments.
 - Restored original authorship (I hope that git send-email tracked that
   correctly as I didn't re-send someone else's patches for a while).
 - Minor rebase, typo fix.
 - Re-sending to remind others about the patch and address all the
   minor comments, so it's in a good shape.

 NEWS         | 1 +
 acinclude.m4 | 6 ++++++
 lib/dpdk.c   | 7 +++++++
 3 files changed, 14 insertions(+)

> From: Rosemarie O'Riorden <roriorde@redhat.com>
> 
> If anonymous memory mapping is supported by the kernel, it's better
> to run OVS entirely in memory rather than creating shared data
> structures. OVS doesn't work in multi-process mode, so there is no need
> to litter a filesystem.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
> Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com>

Thanks for taking care of this Ilya, I validated the v1 a few weeks back, not much has changed here so happy to ack.

Should there be a note added to the docs about running OVS DPDK  in the legacy memory mode? Just thinking is this a noticeable (ideally not) to then end user which they may want to avoid?

Thanks
Ian

> ---
> 
> Version 2:
>  - Adjusted the commit message as per David's comments.
>  - Restored original authorship (I hope that git send-email tracked that
>    correctly as I didn't re-send someone else's patches for a while).
>  - Minor rebase, typo fix.
>  - Re-sending to remind others about the patch and address all the
>    minor comments, so it's in a good shape.
> 
>  NEWS         | 1 +
>  acinclude.m4 | 6 ++++++
>  lib/dpdk.c   | 7 +++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/NEWS b/NEWS
> index 2a4856c1a..c47a6be50 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,7 @@ Post-v2.16.0
>         by default.  'other_config:dpdk-socket-limit' can be set equal to
>         the 'other_config:dpdk-socket-mem' to preserve the legacy memory
>         limiting behavior.
> +     * EAL argument --in-memory is applied by default if supported.
>       * Add hardware offload support for matching IPv4/IPv6 frag types
>         (experimental).
>       * Add support for DPDK 21.11.
> diff --git a/acinclude.m4 b/acinclude.m4
> index 8ab690f47..23cd6df44 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -472,6 +472,12 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>        ], [[#include <rte_config.h>]])
>      ], [], [[#include <rte_config.h>]])
> 
> +    AC_CHECK_DECL([MAP_HUGE_SHIFT], [
> +      AC_DEFINE([DPDK_IN_MEMORY_SUPPORTED], [1], [If MAP_HUGE_SHIFT is
> +                 defined, anonymous memory mapping is supported by the
> +                 kernel, and --in-memory can be used.])
> +    ], [], [[#include <sys/mman.h>]])
> +
>      # DPDK uses dlopen to load plugins.
>      OVS_FIND_DEPENDENCY([dlopen], [dl], [libdl])
> 
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index b2ef31cd2..6cdd69bd2 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -405,6 +405,13 @@ dpdk_init__(const struct smap *ovs_other_config)
>      svec_add(&args, ovs_get_program_name());
>      construct_dpdk_args(ovs_other_config, &args);
> 
> +#ifdef DPDK_IN_MEMORY_SUPPORTED
> +    if (!args_contains(&args, "--in-memory") &&
> +            !args_contains(&args, "--legacy-mem")) {
> +        svec_add(&args, "--in-memory");
> +    }
> +#endif
> +
>      if (args_contains(&args, "-c") || args_contains(&args, "-l")) {
>          auto_determine = false;
>      }
> --
> 2.31.1

On Wed, Dec 15, 2021 at 2:25 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> From: Rosemarie O'Riorden <roriorde@redhat.com>
>
> If anonymous memory mapping is supported by the kernel, it's better
> to run OVS entirely in memory rather than creating shared data
> structures. OVS doesn't work in multi-process mode, so there is no need
> to litter a filesystem.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
> Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com>
> ---
>
> Version 2:
>  - Adjusted the commit message as per David's comments.
>  - Restored original authorship (I hope that git send-email tracked that
>    correctly as I didn't re-send someone else's patches for a while).
>  - Minor rebase, typo fix.
>  - Re-sending to remind others about the patch and address all the
>    minor comments, so it's in a good shape.

Acked-by: David Marchand <david.marchand@redhat.com>

On 12/15/21 16:13, Stokes, Ian wrote:
>> From: Rosemarie O'Riorden <roriorde@redhat.com>
>>
>> If anonymous memory mapping is supported by the kernel, it's better
>> to run OVS entirely in memory rather than creating shared data
>> structures. OVS doesn't work in multi-process mode, so there is no need
>> to litter a filesystem.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
>> Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com>
> 
> Thanks for taking care of this Ilya, I validated the v1 a few weeks back, not much has changed here so happy to ack.
> 
> Should there be a note added to the docs about running OVS DPDK  in
> the legacy memory mode? Just thinking is this a noticeable (ideally not)
> to then end user which they may want to avoid?

If someone already runs in a legacy mode, they will not notice the
change.  If someone doesn't use the legacy mode, there should not be
the case where they might want to do that.  The last reason I can
think of is multiprocessing, but we can not guarantee the correct
work of OVS in that mode.  And, I guess, we're also moving in
direction to just explicitly disable the multiprocessing to allow
using high-numbered CPU cores:
  https://patchwork.ozlabs.org/project/openvswitch/patch/20211110165341.6450-1-david.marchand@redhat.com/

In general, I'd vote to intentionally not document this case.

What do you think?

> On 12/15/21 16:13, Stokes, Ian wrote:
> >> From: Rosemarie O'Riorden <roriorde@redhat.com>
> >>
> >> If anonymous memory mapping is supported by the kernel, it's better
> >> to run OVS entirely in memory rather than creating shared data
> >> structures. OVS doesn't work in multi-process mode, so there is no need
> >> to litter a filesystem.
> >>
> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
> >> Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com>
> >
> > Thanks for taking care of this Ilya, I validated the v1 a few weeks back, not
> much has changed here so happy to ack.
> >
> > Should there be a note added to the docs about running OVS DPDK  in
> > the legacy memory mode? Just thinking is this a noticeable (ideally not)
> > to then end user which they may want to avoid?
> 
> If someone already runs in a legacy mode, they will not notice the
> change.  If someone doesn't use the legacy mode, there should not be
> the case where they might want to do that.  The last reason I can
> think of is multiprocessing, but we can not guarantee the correct
> work of OVS in that mode.  And, I guess, we're also moving in
> direction to just explicitly disable the multiprocessing to allow
> using high-numbered CPU cores:
> 
> https://patchwork.ozlabs.org/project/openvswitch/patch/20211110165341.645
> 0-1-david.marchand@redhat.com/
> 
> In general, I'd vote to intentionally not document this case.

That’s a fair point, if it's part of their configuration to be in legacy mode with a dpdk argument they shouldn't see an issue.

Also it's probably not OVS place to give such low level details on how to provide the dpdk arguments. 

It's documented in the NEWS already as well of the change. So that’s ok I think.

Acked.

Thanks
Ian
> 
> What do you think?

On 12/15/21 17:49, Stokes, Ian wrote:
>> On 12/15/21 16:13, Stokes, Ian wrote:
>>>> From: Rosemarie O'Riorden <roriorde@redhat.com>
>>>>
>>>> If anonymous memory mapping is supported by the kernel, it's better
>>>> to run OVS entirely in memory rather than creating shared data
>>>> structures. OVS doesn't work in multi-process mode, so there is no need
>>>> to litter a filesystem.
>>>>
>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
>>>> Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com>
>>>
>>> Thanks for taking care of this Ilya, I validated the v1 a few weeks back, not
>> much has changed here so happy to ack.
>>>
>>> Should there be a note added to the docs about running OVS DPDK  in
>>> the legacy memory mode? Just thinking is this a noticeable (ideally not)
>>> to then end user which they may want to avoid?
>>
>> If someone already runs in a legacy mode, they will not notice the
>> change.  If someone doesn't use the legacy mode, there should not be
>> the case where they might want to do that.  The last reason I can
>> think of is multiprocessing, but we can not guarantee the correct
>> work of OVS in that mode.  And, I guess, we're also moving in
>> direction to just explicitly disable the multiprocessing to allow
>> using high-numbered CPU cores:
>>
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20211110165341.645
>> 0-1-david.marchand@redhat.com/
>>
>> In general, I'd vote to intentionally not document this case.
> 
> That’s a fair point, if it's part of their configuration to be in legacy mode with a dpdk argument they shouldn't see an issue.
> 
> Also it's probably not OVS place to give such low level details on how to provide the dpdk arguments. 
> 
> It's documented in the NEWS already as well of the change. So that’s ok I think.
> 
> Acked.


Thanks, Ian and David!  Applied.

Best regards, Ilya Maximets.

> 
> Thanks
> Ian
>>
>> What do you think?