Zoltan Kiss <zoltan.k...@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 <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
>
> [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!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
