RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes

From: Parav Pandit <parav@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Jiri Pirko <jiri@nvidia.com>
Subject: RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes
Date: Wed, 25 Nov 2020 18:17:15 +0000	[thread overview]
Message-ID: <BY5PR12MB43222543F49CA9DF7C25FADEDCFA0@BY5PR12MB4322.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20201125094157.737d37aa@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 25, 2020 11:12 PM
> 
> On Wed, 25 Nov 2020 17:21:41 +0000 Parav Pandit wrote:
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Wednesday, November 25, 2020 10:00 PM
> > >
> > > On Wed, 25 Nov 2020 07:13:40 +0000 Parav Pandit wrote:
> > > > > Maybe even add a check that drivers which support reload set
> > > > > namespace local on their netdevs.
> > > > This will break the backward compatibility as orchestration for
> > > > VFs are not using devlink reload, which is supported very
> > > > recently. But yes, for SF who doesn't have backward compatibility
> > > > issue, as soon as initial series is merged, I will mark it as
> > > > local, so that orchestration doesn't start on wrong foot.
> > >
> > > Ah, right, that will not work because of the shenanigans you guys
> > > play with the uplink port. If all reprs are NETNS_LOCAL it'd not be an
> issue.
> > I am not sure what secret are you talking about with uplink.
> 
> I'm referring to Mellanox conflating PF with uplink. It's not a secret,
Ok.
> we argued about it in the past.
> 
> > I am taking about the SF netdevice to have the NETNS_LOCAL not the SF
> rep.
> > SF rep anyway has NETNS_LOCAL set.
> 
> All reps build by mlx5e_build_rep_netdev() have NETNS_LOCAL.
> 
Yes. this is clear to me and we are good here. 😊

> > I do not follow your comment - 'that will not work'. Can you please explain?
> 
> My half-baked suggestion was to basically add a:
> 
> 	WARN_ON(ops->reload_down && ops->reload_up &&
> 		!(netdev->priv & NETIF_F_NETNS_LOCAL));
> 
> to devlink_port_type_netdev_checks(). Given if device has a reload
> callback devlink is the way to change netns. But yeah, we can't break
> existing behavior so your uplink has to be movable and can't have
> NETNS_LOCAL. IOW adding the WARN_ON() won't work.
> 
Right.

> Hope this is crystal clear now.
Yes, its clear. Thanks.

I addressed your comments and cut down both the fixes to merely 7 lines change. Sent v2.