All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jason Wang <jasowang@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Leon Romanovsky <leon@kernel.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Petr Mladek <pmladek@suse.com>,
	John Ogness <john.ogness@linutronix.de>,
	virtualization@lists.linux-foundation.org,
	Amit Shah <amit@kernel.org>, Itay Aveksis <itayav@nvidia.com>,
	Ran Rozenstein <ranro@nvidia.com>,
	netdev <netdev@vger.kernel.org>
Subject: Re: netconsole deadlock with virtnet
Date: Wed, 25 Nov 2020 14:21:12 +0800	[thread overview]
Message-ID: <6e55048e-53ed-c196-729d-f7a5ab3c82fe@redhat.com> (raw)
In-Reply-To: <20201124082035.3e658fa4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>


On 2020/11/25 上午12:20, Jakub Kicinski wrote:
> On Tue, 24 Nov 2020 11:22:03 +0800 Jason Wang wrote:
>>>> Perhaps you need the trylock in virtnet_poll_tx()?
>>> That could work. Best if we used normal lock if !!budget, and trylock
>>> when budget is 0. But maybe that's too hairy.
>> If we use trylock, we probably lose(or delay) tx notification that may
>> have side effects to the stack.
> That's why I said only trylock with budget == 0. Only netpoll calls with
> budget == 0, AFAIK.


Oh right.

So I think maybe we can switch to use trylock when budget is zero and 
try to schedule another TX NAPI if we trylock fail.


>
>>> I'm assuming all this trickiness comes from virtqueue_get_buf() needing
>>> locking vs the TX path? It's pretty unusual for the completion path to
>>> need locking vs xmit path.
>> Two reasons for doing this:
>>
>> 1) For some historical reason, we try to free transmitted tx packets in
>> xmit (see free_old_xmit_skbs() in start_xmit()), we can probably remove
>> this if we remove the non tx interrupt mode.
>> 2) virtio core requires virtqueue_get_buf() to be synchronized with
>> virtqueue_add(), we probably can solve this but it requires some non
>> trivial refactoring in the virtio core
>>
>> Btw, have a quick search, there are several other drivers that uses tx
>> lock in the tx NAPI.
> Unless they do:
>
> 	netdev->priv_flags |= IFF_DISABLE_NETPOLL;
>
> they are all broken.


Yes.

Thanks


WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Petr Mladek <pmladek@suse.com>, Leon Romanovsky <leon@kernel.org>,
	John Ogness <john.ogness@linutronix.de>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	netdev <netdev@vger.kernel.org>, Amit Shah <amit@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	virtualization@lists.linux-foundation.org,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Ran Rozenstein <ranro@nvidia.com>,
	Itay Aveksis <itayav@nvidia.com>
Subject: Re: netconsole deadlock with virtnet
Date: Wed, 25 Nov 2020 14:21:12 +0800	[thread overview]
Message-ID: <6e55048e-53ed-c196-729d-f7a5ab3c82fe@redhat.com> (raw)
In-Reply-To: <20201124082035.3e658fa4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>


On 2020/11/25 上午12:20, Jakub Kicinski wrote:
> On Tue, 24 Nov 2020 11:22:03 +0800 Jason Wang wrote:
>>>> Perhaps you need the trylock in virtnet_poll_tx()?
>>> That could work. Best if we used normal lock if !!budget, and trylock
>>> when budget is 0. But maybe that's too hairy.
>> If we use trylock, we probably lose(or delay) tx notification that may
>> have side effects to the stack.
> That's why I said only trylock with budget == 0. Only netpoll calls with
> budget == 0, AFAIK.


Oh right.

So I think maybe we can switch to use trylock when budget is zero and 
try to schedule another TX NAPI if we trylock fail.


>
>>> I'm assuming all this trickiness comes from virtqueue_get_buf() needing
>>> locking vs the TX path? It's pretty unusual for the completion path to
>>> need locking vs xmit path.
>> Two reasons for doing this:
>>
>> 1) For some historical reason, we try to free transmitted tx packets in
>> xmit (see free_old_xmit_skbs() in start_xmit()), we can probably remove
>> this if we remove the non tx interrupt mode.
>> 2) virtio core requires virtqueue_get_buf() to be synchronized with
>> virtqueue_add(), we probably can solve this but it requires some non
>> trivial refactoring in the virtio core
>>
>> Btw, have a quick search, there are several other drivers that uses tx
>> lock in the tx NAPI.
> Unless they do:
>
> 	netdev->priv_flags |= IFF_DISABLE_NETPOLL;
>
> they are all broken.


Yes.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

  reply	other threads:[~2020-11-25  6:22 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-17 10:23 netconsole deadlock with virtnet Leon Romanovsky
2020-11-17 14:33 ` Steven Rostedt
2020-11-17 18:12   ` Leon Romanovsky
2020-11-18  2:46   ` Sergey Senozhatsky
2020-11-18  3:15     ` Sergey Senozhatsky
2020-11-18  4:09       ` Jason Wang
2020-11-18 14:12         ` Steven Rostedt
2020-11-18 14:12           ` Steven Rostedt
2020-11-23 11:08           ` Leon Romanovsky
2020-11-23 11:08             ` Leon Romanovsky
2020-11-23 14:31             ` Steven Rostedt
2020-11-23 14:31               ` Steven Rostedt
2020-11-23 18:52               ` Jakub Kicinski
2020-11-23 19:09                 ` Steven Rostedt
2020-11-23 19:09                   ` Steven Rostedt
2020-11-23 19:21                   ` Jakub Kicinski
2020-11-24  3:22                     ` Jason Wang
2020-11-24  3:22                       ` Jason Wang
2020-11-24  8:01                       ` Leon Romanovsky
2020-11-24  8:01                         ` Leon Romanovsky
2020-11-24  8:57                         ` Jason Wang
2020-11-24  8:57                           ` Jason Wang
2020-11-24  9:26                           ` Leon Romanovsky
2020-11-24  9:26                             ` Leon Romanovsky
2020-11-24 14:31                       ` Steven Rostedt
2020-11-24 14:31                         ` Steven Rostedt
2020-11-25  6:20                         ` Jason Wang
2020-11-25  6:20                           ` Jason Wang
2020-11-24 16:20                       ` Jakub Kicinski
2020-11-25  6:21                         ` Jason Wang [this message]
2020-11-25  6:21                           ` Jason Wang
2020-11-19 12:55   ` Petr Mladek via Virtualization
2020-11-22  8:41     ` Leon Romanovsky

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=6e55048e-53ed-c196-729d-f7a5ab3c82fe@redhat.com \
    --to=jasowang@redhat.com \
    --cc=amit@kernel.org \
    --cc=itayav@nvidia.com \
    --cc=john.ogness@linutronix.de \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=ranro@nvidia.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=virtualization@lists.linux-foundation.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.