Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration

[Nikolay Aleksandrov]

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)
> 				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,
> and the logic is a bit mind-boggling. I don't know, it is all much
> simpler if we just emit an insertion notification on insertion and a
> deletion notification on deletion. Which way you prefer, really.

Yep, I agree.

Thanks,
 Nik