LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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
Date: Wed, 18 Aug 2021 17:50:11 +0000	[thread overview]
Message-ID: <CO1PR11MB5089A7B1E36B763F075E09FFD6FF9@CO1PR11MB5089.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YRzA3zCKCgAtprwc@unreal>



> -----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 consumers
> 
> 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 consumers
> > >
> > > 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 allow us to
> > > > > > actually delete that lock, but first we need to count all struct devlink
> > > > > > users.
> > > > >
> > > > > Not a problem with this set but to state the obvious the global devlink
> > > > > lock also protects from concurrent execution of all the ops which don't
> > > > > take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely
> 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 that we can run
> > > > get query in parallel.
> > > > 4. Open devlink netlink to parallel ops, ".parallel_ops = true".
> > >
> > > 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 driver 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 devlink_B.
> Only in such flows we will need global lock like we have now - devlink->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?

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 lock but *not* the instance lock. We can't hold the instance lock while adding port without breaking a bunch of drivers that add many devlink objects in response to port creation.. because they'll deadlock when going to add the sub objects.

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 lock otherwise drivers get stuck when trying to add many subobjects)

Thanks,
Jake

  reply	other threads:[~2021-08-18 17:50 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14  9:57 [PATCH net-next 0/6] Devlink cleanup for delay event series Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 1/6] devlink: Simplify devlink_pernet_pre_exit call Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 2/6] devlink: Remove check of always valid devlink pointer Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 3/6] devlink: Count struct devlink consumers Leon Romanovsky
2021-08-16 15:47   ` Jakub Kicinski
2021-08-16 15:53     ` Leon Romanovsky
2021-08-16 16:07       ` Jakub Kicinski
2021-08-16 21:32         ` Keller, Jacob E
2021-08-18  8:12           ` Leon Romanovsky
2021-08-18 17:50             ` Keller, Jacob E [this message]
2021-08-20 13:06               ` Leon Romanovsky
2021-08-20 20:23                 ` Keller, Jacob E
2021-08-18  8:03         ` Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 4/6] devlink: Use xarray to store devlink instances Leon Romanovsky
2021-08-16 21:27   ` Keller, Jacob E
2021-08-14  9:57 ` [PATCH net-next 5/6] devlink: Clear whole devlink_flash_notify struct Leon Romanovsky
2021-08-16 21:29   ` Keller, Jacob E
2021-08-14  9:57 ` [PATCH net-next 6/6] net: hns3: remove always exist devlink pointer check Leon Romanovsky
2021-08-14 13:10 ` [PATCH net-next 0/6] Devlink cleanup for delay event series patchwork-bot+netdevbpf

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=CO1PR11MB5089A7B1E36B763F075E09FFD6FF9@CO1PR11MB5089.namprd11.prod.outlook.com \
    --to=jacob.e.keller@intel.com \
    --cc=davem@davemloft.net \
    --cc=huangguangbin2@huawei.com \
    --cc=jiri@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=moyufeng@huawei.com \
    --cc=netdev@vger.kernel.org \
    --cc=salil.mehta@huawei.com \
    --cc=snelson@pensando.io \
    --cc=yisen.zhuang@huawei.com \
    --subject='RE: [PATCH net-next 3/6] devlink: Count struct devlink consumers' \
    /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

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).