"Traynor, Kevin" <kevin.tray...@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?
>> + {"dpdk_hugepage_dir", "--huge-dir", false, NULL},
>> + };
>> + int i, ret = 1;
>> +
>> + for(i = 0; i < (sizeof(opts) / sizeof(opts[0])); ++i){
>> + const char *lookup = smap_get(&ovs_cfg->other_config,
>> + opts[i].ovs_configuration);
>> + if(!lookup && opts[i].default_enabled)
>> + lookup = opts[i].default_value;
>> +
>> + if(lookup) {
>> + char **newargv = grow_argv(argv, ret, 2);
>> +
>> + if (!newargv)
>> + return ret;
>
> We have not completed creating our eal args. In the unlikely event of
> this happening, I'm not sure if we should continue to call rte_eal_init()
> as we may end up with a silent non-intended config.
I agree, I haven't done much negative testing here; I hope this should
not happen, but hope in one hand, etc. Perhaps I'll release the
argc/argv memory I've allocated here and return NULL. Let the init()
function deal with it (by aborting most likely).
>> +
>> + *argv = newargv;
>> + (*argv)[ret++] = strdup(opts[i].dpdk_option);
>> + (*argv)[ret++] = strdup(lookup);
>> + }
<<snipped>>
>> #else
>> - if (process_vhost_flags("-vhost_sock_dir", strdup(ovs_rundir()),
>> - NAME_MAX, argv, &vhost_sock_dir)) {
>> + if (process_vhost_flags("vhost_sock_dir", strdup(ovs_rundir()),
>> + NAME_MAX, ovs_cfg, &vhost_sock_dir)) {
>> struct stat s;
>> int err;
>>
>> @@ -2167,40 +2217,34 @@ dpdk_init(int argc, char **argv)
>> if (err) {
>> VLOG_ERR("vHostUser socket DIR '%s' does not exist.",
>> vhost_sock_dir);
>> - return err;
>> + return;
>
> We're not returning an error here anymore. I'm wondering should we abort
> or just deal with it later down the line if vhostuser is used
I was thinking of dealing with it if/when vhostuser socket is
used. Actually, what I planned on doing was proposing a directory
creation flag so that the user sees it to 'just work', but that's beyond
the scope of this patch.
Since we did return an error previously, and that error was used to
terminate the program, perhaps I'll do an ovs_abort() for now.
<<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.
Thanks for the review, Kevin!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
