netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nikolay Aleksandrov <nikolay@nvidia.com>
To: Tobias Waldekranz <tobias@waldekranz.com>,
	Vladimir Oltean <olteanv@gmail.com>
Cc: <davem@davemloft.net>, <kuba@kernel.org>, <andrew@lunn.ch>,
	<vivien.didelot@gmail.com>, <f.fainelli@gmail.com>,
	<roopa@nvidia.com>, <netdev@vger.kernel.org>, <jiri@resnulli.us>,
	<idosch@idosch.org>, <stephen@networkplumber.org>
Subject: Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications
Date: Mon, 18 Jan 2021 23:22:35 +0200	[thread overview]
Message-ID: <b5e2e1f7-c8dc-550b-25ec-0dbc23813444@nvidia.com> (raw)
In-Reply-To: <75ba13d0-bc14-f3b7-d842-cee2cd16d854@nvidia.com>

On 18/01/2021 23:17, Nikolay Aleksandrov wrote:
> On 18/01/2021 22:19, Tobias Waldekranz wrote:
>> On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@gmail.com> wrote:
>>> On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
>>>> Ah I see, no I was not aware of that. I just saw that the entry towards
>>>> the CPU was added to the ATU, which it would in both cases. I.e. from
>>>> the switch's POV, in this setup:
>>>>
>>>>    br0
>>>>    / \ (A)
>>>> swp0 dummy0
>>>>        (B)
>>>>
>>>> A "local" entry like (A), or a "static" entry like (B) means the same
>>>> thing to the switch: "it is somewhere behind my CPU-port".
>>>
>>> Yes, except that if dummy0 was a real and non-switchdev interface, then
>>> the "local" entry would probably break your traffic if what you meant
>>> was "static".
>>
>> Agreed.
>>
>>>>> So I think there is a very real issue in that the FDB entries with the
>>>>> is_local bit was never specified to switchdev thus far, and now suddenly
>>>>> is. I'm sorry, but what you're saying in the commit message, that
>>>>> "!added_by_user has so far been indistinguishable from is_local" is
>>>>> simply false.
>>>>
>>>> Alright, so how do you do it? Here is the struct:
>>>>
>>>>     struct switchdev_notifier_fdb_info {
>>>> 	struct switchdev_notifier_info info; /* must be first */
>>>> 	const unsigned char *addr;
>>>> 	u16 vid;
>>>> 	u8 added_by_user:1,
>>>> 	   offloaded:1;
>>>>     };
>>>>
>>>> Which field separates a local address on swp0 from a dynamically learned
>>>> address on swp0?
>>>
>>> None, that's the problem. Local addresses are already presented to
>>> switchdev without saying that they're local. Which is the entire reason
>>> that users are misled into thinking that the addresses are not local.
>>>
>>> I may have misread what you said, but to me, "!added_by_user has so far
>>> been indistinguishable from is_local" means that:
>>> - every struct switchdev_notifier_fdb_info with added_by_user == true
>>>   also had an implicit is_local == false
>>> - every struct switchdev_notifier_fdb_info with added_by_user == false
>>>   also had an implicit is_local == true
>>> It is _this_ that I deemed as clearly untrue.
>>>
>>> The is_local flag is not indistinguishable from !added_by_user, it is
>>> indistinguishable full stop. Which makes it hard to work with in a
>>> backwards-compatible way.
>>
>> This was probably a semantic mistake on my part, we meant the same
>> thing.
>>
>>>> Ok, so just to see if I understand this correctly:
>>>>
>>>> The situation today it that `bridge fdb add ADDR dev DEV master` results
>>>> in flows towards ADDR being sent to:
>>>>
>>>> 1. DEV if DEV belongs to a DSA switch.
>>>> 2. To the host if DEV was a non-offloaded interface.
>>>
>>> Not quite. In the bridge software FDB, the entry is marked as is_local
>>> in both cases, doesn't matter if the interface is offloaded or not.
>>> Just that switchdev does not propagate the is_local flag, which makes
>>> the switchdev listeners think it is not local. The interpretation of
>>> that will probably vary among switchdev drivers.
>>>
>>> The subtlety is that for a non-offloading interface, the
>>> misconfiguration (when you mean static but use local) is easy to catch.
>>> Since only the entry from the software FDB will be hit, this means that
>>> the frame will never be forwarded, so traffic will break.
>>> But in the case of a switchdev offloading interface, the frames will hit
>>> the hardware FDB entry more often than the software FDB entry. So
>>> everything will work just fine and dandy even though it shouldn't.
>>
>> Quite right.
>>
>>>> With this series applied both would result in (2) which, while
>>>> idiosyncratic, is as intended. But this of course runs the risk of
>>>> breaking existing scripts which rely on the current behavior.
>>>
>>> Yes.
>>>
>>> My only hope is that we could just offload the entries pointing towards
>>> br0, and ignore the local ones. But for that I would need the bridge
>>
>> That was my initial approach. Unfortunately that breaks down when the
>> bridge inherits its address from a port, i.e. the default case.
>>
>> When the address is added to the bridge (fdb->dst == NULL), fdb_insert
>> will find the previous local entry that is set on the port and bail out
>> before sending a notification:
>>
>> 	if (fdb) {
>> 		/* it is okay to have multiple ports with same
>> 		 * address, just use the first one.
>> 		 */
>> 		if (test_bit(BR_FDB_LOCAL, &fdb->flags))
>> 			return 0;
>> 		br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
>> 		       source ? source->dev->name : br->dev->name, addr, vid);
>> 		fdb_delete(br, fdb, true);
>> 	}
>>
>> You could change this so that a notification always is sent out. Or you
>> could give precedence to !fdb->dst and update the existing entry.
>>
>>> maintainers to clarify what is the difference between then, as I asked
>>> in your other patch.
>>
>> I am pretty sure they mean the same thing, I believe that !fdb->dst
>> implies is_local. It is just that "bridge fdb add ADDR dev br0 self" is
>> a new(er) thing, and before that there was "local" entries on ports.
>>
>> Maybe I should try to get rid of the local flag in the bridge first, and
>> then come back to this problem once that is done? Either way, I agree
>> that 5/7 is all we want to add to DSA to get this working.
>>
> 
> BR_FDB_LOCAL and !fdb->dst are not the same thing, check fdb_add_entry().
> You cannot get rid of it, !fdb->dst implies BR_FDB_LOCAL, but it's not
> symmetrical.
> 

Scratch that, I spoke too soon. You can get rid of it internally, just need
to be careful not to break user-visible behaviour as Vladimir mentioned.

> Cheers,
>  Nik
> 

 


  reply	other threads:[~2021-01-18 21:24 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-16  1:25 [RFC net-next 0/7] net: dsa: Sync local bridge FDB addresses to hardware Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 1/7] net: bridge: switchdev: Refactor br_switchdev_fdb_notify Tobias Waldekranz
2021-01-17 17:24   ` Vladimir Oltean
2021-01-16  1:25 ` [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications Tobias Waldekranz
2021-01-17 19:30   ` Vladimir Oltean
2021-01-18 18:58     ` Tobias Waldekranz
2021-01-18 19:27       ` Vladimir Oltean
2021-01-18 20:19         ` Tobias Waldekranz
2021-01-18 21:03           ` Vladimir Oltean
2021-01-18 21:17           ` Nikolay Aleksandrov
2021-01-18 21:22             ` Nikolay Aleksandrov [this message]
2021-01-18 21:39               ` Nikolay Aleksandrov
2021-01-18 21:50                 ` Vladimir Oltean
2021-01-18 21:53                   ` Nikolay Aleksandrov
2021-01-18 22:06                     ` Vladimir Oltean
2021-01-18 22:09                       ` Vladimir Oltean
2021-01-18 22:42                       ` Nikolay Aleksandrov
2021-01-19  0:42                         ` Vladimir Oltean
2021-01-19 10:14                           ` Nikolay Aleksandrov
2021-01-18 19:28     ` Ido Schimmel
2021-01-16  1:25 ` [RFC net-next 3/7] net: bridge: switchdev: Send FDB notifications for host addresses Tobias Waldekranz
2021-01-18 11:28   ` Vladimir Oltean
2021-01-16  1:25 ` [RFC net-next 4/7] net: dsa: Include local addresses in assisted CPU port learning Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 5/7] net: dsa: Include bridge " Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 6/7] net: dsa: Sync static FDB entries on foreign interfaces to hardware Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port Tobias Waldekranz
2021-02-01  6:24 ` DENG Qingfang
2021-02-03  9:27   ` Tobias Waldekranz
2021-02-03 10:14     ` Vladimir Oltean
2021-02-03 10:42       ` Tobias Waldekranz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b5e2e1f7-c8dc-550b-25ec-0dbc23813444@nvidia.com \
    --to=nikolay@nvidia.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=idosch@idosch.org \
    --cc=jiri@resnulli.us \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=roopa@nvidia.com \
    --cc=stephen@networkplumber.org \
    --cc=tobias@waldekranz.com \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).