Date:   Mon, 11 Feb 2019 11:05:13 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Ido Schimmel <idosch@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        "ilias.apalodimas@...aro.org" <ilias.apalodimas@...aro.org>,
        "ivan.khoronzhuk@...aro.org" <ivan.khoronzhuk@...aro.org>,
        "roopa@...ulusnetworks.com" <roopa@...ulusnetworks.com>,
        "nikolay@...ulusnetworks.com" <nikolay@...ulusnetworks.com>,
        Petr Machata <petrm@...lanox.com>
Subject: Re: [PATCH net-next v2 01/12] net: bridge: multicast: Propagate
 br_mc_disabled_update() return

On 2/2/19 7:47 AM, Ido Schimmel wrote:
> On Thu, Jan 31, 2019 at 05:19:25PM -0800, Florian Fainelli wrote:
>> On 1/30/19 11:50 PM, Ido Schimmel wrote:
>>> On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote:
>>>> On 1/29/19 11:36 PM, Ido Schimmel wrote:
>>>>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote:
>>>>>> -static void br_mc_disabled_update(struct net_device *dev, bool value)
>>>>>> +static int br_mc_disabled_update(struct net_device *dev, bool value)
>>>>>>  {
>>>>>>  	struct switchdev_attr attr = {
>>>>>>  		.orig_dev = dev,
>>>>>>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
>>>>>> -		.flags = SWITCHDEV_F_DEFER,
>>>>>> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>>>>
>>>>> Actually, since the operation is deferred I don't think the return value
>>>>> from the driver is ever checked. Can you test it?
>>>>
>>>> You are right, you get a WARN() from switchdev_attr_port_set_now(), but
>>>> this does not propagate back to the caller, so you can still create a
>>>> bridge device and enslave a device successfully despite getting warnings
>>>> on the console.
>>>>
>>>>>
>>>>> I think it would be good to convert the attributes to use the switchdev
>>>>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
>>>>> SDO with a notification") did for objects. Then you can have your
>>>>> listener veto the operation in the same context it is happening.
>>>>
>>>> Alright, working on it. Would you do that just for the attr_set, or for
>>>> attr_get as well (to be symmetrical)?
>>>
>>> Yes, then we can get rid of switchdev_ops completely.
>>>
>>
>> OK, so here is what I have so far:
>>
>> https://github.com/ffainelli/linux/pull/new/switchdev-attr
>>
>> although I am seeing some invalid context operations with DSA that I am
>> debugging. Does this look like the right way to go from your perspective?
> 
> That was quick :)
> 
> I think I owe you some explanation as to why I even came up with the
> idea. But before that, I have another idea how to solve your immediate
> problem.
> 
> You can employ a similar trick to the one used to set bridge port flags
> in br_switchdev_set_port_flag(). Like br_mc_disabled_update() it is also
> called from an atomic context, so in order to allow drivers to veto
> unsupported flags we introduced a new switchdev attribute:
> 'SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT'

Yes that is a great idea, for some reason I completely missed your email
here, but this is how I am going to approach it now.

Another way to look at the problem could be to question whether we
really need to be in atomic context when such attributes are pushed?
AFAICT, we are executing with BH disabled because we want to protect the
bridge master's bridge port list, right?

> 
> The attribute is only used in get operations and never deferred. You can
> introduce 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED_SUPPORT' and use it
> before invoking 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED'.
> 
> Regarding the whole notifier business and switchdev in general. Using
> the device chain to propagate various bridge port attributes is wrong.
> We saw it multiple times in the past already. First with routes that
> were converted to use notifiers because the switch needs to offload the
> entire routing table and not only routes whose nexthop device is a
> switch port. Then with FDB entries and recently also with VLANs in a
> VLAN-aware bridge. See merge commit 02e1dbe402de ("Merge branch
> 'Pass-extack-to-SWITCHDEV_PORT_OBJ_ADD'") for more details.
> 
> This is also true for switchdev attributes since we might want to
> support toggling of bridge port flags on a VXLAN device in the future.
> For example, to allow selective flooding. Others might have more use
> cases.
> 
> I want to use the opportunity to pick your brain (and others') about
> more issues I see with switchdev and maybe reach some agreement and form
> a plan. Just to be clear, it is not related to your patchset.
> 
> The prepare-commit model probably made sense in the beginning, but a few
> years later I think we know better. At least in mlxsw we have multiple
> places where we perform all the work in the prepare phase simply because
> that without doing all the work we don't have a way to guarantee that
> the commit phase will succeed. I'm not aware of other instances of this
> model in the networking code, so I wonder why we need it in switchdev
> and if we can simply remove it and simplify things.

If we can at least re-purpose the prepare + commit model such that the
prepare phase is: can you support that operation (without allocating
resources) and do that operation in the caller context, while the commit
is deferred, then that would solve some of my issues but it could create
more for mlxsw possibly?

I have to wonder though, if we have a driver which is constructed such that:

- all HW programming is done
- a SW resource (e.g.: a rule object) is allocated last, then if the
allocation fails, we should be able to easily rollback the HW
programming we just did

Would that work?

> 
> Another issue is that I believe we can completely remove the switchdev
> infrastructure as it basically boils down to bridge-specific notifiers.
> If you look at where switchdev is actually used in the networking stack
> while excluding the obvious suspects you get this:
> 
> $ git grep -i -n -e 'switchdev' -- 'net/*' ':!net/bridge/*' ':!net/dsa/*' ':!net/switchdev/'                                                         
> net/8021q/vlan_dev.c:34:#include <net/switchdev.h>
> net/Kconfig:240:source "net/switchdev/Kconfig"
> net/Makefile:81:ifneq ($(CONFIG_NET_SWITCHDEV),)
> net/Makefile:82:obj-y                           += switchdev/
> net/core/net-sysfs.c:15:#include <net/switchdev.h>
> net/core/net-sysfs.c:504:               struct switchdev_attr attr = {
> net/core/net-sysfs.c:506:                       .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,                                                                                     
> net/core/net-sysfs.c:507:                       .flags = SWITCHDEV_F_NO_RECURSE,
> net/core/net-sysfs.c:510:               ret = switchdev_port_attr_get(netdev, &attr);
> net/core/rtnetlink.c:49:#include <net/switchdev.h>
> net/core/rtnetlink.c:1150:      struct switchdev_attr attr = {
> net/core/rtnetlink.c:1152:              .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> net/core/rtnetlink.c:1153:              .flags = SWITCHDEV_F_NO_RECURSE,
> net/core/rtnetlink.c:1156:      err = switchdev_port_attr_get(dev, &attr);
> net/core/skbuff.c:4925:#ifdef CONFIG_NET_SWITCHDEV
> net/ipv4/ip_forward.c:72:#ifdef CONFIG_NET_SWITCHDEV
> net/ipv4/ipmr.c:70:#include <net/switchdev.h>
> net/ipv4/ipmr.c:841:    struct switchdev_attr attr = {
> net/ipv4/ipmr.c:842:            .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> net/ipv4/ipmr.c:923:    if (!switchdev_port_attr_get(dev, &attr)) {
> net/ipv4/ipmr.c:1802:#ifdef CONFIG_NET_SWITCHDEV
> net/ipv6/ip6_output.c:381:#ifdef CONFIG_NET_SWITCHDEV
> 
> These are all instances related to the use of parent ID. Why not add a
> new ndo that will allow various users to query the parent ID of a
> netdev? bond and team can return an error if their slaves don't all have
> the same parent ID.

This should now be addressed as you might have seen.

> 
> You then end up with basic constructs like notifiers and ndo invocations
> and not with complex objects and attributes that are propagated via the
> device chain with a prepare-commit model.
> 
> WDYT?
> 
>> -- 
>> Florian


-- 
Florian

Powered by blists - more mailing lists