Date:	Sat, 11 Dec 2010 18:53:44 -0600
From:	"Christian Benvenuti (benve)" <benve@...co.com>
To:	"Stefan Berger" <stefanb@...ibm.com>
Cc:	<netdev@...r.kernel.org>, <chrisw@...hat.com>, <arnd@...db.de>,
	"David Miller" <davem@...emloft.net>,
	"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,

>> >> The following series add the new IFLA_PORT_PROTO_* nested protocol 
>> >> attributes to rtnetlink and it updates the enic driver to support 
>> >> them.
>> >> 
>> >> 01/2 - Add new protocol nested IFLA_PORT_PROTO_* attrs
>> >> 02/2 - Update enic driver to support new IFLA_PORT_PROTO_* attrs
>> >> 
>> >> Signed-off-by: Christian Benvenuti <benve <at> cisco.com>
>> >> Signed-off-by: Roopa Prabhu <roprabhu <at> cisco.com>
>> >> Signed-off-by: David Wang <dwang2 <at> cisco.com>
>> >> 
>> > [...]
>> > 
>> >> When the protocol nested attributes IFLA_PORT_PROTO_* will be 
>> >> populated with new sub-attributes (like the CLUSTER_UUID we would 
>> >> like to add), the user space clients will have to adapt to the new 
>> >> attribute scheme if they want to be able to see/receive the new 
>> >> attributes (like CLUSTER_UUID).
>> 
>> >I don't have a problem with these changes. Just on the libvirt level 
>> >it's going to be a lot more messy. We'll need another level of 
>> >#ifdef's
>> 
>> >for when these new attributes became available. In case they are 
>> >there we should not just create the netlink messages with the new 
>> >attributes but first independently probe for 802.1Qbg and 802.1Qbh 
>> >for whether lldpad or the kernel respectively saw the same level of 
>> >if_link.h include and/or support the new attributes and fall back to 
>> >using the old ones in case the probing failed. That way we can 
>> >support multi-boot
>> 
>> >installations with kernels before and after these changes or an 
>> >lldpad that doesn't support the new attributes and still give the 
>> >user the experience that the starting of the VM 'works' as before 
>> >(the new
>> kernel was installed).
>> 
>> We will definitely take care of the libvirt changes necessary to 
>> support this.
>> I can see two points in your comment above. Please correct me if I 
>> did not understand them correctly:
>> 
>> 1) libvirt compilation on multi-boot installations
>> 
>> First let me verify if I understand what you mean by multi-boot
>> installations:
>> 
>>   systems where there are multiple kernel versions installed and
>>   therefore where the running system may potentially be using
>>   different if_link.h versions, with or without the new attributes
>>   ("new" = replacements for the deprecated ones)
>> 
>> Is this correct? 
>
>Yes. The problem case would be a user (re-)compiles libvirt with 
>support for the new attributes on the new kernel and then boots into 
>the old one. The user would expect it to still work as before the
>re-compilation.

(*)

With our proposal it would work because, regardless of the kernel
version in use, libvirt would always send both old and new
attributes (and the kernel, if/when asked, would do the same).
For example, supposing the user wanted to configure a port
profile, libvirt would send (among the other attributes) the
following ones:

...
[IFLA_PORT_PROFILE]  <-------------- OLD ONE

[IFLA_PORT_PROTO_8021QBH]
    [IFLA_PORT_8021QBH_PROFILE] <--- NEW ONE ...

Note that the “NEW” attribute above does not add any new
functionality, and it does not replace the old/legacy one.

Those recipients that support the new attributes will look
for IFLA_PORT_8021QBH_PROFILE and, if not present, will
check for IFLA_PORT_PROFILE (if expected to work with older
kernels/apps).

Those recipients that only understand the old attributes will
only check for IFLA_PORT_PROFILE and will ignore completely
the new nested attr IFLA_PORT_PROTO_8021QBH (as per default
Netlink behavior).

The two attributes (old and new) carry the same exact piece
of information.
They only change the attribute scheme in order to make it
easier to maintain the attribute list:

   it classifies attributes by protocol.

Once this change is in, we will propose the addition of new
(8021Qbh) attributes (ie, CLUSTER_UUID), but the patches
currently pending in netdev do not introduce them yet.
Any new attribute added after the changes proposed with these
patches will be available only using the new scheme.
 
(Note that we are deprecating only two attributes in the
IFLA_PORT_* enum)

>> So what you are saying is that libvirt should use #ifdef to 
>> distinguish between
>> 
>>    if_link.h/pre_new_attributes
>> and 
>>    if_link.h/new_attributes? 
>
> Yes. It looks like you'll have to determine the availability in the 
> configure script and introduce a new #define there.

(*2)

I suppose that based on my comment (*) above, this should not be
necessary because any kernel supported as of today will continue to
work exactly the same way.
 
>> I guess this would be something similar to what 
>> doPortProfileOp8021Qbg (in src/util/macvtap.c) does by checking for 
>> IFLA_VF_PORT_MAX in order to distinguish between 
>> 8021Qbh/pre_port_profile and 8021Qbh/post_port_profile.
>> Is it correct? 
>
> Yes. If you provided the code for 802.1Qbh that'd be great. 

Based on my comment (*2) above we should not need to add any new #ifdef.

>> Is this #ifdef based approach the right way to keep track of the
>> if_link.h/IFLA_PORT_* changes? 
>
>If there's a better way of doing this, let me know. The problem is 
>people compiling libvirt may have all kinds of different levels of 
>systems, without even macvtap, then with macvtap but without the first 
>set of attributes for port profiles etc. - The progress in the kernel 
>is reflected in the code with these #ifdefs... not nice, but I
> doubt there's a better way of doing this.

Well, “versioning” is not a default feature in (RT)Netlink, and other
proposals (in slightly different contexts) got rejected in the past.
(see for example: http://www.spinics.net/lists/netdev/msg127715.html)

Since we are changing the attribute scheme, this could be the right
moment to add versioning to the IFLA_PORT_* attributes group.
We can consider the current one as being version 1 (or 0) and from
now on increment the version by 1 each time we apply some changes to
the IFLA_PORT_* attributes.
The most likely changes that may take place would be the introduction
of new attributes (the BH/BG protocols are still evolving).
This is not the way Netlink is used normally (see Miller's comment
in the URL I provided above), but it would make libvirt code much
easier to update and we would not need to populate it with #ifdefs.

The presence of the version would not be an excuse to be allowed to
make bad changes to the attributes knowing that you can fix them
later with a newer version (I agree with Miller's comment), but
simply to help consumers (ie libvirt in this case) to determine
what level of if_link.h/IFLA_PORT_* they are dealing with, at
run/time (the idea is that of offering a GET for that version number).

Right now (RT)Netlink silently ignores the attributes it does not
understand (ie, type > maxtype), therefore there are no other
mechanisms for determining at run-time what version of the attribute
list the kernel/recipient understands.

>> We may need to change if_link.h again, for example to add new
>> IFLA_PORT_* attributes.
>> Does it mean that in libvirt we will have to add an #ifdef each time? 
>
>I think so, yes. And since they are enums (rather than #define's 
>themselves) you'll have to introduce the #define's name and probe for 
>it by trying to compiling something with it in the configure script.

If this is the only option, we can go for that. No problem. We will
submit the libvirt patches.

If however the versioning scheme was accepted, the libvirt code would
be cleaner and easier to maintain.
Before to discard this option I would like someone on netdev to
comment on that.
Dave, can you confirm us that versioning the IFLA_PORT_* attributes
(not the all if_link.h header) is not an option?

>>> 2) old vs new attributes handling
>>> 
>>> You proposed to probe the recipient of the message to see if it 
>>> supports the new attributes, and downgrade to the old attr scheme if 
>>> the
>> recipient does not understand the new ones.
>> 
>> Current Netlink implementation does not provide such (probing) 
>> facility, and the Kernel for example simply ignores any attr whose 
>> id/type is bigger than the defined __XXX_MAX for the domain the
>> attr belongs to.
>
>Ok, then that's a problem. But it's equally a problem if user's don't 
>see why things don't work anymore today with the new kernel whereas 
>things still worked yesterday with the old one.

As I said in (*) above, with the current changes nothing will break
because the new attributes deprecate the old ones, and support for
the old ones won’t change at all.
This therefore should not be any problem.

>Is there any RTM_GETLINK one could do to determine whether the new 
>attributes are being used?

No.
The versioning mentioned above would add support for that.

>> Because of that there would not be a clean way to probe for remote 
>> support of a given attribute.
>> Is this correct? Please let me know if I misunderstood your "probe"
>> comment. 
>
>You understood the probe comment correctly. 
> 
>> What we propose is this:
>> 
>> - Given that we deprecated only two attributes, we would send always
>>   NEW attr + OLD/DEPRECATED attr
>> 
>> - A recipient that only understands the OLD/DEPRECATED attributes
>>   will parse and process only the OLD/DEPRECATED attributes and ignore
>>   the NEW ones.
>>   Here by "NEW ones" I mean those that replaced the DEPRECATED ones
>>   (ie, I do not refer to those that do not exist today and will be
>>    introduced therefore using the new Netlink scheme directly)
>> 
>> - A recipient that supports the NEW ones can safely use the NEW ones
>>   only and ignore the OLD ones. 
>
>If mixing the old and new ones in one message is ok then probing won't 
>be necessary.

Exactly. That’s the idea.
 
>> Note that if/when we will add new attributes (for example 
>> CLUSTER_UUID) that do not exist today and that therefore do not 
>> represent replacements for deprecated attributes, we will add them 
>> using the new scheme and therefore all recipients/consumers must 
>> update to the new scheme if they want to be able to use any of them.
>> None of the applications using the old attribute-set can expect to be 
>> able to use the new attributes without an upgrade.
>> 
>> > I am assuming that it worked with 802.1Qbh before even though you 
>> > didn't have the CLUSTER_UUID support... Now that will probably add 
>> > quite a bit to the complexity of the code and the testing. I hope 
>> > you'll submit a patch like that to libvirt mailing list.
>> 
>> I am not sure I understand your point here. Can you please clarify? 
>
>802.1Qbh support has been in libvirt for quite a while. I assume it 
>worked without the CLUSTER_UUID attribute that you are introducing only 
>now. I have no 802.1Qbh hardware to verify this, though.

Yes, it worked because so far port profiles were global. Now we would
like to use the concept of CLUSTER_UUID (already in use by most VMM)
to scope port profiles. This way we can limit the scope of a port
profile to those hosts belonging to a given cluster.

The main purpose of the kernel patches we submitted was to cleanup
the attribute scheme used for BH/BG, and at the same time make it
easier to add new attributes if/when needed.
However, this should not come at the cost of making libvirt support
a nightmare (#ifdef, etc).
To me it looks like

(1) sending both legacy and new attributes
 + 
(2) using a versioning scheme for the IFLA_PORT_* attributes

make it easy/clean for libvirt to adapt to the underlying kernel
version (ie, if_link.h/IFLA_PORT_* version).

- Because of (1) above we are not breaking any current user of the
  IFLA_PORT_* attributes.

- (2) above is optional but it would make updating libvirt easier.

Again, if #ifdef is the only option, we will stick to it.

Do you have any comment on the optional attribute IFLA_PORT_DATA
I proposed in PATCH 0/2 section 8 (at the end of the post)?

Thanks
/Christian

Powered by blists - more mailing lists