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

Groups > linux.kernel > #1332113 > unrolled thread

Re: [RFC][PATCH v3 4/4] 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 v3 4/4] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-02-11 15:50 +0100
    Re: [RFC][PATCH v3 4/4] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-02-11 16:10 +0100
      Re: [RFC][PATCH v3 4/4] printk: set may_schedule for some of  console_trylock callers Petr Mladek <pmladek@suse.com> - 2016-02-11 17:20 +0100
        Re: [RFC][PATCH v3 4/4] printk: set may_schedule for some of  console_trylock callers Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-02-12 06:20 +0100

#1332113 — Re: [RFC][PATCH v3 4/4] printk: set may_schedule for some of console_trylock callers

On Sat 2016-01-23 17:15:13, Sergey Senozhatsky wrote:
> console_unlock() allows to cond_resched() if its caller has
> set `console_may_schedule' to 1, since
> 'commit 8d91f8b15361 ("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()) do
> not always call printk() from atomic contexts, and some of them
> can cond_resched() in console_unlock(), so console_trylock()
> can set `console_may_schedule' to 1 for such processes.
> 
> For !CONFIG_PREEMPT_COUNT kernels, however, console_trylock()
> always sets `console_may_schedule' to 0.
> 
> It's possible to drop explicit preempt_disable()/preempt_enable()
> in vprintk_emit(), because console_unlock() and console_trylock()
> are now smart enough:
> a) console_unlock() does not cond_resched() when it's unsafe
>   (console_trylock() takes care of that)
> b) console_unlock() does can_use_console() check.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  kernel/printk/printk.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 99925ce..097ca8b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1769,20 +1769,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())
>  			console_unlock();
> -		preempt_enable();
>  		lockdep_on();
>  	}
>  
> @@ -2115,7 +2107,16 @@ int console_trylock(void)
>  		return 0;
>  	}
>  	console_locked = 1;
> -	console_may_schedule = 0;
> +	/*
> +	 * On !PREEMPT_COUNT kernels we can't reliably detect if it's safe
> +	 * to schedule -- e.g. calling printk while holding a spin_lock,
> +	 * because preempt_disable()/preempt_enable() are just barriers and
> +	 * don't modify preempt_count() there. console_may_schedule is
> +	 * always 0 on !PREEMPT_COUNT kernels.
> +	 */
> +	console_may_schedule = !oops_in_progress &&
> +			preemptible() &&
> +			!rcu_preempt_depth();
>  	return 1;

We discussed this a lot but I am still a bit nervous ;-)

Avoid scheduling when oops_in_progress makes sense.

preemptible() takes care of preemption and IRQ contexts.
The comment above explains that it is safe to use here.

The check for rcu_preempt_depth() makes sense. But is it
safe, please?

rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
enabled. It means that you are not able to detect RCU read
section and it might cause problems.

I rather add Paul into CC.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1332133

Hello Petr,

On (02/11/16 15:41), Petr Mladek wrote:
[..]
> > +	console_may_schedule = !oops_in_progress &&
> > +			preemptible() &&
> > +			!rcu_preempt_depth();
> >  	return 1;
> 
> We discussed this a lot but I am still a bit nervous ;-)

sure, no prob :-)

> Avoid scheduling when oops_in_progress makes sense.
> 
> preemptible() takes care of preemption and IRQ contexts.
> The comment above explains that it is safe to use here.
> 
> The check for rcu_preempt_depth() makes sense. But is it
> safe, please?
> 
> rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
> enabled. It means that you are not able to detect RCU read
> section and it might cause problems.

well, I believe it's ok. __rcu_read_lock() for CONFIG_PREEMPT_RCU
does current->rcu_read_lock_nesting++, so rcu_preempt_depth() works
as expected. otherwise, for !CONFIG_PREEMPT_RCU kernel,
__rcu_read_lock() does

	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
		preempt_disable()


- if we run "CONFIG_PREEMPT_RCU" then rcu_preempt_depth()
  works here.

- if we run "!CONFIG_PREEMPT_RCU && CONFIG_PREEMPT_COUNT"
  then preemptible() works for us

- if we run "!CONFIG_PREEMPT_RCU && !CONFIG_PREEMPT_COUNT"
  then preemptible() is always 0.

> I rather add Paul into CC.

thanks.

	-ss

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

#1332205

On Fri 2016-02-12 00:02:17, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (02/11/16 15:41), Petr Mladek wrote:
> [..]
> > > +	console_may_schedule = !oops_in_progress &&
> > > +			preemptible() &&
> > > +			!rcu_preempt_depth();
> > >  	return 1;
> > 
> > We discussed this a lot but I am still a bit nervous ;-)
> 
> sure, no prob :-)
> 
> > Avoid scheduling when oops_in_progress makes sense.
> > 
> > preemptible() takes care of preemption and IRQ contexts.
> > The comment above explains that it is safe to use here.
> > 
> > The check for rcu_preempt_depth() makes sense. But is it
> > safe, please?
> > 
> > rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
> > enabled. It means that you are not able to detect RCU read
> > section and it might cause problems.
> 
> well, I believe it's ok. __rcu_read_lock() for CONFIG_PREEMPT_RCU
> does current->rcu_read_lock_nesting++, so rcu_preempt_depth() works
> as expected. otherwise, for !CONFIG_PREEMPT_RCU kernel,
> __rcu_read_lock() does
> 
> 	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
> 		preempt_disable()
> 
> 
> - if we run "CONFIG_PREEMPT_RCU" then rcu_preempt_depth()
>   works here.
> 
> - if we run "!CONFIG_PREEMPT_RCU && CONFIG_PREEMPT_COUNT"
>   then preemptible() works for us
> 
> - if we run "!CONFIG_PREEMPT_RCU && !CONFIG_PREEMPT_COUNT"
>   then preemptible() is always 0.

I feel convinced. But we should somehow document it. I think how
to do it effectively. I think that the following text would help
me if I read it:

	/*
	 * Safe context for rescheduling is detected only when
	 * PREEMPT_COUNT is enabled. preemptible() always returns
	 * false otherwise.
	 *
	 * RCU read sections must be detected separately. They
	 * have a separate preemption counter when PREEMPT_RCU
	 * is enabled.
	 */

I wanted to highlight why exactly the check returns 0 in !PREEMPT_COUNT
kernel. I missed this a bit in you original comment. But feel free
to change it as you like.

Best Regards,
Petr

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

#1332535

On (02/11/16 17:10), Petr Mladek wrote:
[..]
> > well, I believe it's ok. __rcu_read_lock() for CONFIG_PREEMPT_RCU
> > does current->rcu_read_lock_nesting++, so rcu_preempt_depth() works
> > as expected. otherwise, for !CONFIG_PREEMPT_RCU kernel,
> > __rcu_read_lock() does
> > 
> > 	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
> > 		preempt_disable()
> > 
> > 
> > - if we run "CONFIG_PREEMPT_RCU" then rcu_preempt_depth()
> >   works here.
> > 
> > - if we run "!CONFIG_PREEMPT_RCU && CONFIG_PREEMPT_COUNT"
> >   then preemptible() works for us
> > 
> > - if we run "!CONFIG_PREEMPT_RCU && !CONFIG_PREEMPT_COUNT"
> >   then preemptible() is always 0.
> 
> I feel convinced. But we should somehow document it. I think how
> to do it effectively. I think that the following text would help
> me if I read it:
> 
> 	/*
> 	 * Safe context for rescheduling is detected only when
> 	 * PREEMPT_COUNT is enabled. preemptible() always returns
> 	 * false otherwise.
> 	 *
> 	 * RCU read sections must be detected separately. They
> 	 * have a separate preemption counter when PREEMPT_RCU
> 	 * is enabled.
> 	 */
> 
> I wanted to highlight why exactly the check returns 0 in !PREEMPT_COUNT
> kernel. I missed this a bit in you original comment. But feel free
> to change it as you like.

good point. thanks! will re-spin the patch set later today,
have no reliable internet connection at the moment.

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web