Hey Kevin, Sorry for the late response to this.
"Traynor, Kevin" <kevin.tray...@intel.com> writes: >> -----Original Message----- >> From: Aaron Conole [mailto:acon...@redhat.com] >> Sent: Thursday, February 11, 2016 8:16 PM >> To: dev@openvswitch.org >> Cc: Flavio Leitner <f...@sysclose.org>; Panu Matilainen >> <pmati...@redhat.com>; >> Traynor, Kevin <kevin.tray...@intel.com>; Wojciechowicz, RobertX >> <robertx.wojciechow...@intel.com>; Zoltan Kiss <zoltan.k...@linaro.org>; >> Ansis Atteka <ansisatt...@gmail.com>; Christian Ehrhardt >> <christian.ehrha...@canonical.com>; Mooney, Sean K <sean.k.moo...@intel.com> >> Subject: [PATCH v9 5/6] netdev-dpdk: Check dpdk-extra when reading db >> >> A previous patch introduced the ability to pass arbitrary EAL command >> line options via the dpdk_extras database entry. This commit enhances >> that by warning the user when such a configuration is detected and >> prefering the value in the database. > > hi Aaron, > > I think we need a small doc update because of this patch. This is > allowing (with warning) and preferring something that docs from 4/6 > say is not allowed. > > + values. You MUST not pass arguments via dpdk-extra if they can be passed > + via another database parameter. D'oh! Okay, yeah I will update this. If the v9 series can go in, I can do it as follow up. If any non-trivial additional work comes I'll respin with this change. Thanks for the thorough review! > I tested this functionality for -c and --socket-mem and I see that > dpdk_extras is > being preferred. > > Tested-by: Kevin Traynor <kevin.tray...@intel.com> Awesome! Good to know. :) > > One other thing I noticed is that 1/6 is giving a build warning, as > 'cuse_dev_name' > now needs to be wrapped in the #ifdef VHOST_CUSE > > lib/netdev-dpdk.c:109:14: warning: 'cuse_dev_name' defined but not > used [-Wunused-variable] > static char *cuse_dev_name = NULL; /* Character device cuse_dev_name. */ > > Not sure if these are big enough to warrant a re-spin or they could be > quick follow ups. D'oh! I saw the warning fly by and thought 'oh I should fix that', then got distracted and forgot about it. Okay, I am game for either; I'll wait for word from the maintainers. Since they are small, it's easy to just quickly follow up _or_ spin a patch, so whatever... doesn't matter for me. Thanks so much for the review, Kevin! -Aaron > Kevin. > >> >> Suggested-by: Sean K Mooney <sean.k.moo...@intel.com> >> Signed-off-by: Aaron Conole <acon...@redhat.com> >> --- >> v9: >> * Added as suggested by Sean K Mooney >> >> lib/netdev-dpdk.c | 66 +++++++++++++++++++++++++++++++++++++++++++++-------- >> -- >> 1 file changed, 55 insertions(+), 11 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 1d6b907..b376e40 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2257,6 +2257,17 @@ dpdk_option_extend(char ***argv, int argc, const char >> *option, >> (*argv)[argc+1] = xstrdup(value); >> } >> >> +static char ** >> +move_argv(char ***argv, size_t cur_size, char **src_argv, size_t src_argc) >> +{ >> + char **newargv = grow_argv(argv, cur_size, src_argc); >> + while(src_argc--) { >> + newargv[cur_size+src_argc] = src_argv[src_argc]; >> + src_argv[src_argc] = 0; >> + } >> + return newargv; >> +} >> + >> static int >> extra_dpdk_args(const char *ovs_cfg, char ***argv, int argc) >> { >> @@ -2274,9 +2285,21 @@ extra_dpdk_args(const char *ovs_cfg, char ***argv, int >> argc) >> return ret; >> } >> >> +static bool >> +argv_contains(char **argv_haystack, const size_t argc_haystack, >> + const char *needle) >> +{ >> + for(size_t i = 0; i < argc_haystack; ++i) { >> + if (!strcmp(argv_haystack[i], needle)) >> + return true; >> + } >> + return false; >> +} >> + >> static int >> construct_dpdk_options(const struct ovsrec_open_vswitch *ovs_cfg, >> - char ***argv, const int initial_size) >> + char ***argv, const int initial_size, >> + char **extra_args, const size_t extra_argc) >> { >> struct dpdk_options_map { >> const char *ovs_configuration; >> @@ -2298,8 +2321,13 @@ construct_dpdk_options(const struct >> ovsrec_open_vswitch *ovs_cfg, >> lookup = opts[i].default_value; >> >> if(lookup) { >> - dpdk_option_extend(argv, ret, opts[i].dpdk_option, lookup); >> - ret += 2; >> + if (!argv_contains(extra_args, extra_argc, opts[i].dpdk_option)) >> { >> + dpdk_option_extend(argv, ret, opts[i].dpdk_option, lookup); >> + ret += 2; >> + } else { >> + VLOG_WARN("Ignoring database defined option '%s' due to " >> + "dpdk_extras config", opts[i].dpdk_option); >> + } >> } >> } >> >> @@ -2308,7 +2336,8 @@ construct_dpdk_options(const struct ovsrec_open_vswitch >> *ovs_cfg, >> >> static int >> construct_dpdk_mutex_options(const struct ovsrec_open_vswitch *ovs_cfg, >> - char ***argv, const int initial_size) >> + char ***argv, const int initial_size, >> + char **extra_args, const size_t extra_argc) >> { >> struct dpdk_exclusive_options_map { >> const char *category; >> @@ -2356,9 +2385,15 @@ construct_dpdk_mutex_options(const struct >> ovsrec_open_vswitch *ovs_cfg, >> ovs_abort(0, "Unable to cope with DPDK settings."); >> } >> >> - dpdk_option_extend(argv, ret, popt->eal_dpdk_options[found_pos], >> - found_value); >> - ret += 2; >> + if (!argv_contains(extra_args, extra_argc, >> + popt->eal_dpdk_options[found_pos])) { >> + dpdk_option_extend(argv, ret, popt->eal_dpdk_options[found_pos], >> + found_value); >> + ret += 2; >> + } else { >> + VLOG_WARN("Ignoring database defined option '%s' due to" >> + " dpdk_extras config", popt- >> >eal_dpdk_options[found_pos]); >> + } >> } >> >> return ret; >> @@ -2369,14 +2404,23 @@ 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); >> + char **extra_args = NULL; >> + int i; >> + size_t extra_argc = 0; >> >> extra_configuration = smap_get(&ovs_cfg->other_config, "dpdk-extra"); >> if (extra_configuration) { >> - i = extra_dpdk_args(extra_configuration, argv, i); >> + extra_argc = extra_dpdk_args(extra_configuration, &extra_args, 0); >> } >> - return i; >> + >> + i = construct_dpdk_options(ovs_cfg, argv, argc, extra_args, extra_argc); >> + i = construct_dpdk_mutex_options(ovs_cfg, argv, i, extra_args, >> extra_argc); >> + >> + if (extra_configuration) { >> + *argv = move_argv(argv, i, extra_args, extra_argc); >> + } >> + >> + return i + extra_argc; >> } >> >> static char **dpdk_argv; >> -- >> 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
