From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org DE51360BD1 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org C8E4B60BAB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=blackwall-org.20221208.gappssmtp.com; s=20221208; t=1684654443; x=1687246443; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=IpkZ4yVV0iu3dRL52S+sHaPiWnMLUExrhT6mC141b0E=; b=qhFODi7rXTXgSVqVvOChzgPFv6jHIXgNH63oMM3AbdOwg/fxUXeKwmHyEvDpkKkgCI 2CycNj1vCjLPEDtuvZNB5VBas9cjhhVf62VUOQkQOtN6yxOMqBb9bbSpTBcr/u+DIuYe vfC2tLa5TuxPBVMs4ej+Nn9ZRvQlgJFxSW3vPtWDA75XRA9RlRkqSvyox+Ed7RAorR1G Vdde8+cW+Z7HcRBP2QO+r9M4qUaszsEUIwvqyYL6lmj4ze5hTL5p653XpdgtCQOSG3y1 46/fek83obPzr1K/1Y1qx8PNR2FDAmvN6Uu3mzfuCHX4sPs/l15M+YFc0az24peGCh0g +0qw== Message-ID: Date: Sun, 21 May 2023 10:34:00 +0300 MIME-Version: 1.0 Content-Language: en-US References: <20230518113328.1952135-1-idosch@nvidia.com> <20230518113328.1952135-2-idosch@nvidia.com> <1ed139d5-6cb9-90c7-323c-22cf916e96a0@blackwall.org> From: Nikolay Aleksandrov In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Bridge] [PATCH net-next 1/5] skbuff: bridge: Add layer 2 miss indication List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ido Schimmel Cc: taras.chornyi@plvision.eu, petrm@nvidia.com, alexandre.belloni@bootlin.com, jiri@resnulli.us, taspelund@nvidia.com, leon@kernel.org, netdev@vger.kernel.org, xiyou.wangcong@gmail.com, bridge@lists.linux-foundation.org, claudiu.manoil@nxp.com, UNGLinuxDriver@microchip.com, vladimir.oltean@nxp.com, edumazet@google.com, jhs@mojatatu.com, roopa@nvidia.com, kuba@kernel.org, pabeni@redhat.com, saeedm@nvidia.com, davem@davemloft.net On 19/05/2023 16:51, Ido Schimmel wrote: > On Thu, May 18, 2023 at 07:08:47PM +0300, Nikolay Aleksandrov wrote: >> On 18/05/2023 14:33, Ido Schimmel wrote: >>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c >>> index fc17b9fd93e6..d8ab5890cbe6 100644 >>> --- a/net/bridge/br_input.c >>> +++ b/net/bridge/br_input.c >>> @@ -334,6 +334,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb) >>> return RX_HANDLER_CONSUMED; >>> >>> memset(skb->cb, 0, sizeof(struct br_input_skb_cb)); >>> + skb->l2_miss = 0; >>> >>> p = br_port_get_rcu(skb->dev); >>> if (p->flags & BR_VLAN_TUNNEL) >> >> Overall looks good, only this part is a bit worrisome and needs some additional >> investigation because now we'll unconditionally dirty a cache line for every >> packet that is forwarded. Could you please check the effect with perf? > > To eliminate it I tried the approach we discussed yesterday: > > First, add the miss indication to the bridge's control block which is > zeroed for every skb entering the bridge: > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 2119729ded2b..bd5c18286a40 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -581,6 +581,7 @@ struct br_input_skb_cb { > #endif > u8 proxyarp_replied:1; > u8 src_port_isolated:1; > + u8 miss:1; /* FDB or MDB lookup miss */ > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > u8 vlan_filtered:1; > #endif > > And set this bit upon misses instead of skb->l2_miss: > > @@ -203,6 +205,8 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb, > struct net_bridge_port *prev = NULL; > struct net_bridge_port *p; > > + BR_INPUT_SKB_CB(skb)->miss = 1; > + > list_for_each_entry_rcu(p, &br->port_list, list) { > /* Do not flood unicast traffic to ports that turn it off, nor > * other traffic if flood off, except for traffic we originate > @@ -295,6 +299,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, > allow_mode_include = false; > } else { > p = NULL; > + BR_INPUT_SKB_CB(skb)->miss = 1; > } > > while (p || rp) { > > Then copy it to skb->l2_miss at the very end where the cache line > containing this field is already written to: > > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > index 84d6dd5e5b1a..89f65564e338 100644 > --- a/net/bridge/br_forward.c > +++ b/net/bridge/br_forward.c > @@ -50,6 +50,8 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb > > br_switchdev_frame_set_offload_fwd_mark(skb); > > + skb->l2_miss = BR_INPUT_SKB_CB(skb)->miss; > + > dev_queue_xmit(skb); > > return 0; > > Also for locally received packets: > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index fc17b9fd93e6..274e55455b15 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -46,6 +46,8 @@ static int br_pass_frame_up(struct sk_buff *skb) > */ > br_switchdev_frame_unmark(skb); > > + skb->l2_miss = BR_INPUT_SKB_CB(skb)->miss; > + > /* Bridge is just like any other port. Make sure the > * packet is allowed except in promisc mode when someone > * may be running packet capture. > > Ran these changes through the selftest and it seems to work. > > WDYT? Looks good to me, this is what I had in mind wrt cache line dirtying. The swdev mark already does it, so putting them together is nice. From bridge POV this is good. Thanks, Nik