[ovs-dev] [RFC 1/2] dpdk: Convert initialization from cmdline to db

Traynor, Kevin kevin.traynor at intel.com
Fri Dec 4 16:16:41 UTC 2015


> -----Original Message-----
> From: Aaron Conole [mailto:aconole at redhat.com]
> Sent: Friday, December 4, 2015 3:37 PM
> To: Traynor, Kevin
> Cc: dev at openvswitch.org; Flavio Leitner
> Subject: Re: [ovs-dev] [RFC 1/2] dpdk: Convert initialization from cmdline to
> db
> 
> "Traynor, Kevin" <kevin.traynor at intel.com> writes:
> >> -----Original Message-----
> <<snipped>>
> >> +static int
> >> +get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv)
> >> +{
> >> +    struct dpdk_options_map {
> >> +        const char *ovs_configuration;
> >> +        const char *dpdk_option;
> >> +        bool default_enabled;
> >> +        const char *default_value;
> >> +    } opts[] = {
> >> +        {"dpdk_core_mask", "-c", true, "0x1"},
> >> +        {"dpdk_mem_channels", "-n", true, "4"},
> >> +        {"dpdk_alloc_mem", "-m", false, NULL},
> >> +        {"dpdk_socket_mem", "--socket-mem", false, NULL},
> >
> > What do you think about adding a default for socket-mem?
> 
> My only concern is I don't know what it should look like. This is the
> per-socket memory allocation and I'm not sure what kind of default makes
> sense, although with my specified defaults in the rest of the
> configuration perhaps "1024,0" is a 'proper' value.
> 
> What do you think?

I think "1024,0" is reasonable default for users who don't want to know too much
low level config. 

> <<snipped code>>
> >>      static const struct option long_options[] = {
> >>          {"help",        no_argument, NULL, 'h'},
> >> @@ -166,7 +157,6 @@ parse_options(int argc, char *argv[], char
> >> **unixctl_pathp)
> >>          {"bootstrap-ca-cert", required_argument, NULL,
> >> OPT_BOOTSTRAP_CA_CERT},
> >>          {"enable-dummy", optional_argument, NULL, OPT_ENABLE_DUMMY},
> >>          {"disable-system", no_argument, NULL, OPT_DISABLE_SYSTEM},
> >> -        {"dpdk", required_argument, NULL, OPT_DPDK},
> >>          {NULL, 0, NULL, 0},
> >>      };
> >>      char *short_options =
> >> ovs_cmdl_long_options_to_short_options(long_options);
> >> @@ -218,10 +208,6 @@ parse_options(int argc, char *argv[], char
> >> **unixctl_pathp)
> >>          case '?':
> >>              exit(EXIT_FAILURE);
> >>
> >> -        case OPT_DPDK:
> >> -            ovs_fatal(0, "--dpdk must be given at beginning of command
> >> line.");
> >
> > It might be helpful to leave the case statement here and call usage() or
> give
> > a deprecation message about the dpdk cmdline params for a release or two to
> > ensure a smooth transition to the new scheme - otherwise it'll probably end
> > up on the mailing list.
> 
> I waffled on whether or not to change it to be "--dpdk is deprecated" or
> just remove it, but there's probably a user who will complain that we
> removed the DPDK feature. Okay, I'll update the NEWS file, and mention
> that the configuration of DPDK via command line is deprecated in favor
> of the db approach.

Yes, please don't put "--dpdk is deprecated" anywhere in the code base!!

It's a cleaner cut to code as you did, but worries me that a user will just
pull from the repo, run the their setup script and suddenly it won't run with
no error message to say why. 

> 
> Thanks for the review, Kevin!



More information about the dev mailing list