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