Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Keller, Jacob E" <jacob.e.keller@intel.com>
To:     Leon Romanovsky <leon@kernel.org>
CC:     Jakub Kicinski <kuba@kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        Guangbin Huang <huangguangbin2@huawei.com>,
        Jiri Pirko <jiri@nvidia.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Salil Mehta <salil.mehta@huawei.com>,
        "Shannon Nelson" <snelson@pensando.io>,
        Yisen Zhuang <yisen.zhuang@huawei.com>,
        "Yufeng Mo" <moyufeng@huawei.com>
Subject: RE: [PATCH net-next 3/6] devlink: Count struct devlink consumers
Thread-Topic: [PATCH net-next 3/6] devlink: Count struct devlink consumers
Thread-Index: AQHXkPLeq0W5yofhY0KH8k2ic9V3+Kt2SjGAgAABsoCAAAO0AIAAWqrwgAJFW4CAAKBNQIAC1qCAgAB5r8A=
Date:   Fri, 20 Aug 2021 20:23:09 +0000
Message-ID: <CO1PR11MB5089D3BF779657D18BCDD4CFD6C19@CO1PR11MB5089.namprd11.prod.outlook.com>
References: <cover.1628933864.git.leonro@nvidia.com>
 <d4d59d801f4521e562c9ecf2d8767077aaefb456.1628933864.git.leonro@nvidia.com>
 <20210816084741.1dd1c415@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <YRqKCVbjTZaSrSy+@unreal>
 <20210816090700.313a54ba@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <CO1PR11MB50895F0BA3FE20CD92D79CB6D6FD9@CO1PR11MB5089.namprd11.prod.outlook.com>
 <YRzA3zCKCgAtprwc@unreal>
 <CO1PR11MB5089A7B1E36B763F075E09FFD6FF9@CO1PR11MB5089.namprd11.prod.outlook.com>
 <YR+o4cNFfg1JqDvJ@unreal>
In-Reply-To: <YR+o4cNFfg1JqDvJ@unreal>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?oGmpgvsFlY4P7z74a+ica9nqHdQWao9m3BMwBypJVpfzwz5OLBju/GNJodVg?=
 =?us-ascii?Q?wYLpVKDWbKekkEKe6chWwNjAK5nSSmaku3D6pRbPXfMPLoo7wYRGSiolRr2a?=
 =?us-ascii?Q?QBIbTBOWnnS/45tbI7CtIPGN4+tMrlX5JPlOmaV2GEMO0tHf5cZTvF95HAex?=
 =?us-ascii?Q?WH/A5qjMdENZHVmpY9IAymFtnfTSi/EQtZU6SnHlaTuUK/Yz5rkzII2yDVm+?=
 =?us-ascii?Q?U0dmSpr5EYErJaQWUtmuNfeTzYIc/BvmNaAXZuqewfIZ98kzIEEVXMVcMOLy?=
 =?us-ascii?Q?9eWlpyoF1ccLTV0slevKeIHC1a7Z7y7Nw0ocUKqTieGs2KR3GC5eizNGiRpm?=
 =?us-ascii?Q?jT8r0QJckEITrn7WZRjLKlee9hdhJGW3kZOJSPE523VvvfSwMY0R61MorV6E?=
 =?us-ascii?Q?1XwPHB5hmuY50NYPHAP/SqPz3EpTeFIiNLA9By1HW5vpMZfag3Dkk3Pdz1c/?=
 =?us-ascii?Q?3dYhzvirLcSqvWqvGO8sdubcdoqFIEZOnkf98lKCZ74mZFNF6Mma48FjKI8o?=
 =?us-ascii?Q?3a8iozCGajr2UnKcOqHX7f+cToVMb7xm6G3g4qCfP8/K9u6AcN1L4C9KFPyb?=
 =?us-ascii?Q?WEal0SOxZs3VyWoLmzdIq7eRb+F5tzFwQ1wr5Yy0pqnMDbZPcYISl7ZFmQi1?=
 =?us-ascii?Q?pIp1NbwOmKx+r10QG/v0feJRx4mYnROaPKHYQhJfOsotl1qCpN9qnO/qHdfq?=
 =?us-ascii?Q?u39eKJJJZ68VFhnEXO7HYkyjvnxe7vOJ+KMqyDn/dWAryhexVitHEb57cq5Z?=
 =?us-ascii?Q?U1RjOjtTSYdt/iX9+7sVEx3pox0fVVn9TpfhcSXWMAnDcDlz/RXmLINciuLV?=
 =?us-ascii?Q?NrVikRc2XO1YTq6T0kb42gvabUDCiAntlaHQQlU/rae3g184FkuriJo+yFWE?=
 =?us-ascii?Q?KUHbFgVnSemVg7TiVwz214aLwjeG7AcwOP7I6m7hFHrJ4lSLcqHxK6fL4Pzy?=
 =?us-ascii?Q?fLinBw/vss8xvs/BeP5mOPBM6lyYi2ALsQfcMCkBl3d3u+KeRWtPuoYleq12?=
 =?us-ascii?Q?XKCgv3/eC+2umQq8OhFEQeZwcCxmhCNZDUKvWU6LHcmFjgAyHVkFCDYW7hzR?=
 =?us-ascii?Q?NBPjJcSshMYKl1JvfCfqM20Lfvr7ByL1Y2h0mIiYt3jomQjvtNXrQM7pyhVY?=
 =?us-ascii?Q?soh6ZZDDBNQqJcK7nRUCmHYBp2a9z5rbnmhxRwQ1H1ckyBjik0MLZ/KPGprx?=
 =?us-ascii?Q?wMShDvj+XoDqZdkelEVWRDuS4XvKppHIKpNEOCfjtNPELdguBE14fxjcnN3k?=
 =?us-ascii?Q?nkKGwYt0IVj5eyeC9LOwqS1YkuFQx8OrYzDAvSIhiZVbtqaOuU3gbGixlhmo?=
 =?us-ascii?Q?b5w=3D?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 45a69f5e-e8ca-4de0-8b75-08d964185381
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Aug 2021 20:23:09.1213
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: w/QhspQJMdckrMBcnIyI74ArYo+voXEwLgG6bZAuqhrSUG+bgF8chwS3VrCvouSaPQVx2NGI6MAoA4feg7TBz55WtvLAWtdGAy072aP3g94=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4635
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, August 20, 2021 6:07 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; David S . Miller <davem@davemloft.n=
et>;
> Guangbin Huang <huangguangbin2@huawei.com>; Jiri Pirko <jiri@nvidia.com>;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org; Salil Mehta
> <salil.mehta@huawei.com>; Shannon Nelson <snelson@pensando.io>; Yisen
> Zhuang <yisen.zhuang@huawei.com>; Yufeng Mo <moyufeng@huawei.com>
> Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers
>=20
> On Wed, Aug 18, 2021 at 05:50:11PM +0000, Keller, Jacob E wrote:
> >
> >
> > > -----Original Message-----
> > > From: Leon Romanovsky <leon@kernel.org>
> > > Sent: Wednesday, August 18, 2021 1:12 AM
> > > To: Keller, Jacob E <jacob.e.keller@intel.com>
> > > Cc: Jakub Kicinski <kuba@kernel.org>; David S . Miller
> <davem@davemloft.net>;
> > > Guangbin Huang <huangguangbin2@huawei.com>; Jiri Pirko
> <jiri@nvidia.com>;
> > > linux-kernel@vger.kernel.org; netdev@vger.kernel.org; Salil Mehta
> > > <salil.mehta@huawei.com>; Shannon Nelson <snelson@pensando.io>; Yisen
> > > Zhuang <yisen.zhuang@huawei.com>; Yufeng Mo <moyufeng@huawei.com>
> > > Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consu=
mers
> > >
> > > On Mon, Aug 16, 2021 at 09:32:17PM +0000, Keller, Jacob E wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jakub Kicinski <kuba@kernel.org>
> > > > > Sent: Monday, August 16, 2021 9:07 AM
> > > > > To: Leon Romanovsky <leon@kernel.org>
> > > > > Cc: David S . Miller <davem@davemloft.net>; Guangbin Huang
> > > > > <huangguangbin2@huawei.com>; Keller, Jacob E
> <jacob.e.keller@intel.com>;
> > > Jiri
> > > > > Pirko <jiri@nvidia.com>; linux-kernel@vger.kernel.org;
> > > netdev@vger.kernel.org;
> > > > > Salil Mehta <salil.mehta@huawei.com>; Shannon Nelson
> > > > > <snelson@pensando.io>; Yisen Zhuang <yisen.zhuang@huawei.com>;
> Yufeng
> > > > > Mo <moyufeng@huawei.com>
> > > > > Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink c=
onsumers
> > > > >
> > > > > On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:
> > > > > > On Mon, Aug 16, 2021 at 08:47:41AM -0700, Jakub Kicinski wrote:
> > > > > > > On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:
> > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > >
> > > > > > > > The struct devlink itself is protected by internal lock and=
 doesn't
> > > > > > > > need global lock during operation. That global lock is used=
 to protect
> > > > > > > > addition/removal new devlink instances from the global list=
 in use by
> > > > > > > > all devlink consumers in the system.
> > > > > > > >
> > > > > > > > The future conversion of linked list to be xarray will allo=
w us to
> > > > > > > > actually delete that lock, but first we need to count all s=
truct devlink
> > > > > > > > users.
> > > > > > >
> > > > > > > Not a problem with this set but to state the obvious the glob=
al devlink
> > > > > > > lock also protects from concurrent execution of all the ops w=
hich don't
> > > > > > > take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most li=
kely
> > > know
> > > > > > > this but I thought I'd comment on an off chance it helps.
> > > > > >
> > > > > > The end goal will be something like that:
> > > > > > 1. Delete devlink lock
> > > > > > 2. Rely on xa_lock() while grabbing devlink instance (past
> devlink_try_get)
> > > > > > 3. Convert devlink->lock to be read/write lock to make sure tha=
t we can
> run
> > > > > > get query in parallel.
> > > > > > 4. Open devlink netlink to parallel ops, ".parallel_ops =3D tru=
e".
> > > > >
> > > > > IIUC that'd mean setting eswitch mode would hold write lock on
> > > > > the dl instance. What locks does e.g. registering a dl port take
> > > > > then?
> > > >
> > > > Also that I think we have some cases where we want to allow the dri=
ver to
> > > allocate new devlink objects in response to adding a port, but still =
want to
> block
> > > other global operations from running?
> > >
> > > I don't see the flow where operations on devlink_A should block devli=
nk_B.
> > > Only in such flows we will need global lock like we have now - devlin=
k->lock.
> > > In all other flows, write lock of devlink instance will protect from
> > > parallel execution.
> > >
> > > Thanks
> >
> >
> > But how do we handle what is essentially recursion?
>=20
> Let's wait till implementation, I promise it will be covered :).
>=20

Sure. It's certainly easier to talk about a proposed implementation once we=
 have it.

> >
> > If we add a port on the devlink A:
> >
> > userspace sends PORT_ADD for devlink A
> > driver responds by creating a port
> > adding a port causes driver to add a region, or other devlink object
> >
> > In the current design, if I understand correctly, we hold the global lo=
ck but
> *not* the instance lock. We can't hold the instance lock while adding por=
t
> without breaking a bunch of drivers that add many devlink objects in resp=
onse to
> port creation.. because they'll deadlock when going to add the sub object=
s.
> >
> > But if we don't hold the global lock, then in theory another userspace =
program
> could attempt to do something inbetween PORT_ADD starting and finishing
> which might not be desirable.  (Remember, we had to drop the instance loc=
k
> otherwise drivers get stuck when trying to add many subobjects)
>=20
> You just surfaced my main issue with the current devlink
> implementation - the purpose of devlink_lock. Over the years devlink
> code lost clear separation between user space flows and kernel flows.
>=20
> Thanks
>=20

Yep. It's definitely complex.

> >
> > Thanks,
> > Jake