[ovs-dev] [netlink 03/16] datapath: Merge "struct dp_port" into "struct vport".

Jesse Gross jesse at nicira.com
Fri Oct 1 00:23:25 UTC 2010


On Fri, Sep 24, 2010 at 4:00 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Sep 23, 2010 at 07:11:43PM -0700, Jesse Gross wrote:
>> On Fri, Sep 10, 2010 at 3:55 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > diff --git a/datapath/datapath.c b/datapath/datapath.c
>> > index b073a70..30117d2 100644
>> > @@ -382,31 +367,20 @@ static int new_dp_port(struct datapath *dp, struct odp_port *odp_port, int port_
>> >        if (IS_ERR(vport))
>> >                return PTR_ERR(vport);
>> >
>> > -       p = kzalloc(sizeof(*p), GFP_KERNEL);
>> > -       if (!p)
>> > -               return -ENOMEM;
>> > -
>> > -       p->port_no = port_no;
>> > -       p->dp = dp;
>> > -       p->vport = vport;
>> > -       atomic_set(&p->sflow_pool, 0);
>> > +       vport->dp = dp;
>> > +       vport->port_no = port_no;
>>
>> You need a write memory barrier here.  Once the port is attached we
>> can start receiving packets from the vport.  There's no guarantee that
>> the cacheline containing the dp will make it before the pointer.  This
>> was previously done by the link from vport to dp_port.
>> rcu_assign_pointer() will do the job.
>
> Thanks for being so careful.
>
> I changed
>        p->dp = dp;
> to
>        rcu_assign_pointer(p->dp, dp);
> which I think is what you meant.
>
> It seems that perhaps we could use an smp_wmb() after the assignment to
> vport->port_no to ensure that it is visible before the attach.

Yeah, I think we need to do something along those lines.  In most
cases, I would expect that there would be a write memory barrier in
the attach method and this would be unnecessary.  However, at the
moment we can start receiving packets immediately after create (i.e.
from tunnels since they have no attach method and attach no longer
does anything besides call that method).  This means that we can
receive a packet with a NULL vport->dp, which leads to oops.  This
will be an even more common case if we get rid of vport_attach() as I
would like.  So probably what we need is roughly:

vport_create();
vport->port_no = port_no;
smp_wmb();
vport->dp = dp;

and then at the beginning of vport_receive:

if (unlikely(!vport->dp)) {
    kfree_skb(skb);
    return;
}

There may be other places where we need to check for !vport->dp as
well.  dp_device_event() also fetches the dp but both it and new_vport
are protected by RTNL mutex so it should be OK as is.




More information about the dev mailing list