* [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
@ 2010-12-08 4:29 Christian Benvenuti
2010-12-08 4:31 ` [RFC][net-next-2.6 PATCH 1/2] Add new protocol nested " Christian Benvenuti
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Christian Benvenuti @ 2010-12-08 4:29 UTC (permalink / raw)
To: davem; +Cc: netdev
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@cisco•com>
Signed-off-by: Roopa Prabhu <roprabhu@cisco•com>
Signed-off-by: David Wang <dwang2@cisco•com>
--------------------------------------------
Here is how the text below is organized:
- 1) INTRO
Describes what 802.1Qbh change led us to propose
the Netlink changes discussed here.
- 2) REASON FOR THIS CHANGE
Describe why we think the changes we propose make sense now.
- 3) OUR PROPOSAL
Describe the changes to the current IFLA_PORT_* attr scheme.
- 4) IFLA_* versus IFLA_PORT_*
Describe an alternative approach to the one described in
the previous section (OUR PROPOSAL).
- 5) DUPLICATE PARAMETERS versus INDEPENDENT PARAMETERS
Describe one more reason why we think it would be useful
to have per-proto nested attributes.
- 6) PARSING OF PROTOCOL ATTRIBUTES
Describe how/when the new sub-attributes of the new protocol
nested attributes are parsed.
- 7) PRIORITY ORDER FOR "DOUBLE" PARAMETERS
Describe how to handle the case where the same attribute
is present both as a common attribute and as a protocol
private attribute.
- 8) OPT1: MORE CONFIGURATION FLEXIBILITY
Describe one optional change which can be used to further
enhance the config flexibility. This change is independent
from the one discussed in "OUR PROPOSAL".
- 9) OPT2: MORE CHANGES TO THE NETLINK SCHEME
Describe how to extend the changes described in "OUR PROPOSAL" to
include the common attributes.
- 10) BACKWARD COMPATIBILITY
A couple of notes about how backward compatibility would
be handled.
--------------------------------------------------
1) INTRO
--------------------------------------------------
In the current implementation of the network port profile feature
the parameters used by 802.1Qbg and 802.1Qbh IEEE protocols share
the same Netlink attribute scheme.
Here is the list of the currently defined attributes:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF, /* __u32 */
IFLA_PORT_PROFILE, /* string */
IFLA_PORT_VSI_TYPE, /* 802.1Qbg (pre-)standard VDP*/
IFLA_PORT_INSTANCE_UUID, /* binary UUID */
IFLA_PORT_HOST_UUID, /* binary UUID */
IFLA_PORT_REQUEST, /* __u8 */
IFLA_PORT_RESPONSE, /* __u16, output only */
__IFLA_PORT_MAX,
};
It is a flat list of attributes:
- some of them are used by 802.1QBH
- some of them are used by 802.1QBG
and
- some others are shared by the two protocols.
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.
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).
Patch_1/2 describes the Netlink scheme change, while Patch_2/2 shows how the
enic driver would change to adapt to the new scheme.
Before to post the (trivial) patch that adds support for the new proposed
IFLA_PORT_CLUSTER_UUID attribute, I would like to see if you seen any value
in the change proposed with this patch.
The rest of the email simply describes into more details the patch and the
various options that we can consider. I included this So that we may be able
to converge faster.
--------------------------------------------------
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.
--------------------------------------------------
3) OUR PROPOSAL
--------------------------------------------------
Instead of a flat list of attributes, the new scheme that we propose defines
two groups of attributes (one per protocol):
- 802.1Qbh parameters
- 802.1Qbg parameters
Each one of the above groups would be represented by an attribute of type
NLA_NESTED. If there will ever be another protocol coming into the picture we
can simply add a new NLA_NESTED attribute which would represent its set of
attributes.
In other words, this is the current scheme:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROFILE,
IFLA_PORT_VSI_TYPE,
IFLA_PORT_INSTANCE_UUID,
IFLA_PORT_HOST_UUID,
IFLA_PORT_REQUEST,
IFLA_PORT_RESPONSE,
__IFLA_PORT_MAX,
};
and this would be the new one:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROFILE, (*)
IFLA_PORT_VSI_TYPE, (*)
IFLA_PORT_INSTANCE_UUID,
IFLA_PORT_HOST_UUID,
IFLA_PORT_REQUEST,
IFLA_PORT_RESPONSE,
IFLA_PORT_PROTO_8021QBG, <--- NEW nested attr
IFLA_PORT_PROTO_8021QBH, <--- NEW nested attr
__IFLA_PORT_MAX,
};
The above attributes marked with (*) are protocol specific (while all the
others are common to the two protocols) and therefore would be deprecated
and replaced with new ones, as shown in the scheme below:
[IFLA_PORT_VF]
[IFLA_PORT_INSTANCE_UUID]
[IFLA_PORT_HOST_UUID]
[IFLA_PORT_REQUEST]
[IFLA_PORT_RESPONSE]
[IFLA_PORT_PROTO_8021QBG]
[IFLA_PORT_8021QBG_VSI_TYPE] <-- NEW sub attr
[IFLA_PORT_PROTO_8021QBH]
[IFLA_PORT_8021QBH_PROFILE] <-- NEW sub attr
In summary:
/* NEW list of 802.1QBG attributes */
enum {
IFLA_PORT_PROTO_8021QBG_VSI_TYPE,
__IFLA_PORT_PROTO_8021QBG_MAX,
};
/* NEW list of 802.1QBH attributes */
enum {
IFLA_PORT_PROTO_8021QBH_PROFILE,
__IFLA_PORT_PROTO_8021QBH_MAX,
};
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROFILE, <--(BH only / deprecated)
IFLA_PORT_VSI_TYPE, <--(BG only / deprecated)
IFLA_PORT_INSTANCE_UUID, <----(common)
IFLA_PORT_HOST_UUID, <----(common)
IFLA_PORT_REQUEST, <----(common)
IFLA_PORT_RESPONSE, <----(common)
IFLA_PORT_PROTO_8021QBH, <------ NEW nested attr
IFLA_PORT_PROTO_8021QBG, <------ NEW nested attr
__IFLA_PORT_MAX,
};
--------------------------------------------------
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 idea is that we can apply the changes one level higher into the Netlink
attribute hierarchy:
IFLA_* (*1)
/ ... \
v v
IFLA_PORT_* .... (*2)
In other words, the previous scheme adds the new attributes at level (*2) while
this alternative solution would add the new attributes at level (*1)
The current scheme at level (*1) uses these two attributes:
IFLA_VF_PORT
IFLA_PORT_SELF
while this new one would use these two new attributes:
IFLA_GEN_VF_PORT
IFLA_GEN_PORT_SELF
which translates to this:
enum {
IFLA_UNSPEC,
...
IFLA_VF_PORTS, <--- OLD / deprecated
IFLA_PORT_SELF, <--- OLD / deprecated
IFLA_GEN_VF_PORTS, <----NEW
IFLA_GEN_PORT_SELF, <----NEW
__IFLA_MAX
};
With this alternative scheme we would be able to define a new list of
attributes IFLA_PORT_GEN_* which would not include any deprecated attr:
enum {
IFLA_PORT_GEN_UNSPEC,
... common attr here ...
IFLA_PORT_GEN_PROTO_8021QBH,
IFLA_PORT_GEN_PROTO_8021QBG,
__IFLA_PORT_MAX,
};
NOTE: I am not suggesting the use of "_GEN". I used that keyword
just for the example.
The consumer of the Netling messages can easily distinguish between legacy
attributes (ie, IFLA_VF_PORTS/IFLA_PORT_SELF attributes) and new ones
(ie, IFLA_GEN_VF_PORTS/IFLA_GEN_PORT_SELF attributes)
--------------------------------------------------
5) DUPLICATE PARAMETERS versus INDEPENDENT PARAMETERS
--------------------------------------------------
One more note about the semantic of this new attribute scheme (which
applies to both proposals above).
There may be cases where a group of protocols (ie, 802.1Qbh and 802.1Qbg
as of today) has similar parameters but the latter do not share the same
exact meaning/syntax.
This situation would not be an issue because each protocol would interpret
those parameters independently accordingly to its semantic.
However, what could represent a limitation/issue is that those protocols
may have different data size/type requirements for the same parameter.
For example 802.1Qbg uses ifla_port_vsi.mgr_id to represent something
very similar to the IFLA_PORT_CLUSTER_UUID attribute that we would like
to add for 802.1Qbh. However:
- 802.1Qbg/ifla_port_vsi.mgr_id (which is already in the kernel)
is a 8-bit field
- 802.1Qbh/CLUSTER_UUID (which we would like to add) requires
more than 8 bits and could be a string
Here are a couple of options to handle this mismatch in the data type/size
requirements:
OPTION_1: we try to find a compromise and share the same data size/type.
In the above example, this may require a change in the
ifla_port_vsi data structure definition (to increase for
example the size of mgr_id) or, if we leave it the way it is
defined right now, the new 802.1Qbh cluster UUID would have to
live with the current 8-bit limitation imposed by
ifla_port_vsi.mgr_id.
OPTION_2: I'll mention this option for the sake of completeness, but I am
not suggesting it.
We could remove the mgr_id field from the ifla_port_vsi structure
and introduce a new shared attribute (ie IFLA_PORT_PROTO_GRP in the
example below). This attribute would then use a union to
provide different data sizes for the same config parameter, so
that we could for example use it to introduce the CLUSTER_UUID
needed for 802.1Qbh:
[IFLA_PORT_PROTO_ALL]
[IFLA_PORT_PROTO_ALL_GRP] = { .type = NLA_BINARY,
.len = sizeof(struct ifla_port_grp)};
struct ifla_port_grp {
union {
struct {
uint8_t managerID;
} 8021qbg;
struct {
uint8_t cluster_uuid[<size_of_cluster_id_here>];
} 8021qbh;
}
};
The keywords "group"/"cluster"/"domain" are pretty overloaded nowadays.
In the example above I used "_grp" just for lack of inspiration.
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.
This case above (mgr_id versus Cluster UUID) is just an example, however the
possibility of having the two parameters being independent allows for an easier
extendibility/adaptation of the two (still evolving) protocol implementations.
--------------------------------------------------
6) PARSING OF PROTOCOL ATTRIBUTES
--------------------------------------------------
When a device driver (or a generic consumer in kernel space) Receives a netlink
message that carries one of the new IFLA_PORT_PROTO_* attributes, it can use
the _new_ rtnetlink API rtnl_link_parse_port_proto:
rtnetlink::do_setlink
|
+--> driver::ndo_set_vf_port
|
+--> rtnetlink::rtnl_link_parse_port_proto
--------------------------------------------------
7) PRIORITY ORDER FOR "DOUBLE" PARAMETERS
--------------------------------------------------
As part of the semantic of this new Netlink attribute scheme, we would
recommend this additional rule:
if one parameter is present twice in a Netlink message, that
is to say it is present both as a shared parameter (ie, in the
IFLA_PORT_* list of attributes) and as a protocol private parameter
(ie, in one of the IFLA_PORT_PROTO_* nested attributes), the
most specific one would win (ie IFLA_PORT_PROTO_* in this case).
This would allow the new scheme to adapt to changes into the scope (and data
type/size) of the parameters without any change to the core code. For example:
Example_1 (shared attr -> private attr):
Today we have 802.1Qbh and 802.1Qbg in the picture. If tomorrow we add
a new IFLA_PORT_PROTO_XYZ protocol, the latter may have different
requirements (with regards to data size and type) for a given parameter,
and may therefore prefer to add its own (private) version of that
parameter inside its IFLA_PORT_PROTO_XYZ nested attribute.
Example_2 (private attr -> shared attr):
This is the reverse case of the previous example. Since the two
protocols are still evolving, new use case scenarios may drive
changes to the current data type/size requirements of the parameters.
Because of this, what is now a protocol private attribute may tomorrow
be eligible for changing to a shared attribute.
In such a case, the protocol would simply stop using its private
attribute (inside IFLA_PORT_PROTO_*) and start using the shared
one (ie IFLA_PORT_*).
--------------------------------------------------
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.
In order to keep the implementation as simple as possible and to allow the
different drivers to change/update/version their private attributes without
having to change each time the IFLA_PORT_* list of attributes, I would propose
to define IFLA_PORT_DATA of type NLA_UNSPEC: it is up to the consumer to
interpret/parse it.
Of course, the hypothesis here is that the sender knows who the receiver (ie,
most likely the driver) is, and viceversa (otherwise they would not be able to
agree on a data structure type).
Here is how the previously described scheme would change with the addition of
this new attribute:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROFILE,
IFLA_PORT_VSI_TYPE,
IFLA_PORT_INSTANCE_UUID,
IFLA_PORT_HOST_UUID,
IFLA_PORT_REQUEST,
IFLA_PORT_RESPONSE,
IFLA_PORT_DATA, <========== NEW
IFLA_PORT_PROTO_8021QBH,
IFLA_PORT_PROTO_8021QBG,
__IFLA_PORT_MAX,
};
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.
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.
I can see one more advantage. If that hypothetical extra feature
provided by driver ABC (which requires the use of IFLA_PORT_ DATA) will
one day become a sort of generic feature that it makes sense to
implement for all drivers (or for a number of them), those attributes
that used to be embedded into the IFLA_PORT_DATA attribute can now be
made visible to the upper (Netlink) layer and be therefore added to
the IFLA_PORT_* list.
This could be the case of a pre-standard config parameter that gets
confirmed and becomes stable.
Of course, the idea is not that of abusing IFLA_PORT_DATA, but rather
that of allowing comsumers (ie, device drivers or user space apps)
to receive extra parameters needed to implement/provide optimizations.
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).
--------------------------------------------------
9) OPT2: MORE CHANGES TO THE NETLINK SCHEME
--------------------------------------------------
On top of the new nested protocol attributes discussed already, we
could define an additional one where we can put all common parameters.
>From a logical perspective it would be something like this:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROTO_ALL, <--- NEW nested attr
IFLA_PORT_PROTO_8021QBG, <--- NEW nested attr
IFLA_PORT_PROTO_8021QBH, <--- NEW nested attr
__IFLA_PORT_MAX,
};
But since we cannot remove the obsolete attributes, the real
scheme would be this:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROFILE, <-------(BH only) / deprecated
IFLA_PORT_VSI_TYPE, <-------(BG only) / deprecated
IFLA_PORT_INSTANCE_UUID, <-------(common) / deprecated (*)
IFLA_PORT_HOST_UUID, <-------(common) / deprecated (*)
IFLA_PORT_REQUEST, <-------(common) / deprecated (*)
IFLA_PORT_RESPONSE, <-------(common) / deprecated (*)
IFLA_PORT_PROTO_ALL, <-- NEW nested attr
IFLA_PORT_PROTO_8021QBH, <-- NEW nested attr
IFLA_PORT_PROTO_8021QBG, <-- NEW nested attr
__IFLA_PORT_MAX,
};
(*) common parameters that would move inside IFLA_PORT_PROTO_ALL.
This approach would allow us to keep adding shared params
and nested proto attributes without mixing them.
In other words this would NOT be possible:
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF,
IFLA_PORT_PROFILE,
IFLA_PORT_VSI_TYPE,
IFLA_PORT_INSTANCE_UUID,
IFLA_PORT_HOST_UUID,
IFLA_PORT_REQUEST,
IFLA_PORT_RESPONSE,
IFLA_PORT_PROTO_8021QBH,
IFLA_PORT_PROTO_8021QBG,
IFLA_PORT_ABCD1, <---New protos and
IFLA_PORT_PROTO_XYZ, <---new common parameters
IFLA_PORT_ABCD2 <---get mixed
__IFLA_PORT_MAX,
};
I know, it's just a cosmetic detail, but I wanted to mention it
for the sake of completeness.
--------------------------------------------------
10) BACKWARD COMPATIBILITY
--------------------------------------------------
Let me split the comments into two parts: GET vs SET.
a) RTM_SETLINK
The recipient of the message, for example the enic driver, should
1st look for the new protocol attributes IFLA_PORT_PROTO_*.
Only when the latter is not present it should check for the
deprecated attributes.
I would suggest making it ILLEGAL to mix new and deprecated
attributes.
b) RTM_GETLINK
The recipient of the message, for example the enic driver, should
return both the deprecated and the new attributes. This would not
involve much overhead as the number of deprecated attributes is small.
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).
Thanks
/Christian
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC][net-next-2.6 PATCH 1/2] Add new protocol nested IFLA_PORT_PROTO_* attr
2010-12-08 4:29 [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Christian Benvenuti
@ 2010-12-08 4:31 ` Christian Benvenuti
2010-12-08 4:32 ` [RFC][net-next-2.6 PATCH 2/2] Update enic drv to support IFLA_PORT_PROTO_* attributes Christian Benvenuti
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Christian Benvenuti @ 2010-12-08 4:31 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Christian Benvenuti <benve@cisco•com>
Signed-off-by: Christian Benvenuti <benve@cisco•com>
Signed-off-by: Roopa Prabhu <roprabhu@cisco•com>
Signed-off-by: David Wang <dwang2@cisco•com>
---
include/linux/if_link.h | 31 +++++++++++++++++++++++++++++--
include/net/rtnetlink.h | 3 +++
net/core/rtnetlink.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 76 insertions(+), 3 deletions(-)
diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 6485d2a..6f331aa 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -332,12 +332,14 @@ enum {
enum {
IFLA_PORT_UNSPEC,
IFLA_PORT_VF, /* __u32 */
- IFLA_PORT_PROFILE, /* string */
- IFLA_PORT_VSI_TYPE, /* 802.1Qbg (pre-)standard VDP */
+ IFLA_PORT_PROFILE, /* (deprecated) */
+ IFLA_PORT_VSI_TYPE, /* (deprecated) */
IFLA_PORT_INSTANCE_UUID, /* binary UUID */
IFLA_PORT_HOST_UUID, /* binary UUID */
IFLA_PORT_REQUEST, /* __u8 */
IFLA_PORT_RESPONSE, /* __u16, output only */
+ IFLA_PORT_PROTO_8021QBH,
+ IFLA_PORT_PROTO_8021QBG,
__IFLA_PORT_MAX,
};
@@ -378,4 +380,29 @@ struct ifla_port_vsi {
__u8 pad[3];
};
+enum port_proto {
+ PORT_PROTO_UNSPEC,
+ PORT_PROTO_8021QBH,
+ PORT_PROTO_8021QBG,
+ __PORT_PROTO_MAX,
+};
+
+#define PORT_PROTO_MAX (__PORT_PROTO_MAX - 1)
+
+enum {
+ IFLA_PORT_8021QBG_UNSPEC,
+ IFLA_PORT_8021QBG_VSI_TYPE, /* (pre-)standard VDP */
+ __IFLA_PORT_8021QBG_MAX,
+};
+
+#define IFLA_PORT_8021QBG_MAX (__IFLA_PORT_8021QBG_MAX - 1)
+
+enum {
+ IFLA_PORT_8021QBH_UNSPEC,
+ IFLA_PORT_8021QBH_PROFILE,
+ __IFLA_PORT_8021QBH_MAX,
+};
+
+#define IFLA_PORT_8021QBH_MAX (__IFLA_PORT_8021QBH_MAX - 1)
+
#endif /* _LINUX_IF_LINK_H */
diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 4093ca7..bea154c 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -118,6 +118,9 @@ extern int rtnl_af_register(struct rtnl_af_ops *ops);
extern void rtnl_af_unregister(struct rtnl_af_ops *ops);
+extern int rtnl_link_parse_port_proto(enum port_proto proto,
+ struct nlattr *proto_attr,
+ struct nlattr *tb[]);
extern struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[]);
extern struct net_device *rtnl_create_link(struct net *src_net, struct net *net,
char *ifname, const struct rtnl_link_ops *ops, struct nlattr *tb[]);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 750db57..7390b60 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -723,7 +723,15 @@ static size_t rtnl_port_size(const struct net_device *dev)
+ nla_total_size(PORT_UUID_MAX) /* PORT_INSTANCE_UUID */
+ nla_total_size(PORT_UUID_MAX) /* PORT_HOST_UUID */
+ nla_total_size(1) /* PROT_VDP_REQUEST */
- + nla_total_size(2); /* PORT_VDP_RESPONSE */
+ + nla_total_size(2) /* PORT_VDP_RESPONSE */
+
+ + nla_total_size(0) /* PROTO_8021QBH */
+ + nla_total_size(PORT_PROFILE_MAX) /* 8021QBH_PROFILE */
+
+ + nla_total_size(0) /* PROTO_8021QBG */
+ + nla_total_size(sizeof(struct ifla_port_vsi));
+ /* 8021QBG_VSI */
+
size_t vf_ports_size = nla_total_size(sizeof(struct nlattr));
size_t vf_port_size = nla_total_size(sizeof(struct nlattr))
+ port_size;
@@ -1067,6 +1075,18 @@ static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
.len = sizeof(struct ifla_vf_tx_rate) },
};
+static
+const struct nla_policy ifla_port_8021qbh_policy[IFLA_PORT_8021QBH_MAX+1] = {
+ [IFLA_PORT_8021QBH_PROFILE] = { .type = NLA_STRING,
+ .len = PORT_PROFILE_MAX },
+};
+
+static
+const struct nla_policy ifla_port_8021qbg_policy[IFLA_PORT_8021QBG_MAX+1] = {
+ [IFLA_PORT_8021QBG_VSI_TYPE] = { .type = NLA_BINARY,
+ .len = sizeof(struct ifla_port_vsi)},
+};
+
static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
[IFLA_PORT_VF] = { .type = NLA_U32 },
[IFLA_PORT_PROFILE] = { .type = NLA_STRING,
@@ -1079,8 +1099,31 @@ static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
.len = PORT_UUID_MAX },
[IFLA_PORT_REQUEST] = { .type = NLA_U8, },
[IFLA_PORT_RESPONSE] = { .type = NLA_U16, },
+ [IFLA_PORT_PROTO_8021QBH] = { .type = NLA_NESTED },
+ [IFLA_PORT_PROTO_8021QBG] = { .type = NLA_NESTED },
};
+int rtnl_link_parse_port_proto(enum port_proto proto, struct nlattr *proto_attr,
+ struct nlattr *tb[])
+{
+ if ((proto > PORT_PROTO_MAX) || (proto_attr == NULL) || (tb == NULL))
+ return -EINVAL;
+
+ switch (proto) {
+ case PORT_PROTO_UNSPEC:
+ return -EINVAL;
+ case PORT_PROTO_8021QBH:
+ return nla_parse_nested(tb, IFLA_PORT_8021QBH_MAX,
+ proto_attr, ifla_port_8021qbh_policy);
+ case PORT_PROTO_8021QBG:
+ return nla_parse_nested(tb, IFLA_PORT_8021QBG_MAX,
+ proto_attr, ifla_port_8021qbg_policy);
+ default:
+ return -ENOTSUPP;
+ }
+}
+EXPORT_SYMBOL(rtnl_link_parse_port_proto);
+
struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
{
struct net *net;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC][net-next-2.6 PATCH 2/2] Update enic drv to support IFLA_PORT_PROTO_* attributes
2010-12-08 4:29 [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Christian Benvenuti
2010-12-08 4:31 ` [RFC][net-next-2.6 PATCH 1/2] Add new protocol nested " Christian Benvenuti
@ 2010-12-08 4:32 ` Christian Benvenuti
2010-12-09 13:33 ` [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Stefan Berger
2010-12-14 15:47 ` Arnd Bergmann
3 siblings, 0 replies; 9+ messages in thread
From: Christian Benvenuti @ 2010-12-08 4:32 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Christian Benvenuti <benve@cisco•com>
NOTE: Since there are other enic patches currently waiting for
approval on netdev, I will update and re-send this patch
should any of the other patches get accepted in the meantime.
Signed-off-by: Christian Benvenuti <benve@cisco•com>
Signed-off-by: Roopa Prabhu <roprabhu@cisco•com>
Signed-off-by: David Wang <dwang2@cisco•com>
---
drivers/net/enic/enic.h | 2 +
drivers/net/enic/enic_main.c | 73 +++++++++++++++++++++++++++++++++++-------
2 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 7067254..a930fb9 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -32,7 +32,7 @@
#define DRV_NAME "enic"
#define DRV_DESCRIPTION "Cisco VIC Ethernet NIC Driver"
-#define DRV_VERSION "1.4.1.7"
+#define DRV_VERSION "1.4.1.7nl"
#define DRV_COPYRIGHT "Copyright 2008-2010 Cisco Systems, Inc"
#define ENIC_BARS_MAX 6
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9f293fa..c15c302 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1223,24 +1223,14 @@ static int enic_set_port_profile(struct enic *enic, u8 *mac)
return 0;
}
-static int enic_set_vf_port(struct net_device *netdev, int vf,
+static void enic_set_vf_port_parse_global_attr(struct enic *enic,
struct nlattr *port[])
{
- struct enic *enic = netdev_priv(netdev);
-
- memset(&enic->pp, 0, sizeof(enic->pp));
-
if (port[IFLA_PORT_REQUEST]) {
enic->pp.set |= ENIC_SET_REQUEST;
enic->pp.request = nla_get_u8(port[IFLA_PORT_REQUEST]);
}
- if (port[IFLA_PORT_PROFILE]) {
- enic->pp.set |= ENIC_SET_NAME;
- memcpy(enic->pp.name, nla_data(port[IFLA_PORT_PROFILE]),
- PORT_PROFILE_MAX);
- }
-
if (port[IFLA_PORT_INSTANCE_UUID]) {
enic->pp.set |= ENIC_SET_INSTANCE;
memcpy(enic->pp.instance_uuid,
@@ -1252,6 +1242,57 @@ static int enic_set_vf_port(struct net_device *netdev, int vf,
memcpy(enic->pp.host_uuid,
nla_data(port[IFLA_PORT_HOST_UUID]), PORT_UUID_MAX);
}
+}
+
+static void enic_set_vf_port_parse_legacy_attr(struct enic *enic,
+ struct nlattr *port[])
+{
+ if (port[IFLA_PORT_PROFILE]) {
+ enic->pp.set |= ENIC_SET_NAME;
+ memcpy(enic->pp.name, nla_data(port[IFLA_PORT_PROFILE]),
+ PORT_PROFILE_MAX);
+ }
+}
+
+static int enic_set_vf_port_parse_proto_attr(struct enic *enic,
+ struct nlattr *port[])
+{
+ struct nlattr *tb_8021qbh[IFLA_PORT_8021QBH_MAX+1];
+ int err = 0;
+
+ if (!port[IFLA_PORT_PROTO_8021QBH])
+ return -1;
+
+ err = rtnl_link_parse_port_proto(PORT_PROTO_8021QBH,
+ port[IFLA_PORT_PROTO_8021QBH],
+ tb_8021qbh);
+ if (err < 0)
+ goto errout;
+
+ if (tb_8021qbh[IFLA_PORT_8021QBH_PROFILE]) {
+ enic->pp.set |= ENIC_SET_NAME;
+ memcpy(enic->pp.name,
+ nla_data(tb_8021qbh[IFLA_PORT_8021QBH_PROFILE]),
+ PORT_PROFILE_MAX);
+ }
+
+errout:
+ return err;
+}
+
+static int enic_set_vf_port(struct net_device *netdev, int vf,
+ struct nlattr *port[])
+{
+ struct enic *enic = netdev_priv(netdev);
+ int err = 0;
+
+ memset(&enic->pp, 0, sizeof(enic->pp));
+
+ enic_set_vf_port_parse_global_attr(enic, port);
+
+ err = enic_set_vf_port_parse_proto_attr(enic, port);
+ if (err < 0)
+ enic_set_vf_port_parse_legacy_attr(enic, port);
/* don't support VFs, yet */
if (vf != PORT_SELF_VF)
@@ -1280,6 +1321,7 @@ static int enic_get_vf_port(struct net_device *netdev, int vf,
struct enic *enic = netdev_priv(netdev);
int err, error, done;
u16 response = PORT_PROFILE_RESPONSE_SUCCESS;
+ struct nlattr *proto;
if (!(enic->pp.set & ENIC_SET_APPLIED))
return -ENODATA;
@@ -1309,7 +1351,7 @@ static int enic_get_vf_port(struct net_device *netdev, int vf,
NLA_PUT_U16(skb, IFLA_PORT_REQUEST, enic->pp.request);
NLA_PUT_U16(skb, IFLA_PORT_RESPONSE, response);
- if (enic->pp.set & ENIC_SET_NAME)
+ if (enic->pp.set & ENIC_SET_NAME) /* Deprecated */
NLA_PUT(skb, IFLA_PORT_PROFILE, PORT_PROFILE_MAX,
enic->pp.name);
if (enic->pp.set & ENIC_SET_INSTANCE)
@@ -1319,6 +1361,13 @@ static int enic_get_vf_port(struct net_device *netdev, int vf,
NLA_PUT(skb, IFLA_PORT_HOST_UUID, PORT_UUID_MAX,
enic->pp.host_uuid);
+ proto = nla_nest_start(skb, IFLA_PORT_PROTO_8021QBH);
+ if (proto == NULL)
+ goto nla_put_failure;
+ NLA_PUT(skb, IFLA_PORT_8021QBH_PROFILE, PORT_PROFILE_MAX,
+ enic->pp.name);
+ nla_nest_end(skb, proto);
+
return 0;
nla_put_failure:
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
2010-12-08 4:29 [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Christian Benvenuti
2010-12-08 4:31 ` [RFC][net-next-2.6 PATCH 1/2] Add new protocol nested " Christian Benvenuti
2010-12-08 4:32 ` [RFC][net-next-2.6 PATCH 2/2] Update enic drv to support IFLA_PORT_PROTO_* attributes Christian Benvenuti
@ 2010-12-09 13:33 ` Stefan Berger
2010-12-12 0:53 ` Christian Benvenuti (benve)
2010-12-14 15:47 ` Arnd Bergmann
3 siblings, 1 reply; 9+ messages in thread
From: Stefan Berger @ 2010-12-09 13:33 UTC (permalink / raw)
To: netdev
Christian Benvenuti <benve <at> cisco.com> writes:
>
> 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). 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.
Stefan
> Thanks
> /Christian
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
2010-12-09 13:33 ` [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Stefan Berger
@ 2010-12-12 0:53 ` Christian Benvenuti (benve)
2010-12-12 3:16 ` Stefan Berger
0 siblings, 1 reply; 9+ messages in thread
From: Christian Benvenuti (benve) @ 2010-12-12 0:53 UTC (permalink / raw)
To: Stefan Berger
Cc: netdev, chrisw, arnd, David Miller, Roopa Prabhu (roprabhu),
David Wang (dwang2)
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
2010-12-12 0:53 ` Christian Benvenuti (benve)
@ 2010-12-12 3:16 ` Stefan Berger
0 siblings, 0 replies; 9+ messages in thread
From: Stefan Berger @ 2010-12-12 3:16 UTC (permalink / raw)
To: netdev
"Christian Benvenuti (benve)" <benve@cisco•com> wrote on 12/11/2010 07:53:44 PM:
[posting via we page since my mail program puts html into the message]
>
> (*)
>
>
> >> 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.
>
What I meant is that the user-level code (i.e., libvirt) with your new
attributes will only compile on a system that has their names in the
if_link.h. So, users that compile libvirt with a kernel that doesn't have
these new attributes in if_link.h should still be able to compile the
macvtap.c there, but will of course only get support for the old attributes,
which at least seem to be in working condition for 802.1Qbg. User who
compile libvirt with a new kernel should then get the support for the old
attributes and the new attributes compiled in. To cover these cases you'd
have to isolate the code dealing with the new attributes with for example
#ifdef NEW_8021QBGH_ATTRIBUTES
[...]
#endif
where the #define NEW_8021QBGH_ATTRIBUTES would be set through
libvirt's configure script that determines it by test-compiling a program
using one of the new attributes and set the #define if one of the new
attributes was found to be available.. Ideally your CLUSTER_UUID support
would not come too long after the first batch of new attributes, but would
be 'covered' with the NEW_8021QBGH_ATTRIBUTES. Along those lines, I hope you
won't have to introduce a VERY_NEW_8021QBGH_ATTRIBUTES #define to cover
anything coming even later.
> >> 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 thing is that you may have users compiling an application, i.e.,
libvirt, on systems with different versions of the kernel, and with
that the if_link.h, and those users may want to be able to follow the
application source tree and not necessarily the linux tree. After
updating the application source tree they should still get working
802.1Qbg/h support. So requirements on the underlying kernel shouldn't
just 'move'.
I wouldn't like it if I updated my libvirt tree and all of a sudden the
802.1Qbg/h support wouldn't work anymore, due to requirements for new
attributes in libvirt's macvtap.c, after a re-compile of libvirt. From
a libvirt programmer perspective *all* the attributes would *ideally*
appear from one kernel version to the other. So if one kernel version
provides working support for 802.1Qbg/h I would want to be able to follow the
libvirt tree without having to upgrade the kernel. So that's why I am
proposing the #ifdef's around nearly each step of new attributes being added
in the if_link.h, assuming that at each step the technology was working
and new attributes only make it work 'better' than before.
>
> 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).
It's fine by me if we end up sending a message containing old and
new attributes. For dealing with the responses we will have to start
looking for the newest attributes first, then go backwards towards the
first ones. Knowing the version of the supported attributes we could
invoke specific code for the attributes that are expected to be supported.
So I think knowing the version would be advantageous in interpreting
the responses from the kernel.
Nevertheless I would go through the complication of #ifdef'ing
each new major step of new attributes in the libvirt code...
>
> 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.
I agree.
> 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?
> >
> >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).
The complication comes through *following* an evolving standard rather
than having the *ideal* case where we go from darkness to light from one
kernel version to another -- or make support for the new technology
only public once it's done.
> 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.
Ok.
>
> 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)?
To quote from there:
"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."
Hm, does this sound like an 'experimental' attribute that can change over time
('some stability')? I hope not, because otherwise you may have mismatches
between what libvirt assumes it needs to pass versus what the evolving device
driver expects. If so, we would have to go as far as determining the version of
the device driver to match up libvirt code against it (?).
Regards
Stefan
>
> Thanks
> /Christian
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
2010-12-08 4:29 [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Christian Benvenuti
` (2 preceding siblings ...)
2010-12-09 13:33 ` [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Stefan Berger
@ 2010-12-14 15:47 ` Arnd Bergmann
2010-12-14 22:59 ` Christian Benvenuti (benve)
3 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2010-12-14 15:47 UTC (permalink / raw)
To: Christian Benvenuti; +Cc: davem, netdev
On Wednesday 08 December 2010, Christian Benvenuti wrote:
>
> 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.
> 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.
> --------------------------------------------------
> 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.
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.
> --------------------------------------------------
> 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.
> 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.
> 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.
> 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.
Just don't add driver-private interfaces, make them official!
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.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
2010-12-14 15:47 ` Arnd Bergmann
@ 2010-12-14 22:59 ` Christian Benvenuti (benve)
2010-12-15 13:33 ` Arnd Bergmann
0 siblings, 1 reply; 9+ messages in thread
From: Christian Benvenuti (benve) @ 2010-12-14 22:59 UTC (permalink / raw)
To: Arnd Bergmann
Cc: netdev, davem, Stefan Berger, Roopa Prabhu (roprabhu),
David Wang (dwang2)
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr
2010-12-14 22:59 ` Christian Benvenuti (benve)
@ 2010-12-15 13:33 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2010-12-15 13:33 UTC (permalink / raw)
To: Christian Benvenuti (benve)
Cc: netdev, davem, Stefan Berger, Roopa Prabhu (roprabhu),
David Wang (dwang2)
On Tuesday 14 December 2010, Christian Benvenuti (benve) wrote:
> 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.
Right. I understood this from your original mail, but I still
disagree. I can't think of any reason why a future could have
a bigger impact than the change you are proposing now.
> > 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.
Yes, that's true. It does mean though that every user of the interface
would have to deal with the old and the new interface. Deprecating a
kernel interface does not mean that we can stop supporting it in the
future.
> > 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.
If it's vendor specific, it's normally a bad idea, or not defined
as generic as it should be ;-)
Think of it this way: if the rest of the world thinks that they
should not implement this feature, maybe you shouldn't either.
If the feature is going to be implemented slightly differently
on other hardware, make the interface generic enough to cope
with either version.
> 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.
No. The right solution would be in that case to not implement the interface
at all until there is agreement on a cross-vendor solution. Generally
this is not the problem, because we deal with smart people that can
introduce an interface in a way that works for themselves and other
future users.
> > > 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).
There is a significant difference between interfaces per functionality,
like we have GETLINK take different arguments on a vlan device than
on a bridge, and having interfaces on multiple device drivers implementing
the same thing. IMHO, there is nothing wrong with having a Cisco switch
specific interface in the IFLA_PORT_* family, and have only enic
implement this, as long as the interface allows other device drivers
to implement the exact same semantics when they want to talk to the
same switch.
> > 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.
Yes, I think it's good that you brought it up. A lot of people want
private interfaces like this, and we should just document that this
is not the way to do it in Linux.
> 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")
It's not a vote, in the end you have to convince davem to take
it. My position is "No", as I have made clear, but this is mostly
because I see the advantages as insignificant and don't think
it's worth it.
> (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.
No objections from me at all here.
> (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).
This is where the large problem arises:
* #ifdef is bad because that means breaking compatibility with other
versions. As the qemu/kvm people found out years ago, the only
sane way to handle this is to keep the latest version of the interface
headers shipping with the code using it, and doing run-time checks
everywhere.
* Versioning is something we don't normally do in kernel interfaces,
it is not enough to describe the complexity, especially with selective
features getting backported into distributions etc. You essentially
have to try using an attribute to see if the kernel supports it.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-12-15 13:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-08 4:29 [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Christian Benvenuti
2010-12-08 4:31 ` [RFC][net-next-2.6 PATCH 1/2] Add new protocol nested " Christian Benvenuti
2010-12-08 4:32 ` [RFC][net-next-2.6 PATCH 2/2] Update enic drv to support IFLA_PORT_PROTO_* attributes Christian Benvenuti
2010-12-09 13:33 ` [RFC][net-next-2.6 PATCH 0/2] rtnetlink: New IFLA_PORT_PROTO_* attr Stefan Berger
2010-12-12 0:53 ` Christian Benvenuti (benve)
2010-12-12 3:16 ` Stefan Berger
2010-12-14 15:47 ` Arnd Bergmann
2010-12-14 22:59 ` Christian Benvenuti (benve)
2010-12-15 13:33 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox