[ovs-dev] [netlink 02/16] datapath: Make adding and attaching a vport a single step.

Ben Pfaff blp at nicira.com
Thu Nov 4 21:54:30 UTC 2010


On Thu, Nov 04, 2010 at 02:21:07PM -0700, Jesse Gross wrote:
> On Wed, Nov 3, 2010 at 3:41 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Wed, Sep 29, 2010 at 05:35:15PM -0700, Jesse Gross wrote:
> >> On Fri, Sep 24, 2010 at 3:10 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > On Thu, Sep 23, 2010 at 07:10:58PM -0700, Jesse Gross wrote:
> >> >> On Fri, Sep 10, 2010 at 3:55 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> >> >  struct odp_port {
> >> >> >     char devname[16];           /* IFNAMSIZ */
> >> >> > +    char type[VPORT_TYPE_SIZE];
> >> >> >     uint16_t port;
> >> >> > -    uint16_t flags;
> >> >> > +    uint16_t reserved1;
> >> >> >     uint32_t reserved2;
> >> >> > +    uint32_t config[VPORT_CONFIG_SIZE / 4]; /* type-specific configuration */
> >> >>
> >> >> Why is this an array of uint32_t with the size divided by 4 instead of
> >> >> an array of uint8_t?
> >> >
> >> > It's to ensure proper alignment of 32-bit members inside 'config', so
> >> > that one can just cast port->config to a configuration struct instead of
> >> > having to memcpy().
> >> >
> >> > It's not strictly necessary since reserved2 is also 32-bits.
> >> >
> >> > I'll do this any way you like, if you'll tell me what you prefer.
> >>
> >> Theoretically, config could have 64-bit members that would like to be
> >> properly aligned.  This is fine because config is 64-bit aligned
> >> currently but it makes the array of uint32_t seem weird.  However,
> >> making it an aligned array probably makes sense since the compiler is
> >> free to pad however it feels like.  64-bit alignment seems better to
> >> me but nothing currently needs it and, as you say, this is going away.
> >
> > aligned_u64 would be a better choice, yes, but datapath-protocol.h is
> > used on non-Linux systems too and I get complaints if I put anything
> > Linux-specific in it.  (That's one of the problems I haven't solved for
> > the end of this series yet, for the new <linux/vswitch.h>.)  uint64_t
> > would just make it look like there wasn't a problem, so I guess I'll
> > leave it as uint32_t.
> 
> I think uint64_t would give the correct alignment, at least on
> architectures that care about it.  I don't really care that much
> though, since this is intended to be temporary and nothing needs
> 64-bit alignment now.

Come to think of it, I've already broken non-Linux builds for
datapath-protocol.h, so in for a dime, in for a dollar.

I changed this to __aligned_u64.

> >> >> > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> >> >> > index 575b5b2..80712e3 100644
> >> >> > +    if (type[0] == '\0' || !strcmp(type, "system")) {
> >> >> > +        strncpy(port->type, "netdev", sizeof port->type);
> >> >> > +    } else {
> >> >>
> >> >> I wonder if we can require ports to have a type, that is, drop the
> >> >> implicit empty equals "system" mapping.  Actual people will use
> >> >> ovs-vsctl and that can keep the same interface.  For programmatic
> >> >> access, I don't see much benefit in allowing the implicit behavior.
> >> >
> >> > I think that we need to have a default in the system at some level (not
> >> > just in ovs-vsctl) because the "type" column in the Interface table is
> >> > optional.  When we do it here at the lowest level, we only have to do it
> >> > in one place.  We could make the "type" column mandatory, I guess,
> >> > although I had not considered it.
> >>
> >> I was thinking about making the type column mandatory.  It would
> >> require fixing controllers but it seems better to  explicit specify
> >> what is wanted, rather than implicitly divining it.  Other than the
> >> backwards compatibility issue, I don't see any benefit in making it
> >> optional.
> >
> > At the time you wrote that I think that it would have been possible to
> > make it mandatory, if we had acted quickly.  But I think that that time
> > has passed us by now.
> 
> Hmm, have we actually locked down the config db schema?  In any case,
> I suppose we don't have to change it now.  We can start encouraging
> people to populate those fields (and do it ourselves through
> ovs-vsctl) and then maybe it will be a fairly painless switch in the
> future.

Hmm, let me ask around.  Maybe everything other than ovs-vsctl already
adds the "type" member, since they are only adding tunnels, for which
"type" would need to be populated anyhow.  If that's the case then maybe
we can just change it.

> >> >> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> >> >> > index 598b001..4ab66ef 100644
> >> >> > +                type = (iface_is_internal(br, if_name)
> >> >> > +                        ? "internal"
> >> >> > +                        : iface->cfg->type);
> >> >> > +
> >> >>
> >> >> Is it possible to require that internal interfaces actually have a
> >> >> type of "internal"?
> >> >
> >> > This seems reasonable.  It is a little tricky, because there is no
> >> > "iface" structure at all for bond fake interfaces (a "feature" I hate,
> >> > by the way).
> >>
> >> Yeah, that is an annoying case.  Maybe we could at least start
> >> requiring actual internal interfaces to have an "internal" type and if
> >> we can clean this up in the future it will make it easier to
> >> transition.
> >
> > I've started on that by adding a commit that makes ovs-vsctl set the
> > "type" column of new bridges to "internal"; previously it left them
> > unset.
> 
> That sounds good.  Is there any reason to not do the same for all
> interfaces that ovs-vsctl creates?  For that matter, we should also do
> it for datapath_type.

I'll look at doing that too, then.




More information about the dev mailing list