Re: [PATCH net-next v2 01/12] net: bridge: multicast: Propagate br_mc_disabled_update() return

[Ido Schimmel <idosch@mellanox.com>]

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'

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.

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.

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