Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Stephen Hemminger <stephen@networkplumber.org>,
	"Ahmed S. Darwish" <a.darwish@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	"Sebastian A. Siewior" <bigeasy@linutronix.de>,
	Steven Rostedt <rostedt@goodmis.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org
Subject: Re: [PATCH v1 01/25] net: core: device_rename: Use rwsem instead of a seqcount
Date: Wed, 20 May 2020 00:23:48 +0200
Message-ID: <87v9kr5zt7.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20200519150159.4d91af93@hermes.lan>

Stephen Hemminger <stephen@networkplumber.org> writes:
> On Tue, 19 May 2020 23:45:23 +0200
> "Ahmed S. Darwish" <a.darwish@linutronix.de> wrote:
>
>> Sequence counters write paths are critical sections that must never be
>> preempted, and blocking, even for CONFIG_PREEMPTION=n, is not allowed.
>> 
>> Commit 5dbe7c178d3f ("net: fix kernel deadlock with interface rename and
>> netdev name retrieval.") handled a deadlock, observed with
>> CONFIG_PREEMPTION=n, where the devnet_rename seqcount read side was
>> infinitely spinning: it got scheduled after the seqcount write side
>> blocked inside its own critical section.
>> 
>> To fix that deadlock, among other issues, the commit added a
>> cond_resched() inside the read side section. While this will get the
>> non-preemptible kernel eventually unstuck, the seqcount reader is fully
>> exhausting its slice just spinning -- until TIF_NEED_RESCHED is set.
>> 
>> The fix is also still broken: if the seqcount reader belongs to a
>> real-time scheduling policy, it can spin forever and the kernel will
>> livelock.
>> 
>> Disabling preemption over the seqcount write side critical section will
>> not work: inside it are a number of GFP_KERNEL allocations and mutex
>> locking through the drivers/base/ :: device_rename() call chain.
>> 
>> From all the above, replace the seqcount with a rwsem.
>> 
>> Fixes: 5dbe7c178d3f (net: fix kernel deadlock with interface rename and netdev name retrieval.)
>> Fixes: 30e6c9fa93cf (net: devnet_rename_seq should be a seqcount)
>> Fixes: c91f6df2db49 (sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name)
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
>> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> Have your performance tested this with 1000's of network devices?

No. We did not. -ENOTESTCASE

> The reason seqcount logic is was done here was to achieve scaleablity
> and a semaphore does not scale as well.

That still does not make the livelock magically going away. Just make a
reader with real-time priority preempt the writer and the system stops
dead. The net result is perfomance <= 0.

This was observed on RT kernels without a special 1000's of network
devices test case.

Just for the record: This is not a RT specific problem. You can
reproduce that w/o an RT kernel as well. Just run the reader with
real-time scheduling policy.

As much as you hate it from a performance POV the only sane rule of
programming is: Correctness first.

And this code clearly violates that rule.

Thanks,

        tglx

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-19 21:45 [PATCH v1 00/25] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 01/25] net: core: device_rename: Use rwsem instead of a seqcount Ahmed S. Darwish
2020-05-19 22:01   ` Stephen Hemminger
2020-05-19 22:23     ` Thomas Gleixner [this message]
2020-05-19 23:11       ` Stephen Hemminger
2020-05-19 23:42         ` Thomas Gleixner
2020-05-20  0:06           ` Stephen Hemminger
2020-05-20  1:55             ` Thomas Gleixner
2020-05-20  2:57           ` David Miller
2020-05-20  3:18             ` Eric Dumazet
2020-05-20  4:36               ` Stephen Hemminger
2020-05-20 19:37             ` Thomas Gleixner
2020-05-20 21:36               ` Stephen Hemminger
2020-05-20  2:01   ` Eric Dumazet
2020-05-20  6:42     ` Ahmed S. Darwish
2020-05-20 12:51       ` Eric Dumazet
2020-06-03 14:33         ` Ahmed S. Darwish
2020-05-20 14:37   ` Dan Carpenter
2020-05-25 16:22     ` Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 03/25] net: phy: fixed_phy: Remove unused seqcount Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 05/25] u64_stats: Document writer non-preemptibility requirement Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 15/25] netfilter: conntrack: Use sequence counter with associated spinlock Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 16/25] netfilter: nft_set_rbtree: Use sequence counter with associated rwlock Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 17/25] xfrm: policy: Use sequence counters with associated lock Ahmed S. Darwish
2020-06-08  0:57 ` [PATCH v2 00/18] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-06-08  0:57   ` [PATCH v2 08/18] netfilter: conntrack: Use sequence counter with associated spinlock Ahmed S. Darwish
2020-06-08  0:57   ` [PATCH v2 09/18] netfilter: nft_set_rbtree: Use sequence counter with associated rwlock Ahmed S. Darwish
2020-06-08  0:57   ` [PATCH v2 10/18] xfrm: policy: Use sequence counters with associated lock Ahmed S. Darwish
2020-06-30  5:44 ` [PATCH v3 00/20] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-06-30  5:44   ` [PATCH v3 10/20] netfilter: conntrack: Use sequence counter with associated spinlock Ahmed S. Darwish
2020-06-30  5:44   ` [PATCH v3 11/20] netfilter: nft_set_rbtree: Use sequence counter with associated rwlock Ahmed S. Darwish
2020-06-30  5:44   ` [PATCH v3 12/20] xfrm: policy: Use sequence counters with associated lock Ahmed S. Darwish
2020-07-20 15:55 ` [PATCH v4 00/24] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-07-20 15:55   ` [PATCH v4 14/24] netfilter: conntrack: Use sequence counter with associated spinlock Ahmed S. Darwish
2020-07-20 15:55   ` [PATCH v4 15/24] netfilter: nft_set_rbtree: Use sequence counter with associated rwlock Ahmed S. Darwish
2020-07-20 15:55   ` [PATCH v4 16/24] xfrm: policy: Use sequence counters with associated lock Ahmed S. Darwish
2020-07-20 16:49   ` [PATCH v4 00/24] seqlock: Extend seqcount API with associated locks Eric Biggers
2020-07-20 17:33     ` Ahmed S. Darwish

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=87v9kr5zt7.fsf@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=a.darwish@linutronix.de \
    --cc=bigeasy@linutronix.de \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=stephen@networkplumber.org \
    --cc=will@kernel.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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git