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

Jesse Gross jesse at nicira.com
Fri Nov 5 00:28:36 UTC 2010


On Thu, Nov 4, 2010 at 3:12 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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.)

Hmm, I forgot about that.  Currently we just return if the port is not
attached, so at the very least it is no worse.

>
> 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.

skb_gso_segment() will generally be called with rcu_read_lock() but
it's not documented as requiring it and I can't say with certainty
that it always will be, so as it currently is we should probably add
it ourselves.  With this patch set it shouldn't matter though because
once the vport->dp is initialized, it will always be valid.

Generally speaking though, network device synchronization is based on
refcounting, not RCU.  It's just that our netdev has a lot of moving
pieces using RCU.

Actually what we should do it use the lockdep annotations available in
new kernels to check that we really have RCU read lock or RTNL where
we think we do.

>
> (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.)

I had forgotten that it was actually used in lookup_minor, so my
comment doesn't really make sense.

skb_gso_segment() should always work though because all it cares about
is the name of the driver, so it doesn't matter whether the bus_info
field has been initialized.  Obviously it shouldn't crash though.




More information about the dev mailing list