Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Leon Romanovsky <leon@kernel.org>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	David Miller <davem@davemloft.net>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	"Cong Wang ." <cong.wang@bytedance.com>,
	Peilin Ye <peilin.ye@bytedance.com>,
	Jiri Pirko <jiri@resnulli.us>
Subject: Re: [PATCH net-next] Revert "netdevsim: Add multi-queue support"
Date: Wed, 4 Aug 2021 08:56:17 -0700	[thread overview]
Message-ID: <20210804085617.58855e62@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <YQqM8XUyHKVaj1WF@unreal>

On Wed, 4 Aug 2021 15:49:53 +0300 Leon Romanovsky wrote:
> It is something preliminary, I have POC code which works but it is far
> from the actual patches yet.
> 
> The problem is that "devlink reload" in its current form causes us
> (mlx5) a lot of grief. We see deadlocks due to combinations of internal
> flows with external ones, without going too much in details loops of
> module removal together with health recovery and devlink reload doesn't
> work properly :).
> 
> The same problem exists in all drivers that implement "devlink reload",
> mlx5 just most complicated one and looks like most tested either.
> 
> My idea (for now) is pretty simple:
> 1. Move devlink ops callbacks from devlink_alloc phase to devlink_register().
> 2. Ensure that devlink_register() is the last command in the probe sequence.

IDK.. does that work with port registration vs netdev registration?
IIRC ports should be registered first.

In general devlink is between bus devices and netdevs so I think
devlink should be initialized first, right?

Is merging the two flows (probe vs reload) possible? What I mean is can
we make the drivers use reload_up() during probe? IOW if driver has
.reload_up() make devlink core call that on register with whatever
locks it holds to prevent double-reload?

Either way please make sure to dump all the knowledge you gain about
the locking to some doc. Seems like that's a major sore spot for
devlink.

> 3. Delete devlink_reload_enable/disable. It is not needed if proper ops used.
> 4. Add reference counting to struct devlink to make sure that we
> properly account netlink users, so we will be able to drop big devlink_lock.
> 5. Convert linked list of devlink instances to be xarray. It gives us an
> option to work relatively lockless.
> ....
> 
> Every step solves some bug, even first one solves current bug where
> devlink reload statistics presented despite devlink_reload_disable().
> 
> Of course, we can try to patch devlink with specific fix for specific
> bug, but better to make it error prone from the beginning.
> 
> So I'm trying to get a sense what can and what can't be done in the netdev.
> And netdevsim combination of devlink and sysfs knobs adds challenges. :)

  reply	other threads:[~2021-08-04 15:56 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 12:39 Jakub Kicinski
2021-08-03 17:11 ` Cong Wang
2021-08-03 21:18   ` Jakub Kicinski
2021-08-03 21:32     ` Cong Wang
2021-08-03 21:51       ` Jakub Kicinski
2021-08-03 22:04         ` Cong Wang
2021-08-04  7:14         ` Leon Romanovsky
2021-08-04 11:52           ` Jakub Kicinski
2021-08-04 12:49             ` Leon Romanovsky
2021-08-04 15:56               ` Jakub Kicinski [this message]
2021-08-04 17:25                 ` Leon Romanovsky
2021-08-03 22:16 ` [PATCH net-next] tc-testing: Add control-plane selftests for sch_mq Peilin Ye
2021-08-03 22:21   ` Cong Wang
2021-08-04 11:50   ` 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=20210804085617.58855e62@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=cong.wang@bytedance.com \
    --cc=davem@davemloft.net \
    --cc=jiri@resnulli.us \
    --cc=leon@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=peilin.ye@bytedance.com \
    --cc=xiyou.wangcong@gmail.com \
    --subject='Re: [PATCH net-next] Revert "netdevsim: Add multi-queue support"' \
    /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).