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
