Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311150 > unrolled thread

Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of console_trylock callers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-17 15:20 +0100
    Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-17 15:30 +0100
      Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-01-18 17:20 +0100
        Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-19 02:20 +0100
          Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-01-19 16:20 +0100
            Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-20 05:00 +0100
              Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-20 13:00 +0100
                Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-01-20 13:40 +0100
              Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-01-20 13:40 +0100
                Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-21 02:30 +0100
                  Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-21 07:00 +0100
                    Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-01-22 10:50 +0100
                      Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-23 05:50 +0100

#1311150 — Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of console_trylock callers

On (01/14/16 13:57), Sergey Senozhatsky wrote:
[..]
> @@ -2138,7 +2130,10 @@ int console_trylock(void)
>  		return 0;
>  	}
>  	console_locked = 1;
> -	console_may_schedule = 0;
> +	console_may_schedule = !(oops_in_progress ||
> +			in_interrupt() ||
> +			irqs_disabled() ||
> +			in_atomic());
>  	return 1;
>  }

should check rcu_preempt_depth() on preempt RCU kernels.
and in_interrupt() can be dropped, in_atomic() does
preempt_count != 0.

===8<===8<===

 console_unlock() allows to cond_resched() if its caller has
set `console_may_schedule' to 1 (this functionality present
since commit 'printk: do cond_resched() between lines while
outputting to consoles').

The rules are:
-- console_lock() always sets `console_may_schedule' to 1
-- console_trylock() always sets `console_may_schedule' to 0

However, console_trylock() callers (among them is printk()) are
not necessarily executing in atomic contexts, and some of them
can cond_resched() in console_unlock(). So console_trylock()
can set `console_may_schedule' to 0 only if cond_resched() is
invalid in the current context, and set it to 1 otherwise.

The patch also drops explicit preempt_disable()/preempt_enable()
calls in vprintk_emit().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
V2: rcu_preempt_depth() on preempt RCU kernels

 kernel/printk/printk.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ae641d7..5752c0e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1777,20 +1777,12 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (!in_sched) {
 		lockdep_off();
 		/*
-		 * Disable preemption to avoid being preempted while holding
-		 * console_sem which would prevent anyone from printing to
-		 * console
-		 */
-		preempt_disable();
-
-		/*
 		 * Try to acquire and then immediately release the console
 		 * semaphore.  The release will print out buffers and wake up
 		 * /dev/kmsg and syslog() users.
 		 */
 		if (console_trylock_for_printk())
 			console_unlock();
-		preempt_enable();
 		lockdep_on();
 	}
 
@@ -2123,7 +2115,10 @@ int console_trylock(void)
 		return 0;
 	}
 	console_locked = 1;
-	console_may_schedule = 0;
+	console_may_schedule = !(oops_in_progress ||
+			irqs_disabled() ||
+			in_atomic(),
+			rcu_preempt_depth());
 	return 1;
 }
 EXPORT_SYMBOL(console_trylock);
-- 
2.7.0

[toc] | [next] | [standalone]

#1311153

On (01/17/16 23:11), Sergey Senozhatsky wrote:
[..]
>  	console_locked = 1;
> -	console_may_schedule = 0;
> +	console_may_schedule = !(oops_in_progress ||
> +			irqs_disabled() ||
> +			in_atomic(),
				^^^^ fat fingers, obviously. ||
> +			rcu_preempt_depth());

sorry.

===8<===8<===

 console_unlock() allows to cond_resched() if its caller has
set `console_may_schedule' to 1 (this functionality present
since commit 'printk: do cond_resched() between lines while
outputting to consoles').

The rules are:
-- console_lock() always sets `console_may_schedule' to 1
-- console_trylock() always sets `console_may_schedule' to 0

However, console_trylock() callers (among them is printk()) are
not necessarily executing in atomic contexts, and some of them
can cond_resched() in console_unlock(). So console_trylock()
can set `console_may_schedule' to 0 only if cond_resched() is
invalid in the current context, and set it to 1 otherwise.

The patch also drops explicit preempt_disable()/preempt_enable()
calls in vprintk_emit().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
V2: rcu_preempt_depth() on preempt RCU kernels

 kernel/printk/printk.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ae641d7..9ac05e0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1777,20 +1777,12 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (!in_sched) {
 		lockdep_off();
 		/*
-		 * Disable preemption to avoid being preempted while holding
-		 * console_sem which would prevent anyone from printing to
-		 * console
-		 */
-		preempt_disable();
-
-		/*
 		 * Try to acquire and then immediately release the console
 		 * semaphore.  The release will print out buffers and wake up
 		 * /dev/kmsg and syslog() users.
 		 */
 		if (console_trylock_for_printk())
 			console_unlock();
-		preempt_enable();
 		lockdep_on();
 	}
 
@@ -2123,7 +2115,10 @@ int console_trylock(void)
 		return 0;
 	}
 	console_locked = 1;
-	console_may_schedule = 0;
+	console_may_schedule = !(oops_in_progress ||
+			irqs_disabled() ||
+			in_atomic() ||
+			rcu_preempt_depth());
 	return 1;
 }
 EXPORT_SYMBOL(console_trylock);
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1311652

On Sun 2016-01-17 23:23:32, Sergey Senozhatsky wrote:
>  console_unlock() allows to cond_resched() if its caller has
> set `console_may_schedule' to 1 (this functionality present
> since commit 'printk: do cond_resched() between lines while
> outputting to consoles').
> 
> The rules are:
> -- console_lock() always sets `console_may_schedule' to 1
> -- console_trylock() always sets `console_may_schedule' to 0
> 
> However, console_trylock() callers (among them is printk()) are
> not necessarily executing in atomic contexts, and some of them
> can cond_resched() in console_unlock(). So console_trylock()
> can set `console_may_schedule' to 0 only if cond_resched() is
> invalid in the current context, and set it to 1 otherwise.
> 
> The patch also drops explicit preempt_disable()/preempt_enable()
> calls in vprintk_emit().

I do not see any explanation why it is safe to remove these
calls in this patch. If they were required only because of the
can_use_console() call, it would make sense to move this change
to the previous patch. The previous patch moved the
can_use_console() to locations protected by lockbuf_lock that
have disabled preemption because of the lock.


> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> V2: rcu_preempt_depth() on preempt RCU kernels
> 
>  kernel/printk/printk.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ae641d7..9ac05e0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1777,20 +1777,12 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	if (!in_sched) {
>  		lockdep_off();
>  		/*
> -		 * Disable preemption to avoid being preempted while holding
> -		 * console_sem which would prevent anyone from printing to
> -		 * console
> -		 */
> -		preempt_disable();
> -
> -		/*
>  		 * Try to acquire and then immediately release the console
>  		 * semaphore.  The release will print out buffers and wake up
>  		 * /dev/kmsg and syslog() users.
>  		 */
>  		if (console_trylock_for_printk())
>  			console_unlock();
> -		preempt_enable();
>  		lockdep_on();
>  	}
>  
> @@ -2123,7 +2115,10 @@ int console_trylock(void)
>  		return 0;
>  	}
>  	console_locked = 1;
> -	console_may_schedule = 0;
> +	console_may_schedule = !(oops_in_progress ||
> +			irqs_disabled() ||
> +			in_atomic() ||
> +			rcu_preempt_depth());

Is it safe to call cond_resched() when the CPU is not online
and preemption is enabled, please? Your previous patch
suggests that this situation might happen. I guess that
it might break the CPU initialization code.

Best Regards,
Petr


>  	return 1;
>  }
>  EXPORT_SYMBOL(console_trylock);
> -- 
> 2.7.0
> 

[toc] | [prev] | [next] | [standalone]

#1311878

Thanks for the review.

On (01/18/16 17:17), Petr Mladek wrote:
> On Sun 2016-01-17 23:23:32, Sergey Senozhatsky wrote:
> >  console_unlock() allows to cond_resched() if its caller has
> > set `console_may_schedule' to 1 (this functionality present
> > since commit 'printk: do cond_resched() between lines while
> > outputting to consoles').
> > 
> > The rules are:
> > -- console_lock() always sets `console_may_schedule' to 1
> > -- console_trylock() always sets `console_may_schedule' to 0
> > 
> > However, console_trylock() callers (among them is printk()) are
> > not necessarily executing in atomic contexts, and some of them
> > can cond_resched() in console_unlock(). So console_trylock()
> > can set `console_may_schedule' to 0 only if cond_resched() is
> > invalid in the current context, and set it to 1 otherwise.
> > 
> > The patch also drops explicit preempt_disable()/preempt_enable()
> > calls in vprintk_emit().
> 
> I do not see any explanation why it is safe to remove these
> calls in this patch. If they were required only because of the
> can_use_console() call

The comment in the code states

	/*
	 * Disable preemption to avoid being preempted while holding
	 * console_sem which would prevent anyone from printing to
	 * console
	 */

which is not really a problem -- we schedule from console_unlock() with
the console_sem being held, it's fine.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/printk/printk.c?id=8d91f8b15361dfb438ab6eb3b319e2ded43458ff

> it would make sense to move this change to the previous patch.
> The previous patch moved the can_use_console() to locations
> protected by lockbuf_lock that have disabled preemption because
> of the lock.

old:
vprintk_emit
  preempt_disable
   can_use_console
    console_unlock
      spin_lock, save IRQ
      spin_unlock
      call_console_drivers
      restore IRQ
  preempt_enable

new:
vprintk_emit
  console_unlock
    spin_lock, save IRQ
    can_use_console
    spin_unlock
    call_console_drivers
    restore IRQ

so preemption is disabled during the transition from can_use_console()
to call_console_drivers().


[..]
> >  	console_locked = 1;
> > -	console_may_schedule = 0;
> > +	console_may_schedule = !(oops_in_progress ||
> > +			irqs_disabled() ||
> > +			in_atomic() ||
> > +			rcu_preempt_depth());
> 
> Is it safe to call cond_resched() when the CPU is not online
> and preemption is enabled, please? Your previous patch
> suggests that this situation might happen. I guess that
> it might break the CPU initialization code.

CPU notifies are preemptible. CPU_UP_PREPARE/etc. can schedule,
there are GFP_KERNEL kmalloc-s from CPU_UP_PREPARE (where cpu
is not yet online), mutex_lock() calls in cpu_notify handlers,
and so on.

	-ss

[toc] | [prev] | [next] | [standalone]

#1312319

On Tue 2016-01-19 10:15:10, Sergey Senozhatsky wrote:
> Thanks for the review.
> 
> On (01/18/16 17:17), Petr Mladek wrote:
> > On Sun 2016-01-17 23:23:32, Sergey Senozhatsky wrote:
> > >  console_unlock() allows to cond_resched() if its caller has
> > > set `console_may_schedule' to 1 (this functionality present
> > > since commit 'printk: do cond_resched() between lines while
> > > outputting to consoles').
> > > 
> > > The rules are:
> > > -- console_lock() always sets `console_may_schedule' to 1
> > > -- console_trylock() always sets `console_may_schedule' to 0
> > > 
> > > However, console_trylock() callers (among them is printk()) are
> > > not necessarily executing in atomic contexts, and some of them
> > > can cond_resched() in console_unlock(). So console_trylock()
> > > can set `console_may_schedule' to 0 only if cond_resched() is
> > > invalid in the current context, and set it to 1 otherwise.
> > > 
> > > The patch also drops explicit preempt_disable()/preempt_enable()
> > > calls in vprintk_emit().
> > 
> > I do not see any explanation why it is safe to remove these
> > calls in this patch. If they were required only because of the
> > can_use_console() call
> 
> The comment in the code states
> 
> 	/*
> 	 * Disable preemption to avoid being preempted while holding
> 	 * console_sem which would prevent anyone from printing to
> 	 * console
> 	 */

Thanks for the pointer. I missed this in the patch and it was not
obvious from the commit message.

> which is not really a problem -- we schedule from console_unlock() with
> the console_sem being held, it's fine.
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/printk/printk.c?id=8d91f8b15361dfb438ab6eb3b319e2ded43458ff

Yup, I agree that this commit is contradicting the above comment.
The question is if the comment describes the only reason for
disabled preemption.

> [..]
> > >  	console_locked = 1;
> > > -	console_may_schedule = 0;
> > > +	console_may_schedule = !(oops_in_progress ||
> > > +			irqs_disabled() ||

IMHO, we should also check if we are in irq context or not.

> > > +			in_atomic() ||

Hmm, it seems that in_atomic() is not safe enough. There is the
following comment:

/*
 * Are we running in atomic context?  WARNING: this macro cannot
 * always detect atomic context; in particular, it cannot know about
 * held spinlocks in non-preemptible kernels.  Thus it should not be
 * used in the general case to determine whether sleeping is possible.
 * Do not use in_atomic() in driver code.
 */
#define in_atomic()	(preempt_count() != 0)

It might mean that there is no safe way to detect a safe context
on non-preemptible kernels. This might be the most important reason
why we disable preemption when calling console in vprint_emit().


> > > +			rcu_preempt_depth());
> > 
> > Is it safe to call cond_resched() when the CPU is not online
> > and preemption is enabled, please? Your previous patch
> > suggests that this situation might happen. I guess that
> > it might break the CPU initialization code.
> 
> CPU notifies are preemptible. CPU_UP_PREPARE/etc. can schedule,
> there are GFP_KERNEL kmalloc-s from CPU_UP_PREPARE (where cpu
> is not yet online), mutex_lock() calls in cpu_notify handlers,
> and so on.

Hmm, the notifiers are called via __raw_notifier_call_chain().
There is a comment above this function:

 *	Calls each function in a notifier chain in turn.  The functions
 *	run in an undefined context.
 *	All locking must be provided by the caller.

But hmm, you are right that the notifiers do malloc, take mutextes,
etc. The question is if schedule does something in this case. I would
expect that the is no running task assigned to this CPU at this stage.
So, cond_resched is probably a noop in this case.

I am always surprised that printk-world is so tricky.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1312858

Hello,

On (01/19/16 16:18), Petr Mladek wrote:
[..]
> > > >  	console_locked = 1;
> > > > -	console_may_schedule = 0;
> > > > +	console_may_schedule = !(oops_in_progress ||
> > > > +			irqs_disabled() ||
> 
> IMHO, we should also check if we are in irq context or not.
> 
> > > > +			in_atomic() ||
> 
> Hmm, it seems that in_atomic() is not safe enough. There is the
> following comment:
>
> /*
>  * Are we running in atomic context?  WARNING: this macro cannot
>  * always detect atomic context; in particular, it cannot know about
>  * held spinlocks in non-preemptible kernels.  Thus it should not be
>  * used in the general case to determine whether sleeping is possible.
>  * Do not use in_atomic() in driver code.
>  */
> #define in_atomic()	(preempt_count() != 0)
> 

I had in_interrupt() check in trylock, but dropped it. As of 4.4 in_interrupt()
does the following

#define hardirq_count()	(preempt_count() & HARDIRQ_MASK)
#define softirq_count()	(preempt_count() & SOFTIRQ_MASK)
#define irq_count()	(preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \
				 | NMI_MASK))

#define in_interrupt()		(irq_count())

while in_atomic() does

#define in_atomic()	(preempt_count() != 0)

so in_atomic() covers both preemption disabled and in_interrupt().


I take your `non-preemptible kernel' point, thanks.


> It might mean that there is no safe way to detect a safe context
> on non-preemptible kernels. This might be the most important reason
> why we disable preemption when calling console in vprint_emit().

well, if we run a !PREEMPT_COUNT kernel, then
#define preempt_disable()	barrier()
#define preempt_enable()	barrier()
#define preemptible()		0

so I don't think that preempt_disable()/preempt_enable() were for
non-preempt kernels there.

otoh, preempt_count still counts irqs, because in_interrupt() (and friends)
macro is supposed to work regardless the preempt config selection, so we
just lose preempt_disable bit from preempt_count... hm... I think I'll
just introduce preemptible() check there.

for preempt kernels, preemptible() does
#define preemptible()	(preempt_count() == 0 && !irqs_disabled())

and for !preempt kernels
#define preemptible()	0

iow, no cond_resched() in console_unlock() called from vprintk_emit()
on non-preempt kernels.

so the console_may_schedule now turns into (composed in mail-app, not
actually tested):


console_may_schedule = !oops_in_progress && preemptible() &&
			!rcu_preempt_depth();

	/*
	 * or a slightly less readable version
	 * !(oops_in_progress || !preemptible() || rcu_preempt_depth())
	 */


preemptible() implies "preempt_enabled && !in_interrupt() && !irqs_disabled()";
or is always 0 (so it can be optimized out).


will test it, but looks legit to me.


> > > > +			rcu_preempt_depth());
> > > 
> > > Is it safe to call cond_resched() when the CPU is not online
> > > and preemption is enabled, please? Your previous patch
> > > suggests that this situation might happen. I guess that
> > > it might break the CPU initialization code.
> > 
> > CPU notifies are preemptible. CPU_UP_PREPARE/etc. can schedule,
> > there are GFP_KERNEL kmalloc-s from CPU_UP_PREPARE (where cpu
> > is not yet online), mutex_lock() calls in cpu_notify handlers,
> > and so on.
> 
> Hmm, the notifiers are called via __raw_notifier_call_chain().
> There is a comment above this function:
> 
>  *	Calls each function in a notifier chain in turn.  The functions
>  *	run in an undefined context.
>  *	All locking must be provided by the caller.
> 
> But hmm, you are right that the notifiers do malloc, take mutextes,
> etc. The question is if schedule does something in this case. I would
> expect that the is no running task assigned to this CPU at this stage.
> So, cond_resched is probably a noop in this case.

CPU's run queue is not marked online yet at this point (it becomes online
in response to CPU_ONLINE notification). so there is not too many tasks to
schedule on that CPU -- swapper/X perhaps.. migration/X?.

> I am always surprised that printk-world is so tricky.

	-ss

[toc] | [prev] | [next] | [standalone]

#1313155

On (01/20/16 12:50), Sergey Senozhatsky wrote:
[..]
> > Hmm, the notifiers are called via __raw_notifier_call_chain().
> > There is a comment above this function:
> > 
> >  *	Calls each function in a notifier chain in turn.  The functions
> >  *	run in an undefined context.
> >  *	All locking must be provided by the caller.
> > 
> > But hmm, you are right that the notifiers do malloc, take mutextes,
> > etc. The question is if schedule does something in this case. I would
> > expect that the is no running task assigned to this CPU at this stage.
> > So, cond_resched is probably a noop in this case.

I did some modifications to notifier, just to check how real that theoretical
race condition can possible be.

And here are notifications sent by *offline* cpu to itself (to offline cpu).

[   73.861462] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:sched_cpu_active+0x0/0x55
[   73.861466] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:sched_domains_numa_masks_update+0x0/0xd
[   73.861469] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cpuset_cpu_active+0x0/0x42
[   73.861473] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:vgetcpu_cpu_notifier+0x0/0x30
[   73.861476] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:uncore_cpu_notifier+0x0/0x20f
[   73.861480] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:perf_cpu_notify+0x0/0x36
[   73.861483] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:x86_pmu_notifier+0x0/0xb0
[   73.861490] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cstate_cpu_notifier+0x0/0x1c1
[   73.861493] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:migration_call+0x0/0x328
[   73.861496] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:smpboot_thread_call+0x0/0x28
[   73.861500] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:workqueue_cpu_up_callback+0x0/0x426
[   73.861503] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:page_alloc_cpu_notify+0x0/0x41
[   73.861506] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:slab_cpuup_callback+0x0/0xed
[   73.861508] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:sched_ilb_notifier+0x0/0x78
[   73.861511] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:rcu_cpu_notify+0x0/0x3f0
[   73.861514] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:rb_cpu_notify+0x0/0xde
[   73.861516] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:rb_cpu_notify+0x0/0xde
[   73.861520] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:radix_tree_callback+0x0/0x50
[   73.861522] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:timer_cpu_notify+0x0/0x136
[   73.861525] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:hrtimer_cpu_notify+0x0/0x290
[   73.861528] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:hotplug_cfd+0x0/0x80
[   73.861531] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:buffer_cpu_notify+0x0/0x85
[   73.861533] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:ratelimit_handler+0x0/0x27
[   73.861536] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cpu_callback+0x0/0x150
[   73.861538] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:relay_hotcpu_callback+0x0/0xc7
[   73.861540] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:hotplug_hrtick+0x0/0x76
[   73.861543] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:memcg_cpu_hotplug_callback+0x0/0x3d
[   73.861546] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:blk_cpu_notify+0x0/0x8d
[   73.861549] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:blk_mq_main_cpu_notify+0x0/0x67
[   73.861552] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:blk_mq_queue_reinit_notify+0x0/0x1b3
[   73.861556] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:dev_cpu_callback+0x0/0x195
[   73.861559] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cpu_callback+0x0/0x4e
[   73.861562] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:vmstat_cpuup_callback+0x0/0x83
[   73.861565] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:percpu_counter_hotcpu_callback+0x0/0x8b
[   73.861567] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cpu_hotplug_notify+0x0/0x65
[   73.861571] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:topology_cpu_callback+0x0/0x6e
[   73.861574] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cacheinfo_cpu_callback+0x0/0x76
[   73.861577] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:mce_cpu_callback+0x0/0xee
[   73.861580] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:mc_cpu_callback+0x0/0xad
[   73.861582] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:console_cpu_notify+0x0/0x35
[   73.861587] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:acpi_cpu_soft_notify+0x0/0xe0 [processor]
[   73.861590] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:boost_notify+0x0/0x5f [acpi_cpufreq]
[   73.861593] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cpufreq_cpu_callback+0x0/0x46
[   73.861597] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:coretemp_cpu_callback+0x0/0x1bd [coretemp]
[   73.861599] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:workqueue_cpu_down_callback+0x0/0xde
[   73.861602] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:hpet_cpuhp_notify+0x0/0x162
[   73.861604] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:sched_cpu_inactive+0x0/0x27
[   73.861606] SELF NOTIFY [by swapper/2] from cpu:2 to:2 (dec:2) action name:CPU_STARTING [code:10] callback:cpuset_cpu_inactive+0x0/0x231


so on my quite small x86_64 kernel (I'm trying to keep unneeded stuff out of the image), that's a bit
bigger than I'd expected.




so I have the new patch set. and it seems to be working on my simple tests.

but... hm... a bit concerned about CPU_DOWN path.

DOWN notification sequence is
CPU_DOWN_PREPARE -> __cpu_disable() -> CPU_DYING -> CPU_DEAD -> CPU_POST_DEAD

__cpu_disable() is getting called before cpu_notify(CPU_DYING), and
__cpu_disable() sets set_cpu_online(cpu, false).

kernel/sched/core.c   migration_call()

	case CPU_DYING:
		sched_ttwu_pending();
		/* Update our root-domain */
		raw_spin_lock_irqsave(&rq->lock, flags);
		if (rq->rd) {
			BUG_ON(!cpumask_test_cpu(cpu, rq->rd->span));
			set_rq_offline(rq);
		}
		migrate_tasks(rq);
		BUG_ON(rq->nr_running != 1); /* the migration thread */
		raw_spin_unlock_irqrestore(&rq->lock, flags);
		break;

so it's only when CPU is already !cpu_online scheduler does set_rq_offline() and
migrate_tasks(rq).

the good news is that CPU_DOWN_PREPARE disables CPU's rq ->hrtick_timer,
so no rq ticks. but what happens on !CONFIG_SCHED_HRTICK systems?... scheduler_tick
is getting disabled because we clear APIC and thus stop timer interrupts? I have a
little knowledge here, sorry. so I'll better ask. Is it possible to get !online cpu
and still iterating in this console_unlock() loop? I suspect that the answer is no,
but better be sure.

	-ss

[toc] | [prev] | [next] | [standalone]

#1313188

On Wed 2016-01-20 20:51:43, Sergey Senozhatsky wrote:
> On (01/20/16 12:50), Sergey Senozhatsky wrote:
> [..]
> > > Hmm, the notifiers are called via __raw_notifier_call_chain().
> > > There is a comment above this function:
> > > 
> > >  *	Calls each function in a notifier chain in turn.  The functions
> > >  *	run in an undefined context.
> > >  *	All locking must be provided by the caller.
> > > 
> > > But hmm, you are right that the notifiers do malloc, take mutextes,
> > > etc. The question is if schedule does something in this case. I would
> > > expect that the is no running task assigned to this CPU at this stage.
> > > So, cond_resched is probably a noop in this case.
> 
> I did some modifications to notifier, just to check how real that theoretical
> race condition can possible be.

After all I think that this was a false alarm. As you said, there are
used locks in the notifiers. I do not see any special handling of
preemption. If the locks do not care about this special CPU state
and rescheduling, printk should not need to take care of it as well.

In each case, thanks a lot for the analyze. These are all interesting
things and any piece of information helps better understanding.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1313180

On Wed 2016-01-20 12:50:56, Sergey Senozhatsky wrote:
> so in_atomic() covers both preemption disabled and in_interrupt().

I see.

> well, if we run a !PREEMPT_COUNT kernel, then
> #define preempt_disable()	barrier()
> #define preempt_enable()	barrier()
> #define preemptible()		0
> 
> so I don't think that preempt_disable()/preempt_enable() were for
> non-preempt kernels there.
> 
> otoh, preempt_count still counts irqs, because in_interrupt() (and friends)
> macro is supposed to work regardless the preempt config selection, so we
> just lose preempt_disable bit from preempt_count... hm... I think I'll
> just introduce preemptible() check there.
> 
> for preempt kernels, preemptible() does
> #define preemptible()	(preempt_count() == 0 && !irqs_disabled())
> 
> and for !preempt kernels
> #define preemptible()	0
> 
> iow, no cond_resched() in console_unlock() called from vprintk_emit()
> on non-preempt kernels.
> 
> so the console_may_schedule now turns into (composed in mail-app, not
> actually tested):
> 
> 
> console_may_schedule = !oops_in_progress && preemptible() &&
> 			!rcu_preempt_depth();

I though about it from some other side and I wonder if we need all
the console_may_schedule stuff at all.

printk_emit() has the following code:

	/* This stops the holder of console_sem just where we want him */
	local_irq_save(flags);
[...]
	lockdep_off();
	raw_spin_lock(&logbuf_lock);
	logbuf_cpu = this_cpu;

[...]

	logbuf_cpu = UINT_MAX;
	raw_spin_unlock(&logbuf_lock);
	lockdep_on();
	local_irq_restore(flags);

	/* If called from the scheduler, we can not call up(). */
	if (!in_sched) {
		lockdep_off();
		/*
		 * Disable preemption to avoid being preempted while holding
		 * console_sem which would prevent anyone from printing to
		 * console
		 */
		preempt_disable();

		/*
		 * Try to acquire and then immediately release the console
		 * semaphore.  The release will print out buffers and wake up
		 * /dev/kmsg and syslog() users.
		 */
		if (console_trylock_for_printk())
			console_unlock();
		preempt_enable();


First, the message "This stops the holder of console_sem just where we
want him" is suspitious. It is there sice the initial git commit on
2005-04-16. I do not understand how this could block the console
holder on another CPU. I think that it rather allows to make the
recursion detection without the lockbuf lock. But this is not
that important.

More interesting is the counter part:

	raw_spin_unlock(&logbuf_lock);
	lockdep_on();
	local_irq_restore(flags);

raw_spin_unlock() calls preempt_enable() that calls
preempt_schedule(). It does nothing here because IRQs
are still disabled.

But if we do not need the lockdep_on() hack, we could use
raw_spin_unlock_irqrestore(&lockbuf_lock, flags). It means
that we do not call cond_resched here "only by chance".

In each case, preemtible kernel seems to be free to reschedule
after we enable the inrerrupts. By other words, preemptible kernel
seems to be able to reschedule in printk() already these days.
Why it might work?

I think that it might work because cond_resched() or rather
preempt_schedule() are clever enough. They both check
preempt_count and do nothing if preemtion is disabled.

As a result, I think that we do not need the extra checks
for the save context in printk(). IMHO, it is safe to remove
all the console_may_schedule stuff and also remove the extra
preempt_disable/preempt_enable() in vprintk_emit().

Or did I miss anything?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1313802

Hello,

On (01/20/16 13:31), Petr Mladek wrote:
[..]
> > console_may_schedule = !oops_in_progress && preemptible() &&
> > 			!rcu_preempt_depth();
> 
> I though about it from some other side and I wonder if we need all
> the console_may_schedule stuff at all.

interesting, thanks.

> printk_emit() has the following code:
> 
> 	/* This stops the holder of console_sem just where we want him */
> 	local_irq_save(flags);
> [...]
> 	lockdep_off();
> 	raw_spin_lock(&logbuf_lock);
> 	logbuf_cpu = this_cpu;
> 
> [...]
> 
> 	logbuf_cpu = UINT_MAX;
> 	raw_spin_unlock(&logbuf_lock);
> 	lockdep_on();
> 	local_irq_restore(flags);
> 
> 	/* If called from the scheduler, we can not call up(). */
> 	if (!in_sched) {
> 		lockdep_off();
> 		/*
> 		 * Disable preemption to avoid being preempted while holding
> 		 * console_sem which would prevent anyone from printing to
> 		 * console
> 		 */
> 		preempt_disable();
> 
> 		/*
> 		 * Try to acquire and then immediately release the console
> 		 * semaphore.  The release will print out buffers and wake up
> 		 * /dev/kmsg and syslog() users.
> 		 */
> 		if (console_trylock_for_printk())
> 			console_unlock();
> 		preempt_enable();
> 
> 
> First, the message "This stops the holder of console_sem just where we
> want him" is suspitious.

this comment is irrelevant, as of today. it was, a long time ago, because
the entire thing was a bit different (linux-2.4.21 kernel/printk.c)

        /* This stops the holder of console_sem just where we want him */
        spin_lock_irqsave(&logbuf_lock, flags);

logbuf_lock does stop the holder, local_irq_save() does not, you are right.


> It is there sice the initial git commit on 2005-04-16. I do not
> understand how this could block the console holder on another CPU.
> I think that it rather allows to make the recursion detection without
> the lockbuf lock. But this is not
> that important.
> 
> More interesting is the counter part:
> 
> 	raw_spin_unlock(&logbuf_lock);
> 	lockdep_on();
> 	local_irq_restore(flags);
> 
> raw_spin_unlock() calls preempt_enable() that calls
> preempt_schedule(). It does nothing here because IRQs
> are still disabled.
> 
> But if we do not need the lockdep_on() hack, we could use
> raw_spin_unlock_irqrestore(&lockbuf_lock, flags). It means
> that we do not call cond_resched here "only by chance".
> 
> In each case, preemtible kernel seems to be free to reschedule
> after we enable the inrerrupts. By other words, preemptible kernel
> seems to be able to reschedule in printk() already these days.
> Why it might work?
> 
> I think that it might work because cond_resched() or rather
> preempt_schedule() are clever enough. They both check
> preempt_count and do nothing if preemtion is disabled.

is cond_resched() clever enough for vprintk_emit()->onsole_unlock()?
I think it can schedule from rcu read critical section, for example.

rcu_read_lock
  printk
    vprintk_emit
      console_unlock
        cond_resched     << will schedule
rcu_read_unlock

because _cond_resched()->should_resched(0) test
	unlikely(preempt_count() == preempt_offset && tif_need_resched());

it does care about preempt_disable (if the kernel is CONFIG_PREEMPT_COUNT,
though) and IRQ count, but no rcu preempt count check. and, I think, we also
don't want to schedule when we are in oops_in_progress.
so we can (probably...) get rid of console_may_schedule, but I don't think
we can unconditionally cond_resched() from console_unlock().

> As a result, I think that we do not need the extra checks
> for the save context in printk(). IMHO, it is safe to remove
> all the console_may_schedule stuff and also remove the extra
> preempt_disable/preempt_enable() in vprintk_emit().
> 
> Or did I miss anything?

hm... I suspect the reason we have console_may_schedule is
console_conditional_schedule() - console_sem owner may want
to have an internal logic to re-schedule [fwiw], while still
holding the console_sem. tty/vt/vt.c or video/console/fbcon.c
for example. (in 2.4 kernel: video/fbcon.c and char/console.c).

cond_resched() helps in console_unlock(); console_conditional_schedule()
is called after console_lock() and _before_ console_unlock()....

static void fbcon_redraw_move(struct vc_data *vc, struct display *p,
			      int line, int count, int dy)
{
	unsigned short *s = (unsigned short *)
		(vc->vc_origin + vc->vc_size_row * line);

	while (count--) {
		unsigned short *start = s;
		unsigned short *le = advance_row(s, 1);
		unsigned short c;
		int x = 0;
		unsigned short attr = 1;

		do {
			c = scr_readw(s);
			if (attr != (c & 0xff00)) {
				attr = c & 0xff00;
				if (s > start) {
					fbcon_putcs(vc, start, s - start,
						    dy, x);
					x += s - start;
					start = s;
				}
			}
			console_conditional_schedule();
			s++;
		} while (s < le);
		if (s > start)
			fbcon_putcs(vc, start, s - start, dy, x);
		console_conditional_schedule();
		dy++;
	}
}

dunno... for non-preempt kernels, perhaps?


	-ss

> Best Regards,
> Petr
> 

[toc] | [prev] | [next] | [standalone]

#1313935

On (01/21/16 10:25), Sergey Senozhatsky wrote:
[..]
> > First, the message "This stops the holder of console_sem just where we
> > want him" is suspitious.
> 
> this comment is irrelevant, as of today. it was, a long time ago, because
> the entire thing was a bit different (linux-2.4.21 kernel/printk.c)
> 
>         /* This stops the holder of console_sem just where we want him */
>         spin_lock_irqsave(&logbuf_lock, flags);
> 
> logbuf_lock does stop the holder, local_irq_save() does not, you are right.

I meant 'irrelevant on its current place'.

[..]
> > As a result, I think that we do not need the extra checks
> > for the save context in printk(). IMHO, it is safe to remove
> > all the console_may_schedule stuff and also remove the extra
> > preempt_disable/preempt_enable() in vprintk_emit().
> > 
> > Or did I miss anything?
> 
> hm... I suspect the reason we have console_may_schedule is
> console_conditional_schedule() - console_sem owner may want
> to have an internal logic to re-schedule [fwiw], while still
> holding the console_sem. tty/vt/vt.c or video/console/fbcon.c
> for example. (in 2.4 kernel: video/fbcon.c and char/console.c).
> 
> cond_resched() helps in console_unlock(); console_conditional_schedule()
> is called after console_lock() and _before_ console_unlock()....

for CONFIG_PREEMPT_COUNT kernel we can do something like

+void __sched console_conditional_schedule(void)
+{
+       if (!oops_in_progress && preemptible() && !rcu_preempt_depth())
+               cond_resched();
+}

and in console_unlock()

-               if (do_cond_resched)
-                       cond_resched();
+               console_conditional_schedule();



but for !CONFIG_PREEMPT_COUNT we can't. because of currently held spin_locks/etc
that we don't know about.

`console_may_schedule' carries a bit of important information for
console_conditional_schedule() caller. if it has acquired console_sem
via console_lock() - then it can schedule, if via console_trylock() - it cannot.

the last `if via console_trylock() - it cannot' rule is not always true,
we clearly can have printk()->console_unlock() from non-atomic contexts
(if we know that its non-atomic, which is not the case with !PREEMPT_COUNT).

	-ss

[toc] | [prev] | [next] | [standalone]

#1314853

On Thu 2016-01-21 14:51:46, Sergey Senozhatsky wrote:
> On (01/21/16 10:25), Sergey Senozhatsky wrote:
> [..]
> > > First, the message "This stops the holder of console_sem just where we
> > > want him" is suspitious.
> > 
> > this comment is irrelevant, as of today. it was, a long time ago, because
> > the entire thing was a bit different (linux-2.4.21 kernel/printk.c)
> > 
> >         /* This stops the holder of console_sem just where we want him */
> >         spin_lock_irqsave(&logbuf_lock, flags);
> > 
> > logbuf_lock does stop the holder, local_irq_save() does not, you are right.
> 
> I meant 'irrelevant on its current place'.

Thanks a lot for confirmation.

> [..]
> > > As a result, I think that we do not need the extra checks
> > > for the save context in printk(). IMHO, it is safe to remove
> > > all the console_may_schedule stuff and also remove the extra
> > > preempt_disable/preempt_enable() in vprintk_emit().
> > > 
> > > Or did I miss anything?
> > 
> > hm... I suspect the reason we have console_may_schedule is
> > console_conditional_schedule() - console_sem owner may want
> > to have an internal logic to re-schedule [fwiw], while still
> > holding the console_sem. tty/vt/vt.c or video/console/fbcon.c
> > for example. (in 2.4 kernel: video/fbcon.c and char/console.c).
> > 
> > cond_resched() helps in console_unlock(); console_conditional_schedule()
> > is called after console_lock() and _before_ console_unlock()....
> 
> for CONFIG_PREEMPT_COUNT kernel we can do something like
> 
> +void __sched console_conditional_schedule(void)
> +{
> +       if (!oops_in_progress && preemptible() && !rcu_preempt_depth())
> +               cond_resched();
> +}
> 
> and in console_unlock()
> 
> -               if (do_cond_resched)
> -                       cond_resched();
> +               console_conditional_schedule();
>
>
> but for !CONFIG_PREEMPT_COUNT we can't. because of currently held spin_locks/etc
> that we don't know about.

Ah, I was not aware that we did not have information about preemption
without PREEMPT_COUNT.


> `console_may_schedule' carries a bit of important information for
> console_conditional_schedule() caller. if it has acquired console_sem
> via console_lock() - then it can schedule, if via console_trylock() - it cannot.
> 
> the last `if via console_trylock() - it cannot' rule is not always true,
> we clearly can have printk()->console_unlock() from non-atomic contexts
> (if we know that its non-atomic, which is not the case with !PREEMPT_COUNT).

By other words, we could automatically detect save context for
cond_resched() only if PREEMPT_COUNT is enabled. Otherwise, we need to
keep the current logic (heuristic). Do I get it correctly, please?

I would personally wait a bit for Jack's async console printing.
It will call console only if oops_in_progress is set. It means that
this partial optimization won't be needed at all.

The other (first) patch still makes sense in the simplified form.


Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1315517

Hello,

On (01/22/16 10:48), Petr Mladek wrote:
[..]
> > and in console_unlock()
> > 
> > -               if (do_cond_resched)
> > -                       cond_resched();
> > +               console_conditional_schedule();
> >
> >
> > but for !CONFIG_PREEMPT_COUNT we can't. because of currently held spin_locks/etc
> > that we don't know about.
> 
> Ah, I was not aware that we did not have information about preemption
> without PREEMPT_COUNT.

yes, for example,

static inline void __raw_spin_lock(raw_spinlock_t *lock)
{
	preempt_disable();
	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
}

where preempt_disable()  include/linux/preempt.h

...
#else /* !CONFIG_PREEMPT_COUNT */

/*
 * Even if we don't have any preemption, we need preempt disable/enable
 * to be barriers, so that we don't have things like get_user/put_user
 * that can cause faults and scheduling migrate into our preempt-protected
 * region.
 */
#define preempt_disable()			barrier()
#define preempt_enable()			barrier()


so on !CONFIG_PREEMPT_COUNT kernels we can't rely on console_trylock()
'magic', we need the existing rules.

> > `console_may_schedule' carries a bit of important information for
> > console_conditional_schedule() caller. if it has acquired console_sem
> > via console_lock() - then it can schedule, if via console_trylock() - it cannot.
> > 
> > the last `if via console_trylock() - it cannot' rule is not always true,
> > we clearly can have printk()->console_unlock() from non-atomic contexts
> > (if we know that its non-atomic, which is not the case with !PREEMPT_COUNT).
> 
> By other words, we could automatically detect save context for
> cond_resched() only if PREEMPT_COUNT is enabled. Otherwise, we need to
> keep the current logic (heuristic). Do I get it correctly, please?

yes, I think so.

> I would personally wait a bit for Jack's async console printing.
> It will call console only if oops_in_progress is set. It means that
> this partial optimization won't be needed at all.

ok, thanks. I'd love to see Jan's printk() rework being merged. I have 99 problems
with printk() and console_unlock(). People usually are not aware of the secrets that
printk-console_unlock have; and tend to think that printk is just 'a kernel way' of
spelling printf, with all the consequences that follows -- excessive printk usage,
RCU stalls, soft lockups, etc. And that printk abuse does not necessarily hit the
abuser. A completely 'innocent' user space application that does a syscall which
involves console_lock-console_unlock, can spend seconds in console_unlock pushing
someone's data to console_drivers. console_lock and console_unlock, I think, have a
bit misleading naming. _lock has acquire semantics, _unlock, however, does not
simply release the lock. I even think it'd be good to have console_unlock_fast(),
that would just up_console_sem() w/o any penalty. So some of console_unlock() that
are 'accessible' by user-space /* for example,
  tty_open()
    tty_lookup_driver()
      console_device()
        console_lock()
        console_unlock()

or reading from /proc/consoles, and so on and forth */
could be replaced with console_unlock_fast().

The patch in question is simply a further extension on Tejun's work. And
these two patches already made my life a bit simpler, albeit not all of the
printk/console_unlock problems were addressed.

Jan's patch set is a much more complicated effort, and it may take 2 or
3 (??) kernel releases to finish (there are corner cases: for example,
workers can stall during OOM, etc.), I'd be happy to see it in -next for 4.6,
personally, not sure how realistic this expectation is.

> The other (first) patch still makes sense in the simplified form.

thanks. let's do it this way - I'll keep the preempt disable/enable
removal patch the last in the series, so we can easily drop it (if
Jan's rework is much-much closer). How does that sound?

	-ss

> Best Regards,
> Petr
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web