[PATCH] lib: Check for migrate_disable only on SMP systems

[PATCH] lib: Check for migrate_disable only on SMP systems

[Daniel Wagner]

In case the kernel configuration is UP is, the migrate_disable member
in task_struct is missing.

linux/lib/smp_processor_id.c: In function ‘check_preemption_disabled’:
linux/lib/smp_processor_id.c:26:13: error: ‘struct task_struct’ has no member named ‘migrate_disable’
    26 |  if (current->migrate_disable)
       |             ^~

Fixes: 425c5b38779a ("sched: Lazy migrate_disable processing")
Cc: Scott Wood <swood@redhat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 lib/smp_processor_id.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index 5f2618d346c4..a914f73e3652 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -23,8 +23,10 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
 	 * Kernel threads bound to a single CPU can safely use
 	 * smp_processor_id():
 	 */
+#if defined(CONFIG_SMP) && defined(CONFIG_PREEMPT_RT_BASE)
 	if (current->migrate_disable)
 		goto out;
+#endif
 
 	if (current->nr_cpus_allowed == 1)
 		goto out;
-- 
2.24.0

Re: [PATCH] lib: Check for migrate_disable only on SMP systems

[Scott Wood]

On Fri, 2019-12-06 at 15:21 +0100, Daniel Wagner wrote:
> In case the kernel configuration is UP is, the migrate_disable member
> in task_struct is missing.
> 
> linux/lib/smp_processor_id.c: In function ‘check_preemption_disabled’:
> linux/lib/smp_processor_id.c:26:13: error: ‘struct task_struct’ has no
> member named ‘migrate_disable’
>     26 |  if (current->migrate_disable)
>        |             ^~
> 
> Fixes: 425c5b38779a ("sched: Lazy migrate_disable processing")
> Cc: Scott Wood <swood@redhat.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  lib/smp_processor_id.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
> index 5f2618d346c4..a914f73e3652 100644
> --- a/lib/smp_processor_id.c
> +++ b/lib/smp_processor_id.c
> @@ -23,8 +23,10 @@ unsigned int check_preemption_disabled(const char
> *what1, const char *what2)
>  	 * Kernel threads bound to a single CPU can safely use
>  	 * smp_processor_id():
>  	 */
> +#if defined(CONFIG_SMP) && defined(CONFIG_PREEMPT_RT_BASE)
>  	if (current->migrate_disable)
>  		goto out;
> +#endif

Won't this give false positives on UP with CONFIG_PREEMPT_RT_BASE and
CONFIG_DEBUG_PREEMPT?  If we have CONFIG_SCHED_DEBUG then we can still check
migrate_disable.  Without that, we don't have enough information to tell
whether it's safe to do smp_processor_id() and would have to skip
check_preemption_disabled() entirely.

-Scott

Re: [PATCH] lib: Check for migrate_disable only on SMP systems

[Daniel Wagner]

Hi Scott,

On Fri, Dec 06, 2019 at 12:49:12PM -0600, Scott Wood wrote:
> On Fri, 2019-12-06 at 15:21 +0100, Daniel Wagner wrote:
> > In case the kernel configuration is UP is, the migrate_disable member
> > in task_struct is missing.
> > 
> > linux/lib/smp_processor_id.c: In function ‘check_preemption_disabled’:
> > linux/lib/smp_processor_id.c:26:13: error: ‘struct task_struct’ has no
> > member named ‘migrate_disable’
> >     26 |  if (current->migrate_disable)
> >        |             ^~
> > 
> > Fixes: 425c5b38779a ("sched: Lazy migrate_disable processing")
> > Cc: Scott Wood <swood@redhat.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Signed-off-by: Daniel Wagner <dwagner@suse.de>
> > ---
> >  lib/smp_processor_id.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
> > index 5f2618d346c4..a914f73e3652 100644
> > --- a/lib/smp_processor_id.c
> > +++ b/lib/smp_processor_id.c
> > @@ -23,8 +23,10 @@ unsigned int check_preemption_disabled(const char
> > *what1, const char *what2)
> >  	 * Kernel threads bound to a single CPU can safely use
> >  	 * smp_processor_id():
> >  	 */
> > +#if defined(CONFIG_SMP) && defined(CONFIG_PREEMPT_RT_BASE)
> >  	if (current->migrate_disable)
> >  		goto out;
> > +#endif
> 
> Won't this give false positives on UP with CONFIG_PREEMPT_RT_BASE and
> CONFIG_DEBUG_PREEMPT? If we have CONFIG_SCHED_DEBUG then we can still check
> migrate_disable.

Ohhh, I see what you mean. I didn't realize that migrate_disable is
also available with CONFIG_SCHED_DEBUG. So the ifdef should be
something like:

#if defined(CONFIG_PREEMP_RT_BASE) && \
    (defined(CONFIG_SMP) || \
      (!defined(CONFIG_SMP) && defined(CONFIG_SCHED_DEBUG)))

?

Hmm, looks a bit ugly but I haven't found a simplification. Long time
since I did logic algebra...

Thanks,
Daniel

Re: [PATCH] lib: Check for migrate_disable only on SMP systems

[Scott Wood]

On Fri, 2019-12-06 at 20:39 +0100, Daniel Wagner wrote:
> Hi Scott,
> 
> On Fri, Dec 06, 2019 at 12:49:12PM -0600, Scott Wood wrote:
> > On Fri, 2019-12-06 at 15:21 +0100, Daniel Wagner wrote:
> > > In case the kernel configuration is UP is, the migrate_disable member
> > > in task_struct is missing.
> > > 
> > > linux/lib/smp_processor_id.c: In function ‘check_preemption_disabled’:
> > > linux/lib/smp_processor_id.c:26:13: error: ‘struct task_struct’ has no
> > > member named ‘migrate_disable’
> > >     26 |  if (current->migrate_disable)
> > >        |             ^~
> > > 
> > > Fixes: 425c5b38779a ("sched: Lazy migrate_disable processing")
> > > Cc: Scott Wood <swood@redhat.com>
> > > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > Signed-off-by: Daniel Wagner <dwagner@suse.de>
> > > ---
> > >  lib/smp_processor_id.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
> > > index 5f2618d346c4..a914f73e3652 100644
> > > --- a/lib/smp_processor_id.c
> > > +++ b/lib/smp_processor_id.c
> > > @@ -23,8 +23,10 @@ unsigned int check_preemption_disabled(const char
> > > *what1, const char *what2)
> > >  	 * Kernel threads bound to a single CPU can safely use
> > >  	 * smp_processor_id():
> > >  	 */
> > > +#if defined(CONFIG_SMP) && defined(CONFIG_PREEMPT_RT_BASE)
> > >  	if (current->migrate_disable)
> > >  		goto out;
> > > +#endif
> > 
> > Won't this give false positives on UP with CONFIG_PREEMPT_RT_BASE and
> > CONFIG_DEBUG_PREEMPT? If we have CONFIG_SCHED_DEBUG then we can still
> > check
> > migrate_disable.
> 
> Ohhh, I see what you mean. I didn't realize that migrate_disable is
> also available with CONFIG_SCHED_DEBUG. So the ifdef should be
> something like:
> 
> #if defined(CONFIG_PREEMP_RT_BASE) && \
>     (defined(CONFIG_SMP) || \
>       (!defined(CONFIG_SMP) && defined(CONFIG_SCHED_DEBUG)))

That would still leave the issue with false positives if you have
CONFIG_DEBUG_PREEMPT but not CONFIG_SCHED_DEBUG (which should be the only
config currently experiencing build problems).

-Scott

Re: [PATCH] lib: Check for migrate_disable only on SMP systems

[Daniel Wagner]

Hi Scott,

On Fri, Dec 06, 2019 at 09:21:36PM -0600, Scott Wood wrote:
> On Fri, 2019-12-06 at 20:39 +0100, Daniel Wagner wrote:
> > On Fri, Dec 06, 2019 at 12:49:12PM -0600, Scott Wood wrote:
> > > On Fri, 2019-12-06 at 15:21 +0100, Daniel Wagner wrote:
> > > Won't this give false positives on UP with CONFIG_PREEMPT_RT_BASE and
> > > CONFIG_DEBUG_PREEMPT? If we have CONFIG_SCHED_DEBUG then we can still
> > > check migrate_disable.
> >
> > Ohhh, I see what you mean. I didn't realize that migrate_disable is
> > also available with CONFIG_SCHED_DEBUG. So the ifdef should be
> > something like:
> >
> > #if defined(CONFIG_PREEMP_RT_BASE) && \
> >     (defined(CONFIG_SMP) || \
> >       (!defined(CONFIG_SMP) && defined(CONFIG_SCHED_DEBUG)))
>
> That would still leave the issue with false positives if you have
> CONFIG_DEBUG_PREEMPT but not CONFIG_SCHED_DEBUG (which should be the only
> config currently experiencing build problems).

I didn't realize that smp_processor_id.c is only build with
CONFIG_DEBUG_PREEMPT. It should be enough to add

	#if defined(CONFIG_SCHED_DEBUG)

Did I get it finally correct? :)

Thanks,
Daniel

Re: [PATCH] lib: Check for migrate_disable only on SMP systems

[Scott Wood]

On Mon, 2019-12-09 at 11:04 +0100, Daniel Wagner wrote:
> Hi Scott,
> 
> On Fri, Dec 06, 2019 at 09:21:36PM -0600, Scott Wood wrote:
> > On Fri, 2019-12-06 at 20:39 +0100, Daniel Wagner wrote:
> > > On Fri, Dec 06, 2019 at 12:49:12PM -0600, Scott Wood wrote:
> > > > On Fri, 2019-12-06 at 15:21 +0100, Daniel Wagner wrote:
> > > > Won't this give false positives on UP with CONFIG_PREEMPT_RT_BASE
> > > > and
> > > > CONFIG_DEBUG_PREEMPT? If we have CONFIG_SCHED_DEBUG then we can
> > > > still
> > > > check migrate_disable.
> > > 
> > > Ohhh, I see what you mean. I didn't realize that migrate_disable is
> > > also available with CONFIG_SCHED_DEBUG. So the ifdef should be
> > > something like:
> > > 
> > > #if defined(CONFIG_PREEMP_RT_BASE) && \
> > >     (defined(CONFIG_SMP) || \
> > >       (!defined(CONFIG_SMP) && defined(CONFIG_SCHED_DEBUG)))
> > 
> > That would still leave the issue with false positives if you have
> > CONFIG_DEBUG_PREEMPT but not CONFIG_SCHED_DEBUG (which should be the
> > only
> > config currently experiencing build problems).

Actually it looks like we're saved from the false positives by the
following check for nr_cpus_allowed == 1, which should always be true on
UP.

If we want this check to be effective on UP we'd need some way to keep
track of whether a thread is forced by the kernel to be bound to one CPU,
rather than whether it just happens to have only one CPU in the mask. 
Barring that, we should just stub out check_preemption_disabled() as a
whole on UP.

> I didn't realize that smp_processor_id.c is only build with
> CONFIG_DEBUG_PREEMPT. It should be enough to add
> 
> 	#if defined(CONFIG_SCHED_DEBUG)
> 
> Did I get it finally correct? :)

No, then we *would* get those false positives on SMP without
CONFIG_SCHED_DEBUG, and it wouldn't build without CONFIG_PREEMPT_RT_BASE.

-Scott

Re: [PATCH RT] lib: Check for migrate_disable only on SMP systems

[Sebastian Andrzej Siewior]

On 2019-12-09 18:40:37 [-0600], Scott Wood wrote:
> Actually it looks like we're saved from the false positives by the
> following check for nr_cpus_allowed == 1, which should always be true on
> UP.

we currently don't.

> If we want this check to be effective on UP we'd need some way to keep
> track of whether a thread is forced by the kernel to be bound to one CPU,
> rather than whether it just happens to have only one CPU in the mask. 
> Barring that, we should just stub out check_preemption_disabled() as a
> whole on UP.
> 
> > I didn't realize that smp_processor_id.c is only build with
> > CONFIG_DEBUG_PREEMPT. It should be enough to add
> > 
> > 	#if defined(CONFIG_SCHED_DEBUG)
> > 
> > Did I get it finally correct? :)
> 
> No, then we *would* get those false positives on SMP without
> CONFIG_SCHED_DEBUG, and it wouldn't build without CONFIG_PREEMPT_RT_BASE.

this should be enough -> 

From: Daniel Wagner <dwagner@suse.de>
Date: Mon, 16 Dec 2019 16:15:57 +0100
Subject: [PATCH] lib/smp_processor_id: Adjust check_preemption_disabled()

The current->migrate_disable counter is not always defined leading to
build failures with DEBUG_PREEMPT && !PREEMPT_RT_BASE.

Restrict the access to ->migrate_disable to same set where
->migrate_disable is modified.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
[bigeasy: adjust condition + description]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 lib/smp_processor_id.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index 5f2618d346c42..c2f5b0f8cacd0 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -23,8 +23,10 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
 	 * Kernel threads bound to a single CPU can safely use
 	 * smp_processor_id():
 	 */
+#if defined(CONFIG_PREEMPT_RT_BASE) && (defined(CONFIG_SMP) || defined(CONFIG_SCHED_DEBUG))
 	if (current->migrate_disable)
 		goto out;
+#endif
 
 	if (current->nr_cpus_allowed == 1)
 		goto out;
-- 
2.24.0

Re: [PATCH RT] lib: Check for migrate_disable only on SMP systems

[Scott Wood]

On Mon, 2019-12-16 at 16:21 +0100, Sebastian Andrzej Siewior wrote:
> On 2019-12-09 18:40:37 [-0600], Scott Wood wrote:
> > Actually it looks like we're saved from the false positives by the
> > following check for nr_cpus_allowed == 1, which should always be true on
> > UP.
> 
> we currently don't.

Currently don't what?

-Scott

Re: [PATCH RT] lib: Check for migrate_disable only on SMP systems

[Sebastian Andrzej Siewior]

On 2019-12-16 11:06:58 [-0600], Scott Wood wrote:
> On Mon, 2019-12-16 at 16:21 +0100, Sebastian Andrzej Siewior wrote:
> > On 2019-12-09 18:40:37 [-0600], Scott Wood wrote:
> > > Actually it looks like we're saved from the false positives by the
> > > following check for nr_cpus_allowed == 1, which should always be true on
> > > UP.
> > 
> > we currently don't.
> 
> Currently don't what?

Update the migrate_disable counter for UP without CONFIG_SCHED_DEBUG.

> -Scott

Sebastian