[ovs-dev] [PATCH v2 3/3] dpdk: Stop configuring socket-limit with the value of socket-mem.

[Kevin Traynor]

On 13/07/2021 20:15, Rosemarie O'Riorden wrote:
> This change removes the automatic memory limit on start-up of OVS with
> DPDK. As DPDK supports dynamic memory allocation, there is no
> need to limit the amount of memory available, if not requested.
> 
> Currently, if socket-limit is not configured, it is set to the value of
> socket-mem. With this change, the user can decide to set it or have no
> memory limit.
> 
> Removed logs added in patch 1 that announce this change.
> 

Can drop the reference to 'patch 1'. When the time is right you can add
the commit as a reference.

> Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1949850
> Signed-off-by: Rosemarie O'Riorden <roriorde at redhat.com>
> ---
> Removes logs added in patch 1 that were not in v1.
> 
>  NEWS                 | 2 ++
>  lib/dpdk.c           | 7 -------
>  vswitchd/vswitch.xml | 6 +++---
>  3 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 948f68283..99b8b9fce 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -31,6 +31,8 @@ Post-v2.15.0
>         Available only if DPDK experimantal APIs enabled during the build.
>       * EAL option --socket-mem is no longer configured by default upon
>         start-up.
> +     * EAL option --socket-limit no longer takes on the value of --socket-mem
> +       by default.
>     - ovsdb-tool:
>       * New option '--election-timer' to the 'create-cluster' command to set the
>         leader election timer during cluster creation.
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 3a6990e2f..266ef20dc 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -415,13 +415,6 @@ dpdk_init__(const struct smap *ovs_other_config)
>                  break;
>              }
>          }
> -        if (i < args.n - 1) {
> -            svec_add(&args, "--socket-limit");
> -            svec_add(&args, args.names[i + 1]);
> -            VLOG_INFO("Using default value for '--socket-limit. OVS will no "
> -                      "longer provide a default for this argument starting "
> -                      "from 2.17 release. DPDK defaults will be used instead.");
> -        }

Looks like the entire 'if (!args_contains(&args, "--legacy-mem")..'
block can be removed as it is not serving a purpose anymore.

>      }
>  
>      if (args_contains(&args, "-c") || args_contains(&args, "-l")) {
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index c64be6c22..c8d61332d 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -381,11 +381,11 @@
>            <code>0</code> will disable the limit for a particular socket.
>          </p>
>          <p>
> -          If not specified, OVS will configure limits equal to the amount of
> -          preallocated memory specified by <ref column="other_config"
> +          If not specified, OVS will not configure limits by default.
> +          Limits can be configured with <ref column="other_config"
>            key="dpdk-socket-mem"/> or <code>--socket-mem</code> in
>            <ref column="other_config" key="dpdk-extra"/>.If none of the above
> -          options specified or <code>--legacy-mem</code> provided in
> +          options are specified or <code>--legacy-mem</code> is provided in
>            <ref column="other_config" key="dpdk-extra"/>, limits will not be
>            applied.

It's not caused by your patch, but this sentence is unclear. It seems to
be referring to 'default limits' as opposed 'user set limits'.

As there is no default OVS set limits in any circumstance with your
patch, I think the sentence can be removed.

> There is no default value from OVS.
>            Changing this value requires restarting the daemon.
>