linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nikolay Aleksandrov <nikolay@nvidia.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"bridge@lists.linux-foundation.org" 
	<bridge@lists.linux-foundation.org>,
	Roopa Prabhu <roopa@nvidia.com>,
	"David S. Miller" <davem@davemloft.net>,
	DENG Qingfang <dqfext@gmail.com>,
	Tobias Waldekranz <tobias@waldekranz.com>,
	Marek Behun <marek.behun@nic.cz>,
	Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	Alexandra Winter <wintera@linux.ibm.com>,
	Jiri Pirko <jiri@resnulli.us>, Ido Schimmel <idosch@idosch.org>,
	Claudiu Manoil <claudiu.manoil@nxp.com>
Subject: Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration
Date: Sun, 13 Dec 2020 16:01:56 +0200	[thread overview]
Message-ID: <e40272d8-0a9c-eb53-c740-dcb156643fda@nvidia.com> (raw)
In-Reply-To: <20201213135557.ysmx3qjnafl5yihm@skbuf>

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


  reply	other threads:[~2020-12-13 14:03 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-13  2:40 [PATCH v2 net-next 0/6] Offload software learnt bridge addresses to DSA Vladimir Oltean
2020-12-13  2:40 ` [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration Vladimir Oltean
2020-12-13 13:22   ` Nikolay Aleksandrov
2020-12-13 13:36     ` Nikolay Aleksandrov
2020-12-13 13:57       ` Vladimir Oltean
2020-12-13 13:55     ` Vladimir Oltean
2020-12-13 14:01       ` Nikolay Aleksandrov [this message]
2020-12-13  2:40 ` [PATCH v2 net-next 2/6] net: dsa: don't use switchdev_notifier_fdb_info in dsa_switchdev_event_work Vladimir Oltean
2020-12-13  3:43   ` Florian Fainelli
2020-12-13  2:40 ` [PATCH v2 net-next 3/6] net: dsa: move switchdev event implementation under the same switch/case statement Vladimir Oltean
2020-12-13  3:31   ` Florian Fainelli
2020-12-13  2:40 ` [PATCH v2 net-next 4/6] net: dsa: exit early in dsa_slave_switchdev_event if we can't program the FDB Vladimir Oltean
2020-12-13  3:29   ` Florian Fainelli
2020-12-13  2:40 ` [PATCH v2 net-next 5/6] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors Vladimir Oltean
2020-12-13  3:48   ` Florian Fainelli
2020-12-13  2:40 ` [PATCH v2 net-next 6/6] net: dsa: ocelot: request DSA to fix up lack of address learning on CPU port Vladimir Oltean
2020-12-13  3:49   ` Florian Fainelli

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=e40272d8-0a9c-eb53-c740-dcb156643fda@nvidia.com \
    --to=nikolay@nvidia.com \
    --cc=andrew@lunn.ch \
    --cc=bridge@lists.linux-foundation.org \
    --cc=claudiu.manoil@nxp.com \
    --cc=davem@davemloft.net \
    --cc=dqfext@gmail.com \
    --cc=f.fainelli@gmail.com \
    --cc=idosch@idosch.org \
    --cc=jiri@resnulli.us \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=marek.behun@nic.cz \
    --cc=netdev@vger.kernel.org \
    --cc=roopa@nvidia.com \
    --cc=tobias@waldekranz.com \
    --cc=vivien.didelot@gmail.com \
    --cc=vladimir.oltean@nxp.com \
    --cc=wintera@linux.ibm.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).