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

[Zoltan Kiss]

On 03/12/15 04:23, Aaron Conole 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 <aconole at 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

[snip]

> +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;

Your second patch mentions that any change for any of the config needs 
an ovs-vswitchd restart, which is probably the right thing to do.
But this function is called from bridge_run(), so a false -> true change 
takes immediate effect, which may not be what the user expects. I think 
it's better to wrap the bool check into an ovsthread_once_start() to 
make sure that it is only checked once.

>       }
> +
> +    initialized = 1;
>
> +    VLOG_INFO("DPDK Enabled, initializing");
> +
> +    /* TODO should check for user permissions. DPDK requires root user. */
> +

[snip]

> --- 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'`

Why do you need to prepend all these command lines with @bindir@

[snip]
> 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);

You should check 'cfg != NULL', just as bridge_init_ofproto() does.

> +
>       /* 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()