Linux-rt-users archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] lib: Check for migrate_disable only on SMP systems
@ 2019-12-06 14:21 Daniel Wagner
  2019-12-06 18:49 ` Scott Wood
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Wagner @ 2019-12-06 14:21 UTC (permalink / raw)
  To: linux-rt-users; +Cc: Daniel Wagner, Scott Wood, Sebastian Andrzej Siewior

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib: Check for migrate_disable only on SMP systems
  2019-12-06 14:21 [PATCH] lib: Check for migrate_disable only on SMP systems Daniel Wagner
@ 2019-12-06 18:49 ` Scott Wood
  2019-12-06 19:39   ` Daniel Wagner
  0 siblings, 1 reply; 9+ messages in thread
From: Scott Wood @ 2019-12-06 18:49 UTC (permalink / raw)
  To: Daniel Wagner, linux-rt-users; +Cc: Sebastian Andrzej Siewior

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib: Check for migrate_disable only on SMP systems
  2019-12-06 18:49 ` Scott Wood
@ 2019-12-06 19:39   ` Daniel Wagner
  2019-12-07  3:21     ` Scott Wood
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Wagner @ 2019-12-06 19:39 UTC (permalink / raw)
  To: Scott Wood; +Cc: linux-rt-users, Sebastian Andrzej Siewior

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib: Check for migrate_disable only on SMP systems
  2019-12-06 19:39   ` Daniel Wagner
@ 2019-12-07  3:21     ` Scott Wood
  2019-12-09 10:04       ` Daniel Wagner
  0 siblings, 1 reply; 9+ messages in thread
From: Scott Wood @ 2019-12-07  3:21 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: linux-rt-users, Sebastian Andrzej Siewior

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib: Check for migrate_disable only on SMP systems
  2019-12-07  3:21     ` Scott Wood
@ 2019-12-09 10:04       ` Daniel Wagner
  2019-12-10  0:40         ` Scott Wood
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Wagner @ 2019-12-09 10:04 UTC (permalink / raw)
  To: Scott Wood; +Cc: linux-rt-users, Sebastian Andrzej Siewior

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib: Check for migrate_disable only on SMP systems
  2019-12-09 10:04       ` Daniel Wagner
@ 2019-12-10  0:40         ` Scott Wood
  2019-12-16 15:21           ` [PATCH RT] " Sebastian Andrzej Siewior
  0 siblings, 1 reply; 9+ messages in thread
From: Scott Wood @ 2019-12-10  0:40 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: linux-rt-users, Sebastian Andrzej Siewior

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RT] lib: Check for migrate_disable only on SMP systems
  2019-12-10  0:40         ` Scott Wood
@ 2019-12-16 15:21           ` " Sebastian Andrzej Siewior
  2019-12-16 17:06             ` Scott Wood
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-12-16 15:21 UTC (permalink / raw)
  To: Scott Wood; +Cc: Daniel Wagner, linux-rt-users

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RT] lib: Check for migrate_disable only on SMP systems
  2019-12-16 15:21           ` [PATCH RT] " Sebastian Andrzej Siewior
@ 2019-12-16 17:06             ` Scott Wood
  2019-12-16 17:09               ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 9+ messages in thread
From: Scott Wood @ 2019-12-16 17:06 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: Daniel Wagner, linux-rt-users

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RT] lib: Check for migrate_disable only on SMP systems
  2019-12-16 17:06             ` Scott Wood
@ 2019-12-16 17:09               ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-12-16 17:09 UTC (permalink / raw)
  To: Scott Wood; +Cc: Daniel Wagner, linux-rt-users

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 14:21 [PATCH] lib: Check for migrate_disable only on SMP systems Daniel Wagner
2019-12-06 18:49 ` Scott Wood
2019-12-06 19:39   ` Daniel Wagner
2019-12-07  3:21     ` Scott Wood
2019-12-09 10:04       ` Daniel Wagner
2019-12-10  0:40         ` Scott Wood
2019-12-16 15:21           ` [PATCH RT] " Sebastian Andrzej Siewior
2019-12-16 17:06             ` Scott Wood
2019-12-16 17:09               ` Sebastian Andrzej Siewior

Linux-rt-users archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rt-users/0 linux-rt-users/git/0.git

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rt-users


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