| Message ID | 20210713191514.34840-3-roriorde@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Stop configuring '--socket-mem'/'--socket-limit' by default for DPDK if not requested. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot | success | github build: passed |
Bleep bloop. Greetings Rosemarie O'Riorden, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line has trailing whitespace
#201 FILE: vswitchd/vswitch.xml:367:
DPDK defaults will be used instead. If dpdk-socket-mem and
WARNING: Line is 81 characters long (recommended limit is 79)
#202 FILE: vswitchd/vswitch.xml:368:
dpdk-alloc-mem are specified at same time, dpdk-socket-mem will be used
Lines checked: 220, Warnings: 2, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 13/07/2021 20:15, Rosemarie O'Riorden wrote: > This change removes the default values for EAL args socket-mem and > socket-limit. As DPDK supports dynamic memory allocation, there is no > need to allocate a certain amount of memory on start-up, nor limit the > amount of memory available, if not requested. > > Currently, socket-mem has a default value of 1024 when it is not > configured by the user, and socket-limit takes on the value of socket-mem, > 1024, by default. With this change, socket-mem is not configured by default, > meaning that socket-limit is not either. Neither, either or both options can be set. > > Removed extra logs added in patch 1 that announce this change. > > Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1949850 > Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com> > --- > Removes logs added in patch 1 that were not in v1. > Removes code added to lib/dpdk.c since v1 that conflicts with this patch series. > > Documentation/intro/install/dpdk.rst | 5 +- > NEWS | 4 +- > lib/dpdk.c | 74 +--------------------------- > vswitchd/vswitch.xml | 16 +++--- > 4 files changed, 11 insertions(+), 88 deletions(-) > > diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst > index d8fa931fa..96843af73 100644 > --- a/Documentation/intro/install/dpdk.rst > +++ b/Documentation/intro/install/dpdk.rst > @@ -290,9 +290,8 @@ listed below. Defaults will be provided for all values not explicitly set. > > ``dpdk-socket-mem`` > Comma separated list of memory to pre-allocate from hugepages on specific > - sockets. If not specified, 1024 MB will be set for each numa node by > - default. This behavior will change with the 2.17 release, with no default > - value from OVS. Instead, DPDK default will be used. > + sockets. If not specified, this option will not be set by default. DPDK > + default will be used instead. > > ``dpdk-hugepage-dir`` > Directory where hugetlbfs is mounted > diff --git a/NEWS b/NEWS > index 126f5a927..948f68283 100644 > --- a/NEWS > +++ b/NEWS > @@ -29,8 +29,8 @@ Post-v2.15.0 > Available only if DPDK experimantal APIs enabled during the build. > * Add hardware offload support for VXLAN flows (experimental). > Available only if DPDK experimantal APIs enabled during the build. > - * EAL options --socket-mem and --socket-limit to have default values > - removed with 2.17 release. Logging added to alert users. > + * EAL option --socket-mem is no longer configured by default upon > + start-up. Fine for now, but the NEWS in 2 and 3 will need to rebase to post 2.16 changes when the time is right. > - 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 ed57067ee..3a6990e2f 100644 > --- a/lib/dpdk.c > +++ b/lib/dpdk.c > @@ -130,74 +130,12 @@ construct_dpdk_options(const struct smap *ovs_other_config, struct svec *args) > } > } > > -static int > -compare_numa_node_list(const void *a_, const void *b_) > -{ > - int a = *(const int *) a_; > - int b = *(const int *) b_; > - > - if (a < b) { > - return -1; > - } > - if (a > b) { > - return 1; > - } > - return 0; > -} > - > -static char * > -construct_dpdk_socket_mem(void) > -{ > - const char *def_value = "1024"; > - struct ds dpdk_socket_mem = DS_EMPTY_INITIALIZER; > - > - /* Build a list of all numa nodes with at least one core. */ > - struct ovs_numa_dump *dump = ovs_numa_dump_n_cores_per_numa(1); > - size_t n_numa_nodes = hmap_count(&dump->numas); > - int *numa_node_list = xcalloc(n_numa_nodes, sizeof *numa_node_list); > - > - const struct ovs_numa_info_numa *node; > - int k = 0, last_node = 0; > - > - FOR_EACH_NUMA_ON_DUMP(node, dump) { > - if (k >= n_numa_nodes) { > - break; > - } > - numa_node_list[k++] = node->numa_id; > - } > - qsort(numa_node_list, k, sizeof *numa_node_list, compare_numa_node_list); > - > - for (int i = 0; i < n_numa_nodes; i++) { > - while (numa_node_list[i] > last_node && > - numa_node_list[i] != OVS_NUMA_UNSPEC && > - numa_node_list[i] <= MAX_NUMA_NODES) { > - if (last_node == 0) { > - ds_put_format(&dpdk_socket_mem, "%s", "0"); > - } else { > - ds_put_format(&dpdk_socket_mem, ",%s", "0"); > - } > - last_node++; > - } > - if (numa_node_list[i] == 0) { > - ds_put_format(&dpdk_socket_mem, "%s", def_value); > - } else { > - ds_put_format(&dpdk_socket_mem, ",%s", def_value); > - } > - last_node++; > - } This code had a short stay :'| > - free(numa_node_list); > - ovs_numa_dump_destroy(dump); > - return ds_cstr(&dpdk_socket_mem); > -} > - > #define MAX_DPDK_EXCL_OPTS 10 > > static void > construct_dpdk_mutex_options(const struct smap *ovs_other_config, > struct svec *args) > { > - char *default_dpdk_socket_mem = construct_dpdk_socket_mem(); > - > struct dpdk_exclusive_options_map { > const char *category; > const char *ovs_dpdk_options[MAX_DPDK_EXCL_OPTS]; > @@ -208,7 +146,7 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, > {"memory type", > {"dpdk-alloc-mem", "dpdk-socket-mem", NULL,}, > {"-m", "--socket-mem", NULL,}, > - default_dpdk_socket_mem, 1 > + NULL, 0 > }, > }; > > @@ -217,7 +155,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, > int found_opts = 0, scan, found_pos = -1; > const char *found_value; > struct dpdk_exclusive_options_map *popt = &excl_opts[i]; > - bool using_default = false; > > for (scan = 0; scan < MAX_DPDK_EXCL_OPTS > && popt->ovs_dpdk_options[scan]; ++scan) { > @@ -234,7 +171,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, > if (popt->default_option) { > found_pos = popt->default_option; > found_value = popt->default_value; > - using_default = true; > } else { > continue; > } > @@ -247,12 +183,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, > } > > if (!args_contains(args, popt->eal_dpdk_options[found_pos])) { > - if (using_default) { > - VLOG_INFO("Using default value for '%s'. OVS wil no longer " > - "provide a default for this argument starting from " > - "2.17 release. DPDK defaults will be used instead.", > - popt->eal_dpdk_options[found_pos]); > - } > svec_add(args, popt->eal_dpdk_options[found_pos]); > svec_add(args, found_value); > } else { > @@ -260,8 +190,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, > "dpdk-extra config", popt->eal_dpdk_options[found_pos]); > } > } > - > - free(default_dpdk_socket_mem); > } > > static void > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index c26ebb796..c64be6c22 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -362,13 +362,11 @@ > preallocate 2048MB and socket 3 (no value given) to preallocate 0MB. > </p> > <p> > - If dpdk-socket-mem and dpdk-alloc-mem are not specified, dpdk-socket-mem > - will be used and the default value is 1024 for each numa node. If > - dpdk-socket-mem and dpdk-alloc-mem are specified at same time, > - dpdk-socket-mem will be used as default. With the 2.17 release, > - dpdk-socket-mem will no longer be used by default. DPDK defaults will > - be used instead. > - Changing this value requires restarting the daemon. > + If dpdk-socket-mem and dpdk-alloc-mem are not specified, neither > + will be used and there will be no default value for each numa node. > + DPDK defaults will be used instead. If dpdk-socket-mem and > + dpdk-alloc-mem are specified at same time, dpdk-socket-mem will be used > + as default. Changing this value requires restarting the daemon. > </p> > </column> > > @@ -389,9 +387,7 @@ > <ref column="other_config" key="dpdk-extra"/>. If none of the above > options specified or <code>--legacy-mem</code> provided in > <ref column="other_config" key="dpdk-extra"/>, limits will not be > - applied. > - With the 2.17 release, the OVS default value will no longer be > - provided, and DPDK defaults will be used instead. > + applied. There is no default value from OVS. > Changing this value requires restarting the daemon. > </p> > </column> >
diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst index d8fa931fa..96843af73 100644 --- a/Documentation/intro/install/dpdk.rst +++ b/Documentation/intro/install/dpdk.rst @@ -290,9 +290,8 @@ listed below. Defaults will be provided for all values not explicitly set. ``dpdk-socket-mem`` Comma separated list of memory to pre-allocate from hugepages on specific - sockets. If not specified, 1024 MB will be set for each numa node by - default. This behavior will change with the 2.17 release, with no default - value from OVS. Instead, DPDK default will be used. + sockets. If not specified, this option will not be set by default. DPDK + default will be used instead. ``dpdk-hugepage-dir`` Directory where hugetlbfs is mounted diff --git a/NEWS b/NEWS index 126f5a927..948f68283 100644 --- a/NEWS +++ b/NEWS @@ -29,8 +29,8 @@ Post-v2.15.0 Available only if DPDK experimantal APIs enabled during the build. * Add hardware offload support for VXLAN flows (experimental). Available only if DPDK experimantal APIs enabled during the build. - * EAL options --socket-mem and --socket-limit to have default values - removed with 2.17 release. Logging added to alert users. + * EAL option --socket-mem is no longer configured by default upon + start-up. - 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 ed57067ee..3a6990e2f 100644 --- a/lib/dpdk.c +++ b/lib/dpdk.c @@ -130,74 +130,12 @@ construct_dpdk_options(const struct smap *ovs_other_config, struct svec *args) } } -static int -compare_numa_node_list(const void *a_, const void *b_) -{ - int a = *(const int *) a_; - int b = *(const int *) b_; - - if (a < b) { - return -1; - } - if (a > b) { - return 1; - } - return 0; -} - -static char * -construct_dpdk_socket_mem(void) -{ - const char *def_value = "1024"; - struct ds dpdk_socket_mem = DS_EMPTY_INITIALIZER; - - /* Build a list of all numa nodes with at least one core. */ - struct ovs_numa_dump *dump = ovs_numa_dump_n_cores_per_numa(1); - size_t n_numa_nodes = hmap_count(&dump->numas); - int *numa_node_list = xcalloc(n_numa_nodes, sizeof *numa_node_list); - - const struct ovs_numa_info_numa *node; - int k = 0, last_node = 0; - - FOR_EACH_NUMA_ON_DUMP(node, dump) { - if (k >= n_numa_nodes) { - break; - } - numa_node_list[k++] = node->numa_id; - } - qsort(numa_node_list, k, sizeof *numa_node_list, compare_numa_node_list); - - for (int i = 0; i < n_numa_nodes; i++) { - while (numa_node_list[i] > last_node && - numa_node_list[i] != OVS_NUMA_UNSPEC && - numa_node_list[i] <= MAX_NUMA_NODES) { - if (last_node == 0) { - ds_put_format(&dpdk_socket_mem, "%s", "0"); - } else { - ds_put_format(&dpdk_socket_mem, ",%s", "0"); - } - last_node++; - } - if (numa_node_list[i] == 0) { - ds_put_format(&dpdk_socket_mem, "%s", def_value); - } else { - ds_put_format(&dpdk_socket_mem, ",%s", def_value); - } - last_node++; - } - free(numa_node_list); - ovs_numa_dump_destroy(dump); - return ds_cstr(&dpdk_socket_mem); -} - #define MAX_DPDK_EXCL_OPTS 10 static void construct_dpdk_mutex_options(const struct smap *ovs_other_config, struct svec *args) { - char *default_dpdk_socket_mem = construct_dpdk_socket_mem(); - struct dpdk_exclusive_options_map { const char *category; const char *ovs_dpdk_options[MAX_DPDK_EXCL_OPTS]; @@ -208,7 +146,7 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, {"memory type", {"dpdk-alloc-mem", "dpdk-socket-mem", NULL,}, {"-m", "--socket-mem", NULL,}, - default_dpdk_socket_mem, 1 + NULL, 0 }, }; @@ -217,7 +155,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, int found_opts = 0, scan, found_pos = -1; const char *found_value; struct dpdk_exclusive_options_map *popt = &excl_opts[i]; - bool using_default = false; for (scan = 0; scan < MAX_DPDK_EXCL_OPTS && popt->ovs_dpdk_options[scan]; ++scan) { @@ -234,7 +171,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, if (popt->default_option) { found_pos = popt->default_option; found_value = popt->default_value; - using_default = true; } else { continue; } @@ -247,12 +183,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, } if (!args_contains(args, popt->eal_dpdk_options[found_pos])) { - if (using_default) { - VLOG_INFO("Using default value for '%s'. OVS wil no longer " - "provide a default for this argument starting from " - "2.17 release. DPDK defaults will be used instead.", - popt->eal_dpdk_options[found_pos]); - } svec_add(args, popt->eal_dpdk_options[found_pos]); svec_add(args, found_value); } else { @@ -260,8 +190,6 @@ construct_dpdk_mutex_options(const struct smap *ovs_other_config, "dpdk-extra config", popt->eal_dpdk_options[found_pos]); } } - - free(default_dpdk_socket_mem); } static void diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index c26ebb796..c64be6c22 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -362,13 +362,11 @@ preallocate 2048MB and socket 3 (no value given) to preallocate 0MB. </p> <p> - If dpdk-socket-mem and dpdk-alloc-mem are not specified, dpdk-socket-mem - will be used and the default value is 1024 for each numa node. If - dpdk-socket-mem and dpdk-alloc-mem are specified at same time, - dpdk-socket-mem will be used as default. With the 2.17 release, - dpdk-socket-mem will no longer be used by default. DPDK defaults will - be used instead. - Changing this value requires restarting the daemon. + If dpdk-socket-mem and dpdk-alloc-mem are not specified, neither + will be used and there will be no default value for each numa node. + DPDK defaults will be used instead. If dpdk-socket-mem and + dpdk-alloc-mem are specified at same time, dpdk-socket-mem will be used + as default. Changing this value requires restarting the daemon. </p> </column> @@ -389,9 +387,7 @@ <ref column="other_config" key="dpdk-extra"/>. If none of the above options specified or <code>--legacy-mem</code> provided in <ref column="other_config" key="dpdk-extra"/>, limits will not be - applied. - With the 2.17 release, the OVS default value will no longer be - provided, and DPDK defaults will be used instead. + applied. There is no default value from OVS. Changing this value requires restarting the daemon. </p> </column>
This change removes the default values for EAL args socket-mem and socket-limit. As DPDK supports dynamic memory allocation, there is no need to allocate a certain amount of memory on start-up, nor limit the amount of memory available, if not requested. Currently, socket-mem has a default value of 1024 when it is not configured by the user, and socket-limit takes on the value of socket-mem, 1024, by default. With this change, socket-mem is not configured by default, meaning that socket-limit is not either. Neither, either or both options can be set. Removed extra logs added in patch 1 that announce this change. Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1949850 Signed-off-by: Rosemarie O'Riorden <roriorde@redhat.com> --- Removes logs added in patch 1 that were not in v1. Removes code added to lib/dpdk.c since v1 that conflicts with this patch series. Documentation/intro/install/dpdk.rst | 5 +- NEWS | 4 +- lib/dpdk.c | 74 +--------------------------- vswitchd/vswitch.xml | 16 +++--- 4 files changed, 11 insertions(+), 88 deletions(-)