Dear Aaron,
The general idea looks good to me.
Just some comments inline
On 3 December 2015 at 05:23, Aaron Conole <acon...@redhat.com> wrote:
> Existing DPDK integration is provided by use of command line options which
> must be split out and passed to librte in a special manner. However, this
> forces any configuration to be passed by way of a special DPDK flag, and
> interferes with ovs+dpdk packaging solutions.
>
> This commit delays dpdk initialization until after the OVS database
> connection
> is established, and then initializes librte. It pulls all of the config
> data
> from the OVS database, and assembles a new argv/argc pair to be passed
> along.
>
> Signed-off-by: Aaron Conole <acon...@redhat.com>
> ---
> lib/netdev-dpdk.c | 126
> ++++++++++++++++++++++++++++++++----------------
> lib/netdev-dpdk.h | 12 ++---
> utilities/ovs-ctl.in | 12 ++---
> vswitchd/bridge.c | 3 ++
> vswitchd/ovs-vswitchd.c | 27 -----------
> 5 files changed, 99 insertions(+), 81 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e3a0771..f3e0840 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -29,6 +29,7 @@
> #include <stdio.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> +#include <getopt.h>
>
> #include "dirs.h"
> #include "dp-packet.h"
> @@ -49,6 +50,8 @@
> #include "timeval.h"
> #include "unixctl.h"
> #include "openvswitch/vlog.h"
> +#include "smap.h"
> +#include "vswitch-idl.h"
>
> #include "rte_config.h"
> #include "rte_mbuf.h"
> @@ -2107,10 +2110,14 @@ unlock_dpdk:
>
> static int
> process_vhost_flags(char *flag, char *default_val, int size,
> - char **argv, char **new_val)
> + const struct ovsrec_open_vswitch *ovs_cfg,
> + char **new_val)
> {
> + const char *val;
> int changed = 0;
>
> + val = smap_get(&ovs_cfg->other_config, flag);
> +
> /* Depending on which version of vhost is in use, process the
> vhost-specific
> * flag if it is provided on the vswitchd command line, otherwise
> resort to
> * a default value.
> @@ -2120,9 +2127,9 @@ process_vhost_flags(char *flag, char *default_val,
> int size,
> * For vhost-cuse: Process "-cuse_dev_name" to set the custom name of
> the
> * vhost-cuse character device.
> */
> - if (!strcmp(argv[1], flag) && (strlen(argv[2]) <= size)) {
> + if (val && (strlen(val) <= size)) {
> changed = 1;
> - *new_val = strdup(argv[2]);
> + *new_val = strdup(val);
> VLOG_INFO("User-provided %s in use: %s", flag, *new_val);
> } else {
> VLOG_INFO("No %s provided - defaulting to %s", flag, default_val);
> @@ -2132,34 +2139,77 @@ process_vhost_flags(char *flag, char *default_val,
> int size,
> return changed;
> }
>
> -int
> -dpdk_init(int argc, char **argv)
> +static char **
> +grow_argv(char ***argv, size_t cur_siz, size_t grow_by)
> {
> - int result;
> - int base = 0;
> - char *pragram_name = argv[0];
> + char **new_argv = realloc(*argv, sizeof(char *) * (cur_siz +
> grow_by));
> + return new_argv;
> +}
>
> - if (argc < 2 || strcmp(argv[1], "--dpdk"))
> - return 0;
> +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},
> + {"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;
> +
> + *argv = newargv;
> + (*argv)[ret++] = strdup(opts[i].dpdk_option);
> + (*argv)[ret++] = strdup(lookup);
> + }
> + }
>
> - /* Remove the --dpdk argument from arg list.*/
> - argc--;
> - argv++;
> + return ret;
> +}
>
>
argv shall be NULL terminated, i.e. argv[argc] shall be NULL.
- /* Reject --user option */
> - int i;
> - for (i = 0; i < argc; i++) {
> - if (!strcmp(argv[i], "--user")) {
> - VLOG_ERR("Can not mix --dpdk and --user options, aborting.");
> - }
> +void
> +dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
> +{
> + static int initialized = 0;
> +
> + char **argv;
> + int result;
> + int argc;
> +
> + if(initialized || !smap_get_bool(&ovs_cfg->other_config, "dpdk",
> false)){
> + return;
> }
> +
> + initialized = 1;
>
> + VLOG_INFO("DPDK Enabled, initializing");
> +
> + /* TODO should check for user permissions. DPDK requires root user. */
> +
> #ifdef VHOST_CUSE
> - if (process_vhost_flags("-cuse_dev_name", strdup("vhost-net"),
> - PATH_MAX, argv, &cuse_dev_name)) {
> + if (process_vhost_flags("cuse_dev_name", strdup("vhost-net"),
> + PATH_MAX, ovs_cfg, &cuse_dev_name)) {
> +
> #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;
> }
> #endif
> - /* Remove the vhost flag configuration parameters from the
> argument
> - * list, so that the correct elements are passed to the DPDK
> - * initialization function
> - */
> - argc -= 2;
> - argv += 2; /* Increment by two to bypass the vhost flag
> arguments */
> - base = 2;
> }
>
> - /* Keep the program name argument as this is needed for call to
> - * rte_eal_init()
> - */
> - argv[0] = pragram_name;
> + argv = (char **)malloc(sizeof(char *));
> + *argv = strdup("ovs"); /* TODO use prctl to get process name */
> +
> + argc = get_dpdk_args(ovs_cfg, &argv);
> +
> + optind = 1;
>
> /* Make sure things are initialized ... */
> result = rte_eal_init(argc, argv);
> if (result < 0) {
> ovs_abort(result, "Cannot init EAL");
> }
> +
> + for(result = 0; result < argc-1; ++result)
> + free(argv[result]);
> +
> + free(argv);
>
> I am not sure if freeing argv is valid in this case, C99 standard states
that it shall remain valid until the program terminates.
> rte_memzone_dump(stdout);
> rte_eal_init_ret = 0;
>
> - if (argc > result) {
> - argv[result] = argv[0];
> - }
> -
> /* We are called from the main thread here */
> RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID;
> -
> - return result + 1 + base;
> }
>
> static const struct netdev_class dpdk_class =
> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
> index 646d3e2..e01adb3 100644
> --- a/lib/netdev-dpdk.h
> +++ b/lib/netdev-dpdk.h
> @@ -4,6 +4,7 @@
> #include <config.h>
>
> struct dp_packet;
> +struct ovsrec_open_vswitch;
>
> #ifdef DPDK_NETDEV
>
> @@ -22,7 +23,7 @@ struct dp_packet;
>
> #define NON_PMD_CORE_ID LCORE_ID_ANY
>
> -int dpdk_init(int argc, char **argv);
> +void dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg);
> void netdev_dpdk_register(void);
> void free_dpdk_buf(struct dp_packet *);
> int pmd_thread_setaffinity_cpu(unsigned cpu);
> @@ -33,13 +34,10 @@ int pmd_thread_setaffinity_cpu(unsigned cpu);
>
> #include "util.h"
>
> -static inline int
> -dpdk_init(int argc, char **argv)
> +static inline void
> +dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg OVS_UNUSED)
> {
> - if (argc >= 2 && !strcmp(argv[1], "--dpdk")) {
> - ovs_fatal(0, "DPDK support not built into this copy of Open
> vSwitch.");
> - }
> - return 0;
> +
> }
>
Why to remove this error handling?
>
> static inline void
> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> index 0082bed..6ae8bbd 100755
> --- a/utilities/ovs-ctl.in
> +++ b/utilities/ovs-ctl.in
> @@ -73,13 +73,13 @@ insert_mod_if_required () {
> }
>
> ovs_vsctl () {
> - ovs-vsctl --no-wait "$@"
> + @bindir@/ovs-vsctl --no-wait "$@"
> }
>
> set_system_ids () {
> set ovs_vsctl set Open_vSwitch .
>
> - OVS_VERSION=`ovs-vswitchd --version | sed 's/.*) //;1q'`
> + OVS_VERSION=`@bindir@/ovs-vswitchd --version | sed 's/.*) //;1q'`
> set "$@" ovs-version="$OVS_VERSION"
>
> case $SYSTEM_ID in
> @@ -131,7 +131,7 @@ check_force_cores () {
> }
>
> del_transient_ports () {
> - for port in `ovs-vsctl --bare -- --columns=name find port
> other_config:transient=true`; do
> + for port in `@bindir@/ovs-vsctl --bare -- --columns=name find port
> other_config:transient=true`; do
> ovs_vsctl -- del-port "$port"
> done
> }
> @@ -193,7 +193,7 @@ add_managers () {
> # won't briefly see empty datapath-id or ofport columns for records
> that
> # exist at startup.)
> action "Enabling remote OVSDB managers" \
> - ovs-appctl -t ovsdb-server ovsdb-server/add-remote \
> + @bindir@/ovs-appctl -t ovsdb-server ovsdb-server/add-remote \
> db:Open_vSwitch,Open_vSwitch,manager_options
> }
>
> @@ -215,7 +215,7 @@ start_forwarding () {
> fi
>
> # Start ovs-vswitchd.
> - set ovs-vswitchd unix:"$DB_SOCK"
> + set @bindir@/ovs-vswitchd unix:"$DB_SOCK"
> set "$@" -vconsole:emer -vsyslog:err -vfile:info
> if test X"$MLOCKALL" != Xno; then
> set "$@" --mlockall
> @@ -276,7 +276,7 @@ save_ofports_if_required () {
> #
> # (Versions 1.10 and later save OpenFlow port numbers without
> assistance,
> # so we don't have to do anything for them.
> - case `ovs-appctl version | sed 1q` in
> + case `@bindir@/ovs-appctl version | sed 1q` in
> "ovs-vswitchd (Open vSwitch) 1."[0-9].*)
> action "Saving ofport values" ovs_save save-ofports \
> "${script_ofports}"
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index b966d92..d33f42c 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -68,6 +68,7 @@
> #include "sflow_api.h"
> #include "vlan-bitmap.h"
> #include "packets.h"
> +#include "lib/netdev-dpdk.h"
>
> VLOG_DEFINE_THIS_MODULE(bridge);
>
> @@ -2921,6 +2922,8 @@ bridge_run(void)
> }
> cfg = ovsrec_open_vswitch_first(idl);
>
> + dpdk_init(cfg);
> +
> /* Initialize the ofproto library. This only needs to run once, but
> * it must be done after the configuration is set. If the
> * initialization has already occurred, bridge_init_ofproto()
> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
> index e78ecda..ae86861 100644
> --- a/vswitchd/ovs-vswitchd.c
> +++ b/vswitchd/ovs-vswitchd.c
> @@ -48,7 +48,6 @@
> #include "openvswitch/vconn.h"
> #include "openvswitch/vlog.h"
> #include "lib/vswitch-idl.h"
> -#include "lib/netdev-dpdk.h"
>
> VLOG_DEFINE_THIS_MODULE(vswitchd);
>
> @@ -71,13 +70,6 @@ main(int argc, char *argv[])
> int retval;
>
> set_program_name(argv[0]);
> - retval = dpdk_init(argc,argv);
> - if (retval < 0) {
> - return retval;
> - }
> -
> - argc -= retval;
> - argv += retval;
>
> ovs_cmdl_proctitle_init(argc, argv);
> service_start(&argc, &argv);
> @@ -152,7 +144,6 @@ parse_options(int argc, char *argv[], char
> **unixctl_pathp)
> OPT_ENABLE_DUMMY,
> OPT_DISABLE_SYSTEM,
> DAEMON_OPTION_ENUMS,
> - OPT_DPDK,
> };
> 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.");
> - break;
> -
> default:
> abort();
> }
> @@ -255,19 +241,6 @@ usage(void)
> stream_usage("DATABASE", true, false, true);
> daemon_usage();
> vlog_usage();
> - printf("\nDPDK options:\n"
> - " --dpdk [VHOST] [DPDK] Initialize DPDK datapath.\n"
> - " where DPDK are options for initializing DPDK lib and VHOST
> is\n"
> -#ifdef VHOST_CUSE
> - " option to override default character device name used for\n"
> - " for use with userspace vHost\n"
> - " -cuse_dev_name NAME\n"
> -#else
> - " option to override default directory where vhost-user\n"
> - " sockets are created.\n"
> - " -vhost_sock_dir DIR\n"
> -#endif
> - );
> printf("\nOther options:\n"
> " --unixctl=SOCKET override default control socket
> name\n"
> " -h, --help display this help message\n"
> --
> 2.6.1.133.gf5b6079
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
