From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92794C169C4 for ; Mon, 11 Feb 2019 19:05:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 446DD20863 for ; Mon, 11 Feb 2019 19:05:34 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="tfu+DwZ8" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732716AbfBKTFc (ORCPT ); Mon, 11 Feb 2019 14:05:32 -0500 Received: from mail-pf1-f195.google.com ([209.85.210.195]:44890 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728877AbfBKTFc (ORCPT ); Mon, 11 Feb 2019 14:05:32 -0500 Received: by mail-pf1-f195.google.com with SMTP id u6so5678584pfh.11 for ; Mon, 11 Feb 2019 11:05:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:openpgp:autocrypt:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3t3i++HqsSUgo39wPyg/Nn+zTDWrkuMea/KsC9Q7+pI=; b=tfu+DwZ8q2L7lTg5HuZ0f1YrmxtJAU+1WqPcx8oVAn3rP0w4t++EePX+M4Xa2qUTeP ZtKSVYRN3Xo2pmHZhWVFV1dnberajtPgnONOE3UtbVn12Kp/QjtycN4AyoVnYeACTnIB Ze076S2YXfcvkfGGl/nXVTR2mKUAKND6bZR08di0C0gx4OE+TBv5L2pdHRWiqtJ7M7Jw 9lPGQ6tiE+CDxaVQA8Du2+/XqK75guYr0lXFAnsETtUzdd3NuwlSoJPgpDVURHOxc4wP nurAYGOHSa0QF+on82YudQCWOK0bnAJ3tDGLpzXprOhRcng9JsO6vK1EUrmXXdPsJ6if drsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=3t3i++HqsSUgo39wPyg/Nn+zTDWrkuMea/KsC9Q7+pI=; b=sm7xSOydH24UtuoylHivIp6dg5lG5ipQbcRGyBLmL/2hlQyUXD4A8uU8WzMG/Weglg CFFVXwatpPRCZ4YWJebgxZFrjkLAr4lX6nnsXv45m3kRXiN5iZ2GQnT5b5QnoQyP2pOn ljijiZCNkOdRpdSbblGuLR2uD4G1PgjKn9evOGmKem6+1mqQOX15LSHRuwRYVK6VNFOk poVjLgyiUR2gNjK0Io6329HWgfj8kQ0LIxo9JXGf3IE0DcXFszpI7eCOEyuHyGjkirZv NN01EwysXyOmnNZtAOsczCx5mxphn+oMVum7djLB6pnid/52H38ugp2EXjr9MItzDdyK 9ILg== X-Gm-Message-State: AHQUAubzu1Qtfi562G930x8ng2r9hmFz7B4tvtsSGNINkUMK3uy6pn0T HtfkMNJzWHHPlmlSO/BATPY= X-Google-Smtp-Source: AHgI3IYuEMx3qBoynFbrx+32O8hV08uNpD9aKH+PC2aNkLrol6GHXnnTwVeMqTBzrMf8g6VABA6GnA== X-Received: by 2002:a63:5861:: with SMTP id i33mr35015729pgm.60.1549911930689; Mon, 11 Feb 2019 11:05:30 -0800 (PST) Received: from [10.67.48.231] ([192.19.223.250]) by smtp.googlemail.com with ESMTPSA id 125sm16196252pfx.159.2019.02.11.11.05.26 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 11 Feb 2019 11:05:29 -0800 (PST) Subject: Re: [PATCH net-next v2 01/12] net: bridge: multicast: Propagate br_mc_disabled_update() return To: Ido Schimmel Cc: "netdev@vger.kernel.org" , "andrew@lunn.ch" , "vivien.didelot@gmail.com" , "davem@davemloft.net" , Jiri Pirko , "ilias.apalodimas@linaro.org" , "ivan.khoronzhuk@linaro.org" , "roopa@cumulusnetworks.com" , "nikolay@cumulusnetworks.com" , Petr Machata References: <20190130005548.2212-1-f.fainelli@gmail.com> <20190130005548.2212-2-f.fainelli@gmail.com> <20190130073656.GA22227@splinter> <63b446c5-8104-d9f9-3924-aaa481ee3b8c@gmail.com> <20190131075013.GA27839@splinter> <061dca81-a5ed-1498-060a-1a7a0e0a1116@gmail.com> <20190202154750.GA2864@splinter> From: Florian Fainelli Openpgp: preference=signencrypt Autocrypt: addr=f.fainelli@gmail.com; prefer-encrypt=mutual; keydata= mQGiBEjPuBIRBACW9MxSJU9fvEOCTnRNqG/13rAGsj+vJqontvoDSNxRgmafP8d3nesnqPyR xGlkaOSDuu09rxuW+69Y2f1TzjFuGpBk4ysWOR85O2Nx8AJ6fYGCoeTbovrNlGT1M9obSFGQ X3IzRnWoqlfudjTO5TKoqkbOgpYqIo5n1QbEjCCwCwCg3DOH/4ug2AUUlcIT9/l3pGvoRJ0E AICDzi3l7pmC5IWn2n1mvP5247urtHFs/uusE827DDj3K8Upn2vYiOFMBhGsxAk6YKV6IP0d ZdWX6fqkJJlu9cSDvWtO1hXeHIfQIE/xcqvlRH783KrihLcsmnBqOiS6rJDO2x1eAgC8meAX SAgsrBhcgGl2Rl5gh/jkeA5ykwbxA/9u1eEuL70Qzt5APJmqVXR+kWvrqdBVPoUNy/tQ8mYc nzJJ63ng3tHhnwHXZOu8hL4nqwlYHRa9eeglXYhBqja4ZvIvCEqSmEukfivk+DlIgVoOAJbh qIWgvr3SIEuR6ayY3f5j0f2ejUMYlYYnKdiHXFlF9uXm1ELrb0YX4GMHz7QnRmxvcmlhbiBG YWluZWxsaSA8Zi5mYWluZWxsaUBnbWFpbC5jb20+iGYEExECACYCGyMGCwkIBwMCBBUCCAME FgIDAQIeAQIXgAUCVF/S8QUJHlwd3wAKCRBhV5kVtWN2DvCVAJ4u4/bPF4P3jxb4qEY8I2gS 6hG0gACffNWlqJ2T4wSSn+3o7CCZNd7SLSC5BA0ESM+4EhAQAL/o09boR9D3Vk1Tt7+gpYr3 WQ6hgYVON905q2ndEoA2J0dQxJNRw3snabHDDzQBAcqOvdi7YidfBVdKi0wxHhSuRBfuOppu pdXkb7zxuPQuSveCLqqZWRQ+Cc2QgF7SBqgznbe6Ngout5qXY5Dcagk9LqFNGhJQzUGHAsIs hap1f0B1PoUyUNeEInV98D8Xd/edM3mhO9nRpUXRK9Bvt4iEZUXGuVtZLT52nK6Wv2EZ1TiT OiqZlf1P+vxYLBx9eKmabPdm3yjalhY8yr1S1vL0gSA/C6W1o/TowdieF1rWN/MYHlkpyj9c Rpc281gAO0AP3V1G00YzBEdYyi0gaJbCEQnq8Vz1vDXFxHzyhgGz7umBsVKmYwZgA8DrrB0M oaP35wuGR3RJcaG30AnJpEDkBYHznI2apxdcuTPOHZyEilIRrBGzDwGtAhldzlBoBwE3Z3MY 31TOpACu1ZpNOMysZ6xiE35pWkwc0KYm4hJA5GFfmWSN6DniimW3pmdDIiw4Ifcx8b3mFrRO BbDIW13E51j9RjbO/nAaK9ndZ5LRO1B/8Fwat7bLzmsCiEXOJY7NNpIEpkoNoEUfCcZwmLrU +eOTPzaF6drw6ayewEi5yzPg3TAT6FV3oBsNg3xlwU0gPK3v6gYPX5w9+ovPZ1/qqNfOrbsE FRuiSVsZQ5s3AAMFD/9XjlnnVDh9GX/r/6hjmr4U9tEsM+VQXaVXqZuHKaSmojOLUCP/YVQo 7IiYaNssCS4FCPe4yrL4FJJfJAsbeyDykMN7wAnBcOkbZ9BPJPNCbqU6dowLOiy8AuTYQ48m vIyQ4Ijnb6GTrtxIUDQeOBNuQC/gyyx3nbL/lVlHbxr4tb6YkhkO6shjXhQh7nQb33FjGO4P WU11Nr9i/qoV8QCo12MQEo244RRA6VMud06y/E449rWZFSTwGqb0FS0seTcYNvxt8PB2izX+ HZA8SL54j479ubxhfuoTu5nXdtFYFj5Lj5x34LKPx7MpgAmj0H7SDhpFWF2FzcC1bjiW9mjW HaKaX23Awt97AqQZXegbfkJwX2Y53ufq8Np3e1542lh3/mpiGSilCsaTahEGrHK+lIusl6mz Joil+u3k01ofvJMK0ZdzGUZ/aPMZ16LofjFA+MNxWrZFrkYmiGdv+LG45zSlZyIvzSiG2lKy kuVag+IijCIom78P9jRtB1q1Q5lwZp2TLAJlz92DmFwBg1hyFzwDADjZ2nrDxKUiybXIgZp9 aU2d++ptEGCVJOfEW4qpWCCLPbOT7XBr+g/4H3qWbs3j/cDDq7LuVYIe+wchy/iXEJaQVeTC y5arMQorqTFWlEOgRA8OP47L9knl9i4xuR0euV6DChDrguup2aJVU4hPBBgRAgAPAhsMBQJU X9LxBQkeXB3fAAoJEGFXmRW1Y3YOj4UAn3nrFLPZekMeqX5aD/aq/dsbXSfyAKC45Go0YyxV HGuUuzv+GKZ6nsysJ7kCDQRXG8fwARAA6q/pqBi5PjHcOAUgk2/2LR5LjjesK50bCaD4JuNc YDhFR7Vs108diBtsho3w8WRd9viOqDrhLJTroVckkk74OY8r+3t1E0Dd4wHWHQZsAeUvOwDM PQMqTUBFuMi6ydzTZpFA2wBR9x6ofl8Ax+zaGBcFrRlQnhsuXLnM1uuvS39+pmzIjasZBP2H UPk5ifigXcpelKmj6iskP3c8QN6x6GjUSmYx+xUfs/GNVSU1XOZn61wgPDbgINJd/THGdqiO iJxCLuTMqlSsmh1+E1dSdfYkCb93R/0ZHvMKWlAx7MnaFgBfsG8FqNtZu3PCLfizyVYYjXbV WO1A23riZKqwrSJAATo5iTS65BuYxrFsFNPrf7TitM8E76BEBZk0OZBvZxMuOs6Z1qI8YKVK UrHVGFq3NbuPWCdRul9SX3VfOunr9Gv0GABnJ0ET+K7nspax0xqq7zgnM71QEaiaH17IFYGS sG34V7Wo3vyQzsk7qLf9Ajno0DhJ+VX43g8+AjxOMNVrGCt9RNXSBVpyv2AMTlWCdJ5KI6V4 KEzWM4HJm7QlNKE6RPoBxJVbSQLPd9St3h7mxLcne4l7NK9eNgNnneT7QZL8fL//s9K8Ns1W t60uQNYvbhKDG7+/yLcmJgjF74XkGvxCmTA1rW2bsUriM533nG9gAOUFQjURkwI8jvMAEQEA AYkCaAQYEQIACQUCVxvH8AIbAgIpCRBhV5kVtWN2DsFdIAQZAQIABgUCVxvH8AAKCRCH0Jac RAcHBIkHD/9nmfog7X2ZXMzL9ktT++7x+W/QBrSTCTmq8PK+69+INN1ZDOrY8uz6htfTLV9+ e2W6G8/7zIvODuHk7r+yQ585XbplgP0V5Xc8iBHdBgXbqnY5zBrcH+Q/oQ2STalEvaGHqNoD UGyLQ/fiKoLZTPMur57Fy1c9rTuKiSdMgnT0FPfWVDfpR2Ds0gpqWePlRuRGOoCln5GnREA/ 2MW2rWf+CO9kbIR+66j8b4RUJqIK3dWn9xbENh/aqxfonGTCZQ2zC4sLd25DQA4w1itPo+f5 V/SQxuhnlQkTOCdJ7b/mby/pNRz1lsLkjnXueLILj7gNjwTabZXYtL16z24qkDTI1x3g98R/ xunb3/fQwR8FY5/zRvXJq5us/nLvIvOmVwZFkwXc+AF+LSIajqQz9XbXeIP/BDjlBNXRZNdo dVuSU51ENcMcilPr2EUnqEAqeczsCGpnvRCLfVQeSZr2L9N4svNhhfPOEscYhhpHTh0VPyxI pPBNKq+byuYPMyk3nj814NKhImK0O4gTyCK9b+gZAVvQcYAXvSouCnTZeJRrNHJFTgTgu6E0 caxTGgc5zzQHeX67eMzrGomG3ZnIxmd1sAbgvJUDaD2GrYlulfwGWwWyTNbWRvMighVdPkSF 6XFgQaosWxkV0OELLy2N485YrTr2Uq64VKyxpncLh50e2RnyAJ9Za0Dx0yyp44iD1OvHtkEI M5kY0ACeNhCZJvZ5g4C2Lc9fcTHu8jxmEkI= Message-ID: <5f6bed39-020a-c826-979b-c27e2ae75402@gmail.com> Date: Mon, 11 Feb 2019 11:05:13 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190202154750.GA2864@splinter> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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/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/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/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/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