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

[Aaron Conole]

Zoltan Kiss <zoltan.kiss at linaro.org> writes:
> 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.

Good catch. I'll do that.

>
>>       }
>> +
>> +    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@

I didn't intend to include this with this patch series, but the answer
is in how systemd integration is done. The way the systemd code works,
it calls ovs_ctl to start everything (good). ovs_ctl relies on the path,
though. There's a block at the beginning where a new path is built up,
but there is an edge case in there. I'll drop this change and submit it
separately.

> [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.

Yes, I will.

Thanks for the review, Zoltan!