[PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This series moves bridge stp update api to follow the setlink/dellink
apis to propagate stp state updates to switch ports on stacked netdevs

v1 -> v2:
	fix comment
	add CONFIG_NET_SWITCHDEV checks

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Roopa Prabhu (3):
  switchdev: fix stp update API to work with layered netdevices
  bond: add support for ndo_switch_port_stp_update when bond is a
    bridge port
  team: add support for ndo_switch_port_stp_update when team dev is a
    bridge port

 drivers/net/bonding/bond_main.c |    3 +++
 drivers/net/team/team.c         |    3 +++
 include/net/switchdev.h         |    4 ++++
 net/switchdev/switchdev.c       |   30 +++++++++++++++++++++++++++++-
 4 files changed, 39 insertions(+), 1 deletion(-)

-- 
1.7.10.4

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[David Miller]

From: roopa@cumulusnetworks.com
Date: Thu, 12 Mar 2015 17:29:55 -0700

> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This series moves bridge stp update api to follow the setlink/dellink
> apis to propagate stp state updates to switch ports on stacked netdevs
> 
> v1 -> v2:
> 	fix comment
> 	add CONFIG_NET_SWITCHDEV checks
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Can't we just do this stacked device traveral transparently?

That seems much cleaner to me.

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[roopa]

On 3/12/15, 9:10 PM, David Miller wrote:
> From: roopa@cumulusnetworks.com
> Date: Thu, 12 Mar 2015 17:29:55 -0700
>
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This series moves bridge stp update api to follow the setlink/dellink
>> apis to propagate stp state updates to switch ports on stacked netdevs
>>
>> v1 -> v2:
>> 	fix comment
>> 	add CONFIG_NET_SWITCHDEV checks
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> Can't we just do this stacked device traveral transparently?
>
> That seems much cleaner to me.
David, if you mean not touch bond and team but have the switchdev api do 
it transparently, yes, i had it that way initially.
And i do liked it that way as well. But the feedback i received (during 
the initial introduction of this for setlink/dellink) was to make it 
explicit for each master.

Thanks,
Roopa

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[David Miller]

From: roopa <roopa@cumulusnetworks.com>
Date: Fri, 13 Mar 2015 06:25:24 -0700

> David, if you mean not touch bond and team but have the switchdev
> api do it transparently, yes, i had it that way initially.  And i do
> liked it that way as well. But the feedback i received (during the
> initial introduction of this for setlink/dellink) was to make it
> explicit for each master.

I think the concern is that we only want to do this for devices
for which it is safe to "traverse" down like this.

But frankly I cannot think of any layered device where we would
not want to do this.

Let's go back to the simple scheme where we unconditionally traverse
and if we hit a problem case we'll figure out how to deal with it
then, ok?

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[roopa]

On 3/13/15, 9:31 AM, David Miller wrote:
> From: roopa <roopa@cumulusnetworks.com>
> Date: Fri, 13 Mar 2015 06:25:24 -0700
>
>> David, if you mean not touch bond and team but have the switchdev
>> api do it transparently, yes, i had it that way initially.  And i do
>> liked it that way as well. But the feedback i received (during the
>> initial introduction of this for setlink/dellink) was to make it
>> explicit for each master.
> I think the concern is that we only want to do this for devices
> for which it is safe to "traverse" down like this.
>
> But frankly I cannot think of any layered device where we would
> not want to do this.
>
> Let's go back to the simple scheme where we unconditionally traverse
> and if we hit a problem case we'll figure out how to deal with it
> then, ok?
ok, sounds great. I will respin. Thanks!.

I wanted to move all three ops setlink/dellink/stpupdate initially, but 
now realize that
for something like the below to work, you will need the setlink/dellink 
support in the
bond and team driver because the call goes into the 'dev' driver 
directly without
calling into the switchdev api.

bridge link set dev bond0 learning off self

This case can be fixed too, however, it needs some more work in 
rtnetlink.c handling of 'self'.
The stp update api does not have this problem. I will respin the current 
stp update series first.

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[Jiri Pirko]

Fri, Mar 13, 2015 at 05:31:56PM CET, davem@davemloft.net wrote:
>From: roopa <roopa@cumulusnetworks.com>
>Date: Fri, 13 Mar 2015 06:25:24 -0700
>
>> David, if you mean not touch bond and team but have the switchdev
>> api do it transparently, yes, i had it that way initially.  And i do
>> liked it that way as well. But the feedback i received (during the
>> initial introduction of this for setlink/dellink) was to make it
>> explicit for each master.
>
>I think the concern is that we only want to do this for devices
>for which it is safe to "traverse" down like this.

Yes, that was my point. Also, some layered drivers might want to do some
individual magic, propagate on condition, etc. I think it is clearer
architecture. And also, you can see right away what is happening. By
doing the travelsal directly in switchdev code, that is in the shadow :/


>
>But frankly I cannot think of any layered device where we would
>not want to do this.
>
>Let's go back to the simple scheme where we unconditionally traverse
>and if we hit a problem case we'll figure out how to deal with it
>then, ok?

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[Scott Feldman]

On Fri, Mar 13, 2015 at 9:31 AM, David Miller <davem@davemloft.net> wrote:
> From: roopa <roopa@cumulusnetworks.com>
> Date: Fri, 13 Mar 2015 06:25:24 -0700
>
>> David, if you mean not touch bond and team but have the switchdev
>> api do it transparently, yes, i had it that way initially.  And i do
>> liked it that way as well. But the feedback i received (during the
>> initial introduction of this for setlink/dellink) was to make it
>> explicit for each master.
>
> I think the concern is that we only want to do this for devices
> for which it is safe to "traverse" down like this.
>
> But frankly I cannot think of any layered device where we would
> not want to do this.
>
> Let's go back to the simple scheme where we unconditionally traverse
> and if we hit a problem case we'll figure out how to deal with it
> then, ok?

Here's a way to do it without touching team/bonding/vlan drivers, but
also giving us flexibility to down the road to intercept the call in
the team/bonding/vlan driver and change the behavior by implementing
swdev_port_stp_update in the middle driver.

I would prefer this approach not only for STP updates but also for
port attr set/get calls.


/**
 *      netdev_switch_port_stp_update - Notify switch device port of STP
 *                                      state change
 *      @dev: bridge port device
 *      @state: bridge port STP state
 *
 *      Notify switch device port of bridge port STP state change.
 */
int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
{
        const struct swdev_ops *ops = dev->swdev_ops;
        struct net_device *lower_dev;
        struct list_head *iter;
        int err = -EOPNOTSUPP;

        if (ops && ops->swdev_port_stp_update)
                return ops->swdev_port_stp_update(dev, state);

        /* Bridged switch device port(s) may be stacked under
         * bond/team/vlan dev, so recurse down to stp-update
         * them one at a time.
         */

        netdev_for_each_lower_dev(dev, lower_dev, iter) {
                err = netdev_switch_port_stp_update(lower_dev, state);
                if (err && err != -EOPNOTSUPP)
                        return err;
        }

        return err;
}
EXPORT_SYMBOL_GPL(netdev_switch_port_stp_update);

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[Jiri Pirko]

Sun, Mar 15, 2015 at 07:08:02PM CET, sfeldma@gmail.com wrote:
>On Fri, Mar 13, 2015 at 9:31 AM, David Miller <davem@davemloft.net> wrote:
>> From: roopa <roopa@cumulusnetworks.com>
>> Date: Fri, 13 Mar 2015 06:25:24 -0700
>>
>>> David, if you mean not touch bond and team but have the switchdev
>>> api do it transparently, yes, i had it that way initially.  And i do
>>> liked it that way as well. But the feedback i received (during the
>>> initial introduction of this for setlink/dellink) was to make it
>>> explicit for each master.
>>
>> I think the concern is that we only want to do this for devices
>> for which it is safe to "traverse" down like this.
>>
>> But frankly I cannot think of any layered device where we would
>> not want to do this.
>>
>> Let's go back to the simple scheme where we unconditionally traverse
>> and if we hit a problem case we'll figure out how to deal with it
>> then, ok?
>
>Here's a way to do it without touching team/bonding/vlan drivers, but
>also giving us flexibility to down the road to intercept the call in
>the team/bonding/vlan driver and change the behavior by implementing
>swdev_port_stp_update in the middle driver.
>
>I would prefer this approach not only for STP updates but also for
>port attr set/get calls.
>
>
>/**
> *      netdev_switch_port_stp_update - Notify switch device port of STP
> *                                      state change
> *      @dev: bridge port device
> *      @state: bridge port STP state
> *
> *      Notify switch device port of bridge port STP state change.
> */
>int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
>{
>        const struct swdev_ops *ops = dev->swdev_ops;
>        struct net_device *lower_dev;
>        struct list_head *iter;
>        int err = -EOPNOTSUPP;
>
>        if (ops && ops->swdev_port_stp_update)
>                return ops->swdev_port_stp_update(dev, state);
>
>        /* Bridged switch device port(s) may be stacked under
>         * bond/team/vlan dev, so recurse down to stp-update
>         * them one at a time.
>         */
>
>        netdev_for_each_lower_dev(dev, lower_dev, iter) {
>                err = netdev_switch_port_stp_update(lower_dev, state);
>                if (err && err != -EOPNOTSUPP)
>                        return err;
>        }


I don't like this blind approach. Note that with this, you enable this
for lower devices of:
bond
ipvlan
macvlan
team
vlan
batman-adv

Does that make sense? I think it is better just pick what we want to
support now and implement it there. That makes things clearer and
confusion-prone.

Jiri

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[Scott Feldman]

On Sun, Mar 15, 2015 at 12:17 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sun, Mar 15, 2015 at 07:08:02PM CET, sfeldma@gmail.com wrote:
>>On Fri, Mar 13, 2015 at 9:31 AM, David Miller <davem@davemloft.net> wrote:
>>> From: roopa <roopa@cumulusnetworks.com>
>>> Date: Fri, 13 Mar 2015 06:25:24 -0700
>>>
>>>> David, if you mean not touch bond and team but have the switchdev
>>>> api do it transparently, yes, i had it that way initially.  And i do
>>>> liked it that way as well. But the feedback i received (during the
>>>> initial introduction of this for setlink/dellink) was to make it
>>>> explicit for each master.
>>>
>>> I think the concern is that we only want to do this for devices
>>> for which it is safe to "traverse" down like this.
>>>
>>> But frankly I cannot think of any layered device where we would
>>> not want to do this.
>>>
>>> Let's go back to the simple scheme where we unconditionally traverse
>>> and if we hit a problem case we'll figure out how to deal with it
>>> then, ok?
>>
>>Here's a way to do it without touching team/bonding/vlan drivers, but
>>also giving us flexibility to down the road to intercept the call in
>>the team/bonding/vlan driver and change the behavior by implementing
>>swdev_port_stp_update in the middle driver.
>>
>>I would prefer this approach not only for STP updates but also for
>>port attr set/get calls.
>>
>>
>>/**
>> *      netdev_switch_port_stp_update - Notify switch device port of STP
>> *                                      state change
>> *      @dev: bridge port device
>> *      @state: bridge port STP state
>> *
>> *      Notify switch device port of bridge port STP state change.
>> */
>>int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
>>{
>>        const struct swdev_ops *ops = dev->swdev_ops;
>>        struct net_device *lower_dev;
>>        struct list_head *iter;
>>        int err = -EOPNOTSUPP;
>>
>>        if (ops && ops->swdev_port_stp_update)
>>                return ops->swdev_port_stp_update(dev, state);
>>
>>        /* Bridged switch device port(s) may be stacked under
>>         * bond/team/vlan dev, so recurse down to stp-update
>>         * them one at a time.
>>         */
>>
>>        netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>                err = netdev_switch_port_stp_update(lower_dev, state);
>>                if (err && err != -EOPNOTSUPP)
>>                        return err;
>>        }
>
>
> I don't like this blind approach. Note that with this, you enable this
> for lower devices of:
> bond
> ipvlan
> macvlan
> team
> vlan
> batman-adv

Only when they're used under a bridge because
netdev_switch_port_stp_update() is currently only called from the
bridge.  So this is what we want, to propagate the stp state change
down to the base port, regardless of middle drivers.  If you want to
exclude a middle driver from propagating stp down to the base port, or
otherwise change the default propagation, add stub
swdev_port_stp_update to middle driver.

> Does that make sense? I think it is better just pick what we want to
> support now and implement it there. That makes things clearer and
> confusion-prone.

I'm proposing a simple, permissive solution, with a provision for an
out on a case-by-case basis.  Right now I don't see any reason to
special case any middle driver; just let STP state propagate down to
the port device.

-scott

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Sun, 15 Mar 2015 17:56:32 +0100

> Fri, Mar 13, 2015 at 05:31:56PM CET, davem@davemloft.net wrote:
>>From: roopa <roopa@cumulusnetworks.com>
>>Date: Fri, 13 Mar 2015 06:25:24 -0700
>>
>>> David, if you mean not touch bond and team but have the switchdev
>>> api do it transparently, yes, i had it that way initially.  And i do
>>> liked it that way as well. But the feedback i received (during the
>>> initial introduction of this for setlink/dellink) was to make it
>>> explicit for each master.
>>
>>I think the concern is that we only want to do this for devices
>>for which it is safe to "traverse" down like this.
> 
> Yes, that was my point. Also, some layered drivers might want to do some
> individual magic, propagate on condition, etc. I think it is clearer
> architecture. And also, you can see right away what is happening. By
> doing the travelsal directly in switchdev code, that is in the shadow :/

I think special behavior would be the exception rather than the norm,
and we should code our interfaces for the common case.

This means we can add a per-device flag to avoid this logic if
necessary.

Notice how you use lots of words like "might", and I hate designing
interfaces without hard explicit real word examples of the actual
precise need.

So don't use open ended reasons like that for justifying the design of
our interfaces.

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Sun, 15 Mar 2015 20:17:22 +0100

> Does that make sense? I think it is better just pick what we want to
> support now and implement it there. That makes things clearer and
> confusion-prone.

You can't even say whether it makes sense or not, you yourself
even have to ask.

Again, I don't want to design our interfaces for potential needs,
I want to design them for actually known needs.

We'll deal with the discovered exceptions on an as-needed basis,
not from the beginning.

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[David Miller]

From: Scott Feldman <sfeldma@gmail.com>
Date: Sun, 15 Mar 2015 13:03:06 -0700

> I'm proposing a simple, permissive solution, with a provision for an
> out on a case-by-case basis.  Right now I don't see any reason to
> special case any middle driver; just let STP state propagate down to
> the port device.

+1

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[Jiri Pirko]

Sun, Mar 15, 2015 at 11:32:13PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Sun, 15 Mar 2015 20:17:22 +0100
>
>> Does that make sense? I think it is better just pick what we want to
>> support now and implement it there. That makes things clearer and
>> confusion-prone.
>
>You can't even say whether it makes sense or not, you yourself
>even have to ask.
>
>Again, I don't want to design our interfaces for potential needs,
>I want to design them for actually known needs.
>
>We'll deal with the discovered exceptions on an as-needed basis,
>not from the beginning.


Fair enough...

Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

[roopa]

On 3/15/15, 3:33 PM, David Miller wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> Date: Sun, 15 Mar 2015 13:03:06 -0700
>
>> I'm proposing a simple, permissive solution, with a provision for an
>> out on a case-by-case basis.  Right now I don't see any reason to
>> special case any middle driver; just let STP state propagate down to
>> the port device.
> +1
ok. That was the path i had taken initially (see for getlink/setlink):
http://www.spinics.net/lists/netdev/msg313436.html

I have resubmitted the patch with rollback to transparently following 
lowerdevs.
My version also does not follow lowerdevs if NETIF_F_HW_SWITCH_OFFLOAD 
is not
set. And this is set on bonds and teams if any one of their lowerdevs 
have it currently.

thanks.