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

[Ben Pfaff]

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.

> > @@ -538,7 +509,7 @@ void dp_process_received_packet(struct dp_port *p, struct sk_buff *skb)
> 
> You need a data dependency read memory barrier when going from vport
> to dp.  rcu_dereference works.  This was previously done in
> vport_get_dp_port().

I'm a little confused about that.  rcu_dereference() is normally used
for data that can vary.  vport->dp is immutable during the lifetime of
the vport.  Anyone who gets to a vport should have already taken a read
memory barrier (e.g. the rcu_dereference(skb->dev->br_port) in
handle_bridge() in net/core/dev.c), so they should see the correct 'dp'
value.  No?

> > diff --git a/datapath/dp_sysfs_if.c b/datapath/dp_sysfs_if.c
> > index b40523a..ac86ac3 100644
> >  #define to_brport_attr(_at) container_of(_at, struct brport_attribute, attr)
> > -#define to_brport(obj) container_of(obj, struct dp_port, kobj)
> > +#define to_brport(obj) container_of(obj, struct vport, kobj)
> 
> We should rename this to to_vport.

OK, done.

> > diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
> > index a6e7262..b077a8a 100644
> > -       dp_port = vport_get_dp_port(vport);
> > -       if (dp_port)
> > -               sprintf(info->bus_info, "%d.%d", dp_port->dp->dp_idx, dp_port->port_no);
> > +       sprintf(info->bus_info, "%d.%d", vport->dp->dp_idx, vport->port_no);
> 
> There's a race here between device creation and dp getting assigned
> (and this access is unlocked, I believe).

I've run out of time for today but I'm sending this out in case you can
clarify the need for the rcu_dereference() above.  I'll fix and respond
to the rest later.

Thanks,

Ben.