[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