[ovs-dev] [PATCH v2] datapath: Fully initialize datapath before local port.

[Jesse Gross]

On Fri, Sep 16, 2011 at 9:42 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Sep 15, 2011 at 05:50:16PM -0700, Jesse Gross wrote:
>> It's possible to start receiving packets on a datapath as soon as
>> the internal device is created.  It's therefore important that the
>> datapath be fully initialized before this, which it currently isn't.
>> In particularly, the fact that dp->stats_percpu is not yet set is
>> potentially fatal.  In addition, if allocation of the Netlink response
>> failed it would leak the percpu memory.  This fixes both problems.
>>
>> Found by code inspection, in practice the datapath is probably always
>> done initializing before someone can send a packet on it.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> This patch is not exactly a thing of beauty.  Use of
> smp_mb__before_clear_bit() to protect dp_ifindex seems very much an
> implementation detail that could change without us noticing.

Clearly.

> 'dp_ifindex' is never used in the fast path, only in Netlink handling
> and in the slow path that sends packets that miss up to userspace.  We
> could get rid of dp_ifindex, replacing it by a function that obtains
> the ifindex from vport numbered OVSP_LOCAL or returns 0 if there isn't
> one (which could only happen in the slow path).  When the slow path
> sees a return value of 0, it would just drop the packet.

I think it's actually even simpler than that - we should be able to
just replace dp_ifindex with calls to get the ifindex of OVSP_LOCAL
and it should always be available.  All uses other than upcalls are
definitely OK because both they and the initialization of the datapath
are protected by genl_mutex.  Packets received on the datapath device
are also fine because in order for the packet to be received the
device (and all of its members) must be visible on the current CPU.
Finally, if the packet is received on a different device, we're also
fine because when accessing the internal device, there is a data
dependency on the DP and we already have appropriate memory barriers
(though RCU) for that.