Date:	Tue, 14 Dec 2010 16:59:43 -0600
From:	"Christian Benvenuti (benve)" <benve@...co.com>
To:	"Arnd Bergmann" <arnd@...db.de>
Cc:	<netdev@...r.kernel.org>, <davem@...emloft.net>,
	"Stefan Berger" <stefanb@...ibm.com>,
	"Roopa Prabhu (roprabhu)" <roprabhu@...co.com>,
	"David Wang (dwang2)" <dwang2@...co.com>
Subject: RE: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr

Hi,

> > In order to be able to scope a port profile, as part of the 802.1Qbh 
> > implementation we would like to add a new attribute:
> > IFLA_PORT_CLUSTER_UUID.
> > This parameter (perhaps known under a different name) is already in 
> > use (or going to be added) by most Virtual Machine Managers to 
> > define migration domains.
> > In the case of 802.1Qbh a port profile would most likely be scoped 
> > using the same ID used by VM manager to represent the migration 
> > domain.
> >
> > Adding another attribute (IFLA_PORT_CLUSTER_UUID in this case) to 
> > the list of IFLA_PORT_* attributes is an option.
> 
> Sounds reasonable.

Good.

> > However, we thought that it would be better to 1st re-arrange the 
> > current Netlink attribute scheme in order to better group the
> > IFLA_PORT_* attributes (for example by protocol).
> 
> We don't normally rearrange protocols once they are in an upstream 
> release. Having to maintain compatibility to two different versions of 
> the API is a huge burden for maintainance, so IMHO the only reason why 
> we should deprecate the current API and introduce a new one is if it 
> is absolutely impossible to implement necessary features without 
> breaking compatibility. Your explanations are very detailed and well 
> explained, but I have not found anything in there that describes why 
> it cannot be done without changing the existing interface.

You are right, we do not need to change the current interface in
order to add a new attribute like IFLA_PORT_CLUSTER_UUID.
We just thought that before to add a new attribute it would have
made sense to 1st re-organize (cleanup) the attributes and group
them by protocol, given that:

- the current list of attributes is a mix of attributes from two
  protocols (ie, not all of them are shared)

and

- new (not shared) attributes may need to be added in the future
  (given the state of the protocols)

If we do not do it now, for sure later it will be either more
painful or impossible (as of now we would need to deprecate only
two attributes and nothing would break).
It is not a mandatory step, but to us it would make sense to think
in the long term too.

> > --------------------------------------------------
> > 2) REASON FOR THIS CHANGE
> > --------------------------------------------------
> > We would like to add one more attribute (IFLA_PORT_CLUSTER_UUID), 
> > and the list of IFLA_PORT_* attributes may need to grow again due to 
> > the changes that may be required by the two still evolving standard 
> > protocols 802.1Qbh/802.1Qbg.
> > Because of that, if you see a value in the re-organization of the
> > IFLA_PORT_* attributes that we are proposing, it would be better to 
> > address such changes sooner than later, in order to reduce the 
> > impact of backward compatibility issues later.
> 
> The changes you propose now seem reasonable and we could probably have 
> done it that way initially, but as far as I'm concerned they are too 
> late.
> The burden imposed by the change is larger than the risk of breaking 
> backwards compatibility later by not fixing it now, as far as I'm 
> concerned.

We would only deprecate two attributes and nothing would break actually.

> To give another example, the split between IFLA_VFINFO_LIST and 
> IFLA_VF_PORTS is totally arbitrary, we should have merged them at the 
> time, but because of timing concerns of the two going in during the 
> merge window, we are stuck with two separate lists of VFs now, and I 
> don't think we should change them any more.
> > --------------------------------------------------
> > 4) IFLA_* versus IFLA_PORT_*
> > --------------------------------------------------
> >
> > Here is an alternative way to introduce the new Netlink attribute 
> > scheme. We personally like better the previous scheme, but I'll 
> > include this one too should someone find it interesting.
> 
> The impact of doing this would be even bigger.
> 
> > OPTION_3: According to the new Netlink attribute scheme that we
> >           are proposing, each protocol has its own set of
> >           attributes and therefore it would not be considered
> >           superfluous to have the same (or a similar) attribute
> >           defined for both protocols.
> >           (in this case it would be manager_ID for 802.1qbg and
> >           cluster_uuid for 802.1qbh).
> >
> > To us OPTION_3 looks like the option that offers most flexibility.
> 
> I don't see this depending on the change to split attributes per 
> protocol. Just introducing a new IFLA_PORT_CLUSTER_UUID should be all 
> you need. Since a cluster UUID is not exactly the same concept as a 
> vsi manager id, there is no need to share the same netlink attribute.

Right now it is all we need (well, we need a CLIENT_TYPE too actually).
However, the redesign we proposed was not just to get CLUSTER_UUID
in, but it would rather be a change aimed at keeping the code cleaner
in the long term too (I think it is reasonable to think that more
changes to that IFLA_PORT_* list are likely to happen given the
focus there is nowadays on the virt area (new protocols, new extensions, etc).

> > --------------------------------------------------
> > 8) OPT1: MORE CONFIGURATION FLEXIBILITY
> > --------------------------------------------------
> > The change described in this section is orthogonal to the ones 
> > discussed above.
> > We believe it would add value to the new scheme.
> > We would like to include it as part of the new Netlink scheme (but 
> > the current patch does not include it).
> >
> > In order to allow device drivers (or a generic consumer of the 
> > Netlink messages) to provide extra features or simple optimizations 
> > I would suggest the introduction of a new nested attribute that I 
> > will call IFLA_PORT_DATA for now.
> >
> > This attribute would allow the use of extra attributes that are not 
> > part of the official protocol specs (802.1Qbg/bh for now) or simply 
> > allow device drivers to start supporting pre-standard parameters 
> > that would not be included in the Netlink scheme before they reach 
> > some stability.
> 
> I really don't think that you should add per-driver attributes. If we 
> believe that we need an extension for a specific feature in the 
> netlink interface, it should be defined in a way that is generic 
> enough to work for other hardware implementing the same feature.

In general, yes, that's the way to go.
However, the use case of IFLA_PORT_DATA would be that where the
"extra config" is NOT generic enough to be shared with other hardware
implementing the same feature. And because of that it may be hard
to justify a change to the "shared" attribute scheme to include
support for a vendor specific extra attribute.

> > Here are a couple of examples of use.
> > Let's suppose that driver ABC needed to receive a couple of 
> > parameters more (that are not part of the official 802.1Qbh/bg 
> > protocols).
> > In this case driver ABC can use the new attribute IFLA_PORT_DATA to 
> > receive its two additional parameters without any need to 
> > touch/modify the IFLA_PORT_* list of attributes.
> 
> We can in theory add features that are not part of the official 
> standard. IMHO it is more important that the features are of general 
> interest and are being actively used. They should of course not 
> conflict with other features or the standard.

I agree.
However, from a vendor/driver perspective, what matters more is the
functionality/usefulness/performance_gain of the optional feature, and
not the likelihood of having other vendors support the same feature.
The idea behind IFLA_PORT_DATA is that of not having to change the
Netlink (shared) attribute scheme to support anything that is not
yet standard or will never be. As I said in the original email, it
is just a way to pass more info down the stack synchronously with
the shared attributes/data.

Here is an example. Supposing there was _not_ agreement on the
introduction of the a new IFLA_PORT_XYZ attribute because considered
too vendor/driver-centric.
In that case you either pass (async) XYZ to the driver using something
like the Generic Netlink proto or sysfs or ... one cleaner option would
be that of using IFLA_PORT_DATA, which would guarantee that the extra
config gets sent synchronously with the rest of the config.

> > If in the future driver ABC needed to change any of its private 
> > parameters (those it receives through the IFLA_PORT_DATA attribute), 
> > it can do it by updating its parsing routine (of course it would 
> > need to implement a basic versioning scheme for its private 
> > attributes), but no change would be required in the core Netlink 
> > code.
> 
> A data structure being private to a driver would not save you from 
> maintaining backwards compatibility, you still cannot just go and 
> change it as you like.

Yes of course.
However in this case the driver itself would take care of it
(RTNetlink does not need to know what is inside the IFLA_PORT_DATA
attribute).

> > If we do not want to add IFLA_PORT_DATA, an alternative solution 
> > would be that of using a separate control channel to provide that 
> > extra info, for example based on something like the NETLINK_GENERIC 
> > Netlink protocol.
> > This alternative approach would offer the same flexibility, but I 
> > can see One drawback: this solution would require some extra code to 
> > synchronize the two control channels (generic 
> > NETLINK_ROUTE/IFLA_PORT_XXX and NETLINK_GENERIC/Driver).
> 
> Right, using generic netlink for this does not help, it has all the 
> problems of your IFLA_PORT_DATA suggestions and is more complex.

Agree.

> Just don't add driver-private interfaces, make them official!

I would not look at IFLA_PORT_DATA as a private-interface replacing
the public one/s, but rather as a way to make it easier to configure
new extensions without having to wait for a standard/spec to show up
and make a case for the change to the shared Netlink scheme.

Anyway, IFLA_PORT_DATA was just a proposal (which I think makes
sense) and can be added/revisited at any time, not necessarily now.
Since we were proposing a change to the Netlink scheme, I thought
it was the right context to mention it too.
 
> If we give driver writers a way to add their own interfaces, there is 
> a very realistic risk of these interface being defined in a broken 
> way, with people relying on them before the code gets submitted for 
> mainline inclusion.

Of course there is the risk that some device driver writers will
abuse it. I think that by properly documenting it, device driver
writers should think twice before abusing it.

In summary, here are the three points I would like to reach an
agreement on:

(A) Redesign of IFLA_PORT_* attributes as described in the
    original patch 0/2 post

       YES/NO?

    (I vote for "Yes")

(B) Introduction of the following new attributes (with
    CLUSTER_UUID being top priority):

    - IFLA_PORT_CLUSTER_UUID : string UUID
        Used to scope the port profile

    - IFLA_PORT_CLIENT_TYPE  : string
        Used to identify the type of entity using the port
        profile (OS type, etc).

  If we go for A/YES, the above two attributes would go
  into the new IFLA_PORT_8021QBH_* attr list.

  If we go for A/NO, they would be added to the
  IFLA_PORT_* attribute list and would be usable by all
  protocols.

(C) Attribute versioning: #ifdef vs GET_VERSION

    On the libvirt side we need to #ifdef each time the attribute
    list changes. This is the default way of handling this kind
    of situation, however, by adding support for versioning (see
    my previous post) libvirt could detect the attribute "version"
    at run-time.
    I am fine with going with #ifdefs, unless someone expresses some
    interest in adding support for versioning (I would add it).

    I am OK in both cases (I think versioning is better but I
    understand the counter-argument).

As soon as we converge on (A)/(B)/(C) I'll post the patches for
- Kernel
- libvirt
- iproute2.
  
/Christian

Powered by blists - more mailing lists