Received-SPF: pass (google.com: domain of linux-kernel+bounces-158927-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 25 Apr 2024 09:47:52 -0700
In-Reply-To: <ZioJN6ClnlFIQIBg@chenyu5-mobl2>
Precedence: bulk
Mime-Version: 1.0
References: <20240312193911.1796717-1-seanjc@google.com> <20240312193911.1796717-3-seanjc@google.com>
 <ZioJN6ClnlFIQIBg@chenyu5-mobl2>
Message-ID: <ZiqJOJEBshsQLl0M@google.com>
Subject: Re: [PATCH v2 2/2] sched/core: Drop spinlocks on contention iff
 kernel is preemptible
From: Sean Christopherson <seanjc@google.com>
To: Chen Yu <yu.c.chen@intel.com>
Cc: Jonathan Corbet <corbet@lwn.net>, Ingo Molnar <mingo@redhat.com>, 
	Peter Zijlstra <peterz@infradead.org>, Juri Lelli <juri.lelli@redhat.com>, 
	Vincent Guittot <vincent.guittot@linaro.org>, Will Deacon <will@kernel.org>, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Valentin Schneider <valentin.schneider@arm.com>, 
	Marco Elver <elver@google.com>, Frederic Weisbecker <frederic@kernel.org>, 
	David Matlack <dmatlack@google.com>, Friedrich Weber <f.weber@proxmox.com>, 
	Ankur Arora <ankur.a.arora@oracle.com>, Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="us-ascii"

On Thu, Apr 25, 2024, Chen Yu wrote:
> Hi Sean,
> 
> On 2024-03-12 at 12:39:11 -0700, Sean Christopherson wrote:
> > Use preempt_model_preemptible() to detect a preemptible kernel when
> > deciding whether or not to reschedule in order to drop a contended
> > spinlock or rwlock.  Because PREEMPT_DYNAMIC selects PREEMPTION, kernels
> 
> It took me a while to wonder why PREEMPT_DYNAMIC selects PREEMPTION
> in Kconfig, then I assume that you mean the static config is CONFIG_PREEMPTION,
> but the live preemption model is "none" or "voluntary", which makes the
> static check of CONFIG_PREEMPTION in spin_needbreak() and rwlock_needbreak()
> invalid?

Yep, exactly.

> > diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> > index 3fcd20de6ca8..63dd8cf3c3c2 100644
> > --- a/include/linux/spinlock.h
> > +++ b/include/linux/spinlock.h
> > @@ -462,11 +462,10 @@ static __always_inline int spin_is_contended(spinlock_t *lock)
> >   */
> >  static inline int spin_needbreak(spinlock_t *lock)
> >  {
> > -#ifdef CONFIG_PREEMPTION
> > +	if (!preempt_model_preemptible())
> 
> The old version checks against static CONFIG_PREEMPTION, now we check
> the live CONFIG_PREEMPTION and static CONFIG_PREEMPT_RT, just wonder
> if the rt check is needed here?

It's required, as CONFIG_PREEMPT_RT=y doesn't imply CONFIG_PREEMPT, and
CONFIG_PREEMPT_RT=y is mutually exclusive with CONFIG_PREEMPT_DYNAMIC.  I.e. a
CONFIG_PREEMPT_RT=y kernel will look yield:

  CONFIG_PREEMPT_RT=y
  CONFIG_PREEMPT_DYNAMIC=n
  CONFIG_PREEMPT=n

which in turn generates:

  static inline bool preempt_model_full(void)
  {
	return IS_ENABLED(CONFIG_PREEMPT);
  }

and so just checking preempt_model_full() would incorrectly return false for
CONFIG_PREEMPT_RT=y.