[ovs-dev] [netlink v3 2/5] datapath: Make adding and attaching a vport a single step.

Ben Pfaff blp at nicira.com
Fri Dec 3 20:14:01 UTC 2010


On Thu, Dec 02, 2010 at 02:31:17PM -0800, Jesse Gross wrote:
> On Tue, Nov 16, 2010 at 5:11 PM, Ben Pfaff <blp at nicira.com> wrote:
> > -static int do_vport_mod(struct odp_vport_mod *vport_config)
> > +static int do_vport_mod(struct odp_port *port)
> >  {
> 
> There's now only caller of this function (vport_mod) and they're both
> pretty small.  Originally the purpose of having two was to separate
> out the compat code but that's no long needed.  Does it make sense to
> combine them?

Yes, thanks for pointing that out.  Now I've combined them.

> > -/**
> > - *     vport_user_del - delete existing vport device (for userspace callers)
> > - *
> > - * @udevname: Name of device to delete
> > - *
> > - * Deletes the specified device.  Detaches the device from a datapath first
> > - * if it is attached.  Deleting the device will fail if it does not exist or it
> > - * is the datapath local port.  It is also possible to fail for less obvious
> > - * reasons, such as lack of memory.  This function is for userspace callers and
> > - * assumes no locks are held.
> > - */
> > -int vport_user_del(const char __user *udevname)
> 
> This function is gone now but the prototype still exists in vport.h.

Thanks, I've deleted the prototype now.

> > diff --git a/lib/dpif.h b/lib/dpif.h
> > index c844289..e1bfeee 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -30,7 +30,9 @@ extern "C" {
> >  #endif
> >
> >  struct dpif;
> > +struct netdev;
> >  struct ofpbuf;
> > +struct shash;
> 
> It doesn't look like struct shash is actually used here.

Oops.  Thanks, I've deleted this now.

> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 39e21bc..3779ada 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > +const struct netdev_class netdev_linux_class =
> > +    NETDEV_LINUX_CLASS(
> > +        "system",
> > +        netdev_linux_create,
> > +        netdev_linux_enumerate,
> > +        netdev_vport_set_stats);
> > +
> > +const struct netdev_class netdev_tap_class =
> > +    NETDEV_LINUX_CLASS(
> > +        "tap",
> > +        netdev_linux_create_tap,
> > +        NULL,                   /* enumerate */
> > +        NULL);                  /* set_stats */
> > +
> > +const struct netdev_class netdev_internal_class =
> > +    NETDEV_LINUX_CLASS(
> > +        "internal",
> > +        netdev_linux_create,
> > +        NULL,                   /* enumerate */
> > +        NULL);                  /* set_stats */
> 
> Internal devices need a set_stats function, since that's what we're
> using for the bonding stats.  It's not clear whether we should expose
> it for system devices.  It will work (at least on the kernel datapath)
> but I don't think anything should be using it and we don't want to
> encourage it.

I agree.

I'll add another commit that drops the set_stats function from these
"netdev_class"es.  I want to keep it separate to make it clear that it
is an intentional change.

> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 13e897f..6e42470 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > +                error = netdev_open(&options, &netdev);
> > +                shash_destroy(&args);
> >
> > -            /* If it's not part of the datapath, add it. */
> > -            if (!dpif_port) {
> > -                error = dpif_port_add(br->dpif, if_name,
> > -                                      internal ? ODP_PORT_INTERNAL : 0, NULL);
> 
> Isn't the netdev_open() call going to fail for internal devices since
> they won't be created until the call to dpif_port_add()?  For vports
> we get around this by just storing the config until the call to open
> but internal devices check immediately for the device.  Maybe we have
> to do something similar to vports?  I do like that it's consistent for
> different device types.

Hmm, I tested this, why did it work?

Oh I see, I only tested it for the "local" port, which always exists
here because it gets created when the bridge does.  So I need a fallback
for other internal devices.  Ugh.

The real problem here, I think, is that internal devices are a hybrid of
vports and system netdevs.  They need properties of both.  I'll see what
I can do.

> Generally speaking though, I'm happy with this.  So whatever you think
> on the above comments is fine with me:
> Acked-by: Jesse Gross <jesse at nicira.com>

Thanks.




More information about the dev mailing list