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

Jesse Gross jesse at nicira.com
Thu Nov 4 21:21:07 UTC 2010


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.

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

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




More information about the dev mailing list