[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