[RFC] bridge and friends: reduce TheLinuxWay(tm)

[RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

IOW, TheCutNpasteTrain.

There's a lot of clutter on the netlink interface used
by bridge and vxlan.
1) A lot of things which are boolean on/off end up using a uchar.
Example:
IFLA_BRPORT_MODE, IFLA_BRPORT_GUARD, IFLA_BRPORT_PROTECT,
IFLA_BRPORT_LEARNING, IFLA_BRPORT_UNICAST_FLOOD, BRIDGE_VLAN_INFO_PVID,
BRIDGE_VLAN_INFO_UNTAGGED, BRIDGE_MODE_VEPA,
IFLA_VXLAN_PROXY,IFLA_VXLAN_RSC, etc

2) There's a few fields which are basically intended to project the
same message to the kernel but are redefined a few times:
Example:
BRIDGE_VLAN_INFO_MASTER vs NTF_MASTER vs BRIDGE_FLAGS_MASTER

Also i am not sure why multicast snooping needs its own subheader.

Is it too late to make changes? git logs shows some of these feature
have only been on the last 2-3 months.
One approach to resolve this is introduce a new BRIDGE_FLAGS TLV
which will work like the ifi_flags/change and put all these flags in
one spot. New iproute will use these and the old one will continue using
the old approach. It would require to EOL the old interface at some
point.

If this is agreeable and it is not on someone todo list - I will send
patches later on.

cheers,
jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Stephen Hemminger]

On Mon, 14 Oct 2013 17:32:05 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> IOW, TheCutNpasteTrain.
> 
> There's a lot of clutter on the netlink interface used
> by bridge and vxlan.
> 1) A lot of things which are boolean on/off end up using a uchar.
> Example:
> IFLA_BRPORT_MODE, IFLA_BRPORT_GUARD, IFLA_BRPORT_PROTECT,
> IFLA_BRPORT_LEARNING, IFLA_BRPORT_UNICAST_FLOOD, BRIDGE_VLAN_INFO_PVID,
> BRIDGE_VLAN_INFO_UNTAGGED, BRIDGE_MODE_VEPA,
> IFLA_VXLAN_PROXY,IFLA_VXLAN_RSC, etc
> 
> 2) There's a few fields which are basically intended to project the
> same message to the kernel but are redefined a few times:
> Example:
> BRIDGE_VLAN_INFO_MASTER vs NTF_MASTER vs BRIDGE_FLAGS_MASTER
> 
> Also i am not sure why multicast snooping needs its own subheader.
> 
> Is it too late to make changes? git logs shows some of these feature
> have only been on the last 2-3 months.
> One approach to resolve this is introduce a new BRIDGE_FLAGS TLV
> which will work like the ifi_flags/change and put all these flags in
> one spot. New iproute will use these and the old one will continue using
> the old approach. It would require to EOL the old interface at some
> point.

Unfortunately, by now this is all set in ABI.
It was a side effect of the per-feature evolutionary style of development.

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

On 10/14/13 17:41, Stephen Hemminger wrote:

> Unfortunately, by now this is all set in ABI.
> It was a side effect of the per-feature evolutionary style of development.

Sadly, I agree. This is the dark side of "have code will travel";
you let these things out in the wild, they breed and you cant
take them back.
BTW: I dont think what i suggested will be a harmful refactoring because
no existing interfaces are removed - your call.

In similar vein:
What is the motivation behind IFLA_EXT_MASK? Could you not have used
ifm ifindex to relay the interface of interest? Currently the ifindex is
not used at all. IMO, the following interfaces are useful:
- get attributes for all bridge ports (this is there)
- get attributes for bridge interface XXX; there using IFLA_EXT_MASK
I think it should be using ifm->ifindex
- get attributes for all bridge ports for bridge br-blah (not there)
you could also use the ifindex of br-blah here instead

Separate issue:
To provide equivalence to brctl:
- PF_BRIDGE should allow me to attach a bridge port to bridge of choice
with SETLINK

cheers,
jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

On 10/16/13 09:54, Jamal Hadi Salim wrote:

> - get attributes for bridge interface XXX; there using IFLA_EXT_MASK
> I think it should be using ifm->ifindex


No this doesnt work. Filtering needs to be done in user space ;-<
IMO: The behavior here should be no different then when getting a link.
Will you accept a patch for this?

cheers,
jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Vlad Yasevich]

On 10/16/2013 09:54 AM, Jamal Hadi Salim wrote:
> On 10/14/13 17:41, Stephen Hemminger wrote:
>
>> Unfortunately, by now this is all set in ABI.
>> It was a side effect of the per-feature evolutionary style of
>> development.
>
> Sadly, I agree. This is the dark side of "have code will travel";
> you let these things out in the wild, they breed and you cant
> take them back.
> BTW: I dont think what i suggested will be a harmful refactoring because
> no existing interfaces are removed - your call.
>
> In similar vein:
> What is the motivation behind IFLA_EXT_MASK?

This was to display or filter out virtual function data.

> Could you not have used
> ifm ifindex to relay the interface of interest? Currently the ifindex is
> not used at all. IMO, the following interfaces are useful:
> - get attributes for all bridge ports (this is there)
> - get attributes for bridge interface XXX; there using IFLA_EXT_MASK
> I think it should be using ifm->ifindex

I probably doesn't need to use this as we want the bridge data, not the 
VF data stored as part of PF interface.

> - get attributes for all bridge ports for bridge br-blah (not there)
> you could also use the ifindex of br-blah here instead

This would be usefull.


>
> Separate issue:
> To provide equivalence to brctl:
> - PF_BRIDGE should allow me to attach a bridge port to bridge of choice
> with SETLINK

You can already do this with:

	ip link set dev ethX master brX

I know, not very intuative, but it's there :(

-vlad



>
> cheers,
> jamal
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

On 10/16/13 13:11, Vlad Yasevich wrote:
> On 10/16/2013 09:54 AM, Jamal Hadi Salim wrote:
>
>
> This was to display or filter out virtual function data.
>

Stephen explained - and it seems to make sense (working around
netlink weaknesses). It could be made more generic.


> I probably doesn't need to use this as we want the bridge data, not the
> VF data stored as part of PF interface.
>

I think we need some filtering in the kernel. As it stands today,
unfortunately you get everything ;-> I was suprised at the amount of
data i get from the kernel these days when i ask for a simple netdev
info (I think no less than 1K per netdev; everything from /proc entries
from some bread crumbs i dont see any use for.

>> - get attributes for all bridge ports for bridge br-blah (not there)
>> you could also use the ifindex of br-blah here instead
>
> This would be usefull.
>

Its a usability improvement.
We still have the ifi->ifindex available. It could be used to signal
which bridge device's ports i want.
But then i get _all_ the ports for that bridge.

>
>
> You can already do this with:
>
>      ip link set dev ethX master brX
>

Yes, I know - it sucks from a usability pov.

cheers,
jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Eric Dumazet]

On Wed, 2013-10-16 at 13:49 -0400, Jamal Hadi Salim wrote:

> I think we need some filtering in the kernel. As it stands today,
> unfortunately you get everything ;-> I was suprised at the amount of
> data i get from the kernel these days when i ask for a simple netdev
> info (I think no less than 1K per netdev; everything from /proc entries
> from some bread crumbs i dont see any use for.

By the way, "ip link show dev xxxx" seems to dump all devices info from
the kernel...

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Stephen Hemminger]

On Wed, 16 Oct 2013 11:35:50 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2013-10-16 at 13:49 -0400, Jamal Hadi Salim wrote:
> 
> > I think we need some filtering in the kernel. As it stands today,
> > unfortunately you get everything ;-> I was suprised at the amount of
> > data i get from the kernel these days when i ask for a simple netdev
> > info (I think no less than 1K per netdev; everything from /proc entries
> > from some bread crumbs i dont see any use for.
> 
> By the way, "ip link show dev xxxx" seems to dump all devices info from
> the kernel...
> 
> 
As I remember.

The problem is that according to original netlink design there is supposed to
be a NLM_F_MATCH, but it seems it never got implemented right, and the legacy
of breaking applications keeps it from happening.

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Vlad Yasevich]

On 10/16/2013 02:35 PM, Eric Dumazet wrote:
> On Wed, 2013-10-16 at 13:49 -0400, Jamal Hadi Salim wrote:
>
>> I think we need some filtering in the kernel. As it stands today,
>> unfortunately you get everything ;-> I was suprised at the amount of
>> data i get from the kernel these days when i ask for a simple netdev
>> info (I think no less than 1K per netdev; everything from /proc entries
>> from some bread crumbs i dont see any use for.
>
> By the way, "ip link show dev xxxx" seems to dump all devices info from
> the kernel...
>
>

Right.  ip link show is dumb.  It asks for info from all devices and
then filters based on the device you asked for, but the filtering
is done in iproute instead of the kernel.

brdige command inherited this through code re-use.

-vlad

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

On 10/16/13 14:50, Vlad Yasevich wrote:

> Right.  ip link show is dumb.  It asks for info from all devices and
> then filters based on the device you asked for, but the filtering
> is done in iproute instead of the kernel.
>

That maybe small flaw in iproute2 - but there's no issue in the kernel.
You can send an ifi and specify the proper ifindex of choice.
But you cant do the same with bridge.
Thats what i was whining to Stephen about. I get every
bridge port with no exception and there's no way to specify one
(the ifi ifindex is never used).
Think 20K bridge ports - each giving me 1K of data ...
scalability problem

cheers,
jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

And another little glitch, iflink specifies

     [IFLA_AF_SPEC] = {
         [AF_INET] = {
             [IFLA_INET_CONF] = ...,
         },
         [AF_INET6] = {
             [IFLA_INET6_FLAGS] = ...,
             [IFLA_INET6_CONF] = ...,
         }

But then bridge hijacks it and specifies:
   [IFLA_AF_SPEC] = {
       [IFLA_BRIDGE_FLAGS]
       [IFLA_BRIDGE_MODE]
       [IFLA_BRIDGE_VLAN_INFO]
   }

Was that intended as:

  [IFLA_AF_SPEC] = {
    [AF_BRIDGE] = {
        [IFLA_BRIDGE_FLAGS]
        [IFLA_BRIDGE_MODE]
        [IFLA_BRIDGE_VLAN_INFO]
    }
  }

Now granted that bridging uses PF_BRIDGE as the family and iflink uses
PF_UNSPEC - but i do think this one is polluting the namespace.

cheers,
jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Vlad Yasevich]

On 10/16/2013 04:19 PM, Jamal Hadi Salim wrote:
>
> And another little glitch, iflink specifies
>
>      [IFLA_AF_SPEC] = {
>          [AF_INET] = {
>              [IFLA_INET_CONF] = ...,
>          },
>          [AF_INET6] = {
>              [IFLA_INET6_FLAGS] = ...,
>              [IFLA_INET6_CONF] = ...,
>          }
>
> But then bridge hijacks it and specifies:
>    [IFLA_AF_SPEC] = {
>        [IFLA_BRIDGE_FLAGS]
>        [IFLA_BRIDGE_MODE]
>        [IFLA_BRIDGE_VLAN_INFO]
>    }
>
> Was that intended as:
>
>   [IFLA_AF_SPEC] = {
>     [AF_BRIDGE] = {
>         [IFLA_BRIDGE_FLAGS]
>         [IFLA_BRIDGE_MODE]
>         [IFLA_BRIDGE_VLAN_INFO]
>     }
>   }
>
> Now granted that bridging uses PF_BRIDGE as the family and iflink uses
> PF_UNSPEC - but i do think this one is polluting the namespace.
>

Yes, I know about that one.  BRIDGE_FLAGS started the polution and then 
we continued it with the other attributes....

-vlad

> cheers,
> jamal

Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)

[Jamal Hadi Salim]

On 10/16/13 16:22, Vlad Yasevich wrote:

> Yes, I know about that one.  BRIDGE_FLAGS started the polution and then
> we continued it with the other attributes....
>

Understood - note the email has subject TheLinuxWay ;->
So i fear this one also falls under "it is already in the wild, cant do
anything about it".

BTW: the flooding and learning knobs are still not exposed to iproute2.

In regards to a GET always being translated to DUMP forcing user space
to filter:
If neither you nor Stephen planning to take of it, I will because it
is a scaling problem.  Please let me know.


cheers,
jamal