[ovs-dev] [PATCH v9 5/6] netdev-dpdk: Check dpdk-extra when reading db
Aaron Conole
aconole at redhat.com
Wed Feb 17 16:38:33 UTC 2016
Hey Kevin,
Sorry for the late response to this.
"Traynor, Kevin" <kevin.traynor at intel.com> writes:
>> -----Original Message-----
>> From: Aaron Conole [mailto:aconole at redhat.com]
>> Sent: Thursday, February 11, 2016 8:16 PM
>> To: dev at openvswitch.org
>> Cc: Flavio Leitner <fbl at sysclose.org>; Panu Matilainen <pmatilai at redhat.com>;
>> Traynor, Kevin <kevin.traynor at intel.com>; Wojciechowicz, RobertX
>> <robertx.wojciechowicz at intel.com>; Zoltan Kiss <zoltan.kiss at linaro.org>;
>> Ansis Atteka <ansisatteka at gmail.com>; Christian Ehrhardt
>> <christian.ehrhardt at canonical.com>; Mooney, Sean K <sean.k.mooney at 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.traynor at 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.mooney at intel.com>
>> Signed-off-by: Aaron Conole <aconole at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list