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

Ben Pfaff blp at nicira.com
Thu Nov 4 22:12:51 UTC 2010


On Thu, Nov 04, 2010 at 03:06:49PM -0700, Jesse Gross wrote:
> On Wed, Nov 3, 2010 at 4:43 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > 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).
> >
> > Ouch, it sure is unlocked.  get_drvinfo() can be called from softirq
> > context without any locks (from skb_gso_segment()).
> >
> > For now I changed this to just return if vport->dp is NULL but that's
> > not a very good solution.
> 
> I think it's fine.  It's just debugging information anyways.

1. It's used in lib/dpif-linux.c, in lookup_minor(), to translate a
   datapath name into a device major:minor.  (This will go away when we
   switch to Netlink of course.)

2. Does anything keep dp_port from going away between checking and
   dereferencing it?  skb_gso_segment() doesn't even have an RCU lock.
   I find network device synchronization kind of mystifying anyhow.

(Maybe you just mean that skb_gso_segment() is just printing debugging
information and doesn't have to be perfect.  If so then I agree.)




More information about the dev mailing list