netdev - Re: [PATCH v2 net-next 00/15] locking: Introduce nested-BH locking.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240506144358.Dqd57lTp@linutronix.de>
Date: Mon, 6 May 2024 16:43:58 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Paolo Abeni <pabeni@...hat.com>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Boqun Feng <boqun.feng@...il.com>,
	Daniel Borkmann <daniel@...earbox.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Frederic Weisbecker <frederic@...nel.org>,
	Ingo Molnar <mingo@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Waiman Long <longman@...hat.com>, Will Deacon <will@...nel.org>
Subject: Re: [PATCH v2 net-next 00/15] locking: Introduce nested-BH locking.

On 2024-05-06 16:12:00 [+0200], Paolo Abeni wrote:
> 
> I think sometimes the stack could call local_bh_enable() after a while
> WRT the paired spin lock release, to enforce some serialization - alike
> what inet_twsk_purge() is doing - but I can't point to any specific
> line on top of my head.

I *think* the inet_twsk_purge() is special because the timer is pinned
and that bh_disable call ensures that the timer does not fire.

> A possible side-effect you should/could observe in the final tree is
> more pressure on the process scheduler, as something alike:
> 
> local_bh_disable()
> 
> <spinlock lock unlock>
> 
> <again spinlock lock unlock>
> 
> local_bh_enable()
> 
> could results in more invocation of the scheduler, right? 

Yes, to some degree.
On PREEMPT_RT "spinlock lock" does not disable preemption so the section
remains preemptible. A task with elevated priority (SCHED_RR/FIFO/DL)
remains on the CPU unless preempted by task with higher priority.
Regardless of the locks.

A SCHED_OTHER task can be preempted by another SCHED_OTHER task even
with an acquired spinlock_t. This can be bad performance wise if this
other SCHED_OTHER task preempts the lock owner and blocks on the same
lock. To cope with this we had something called PREEMPT_LAZY (now
PREEMPT_AUTO) in the RT-queue to avoid preemption within SCHED_OTHER
tasks as long as a spinlock_t (or other lock that spins on !RT) is
acquired.
By removing the lock from local_bh_disable() we lose that "please don't
preempt me" feature from your scenario above across the BH disabled
section for SCHED_OTHER tasks. Nothing changes for tasks with elevated
priority.

> Cheers,
> 
> Paolo

Sebastian