Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
On 13/12/2020 15:55, Vladimir Oltean wrote:
> Hi Nik,
>
> On Sun, Dec 13, 2020 at 03:22:16PM +0200, Nikolay Aleksandrov wrote:
>> Hi Vladimir,
>> Thank you for the good explanation, it really helps a lot to understand the
>> issue.
>> Even though it's deceptively simple, that call adds another lock/unlock for
>> everyone
>> when moving or learning (due to notifier lock)
>
> This unlikely code path is just on movement, as far as I understand it.
> How often do we expect that to happen? Is there any practical use case
> where an FDB entry ping pongs between ports?
>
It was my bad because I was looking at the wrong atomic notifier call function.
Switchdev uses the standard atomic notifier call chain with RCU only which is
fine
and there are no locks involved.
I was looking at the _robust version with a spin_lock and that would've meant
that
learning (because of notifications) would also block movements and vice versa.
Anyway as I said all of that is not an issue, the patch is good. I've replied
to my comment
and acked it a few minutes ago.
>> , but I do like how simple the solution
>> becomes with this change, so I'm not strictly against it. I think I'll add a
>> "refcnt"-like
>> check in the switchdev fn which would process the chain only when there are
>> registered users
>> to avoid any locks when moving fdbs on pure software bridges (like it was
>> before swdev).
>
> That makes sense.
>
>> I get that the alternative is to track these within DSA, I'm tempted to say
>> that's not such
>> a bad alternative as this change would make moving fdbs slower in general.
>
> I deliberately said "rule" instead of "static FDB entry" and "control
> interface" instead of "CPU port" because this is not only about DSA.
> I know of at least one other switchdev device which doesn't support
> source address learning for host-injected traffic. It isn't even so much
> of a quirk as it is the way that the hardware works. If you think of it
> as a "switch with queues", there would be little reason for a hardware
> designer to not just provide you the means to inject directly into the
> queues of the egress port, therefore bypassing the normal analyzer and
> forwarding logic.
>
> Everything we do in DSA must be copied sooner or later in other similar
> drivers, to get the same functionality. So I would really like to keep
> this interface simple, and not inflict unnecessary complications if
> possible.
>
Right, I like how the solution and this set look.
>> Have you thought about another way to find out, e.g. if more fdb
>> information is passed to the notifications ?
>
> Like what, keep emitting just the ADD notification, but put some
> metadata in it letting listeners know that it was actually migrated from
> a different bridge port, in order to save one notification? That would
> mean that we would need to:
>
> case SWITCHDEV_FDB_ADD_TO_DEVICE:
> fdb_info = ptr;
>
> if (dsa_slave_dev_check(dev)) {
> if (!fdb_info->migrated_from_dev ||
> dsa_slave_dev_check(fdb_info->migrated_from_dev)) {
> if (!fdb_info->added_by_user)
> return NOTIFY_OK;
>
> dp = dsa_slave_to_port(dev);
>
> add = true;
> } else if (fdb_info->migrated_from_dev &&
> !dsa_slave_dev_check(fdb_info->migrated_from_dev)) {
> /* An address has migrated from a non-DSA port
>* to a DSA port. Check if that non-DSA port was
>* bridged with us, aka if we previously had
> that
>* address installed towards the CPU.
>*/
> struct net_device *br_dev;
> struct dsa_slave_priv *p;
>
> br_dev = netdev_master_upper_dev_get_rcu(dev);
> if (!br_dev)
> return NOTIFY_DONE;
>
> if (!netif_is_bridge_master(br_dev))
> return NOTIFY_DONE;
>
> p = dsa_slave_dev_lower_find(br_dev);
> if (!p)
> return NOTIFY_DONE;
>
> delete = true;
> }
> } else {
> /* Snoop addresses learnt on foreign interfaces
>* bridged with us, for switches that don't
>* automatically learn SA from CPU-injected traffic
>*/
> struct net_device *br_dev;
> struct dsa_slave_priv *p;
>
> br_dev = netdev_master_upper_dev_get_rcu(dev);
> if (!br_dev)
>
Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
On Sun, Dec 13, 2020 at 03:36:13PM +0200, Nikolay Aleksandrov wrote: > Nevermind the whole comment. :) I was looking at the wrong code and got > confused. > > All is well (thanks to Ido). > > Acked-by: Nikolay Aleksandrov Ok, thanks. By the way, which wrong code were you looking at?
Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
Hi Nik,
On Sun, Dec 13, 2020 at 03:22:16PM +0200, Nikolay Aleksandrov wrote:
> Hi Vladimir,
> Thank you for the good explanation, it really helps a lot to understand the
> issue.
> Even though it's deceptively simple, that call adds another lock/unlock for
> everyone
> when moving or learning (due to notifier lock)
This unlikely code path is just on movement, as far as I understand it.
How often do we expect that to happen? Is there any practical use case
where an FDB entry ping pongs between ports?
> , but I do like how simple the solution
> becomes with this change, so I'm not strictly against it. I think I'll add a
> "refcnt"-like
> check in the switchdev fn which would process the chain only when there are
> registered users
> to avoid any locks when moving fdbs on pure software bridges (like it was
> before swdev).
That makes sense.
> I get that the alternative is to track these within DSA, I'm tempted to say
> that's not such
> a bad alternative as this change would make moving fdbs slower in general.
I deliberately said "rule" instead of "static FDB entry" and "control
interface" instead of "CPU port" because this is not only about DSA.
I know of at least one other switchdev device which doesn't support
source address learning for host-injected traffic. It isn't even so much
of a quirk as it is the way that the hardware works. If you think of it
as a "switch with queues", there would be little reason for a hardware
designer to not just provide you the means to inject directly into the
queues of the egress port, therefore bypassing the normal analyzer and
forwarding logic.
Everything we do in DSA must be copied sooner or later in other similar
drivers, to get the same functionality. So I would really like to keep
this interface simple, and not inflict unnecessary complications if
possible.
> Have you thought about another way to find out, e.g. if more fdb
> information is passed to the notifications ?
Like what, keep emitting just the ADD notification, but put some
metadata in it letting listeners know that it was actually migrated from
a different bridge port, in order to save one notification? That would
mean that we would need to:
case SWITCHDEV_FDB_ADD_TO_DEVICE:
fdb_info = ptr;
if (dsa_slave_dev_check(dev)) {
if (!fdb_info->migrated_from_dev ||
dsa_slave_dev_check(fdb_info->migrated_from_dev)) {
if (!fdb_info->added_by_user)
return NOTIFY_OK;
dp = dsa_slave_to_port(dev);
add = true;
} else if (fdb_info->migrated_from_dev &&
!dsa_slave_dev_check(fdb_info->migrated_from_dev)) {
/* An address has migrated from a non-DSA port
* to a DSA port. Check if that non-DSA port was
* bridged with us, aka if we previously had
that
* address installed towards the CPU.
*/
struct net_device *br_dev;
struct dsa_slave_priv *p;
br_dev = netdev_master_upper_dev_get_rcu(dev);
if (!br_dev)
return NOTIFY_DONE;
if (!netif_is_bridge_master(br_dev))
return NOTIFY_DONE;
p = dsa_slave_dev_lower_find(br_dev);
if (!p)
return NOTIFY_DONE;
delete = true;
}
} else {
/* Snoop addresses learnt on foreign interfaces
* bridged with us, for switches that don't
* automatically learn SA from CPU-injected traffic
*/
struct net_device *br_dev;
struct dsa_slave_priv *p;
br_dev = netdev_master_upper_dev_get_rcu(dev);
if (!br_dev)
return NOTIFY_DONE;
if (!netif_is_bridge_master(br_dev))
return NOTIFY_DONE;
p = dsa_slave_dev_lower_find(br_dev);
if (!p)
return NOTIFY_DONE;
dp = p->dp->cpu_dp;
if (!dp->ds->assisted_learning_on_cpu_port)
return NOTIFY_DONE;
}
case SWITCHDEV_FDB_DEL_TO_DEVICE:
not shown here
I probably didn't even get it right. We would need to delete an entry
from the notification of a FDB insertion, which is a bit counter-intuitive,
Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
On 13/12/2020 15:22, Nikolay Aleksandrov wrote: > On 13/12/2020 04:40, Vladimir Oltean wrote: >> Currently the bridge emits atomic switchdev notifications for >> dynamically learnt FDB entries. Monitoring these notifications works >> wonders for switchdev drivers that want to keep their hardware FDB in >> sync with the bridge's FDB. >> >> For example station A wants to talk to station B in the diagram below, >> and we are concerned with the behavior of the bridge on the DUT device: >> >>DUT >> +-+ >> | br0 | >> | +--+ +--+ +--+ +--+ | >> | | | | | | | | | | >> | | swp0 | | swp1 | | swp2 | | eth0 | | >> +-+ >> || | >> Station A| | >>| | >> +--+--+--++--+--+--+ >> | | | || | | | >> | | swp0 | || | swp0 | | >> Another | +--+ || +--+ | Another >> switch | br0|| br0| switch >> | +--+ || +--+ | >> | | | || | | | >> | | swp1 | || | swp1 | | >> +--+--+--++--+--+--+ >> | >> Station B >> >> Interfaces swp0, swp1, swp2 are handled by a switchdev driver that has >> the following property: frames injected from its control interface bypass >> the internal address analyzer logic, and therefore, this hardware does >> not learn from the source address of packets transmitted by the network >> stack through it. So, since bridging between eth0 (where Station B is >> attached) and swp0 (where Station A is attached) is done in software, >> the switchdev hardware will never learn the source address of Station B. >> So the traffic towards that destination will be treated as unknown, i.e. >> flooded. >> >> This is where the bridge notifications come in handy. When br0 on the >> DUT sees frames with Station B's MAC address on eth0, the switchdev >> driver gets these notifications and can install a rule to send frames >> towards Station B's address that are incoming from swp0, swp1, swp2, >> only towards the control interface. This is all switchdev driver private >> business, which the notification makes possible. >> >> All is fine until someone unplugs Station B's cable and moves it to the >> other switch: >> >>DUT >> +-+ >> | br0 | >> | +--+ +--+ +--+ +--+ | >> | | | | | | | | | | >> | | swp0 | | swp1 | | swp2 | | eth0 | | >> +-+ >> || | >> Station A| | >>| | >> +--+--+--++--+--+--+ >> | | | || | | | >> | | swp0 | || | swp0 | | >> Another | +--+ || +--+ | Another >> switch | br0|| br0| switch >> | +--+ || +--+ | >> | | | || | | | >> | | swp1 | || | swp1 | | >> +--+--+--++--+--+--+ >>| >>Station B >> >> Luckily for the use cases we care about, Station B is noisy enough that >> the DUT hears it (on swp1 this time). swp1 receives the frames and >> delivers them to the bridge, who enters the unlikely path in br_fdb_update >> of updating an existing entry. It moves the entry in the software bridge >> to swp1 and emits an addition notification towards that. >> >> As far as the switchdev driver is concerned, all that it needs to ensure >> is that traffic between Station A and Station B is not forever broken. >> If it does nothing, then the stale rule to send frames for Station B >> towards the control interface remains in place. But Station B is no >> longer reachable via the control interface, but via a port that can >> offload the bridge port learning attribute. It's just that the port is >> prevented from learning this address, since the rule overrides FDB >> updates. So the rule needs to go. The question is via what mechanism. >> >> It sure would be possible for this switchdev driver to keep track of all >> addresses which are sent to the control interface, and then also listen >> for bridge notifier events on its own ports, searching for the ones that >> have a MAC address which was previously sent to the control interface. >> But this is cumbersome and inefficient. Instead, with one small change, >> the bridge could notify of the address deletion from the old port, in a >> symmetrical manner with how it did for the insertion. Then the switchdev >> driver would not be required to monitor learn/forget events for its own >> ports. It could just delete the rule
Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
On 13/12/2020 04:40, Vladimir Oltean wrote: > Currently the bridge emits atomic switchdev notifications for > dynamically learnt FDB entries. Monitoring these notifications works > wonders for switchdev drivers that want to keep their hardware FDB in > sync with the bridge's FDB. > > For example station A wants to talk to station B in the diagram below, > and we are concerned with the behavior of the bridge on the DUT device: > >DUT > +-+ > | br0 | > | +--+ +--+ +--+ +--+ | > | | | | | | | | | | > | | swp0 | | swp1 | | swp2 | | eth0 | | > +-+ > || | > Station A| | >| | > +--+--+--++--+--+--+ > | | | || | | | > | | swp0 | || | swp0 | | > Another | +--+ || +--+ | Another > switch | br0|| br0| switch > | +--+ || +--+ | > | | | || | | | > | | swp1 | || | swp1 | | > +--+--+--++--+--+--+ > | > Station B > > Interfaces swp0, swp1, swp2 are handled by a switchdev driver that has > the following property: frames injected from its control interface bypass > the internal address analyzer logic, and therefore, this hardware does > not learn from the source address of packets transmitted by the network > stack through it. So, since bridging between eth0 (where Station B is > attached) and swp0 (where Station A is attached) is done in software, > the switchdev hardware will never learn the source address of Station B. > So the traffic towards that destination will be treated as unknown, i.e. > flooded. > > This is where the bridge notifications come in handy. When br0 on the > DUT sees frames with Station B's MAC address on eth0, the switchdev > driver gets these notifications and can install a rule to send frames > towards Station B's address that are incoming from swp0, swp1, swp2, > only towards the control interface. This is all switchdev driver private > business, which the notification makes possible. > > All is fine until someone unplugs Station B's cable and moves it to the > other switch: > >DUT > +-+ > | br0 | > | +--+ +--+ +--+ +--+ | > | | | | | | | | | | > | | swp0 | | swp1 | | swp2 | | eth0 | | > +-+ > || | > Station A| | >| | > +--+--+--++--+--+--+ > | | | || | | | > | | swp0 | || | swp0 | | > Another | +--+ || +--+ | Another > switch | br0|| br0| switch > | +--+ || +--+ | > | | | || | | | > | | swp1 | || | swp1 | | > +--+--+--++--+--+--+ >| >Station B > > Luckily for the use cases we care about, Station B is noisy enough that > the DUT hears it (on swp1 this time). swp1 receives the frames and > delivers them to the bridge, who enters the unlikely path in br_fdb_update > of updating an existing entry. It moves the entry in the software bridge > to swp1 and emits an addition notification towards that. > > As far as the switchdev driver is concerned, all that it needs to ensure > is that traffic between Station A and Station B is not forever broken. > If it does nothing, then the stale rule to send frames for Station B > towards the control interface remains in place. But Station B is no > longer reachable via the control interface, but via a port that can > offload the bridge port learning attribute. It's just that the port is > prevented from learning this address, since the rule overrides FDB > updates. So the rule needs to go. The question is via what mechanism. > > It sure would be possible for this switchdev driver to keep track of all > addresses which are sent to the control interface, and then also listen > for bridge notifier events on its own ports, searching for the ones that > have a MAC address which was previously sent to the control interface. > But this is cumbersome and inefficient. Instead, with one small change, > the bridge could notify of the address deletion from the old port, in a > symmetrical manner with how it did for the insertion. Then the switchdev > driver would not be required to monitor learn/forget events for its own > ports. It could just delete the rule towards the control interface upon > bridge entry migration. This would make hardware address learning be > possible again. Then it would take
[PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
Currently the bridge emits atomic switchdev notifications for dynamically learnt FDB entries. Monitoring these notifications works wonders for switchdev drivers that want to keep their hardware FDB in sync with the bridge's FDB. For example station A wants to talk to station B in the diagram below, and we are concerned with the behavior of the bridge on the DUT device: DUT +-+ | br0 | | +--+ +--+ +--+ +--+ | | | | | | | | | | | | | swp0 | | swp1 | | swp2 | | eth0 | | +-+ || | Station A| | | | +--+--+--++--+--+--+ | | | || | | | | | swp0 | || | swp0 | | Another | +--+ || +--+ | Another switch | br0|| br0| switch | +--+ || +--+ | | | | || | | | | | swp1 | || | swp1 | | +--+--+--++--+--+--+ | Station B Interfaces swp0, swp1, swp2 are handled by a switchdev driver that has the following property: frames injected from its control interface bypass the internal address analyzer logic, and therefore, this hardware does not learn from the source address of packets transmitted by the network stack through it. So, since bridging between eth0 (where Station B is attached) and swp0 (where Station A is attached) is done in software, the switchdev hardware will never learn the source address of Station B. So the traffic towards that destination will be treated as unknown, i.e. flooded. This is where the bridge notifications come in handy. When br0 on the DUT sees frames with Station B's MAC address on eth0, the switchdev driver gets these notifications and can install a rule to send frames towards Station B's address that are incoming from swp0, swp1, swp2, only towards the control interface. This is all switchdev driver private business, which the notification makes possible. All is fine until someone unplugs Station B's cable and moves it to the other switch: DUT +-+ | br0 | | +--+ +--+ +--+ +--+ | | | | | | | | | | | | | swp0 | | swp1 | | swp2 | | eth0 | | +-+ || | Station A| | | | +--+--+--++--+--+--+ | | | || | | | | | swp0 | || | swp0 | | Another | +--+ || +--+ | Another switch | br0|| br0| switch | +--+ || +--+ | | | | || | | | | | swp1 | || | swp1 | | +--+--+--++--+--+--+ | Station B Luckily for the use cases we care about, Station B is noisy enough that the DUT hears it (on swp1 this time). swp1 receives the frames and delivers them to the bridge, who enters the unlikely path in br_fdb_update of updating an existing entry. It moves the entry in the software bridge to swp1 and emits an addition notification towards that. As far as the switchdev driver is concerned, all that it needs to ensure is that traffic between Station A and Station B is not forever broken. If it does nothing, then the stale rule to send frames for Station B towards the control interface remains in place. But Station B is no longer reachable via the control interface, but via a port that can offload the bridge port learning attribute. It's just that the port is prevented from learning this address, since the rule overrides FDB updates. So the rule needs to go. The question is via what mechanism. It sure would be possible for this switchdev driver to keep track of all addresses which are sent to the control interface, and then also listen for bridge notifier events on its own ports, searching for the ones that have a MAC address which was previously sent to the control interface. But this is cumbersome and inefficient. Instead, with one small change, the bridge could notify of the address deletion from the old port, in a symmetrical manner with how it did for the insertion. Then the switchdev driver would not be required to monitor learn/forget events for its own ports. It could just delete the rule towards the control interface upon bridge entry migration. This would make hardware address learning be possible again. Then it would take a few more packets until the hardware and software FDB would be in sync again. Signed-off-by: Vladimir Oltean --- Changes in v2: Patch is new. net/bridge/br_fdb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/bridge/br_fdb.c
