[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