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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 053F2C54EBD for ; Mon, 9 Jan 2023 11:44:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234154AbjAILoj (ORCPT ); Mon, 9 Jan 2023 06:44:39 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58980 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237239AbjAILny (ORCPT ); Mon, 9 Jan 2023 06:43:54 -0500 Received: from wout1-smtp.messagingengine.com (wout1-smtp.messagingengine.com [64.147.123.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 686982DFC; Mon, 9 Jan 2023 03:43:53 -0800 (PST) Received: from compute6.internal (compute6.nyi.internal [10.202.2.47]) by mailout.west.internal (Postfix) with ESMTP id A5BC23200564; Mon, 9 Jan 2023 06:43:50 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Mon, 09 Jan 2023 06:43:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1673264630; x=1673351030; bh=bboapEA3vdgSLWm56a8XSVtdTYNt 0MFJHdjnyOsqZws=; b=Wx0NqN6ygqqa2SxoxoROtpr3/Hkbs+nwqruUtIS5aAHG 16wXZLBSnARXMXgHHSxdbG2ij4c9Yya9lLt1jOBREFxoQh+M3NJgjcj27pGEzhcn YIJcw3o8XqWiGakmWB90s0YiZI0zZ4T9wPRsSy0A1niEpuI+uuIyViMCFnYfZ6/x BYDqMF4Up7zindbrNkHZcZqD1Y+38daStRBtr7/c0VBgcoo3PEfHqcC/rXda5Fov wom76CULKqAEh5HbBGGHmMuhxiEZYKaKz8kDKDE1osJPHqDf5tqP5/fgHj4K7ddA AmEbhnuCFUHqqcH6a0HTR7Hk+6HvVQ3n1TezoqHh6A== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrkeeigddviecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehttdertddttddvnecuhfhrohhmpefkughoucfu tghhihhmmhgvlhcuoehiughoshgthhesihguohhstghhrdhorhhgqeenucggtffrrghtth gvrhhnpedvudefveekheeugeeftddvveefgfduieefudeifefgleekheegleegjeejgeeg hfenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehiug hoshgthhesihguohhstghhrdhorhhg X-ME-Proxy: Feedback-ID: i494840e7:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 9 Jan 2023 06:43:48 -0500 (EST) Date: Mon, 9 Jan 2023 13:43:46 +0200 From: Ido Schimmel To: Vladimir Oltean Cc: Tobias Waldekranz , davem@davemloft.net, kuba@kernel.org, Nikolay Aleksandrov , Andrew Lunn , Vivien Didelot , Florian Fainelli , Jiri Pirko , Ivan Vecera , Roopa Prabhu , Russell King , Petr Machata , Ido Schimmel , Matt Johnston , Cooper Lees , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, bridge@lists.linux-foundation.org Subject: Re: [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode Message-ID: References: <20220316150857.2442916-1-tobias@waldekranz.com> <20220316150857.2442916-2-tobias@waldekranz.com> <20230109100236.euq7iaaorqxrun7u@skbuf> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230109100236.euq7iaaorqxrun7u@skbuf> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 09, 2023 at 12:02:36PM +0200, Vladimir Oltean wrote: > On Mon, Jan 09, 2023 at 10:05:53AM +0200, Ido Schimmel wrote: > > > + if (on) > > > + static_branch_enable(&br_mst_used); > > > + else > > > + static_branch_disable(&br_mst_used); > > > > Hi, > > > > I'm not actually using MST, but I ran into this code and was wondering > > if the static key usage is correct. The static key is global (not > > per-bridge), so what happens when two bridges have MST enabled and then > > it is disabled on one? I believe it would be disabled for both. If so, > > maybe use static_branch_inc() / static_branch_dec() instead? > > Sounds about right. FWIW, br_switchdev_tx_fwd_offload does use > static_branch_inc() / static_branch_dec(). OK, thanks for confirming. Will send a patch later this week if Tobias won't take care of it by then. First patch will probably be [1] to make sure we dump the correct MST state to user space. It will also make it easier to show the problem and validate the fix. [1] diff --git a/net/bridge/br.c b/net/bridge/br.c index 4f5098d33a46..f02a1ad589de 100644 --- a/net/bridge/br.c +++ b/net/bridge/br.c @@ -286,7 +286,7 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) case BR_BOOLOPT_MCAST_VLAN_SNOOPING: return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); case BR_BOOLOPT_MST_ENABLE: - return br_opt_get(br, BROPT_MST_ENABLED); + return br_mst_is_enabled(br); default: /* shouldn't be called with unsupported options */ WARN_ON(1); diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 75aff9bbf17e..7f0475f62d45 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -1827,7 +1827,7 @@ static inline bool br_vlan_state_allowed(u8 state, bool learn_allow) /* br_mst.c */ #ifdef CONFIG_BRIDGE_VLAN_FILTERING DECLARE_STATIC_KEY_FALSE(br_mst_used); -static inline bool br_mst_is_enabled(struct net_bridge *br) +static inline bool br_mst_is_enabled(const struct net_bridge *br) { return static_branch_unlikely(&br_mst_used) && br_opt_get(br, BROPT_MST_ENABLED); @@ -1845,7 +1845,7 @@ int br_mst_fill_info(struct sk_buff *skb, int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, struct netlink_ext_ack *extack); #else -static inline bool br_mst_is_enabled(struct net_bridge *br) +static inline bool br_mst_is_enabled(const struct net_bridge *br) { return false; } From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 5A97760EA1 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 1E37A607BC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1673264630; x=1673351030; bh=bboapEA3vdgSLWm56a8XSVtdTYNt 0MFJHdjnyOsqZws=; b=Wx0NqN6ygqqa2SxoxoROtpr3/Hkbs+nwqruUtIS5aAHG 16wXZLBSnARXMXgHHSxdbG2ij4c9Yya9lLt1jOBREFxoQh+M3NJgjcj27pGEzhcn YIJcw3o8XqWiGakmWB90s0YiZI0zZ4T9wPRsSy0A1niEpuI+uuIyViMCFnYfZ6/x BYDqMF4Up7zindbrNkHZcZqD1Y+38daStRBtr7/c0VBgcoo3PEfHqcC/rXda5Fov wom76CULKqAEh5HbBGGHmMuhxiEZYKaKz8kDKDE1osJPHqDf5tqP5/fgHj4K7ddA AmEbhnuCFUHqqcH6a0HTR7Hk+6HvVQ3n1TezoqHh6A== Date: Mon, 9 Jan 2023 13:43:46 +0200 From: Ido Schimmel Message-ID: References: <20220316150857.2442916-1-tobias@waldekranz.com> <20220316150857.2442916-2-tobias@waldekranz.com> <20230109100236.euq7iaaorqxrun7u@skbuf> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230109100236.euq7iaaorqxrun7u@skbuf> Subject: Re: [Bridge] [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Oltean Cc: Ivan Vecera , Andrew Lunn , Florian Fainelli , Jiri Pirko , Petr Machata , Nikolay Aleksandrov , bridge@lists.linux-foundation.org, Russell King , Vivien Didelot , Ido Schimmel , netdev@vger.kernel.org, Cooper Lees , Roopa Prabhu , kuba@kernel.org, Matt Johnston , davem@davemloft.net, linux-kernel@vger.kernel.org, Tobias Waldekranz On Mon, Jan 09, 2023 at 12:02:36PM +0200, Vladimir Oltean wrote: > On Mon, Jan 09, 2023 at 10:05:53AM +0200, Ido Schimmel wrote: > > > + if (on) > > > + static_branch_enable(&br_mst_used); > > > + else > > > + static_branch_disable(&br_mst_used); > > > > Hi, > > > > I'm not actually using MST, but I ran into this code and was wondering > > if the static key usage is correct. The static key is global (not > > per-bridge), so what happens when two bridges have MST enabled and then > > it is disabled on one? I believe it would be disabled for both. If so, > > maybe use static_branch_inc() / static_branch_dec() instead? > > Sounds about right. FWIW, br_switchdev_tx_fwd_offload does use > static_branch_inc() / static_branch_dec(). OK, thanks for confirming. Will send a patch later this week if Tobias won't take care of it by then. First patch will probably be [1] to make sure we dump the correct MST state to user space. It will also make it easier to show the problem and validate the fix. [1] diff --git a/net/bridge/br.c b/net/bridge/br.c index 4f5098d33a46..f02a1ad589de 100644 --- a/net/bridge/br.c +++ b/net/bridge/br.c @@ -286,7 +286,7 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) case BR_BOOLOPT_MCAST_VLAN_SNOOPING: return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); case BR_BOOLOPT_MST_ENABLE: - return br_opt_get(br, BROPT_MST_ENABLED); + return br_mst_is_enabled(br); default: /* shouldn't be called with unsupported options */ WARN_ON(1); diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 75aff9bbf17e..7f0475f62d45 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -1827,7 +1827,7 @@ static inline bool br_vlan_state_allowed(u8 state, bool learn_allow) /* br_mst.c */ #ifdef CONFIG_BRIDGE_VLAN_FILTERING DECLARE_STATIC_KEY_FALSE(br_mst_used); -static inline bool br_mst_is_enabled(struct net_bridge *br) +static inline bool br_mst_is_enabled(const struct net_bridge *br) { return static_branch_unlikely(&br_mst_used) && br_opt_get(br, BROPT_MST_ENABLED); @@ -1845,7 +1845,7 @@ int br_mst_fill_info(struct sk_buff *skb, int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, struct netlink_ext_ack *extack); #else -static inline bool br_mst_is_enabled(struct net_bridge *br) +static inline bool br_mst_is_enabled(const struct net_bridge *br) { return false; }