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
