On 09/03/2016 10:38, "Aaron Conole" <acon...@redhat.com> wrote:
>A previous change moved some commonly used arguments from commandline to
>the database, and with it the ability to pass arbitrary arguments to
>EAL. This change allows arbitrary eal arguments to be provided
>via a new db entry 'other_config:dpdk-extra' which will tokenize the
>string and add it to the argument list. The only argument which will not
>be supported with this change is '--no-huge', which appears to break the
>system in other ways.
>
>Signed-off-by: Aaron Conole <acon...@redhat.com>
>Tested-by: Sean K Mooney <sean.k.moo...@intel.com>
>Tested-by: RobertX Wojciechowicz <robertx.wojciechow...@intel.com>
>Tested-by: Kevin Traynor <kevin.tray...@intel.com>
>Acked-by: Panu Matilainen <pmati...@redhat.com>
>Acked-by: Kevin Traynor <kevin.tray...@intel.com>
>Acked-by: Flavio Leitner <f...@sysclose.org>
>---
>v4:
>* Added by suggestion of Panu, making socket-mem non-default
>
>v5:
>* Keep the socket-mem as default parameter, and mention that we
> do not support --no-huge
>* Update ovs-dev.py with the new mechanism for passing arbitrary dpdk
> options
>
>v6->v9:
>* No change
>
>v10:
>* INSTALL.DPDK.md - removed the note since a future commit in the series
>makes
> that documentation invalid (and it seems silly to add it here, only to
>remove
> in in the next commit)
>
> INSTALL.DPDK.md | 5 +++++
> lib/netdev-dpdk.c | 49
>+++++++++++++++++++++++++++++++++++--------------
> utilities/ovs-dev.py | 6 ++++--
> vswitchd/vswitch.xml | 11 +++++++++++
> 4 files changed, 55 insertions(+), 16 deletions(-)
>
>diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
>index 613764f..3b3d3a0 100644
>--- a/INSTALL.DPDK.md
>+++ b/INSTALL.DPDK.md
>@@ -178,6 +178,11 @@ Using the DPDK with ovs-vswitchd:
> * dpdk-hugepage-dir
> Directory where hugetlbfs is mounted
>
>+ * dpdk-extra
>+ Extra arguments to provide to DPDK EAL, as previously specified on the
>+ command line. Do not pass '--no-huge' to the system in this way.
>Support
>+ for running the system without hugepages is nonexistent.
>+
> * cuse-dev-name
> Option to set the vhost_cuse character device name.
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 289f916..4d3720f 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -2739,6 +2739,9 @@ static char **
> grow_argv(char ***argv, size_t cur_siz, size_t grow_by)
> {
> char **new_argv = realloc(*argv, sizeof(char *) * (cur_siz +
>grow_by));
>+ if (!new_argv) {
>+ ovs_abort(0, "grow_argv() failed to allocate memory.");
>+ }
No need to check if we use xrealloc()
> return new_argv;
> }
>
>@@ -2748,16 +2751,29 @@ dpdk_option_extend(char ***argv, int argc, const
>char *option,
> {
> char **newargv = grow_argv(argv, argc, 2);
>
>- if (!newargv) {
>- ovs_abort(0, "grow_argv() failed to allocate memory.");
>- }
>-
> *argv = newargv;
> (*argv)[argc] = xstrdup(option);
> (*argv)[argc+1] = xstrdup(value);
> }
>
> static int
>+extra_dpdk_args(const char *ovs_cfg, char ***argv, int argc)
Would you mind using another name for "ovs_cfg", please?
It's used elsewhere with another meaning
>+{
>+ int ret = argc;
>+ char *release_tok = xstrdup(ovs_cfg);
>+ char *tok = release_tok, *endptr = NULL;
>+
>+ for(tok = strtok_r(release_tok, " ", &endptr); tok != NULL;
Space between for and (
>+ tok = strtok_r(NULL, " ", &endptr)) {
>+ char **newarg = grow_argv(argv, ret, 1);
>+ *argv = newarg;
>+ (*argv)[ret++] = xstrdup(tok);
I'd use "newarg" instead of "(*argv)"
>+ }
>+ free(release_tok);
>+ return ret;
>+}
>+
>+static int
> construct_dpdk_options(const struct ovsrec_open_vswitch *ovs_cfg,
> char ***argv, const int initial_size)
> {
>@@ -2851,8 +2867,14 @@ static int
> get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv,
> int argc)
> {
>+ const char *extra_configuration;
> int i = construct_dpdk_options(ovs_cfg, argv, argc);
> i = construct_dpdk_mutex_options(ovs_cfg, argv, i);
>+
>+ extra_configuration = smap_get(&ovs_cfg->other_config, "dpdk-extra");
>+ if (extra_configuration) {
>+ i = extra_dpdk_args(extra_configuration, argv, i);
>+ }
> return i;
> }
>
>@@ -2911,17 +2933,15 @@ __dpdk_init(const struct ovsrec_open_vswitch
>*ovs_cfg)
> }
>
> argv = grow_argv(&argv, argc, argc+1);
>- if (!argv) {
>- ovs_abort(0, "Unable to allocate an initial argv.");
>- }
> argv[argc++] = xstrdup("ovs"); /* TODO use prctl to get process name
>*/
> argc_tmp = get_dpdk_args(ovs_cfg, &argv, argc);
>
> while(argc_tmp != argc) {
>- if (!strcmp("-c", argv[argc++])) {
>+ if (!strcmp("-c", argv[argc]) || !strcmp("-l", argv[argc])) {
> auto_determine = false;
> break;
> }
>+ argc++;
> }
> argc = argc_tmp;
>
>@@ -2936,9 +2956,6 @@ __dpdk_init(const struct ovsrec_open_vswitch
>*ovs_cfg)
> char buf[MAX_BUFSIZ];
> snprintf(buf, MAX_BUFSIZ, "0x%08llX", (1ULL<<i));
> argv = grow_argv(&argv, argc, 2);
>- if (!argv) {
>- ovs_abort(0, "Unable to grow argv for coremask");
>- }
> argv[argc++] = xstrdup("-c");
> argv[argc++] = xstrdup(buf);
> i = CPU_SETSIZE;
>@@ -2947,13 +2964,17 @@ __dpdk_init(const struct ovsrec_open_vswitch
>*ovs_cfg)
> }
>
> argv = grow_argv(&argv, argc, 1);
>- if (!argv) {
>- ovs_abort(0, "Unable to make final argv allocation.");
>- }
> argv[argc] = 0;
>
> optind = 1;
>
>+ if (VLOG_IS_DBG_ENABLED()) {
>+ int opt;
>+ for(opt = 0; opt < argc; ++opt) {
Space between for and (
>+ VLOG_DBG("EAL CMDLINE ARG: %s", argv[opt]);
I'd prefer having all the options printed on a single line
(you can use lib/dynamic-string to create the string),
and with a VLOG_INFO, instead of DBG
>+ }
>+ }
>+
> /* Make sure things are initialized ... */
> result = rte_eal_init(argc, argv);
> if (result < 0) {
>diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py
>index f2368c2..78e6d0b 100755
>--- a/utilities/ovs-dev.py
>+++ b/utilities/ovs-dev.py
>@@ -266,6 +266,7 @@ def run():
>
> if options.dpdk:
> _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, options.dpdk))
options.dpdk is not a string anymore. I had to change the above line to:
_sh("ovs-vsctl --no-wait set Open_vSwitch %s
other_config:dpdk-extra=\"%s\"" % (root_uuid, ' '.join(options.dpdk)))
> else:
> _sh("ovs-vsctl --no-wait set Open_vSwitch %s
>other_config:dpdk-init=false" % root_uuid)
>
>@@ -421,8 +422,9 @@ def main():
> help="run ovs-vswitchd under gdb")
> group.add_option("--valgrind", dest="valgrind", action="store_true",
> help="run ovs-vswitchd under valgrind")
>- group.add_option("--dpdk", dest="dpdk", action="store_true",
>- help="run ovs-vswitchd with dpdk")
>+ group.add_option("--dpdk", dest="dpdk", action="callback",
>+ callback=parse_subargs,
>+ help="run ovs-vswitchd with dpdk subopts (ended by
>--)")
> group.add_option("--clang", dest="clang", action="store_true",
> help="Use binaries built by clang")
> group.add_option("--user", dest="user", action="store", default="",
>diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>index 0aae391..5e085d0 100644
>--- a/vswitchd/vswitch.xml
>+++ b/vswitchd/vswitch.xml
>@@ -292,6 +292,17 @@
> </p>
> </column>
>
>+ <column name="other_config" key="dpdk-extra"
>+ type='{"type": "string"}'>
>+ <p>
>+ Specifies additional eal command line arguments for DPDK.
>+ </p>
>+ <p>
>+ The default is empty. Changing this value requires restarting
>the
>+ forwarding path.
Again, I'd prefer daemon instead of "forwarding path".
>+ </p>
>+ </column>
>+
> <column name="other_config" key="cuse-dev-name"
> type='{"type": "string"}'>
> <p>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
