Hi Ilya,
Thanks for the patch and I think we knew about that pain when we exposed the very first parameter. I still remember Aaron struggling to find the best path forward. Time flies and here we are. The problem is that this change is not friendly from the community perspective to its users. That is like an exposed API which we should avoid break as much as possible. For instance, there are users (OpenStack) with support life cycle of 5+ years that cannot keep up with this kind of change but they expect to be able to update to newer OVS. One idea is to freeze whatever we have now and update the documentation to recommend the new way. We give like a couple OVS releases and only then ignore (or remove?) those parameters. IMO in the end we are moving the problem from one place to another because even with a single string, OVS users will be caught off guard when DPDK changes. Yes, less pain to OVS community in the sense that we don't have to add/remove/deprecate stuff, but it is still a bad user experience regardless, which is not what OVS is known for. Thanks, fbl On Fri, Feb 01, 2019 at 04:11:12PM +0300, Ilya Maximets wrote: > This patch deprecates the usage of current configuration knobs for > DPDK EAL arguments: > > - dpdk-alloc-mem > - dpdk-socket-mem > - dpdk-socket-limit > - dpdk-lcore-mask > - dpdk-hugepage-dir > - dpdk-extra > > All above configuration options replaced with single 'dpdk-options' > which has same format as old 'dpdk-extra', i.e. just a string with > all the DPDK EAL arguments. > > All the documentation about old configuration knobs removed. Users > could still use an old interface, but the deprecation warning will be > printed. In case 'dpdk-options' provided, values of old options will > be ignored. New users will start using new 'dpdk-options' as this is > the only documented way providing EAL arguments. > > Rationale: > > From release to release DPDK becomes more complex. New EAL arguments > appears, old arguments becomes deprecated. Some changes their meaning > and behaviour. It's not easy task to manage all this and current > code in OVS that tries to manage DPDK options is not flexible/extendable > enough even to track all the dependencies between options in DPDK 18.11. > For example, '--socket-mem' changed its meaning, '--legacy-mem' and > '--socket-limit' appeared. But '--legacy-mem' and '--socket-limit' > could not be used at the same time and, also, we want to provide > good default value for '--socket-limit' to keep the consistent > behaviour of OVS memory usage. And this will be worse in the future. > > Another point is that even now we have mutually exclusive database > configs in OVS and we have to handle them. i.e we're providing > 'dpdk-alloc-mem' and 'dpdk-socket-mem' at the same time. Additionally, > user allowed to configure same options in 'dpdk-extra'. So, we have > similar/equal options in a three places in ovsdb and we need to > choose which one to use. It's pretty easy for user to get into > inconsistent database configuration, because users could update > one field without removing others, and OVS has to resolve all the > conflicts somehow constructing the EAL args. But we do not know in > practice, if the resulted arguments are the arguments that user wanted > to see or just forget to remove the higher priority knob. > > Next point is that we have 'dpdk-lcore-mask' (-c) which is actually not > so user-friendly as '-l', but we have no option for it. Will we create > additional 'dpdk-lcore-list' ? I guess, no, because it's not really > important thing. But users will stuck with this not so user-friendly > masks. > > Another thing is that OVS is not really need to check the consistency > of the EAL arguments because DPDK could check it instead. DPDK will > always check the args and it will do it better. There is no real > need to duplicate this functionality. > > Keeping all the options in a single string 'dpdk-options' allows: > > * To have only one place for all the configs, i.e. it will be harder > for user to forget to remove something from the single string while > updating it. > * Not to check the consistency between different configs, DPDK could > check the cmdline itself. > * Not to document every DPDK EAL argument in OVS. We can just > describe few of them and point to DPDK docs for more information. > * OVS still able to provide some minimal default arguments. > Thanks to DPDK 18.11 we only need to specify an lcore. All other > arguments are not necessary. (We still also providing default > --socket-limit in case --socket-mem passed without it and without > --legacy-mem) > > Usage example: > > ovs-vsctl set Open_vSwitch . \ > other_config:dpdk-options="-l 1 -n 4 --socket-mem 1024,1024" > > Signed-off-by: Ilya Maximets <i.maxim...@samsung.com> > --- > Documentation/intro/install/dpdk.rst | 40 +++++--- > NEWS | 7 +- > lib/dpdk.c | 49 +++++++++- > utilities/ovs-dev.py | 2 +- > vswitchd/vswitch.xml | 132 ++++++--------------------- > 5 files changed, 108 insertions(+), 122 deletions(-) > > diff --git a/Documentation/intro/install/dpdk.rst > b/Documentation/intro/install/dpdk.rst > index 344d2b3a6..2243fde53 100644 > --- a/Documentation/intro/install/dpdk.rst > +++ b/Documentation/intro/install/dpdk.rst > @@ -235,32 +235,44 @@ listed below. Defaults will be provided for all values > not explicitly set. > A value of ``try`` will imply that the ovs-vswitchd process should > continue running even if the EAL initialization fails. > > -``dpdk-lcore-mask`` > - Specifies the CPU cores on which dpdk lcore threads should be spawned and > - expects hex string (eg '0x123'). > +``dpdk-options`` > + Specifies the string of EAL command line arguments for DPDK. > + For example, ``other_config:dpdk-options="-l 0 --socket-mem 1024,1024"``. > + Important arguments that could be passed there are: > > -``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. > + - ``-c`` or ``-l`` > > -``dpdk-hugepage-dir`` > - Directory where hugetlbfs is mounted > + Specifies the CPU cores on which dpdk lcore threads should be spawned. > + ``-c`` expects hex string (eg ``0x123``) while ``-l`` expects core list > + (eg ``0-5,8``). > + > + - ``--socket-mem`` > + > + Comma separated list of memory to pre-allocate from hugepages on specific > + sockets. > + > + - ``--huge-dir`` > + > + Directory where hugetlbfs is mounted > + > + - See DPDK documentation for the full list: > + > + https://doc.dpdk.org/guides-18.11/linux_gsg/linux_eal_parameters.html > > ``vhost-sock-dir`` > Option to set the path to the vhost-user unix socket files. > > -If allocating more than one GB hugepage, you can configure the > -amount of memory used from any given NUMA nodes. For example, to use 1GB from > -NUMA node 0 and 0GB for all other NUMA nodes, run:: > +You can configure the amount of memory preallocated from any given NUMA > nodes. > +For example, to preallocate ``1GB`` from NUMA node ``0`` and ``0GB`` for all > +other NUMA nodes, run:: > > $ ovs-vsctl --no-wait set Open_vSwitch . \ > - other_config:dpdk-socket-mem="1024,0" > + other_config:dpdk-options="<...> --socket-mem 1024,0" > > or:: > > $ ovs-vsctl --no-wait set Open_vSwitch . \ > - other_config:dpdk-socket-mem="1024" > + other_config:dpdk-options="<...> --socket-mem 1024" > > .. note:: > Changing any of these options requires restarting the ovs-vswitchd > diff --git a/NEWS b/NEWS > index a64b9d94d..1c09e9d57 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,8 +1,11 @@ > Post-v2.11.0 > --------------------- > - DPDK: > - * New option 'other_config:dpdk-socket-limit' to limit amount of > - hugepage memory that can be used by DPDK. > + * New option 'other_config:dpdk-options' to pass all the > + DPDK EAL argumants in a single string. > + * Following config options deprecated: > + 'dpdk-alloc-mem', 'dpdk-socket-mem', 'dpdk-socket-limit', > + 'dpdk-lcore-mask', 'dpdk-hugepage-dir' and 'dpdk-extra'. > > > v2.11.0 - xx xxx xxxx > diff --git a/lib/dpdk.c b/lib/dpdk.c > index 53b74fba4..0c37f231d 100644 > --- a/lib/dpdk.c > +++ b/lib/dpdk.c > @@ -91,6 +91,41 @@ args_contains(const struct svec *args, const char *value) > return false; > } > > +static bool > +report_deprecated_configs(const struct smap *ovs_other_config, bool ignore) > +{ > + struct option { > + const char *opt; > + const char *replacement; > + } options[] = { > + { "dpdk-alloc-mem", "-m" }, > + { "dpdk-socket-mem", "--socket-mem" }, > + { "dpdk-socket-limit", "--socket-limit" }, > + { "dpdk-lcore-mask", "-c" }, > + { "dpdk-hugepage-dir", "--huge-dir" }, > + { "dpdk-extra", "" } > + }; > + int i; > + bool found = false; > + > + for (i = 0; i < ARRAY_SIZE(options); i++) { > + const char *value = smap_get(ovs_other_config, options[i].opt); > + > + if (value) { > + VLOG_WARN("Detected deprecated '%s' config. Use '%s %s'" > + " in 'dpdk-options' instead.%s", > + options[i].opt, options[i].replacement, value, > + ignore ? " Value ignored." : ""); > + found = true; > + } > + } > + if (found) { > + VLOG_WARN("Deprecated options will be " > + "silently ignored in the future."); > + } > + return found; > +} > + > static void > construct_dpdk_options(const struct smap *ovs_other_config, struct svec > *args) > { > @@ -270,8 +305,10 @@ dpdk_init__(const struct smap *ovs_other_config) > char **argv = NULL; > int result; > bool auto_determine = true; > + bool deprecated_found; > int err = 0; > cpu_set_t cpuset; > + const char *dpdk_options; > struct svec args = SVEC_EMPTY_INITIALIZER; > > log_stream = fopencookie(NULL, "w+", dpdk_log_func); > @@ -317,7 +354,15 @@ dpdk_init__(const struct smap *ovs_other_config) > per_port_memory ? "enabled" : "disabled"); > > svec_add(&args, ovs_get_program_name()); > - construct_dpdk_args(ovs_other_config, &args); > + dpdk_options = smap_get(ovs_other_config, "dpdk-options"); > + > + deprecated_found = report_deprecated_configs(ovs_other_config, > + dpdk_options ? true : > false); > + if (dpdk_options) { > + svec_parse_words(&args, dpdk_options); > + } else if (deprecated_found) { > + construct_dpdk_args(ovs_other_config, &args); > + } > > if (!args_contains(&args, "--legacy-mem") > && !args_contains(&args, "--socket-limit")) { > @@ -357,7 +402,7 @@ dpdk_init__(const struct smap *ovs_other_config) > } > } > } else { > - /* User did not set dpdk-lcore-mask and unable to get current > + /* User did not set lcore mask and unable to get current > * thread affintity - default to core #0 */ > VLOG_ERR("Thread getaffinity error %d. Using core #0", err); > } > diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py > index 248d22ab9..d10b40c87 100755 > --- a/utilities/ovs-dev.py > +++ b/utilities/ovs-dev.py > @@ -283,7 +283,7 @@ def run(): > _sh("ovs-vsctl --no-wait set Open_vSwitch %s " > "other_config:dpdk-init=true" % root_uuid) > _sh("ovs-vsctl --no-wait set Open_vSwitch %s other_config:" > - "dpdk-extra=\"%s\"" % (root_uuid, ' '.join(options.dpdk))) > + "dpdk-options=\"%s\"" % (root_uuid, ' '.join(options.dpdk))) > else: > _sh("ovs-vsctl --no-wait set Open_vSwitch %s " > "other_config:dpdk-init=false" % root_uuid) > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 1db4e8694..c30265d9e 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -234,56 +234,6 @@ > </p> > </column> > > - <column name="other_config" key="dpdk-init" > - type='{"type": "string", > - "enum": ["set", ["false", "true", "try"]]}'> > - <p> > - Set this value to <code>true</code> or <code>try</code> to enable > - runtime support for DPDK ports. The vswitch must have compile-time > - support for DPDK as well. > - </p> > - <p> > - A value of <code>true</code> will cause the ovs-vswitchd process to > - abort if DPDK cannot be initialized. A value of <code>try</code> > - will allow the ovs-vswitchd process to continue running even if > DPDK > - cannot be initialized. > - </p> > - <p> > - The default value is <code>false</code>. Changing this value > requires > - restarting the daemon > - </p> > - <p> > - If this value is <code>false</code> at startup, any dpdk ports > which > - are configured in the bridge will fail due to memory errors. > - </p> > - </column> > - > - <column name="other_config" key="dpdk-lcore-mask" > - type='{"type": "integer", "minInteger": 1}'> > - <p> > - Specifies the CPU cores where dpdk lcore threads should be spawned. > - The DPDK lcore threads are used for DPDK library tasks, such as > - library internal message processing, logging, etc. Value should be > in > - the form of a hex string (so '0x123') similar to the 'taskset' mask > - input. > - </p> > - <p> > - The lowest order bit corresponds to the first CPU core. A set bit > - means the corresponding core is available and an lcore thread will > be > - created and pinned to it. If the input does not cover all cores, > - those uncovered cores are considered not set. > - </p> > - <p> > - For performance reasons, it is best to set this to a single core on > - the system, rather than allow lcore threads to float. > - </p> > - <p> > - If not specified, the value will be determined by choosing the > lowest > - CPU core from initial cpu affinity list. Otherwise, the value will > be > - passed directly to the DPDK library. > - </p> > - </column> > - > <column name="other_config" key="pmd-cpu-mask"> > <p> > Specifies CPU mask for setting the cpu affinity of PMD (Poll > @@ -303,78 +253,54 @@ > </p> > </column> > > - <column name="other_config" key="dpdk-alloc-mem" > - type='{"type": "integer", "minInteger": 0}'> > + <column name="other_config" key="dpdk-init" > + type='{"type": "string", > + "enum": ["set", ["false", "true", "try"]]}'> > <p> > - Specifies the amount of memory to preallocate from the hugepage > pool, > - regardless of socket. It is recommended that dpdk-socket-mem is > used > - instead. > + Set this value to <code>true</code> or <code>try</code> to enable > + runtime support for DPDK ports. The vswitch must have compile-time > + support for DPDK as well. > </p> > - </column> > - > - <column name="other_config" key="dpdk-socket-mem" > - type='{"type": "string"}'> > <p> > - Specifies the amount of memory to preallocate from the hugepage > pool, > - on a per-socket basis. > + A value of <code>true</code> will cause the ovs-vswitchd process to > + abort if DPDK cannot be initialized. A value of <code>try</code> > + will allow the ovs-vswitchd process to continue running even if > DPDK > + cannot be initialized. > </p> > <p> > - The specifier is a comma-separated string, in ascending order of > CPU > - socket. E.g. On a four socket system 1024,0,2048 would set socket 0 > - to preallocate 1024MB, socket 1 to preallocate 0MB, socket 2 to > - preallocate 2048MB and socket 3 (no value given) to preallocate > 0MB. > + The default value is <code>false</code>. Changing this value > requires > + restarting the daemon > </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. Changing this value > - requires restarting the daemon. > + If this value is <code>false</code> at startup, any dpdk ports > which > + are configured in the bridge will fail due to memory errors. > </p> > </column> > > - <column name="other_config" key="dpdk-socket-limit" > + <column name="other_config" key="dpdk-options" > type='{"type": "string"}'> > <p> > - Limits the maximum amount of memory that can be used from the > - hugepage pool, on a per-socket basis. > + Specifies EAL command line arguments for DPDK. For example, > + <code>"-l 1 --socket-mem 1024,1024 -n 4"</code> > </p> > <p> > - The specifier is a comma-separated list of memory limits per > socket. > - <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" > - 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 > - <ref column="other_config" key="dpdk-extra"/>, limits will not be > - applied. > Changing this value requires restarting the daemon. > </p> > - </column> > - > - <column name="other_config" key="dpdk-hugepage-dir" > - type='{"type": "string"}'> > - <p> > - Specifies the path to the hugetlbfs mount point. > - </p> > - <p> > - If not specified, this will be guessed by the DPDK library (default > - is /dev/hugepages). Changing this value requires restarting the > - daemon. > - </p> > - </column> > - > - <column name="other_config" key="dpdk-extra" > - type='{"type": "string"}'> > <p> > - Specifies additional eal command line arguments for DPDK. > + If <code>-l</code> and <code>-c</code> options are not specified, > + the default value will be provided by choosing the lowest CPU core > + from initial cpu affinity list. > + For performance reasons, it is best to set lcore set/list to a > single > + core on the system, rather than allow lcore threads to float. > + CPU cores proveded by <code>-l</code> and <code>-c</code> options > + only used by DPDK internal threads. To manage affinity of PMD > threads > + use <ref column="other_config" key="pmd-cpu-mask"/>. > </p> > <p> > - The default is empty. Changing this value requires restarting the > - daemon > + if <code>--socket-mem</code> option provided without > + <code>--legacy-mem</code> and <code>--socket-limit</code>, OVS will > + provide <code>--socket-limit</code> equal to the amount of > + preallocated memory specified by <code>--socket-mem</code>. > </p> > </column> > > -- > 2.17.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
