[ovs-dev] [netlink 03/16] datapath: Merge "struct dp_port" into "struct vport".
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