[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:56:16 UTC 2010


On Fri, Dec 03, 2010 at 12:14:01PM -0800, Ben Pfaff wrote:
> 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:
> > > 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.

OK.  I added the netdev_vport_set_stats to netdev_internal_class and
added the following commit as a followup:

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Fri, 3 Dec 2010 12:54:08 -0800
Subject: [PATCH] netdev-linux: Don't treat "system" devices as vports for setting stats.

Linux kernel datapath vports have a "set_stats" method.  Until now,
internal vports have been handled in the userspace netdev library as
type "system", so the "system" netdevs would try to use the vport
"set_stats" method.  Now, however, internal netdevs have been broken out
as a separate netdev type, so only that new type of netdev has to be able
to call into "set_stats".  This commit, therefore, removes it from the
"system" netdevs.
---
 lib/netdev-linux.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index cbe4222..b7447f8 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2184,7 +2184,7 @@ const struct netdev_class netdev_linux_class =
         "system",
         netdev_linux_create,
         netdev_linux_enumerate,
-        netdev_vport_set_stats);
+        NULL);                  /* set_stats */
 
 const struct netdev_class netdev_tap_class =
     NETDEV_LINUX_CLASS(
-- 
1.7.1

--8<--------------------------cut here-------------------------->8--

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

Here's the fix I folded in, which meant that I didn't have to change the
above fairly clean code in bridge.c:

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index bcd1633..b7447f8 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -629,9 +630,19 @@ netdev_linux_open(struct netdev_dev *netdev_dev_, int ethertype,
     netdev->fd = -1;
     netdev_init(&netdev->netdev, netdev_dev_);
 
-    error = netdev_get_flags(&netdev->netdev, &flags);
-    if (error == ENODEV) {
-        goto error;
+    /* Verify that the device really exists, by attempting to read its flags.
+     * (The flags might be cached, in which case this won't actually do an
+     * ioctl.)
+     *
+     * Don't do this for "internal" netdevs, though, because those have to be
+     * created as netdev objects before they exist in the kernel, because
+     * creating them in the kernel happens by passing a netdev object to
+     * dpif_port_add(). */
+    if (netdev_dev_get_class(netdev_dev_) != &netdev_internal_class) {
+        error = netdev_get_flags(&netdev->netdev, &flags);
+        if (error == ENODEV) {
+            goto error;
+        }
     }
 
     if (!strcmp(netdev_dev_get_type(netdev_dev_), "tap") &&




More information about the dev mailing list