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

Groups > linux.kernel > #1311625 > unrolled thread

Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of console_trylock_for_printk

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 1/2] printk: move can_use_console out of  console_trylock_for_printk Petr Mladek <pmladek@suse.com> - 2016-01-18 16:50 +0100
    Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of  console_trylock_for_printk Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-19 01:50 +0100
      Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of  console_trylock_for_printk Petr Mladek <pmladek@suse.com> - 2016-01-19 14:40 +0100
        Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of  console_trylock_for_printk Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-19 16:10 +0100
          Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of  console_trylock_for_printk Petr Mladek <pmladek@suse.com> - 2016-01-19 17:20 +0100
            Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of  console_trylock_for_printk Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-20 05:20 +0100
              Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of  console_trylock_for_printk Petr Mladek <pmladek@suse.com> - 2016-01-20 11:10 +0100

#1311625 — Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of console_trylock_for_printk

On Thu 2016-01-14 13:57:22, Sergey Senozhatsky wrote:
> vprintk_emit() disables preemption around console_trylock_for_printk()
> and console_unlock() calls for a strong reason -- can_use_console()
> check. The thing is that vprintl_emit() can be called on a CPU that
> is not fully brought up yet (!cpu_online()), which potentially can
> cause problems if console driver accesses per-cpu data. A console
> driver can explicitly state that it's safe to call it from !online
> cpu by setting CON_ANYTIME bit in console ->flags. That's why for
> !cpu_online() can_use_console() iterates all the console to find out
> if there is a CON_ANYTIME console, otherwise console_unlock() must be
> avoided.
> 
> call_console_drivers(), called from console_cont_flush() and
> console_unlock(), does the same test during for_each_console() loop.
> However, we can have the following corner case. Assume that we have 2
> cpus -- CPU0 is online, CPU1 is !online; and no CON_ANYTIME consoles
> available.
> 
> CPU0 online                        CPU1 !online
>                                  console_trylock()
>                                  ...
>                                  console_unlock()

Please, where this console_unlock() comes from? If I get this
correctly, this CPU is not online and no CON_ANYTIME console exists
=> can_use_console() fails
  => console_trylock() fails
    => console_unlock() is not called from vprintk_emit().

Best Regards,
Petr

>                                    console_cont_flush
>                                      spin_lock logbuf_lock
>                                      if (!cont.len) {
>                                         spin_unlock logbuf_lock
>                                         return
>                                      }
>                                    for (;;) {
> vprintk_emit
>   spin_lock logbuf_lock
>   log_store
>   spin_unlock logbuf_lock
>                                      spin_lock logbuf_lock
>   !console_trylock_for_printk        msg_print_text
>  return                              console_idx = log_next()
>                                      console_seq++
>                                      console_prev = msg->flags
>                                      spin_unlock logbuf_lock
> 
>                                      call_console_drivers()
>                                        for_each_console(con) {
>                                          if (!cpu_online() &&
>                                              !(con->flags & CON_ANYTIME))
>                                                  continue;
>                                          }
>                                    /*
>                                     * no message printed, we lost it
>                                     */
> vprintk_emit
>   spin_lock logbuf_lock
>   log_store
>   spin_unlock logbuf_lock
>   !console_trylock_for_printk
>  return
>                                    /*
>                                     * go to the beginning of the loop,
>                                     * find out there are new messages,
>                                     * lose it
>                                     */
>                                    }
> 
> This patch moves can_use_console() check out of
> console_trylock_for_printk(). Instead it calls it in two
> non-preemptible sections:
> -- console_cont_flush()
>    can_use_console() call happens with spin_lock logbuf_lock
>    taken and local IRQs disabled, local IRQs are not enabled
>    until call_console_drivers() returns.
> 
> -- console_unlock()
>    can_use_console() call happens with spin_lock logbuf_lock
>    taken and local IRQs disabled, local IRQs are not enabled
>    until call_console_drivers() returns.
> 
> In both cases, we need to know that call_console_drivers() call
> was aborted because of !can_use_console() and we must exit
> console_unlock() function.
> 
> This also lets to drop '!cpu_online() && !(con->flags & CON_ANYTIME)'
> test from call_console_drivers().
> 
> At the same time this patch serves as a preparation to enable
> cond_resched for 'in-direct' console_unlock() callers -- the ones
> that come from vprintk_emit(). The 'direct' ones -- the ones that
> do console_lock()/console_unlock() -- already can cond_resched.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  kernel/printk/printk.c | 71 +++++++++++++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 35 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 7ebcfea..5cc0ce6 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1452,9 +1452,6 @@ static void call_console_drivers(int level,
>  			continue;
>  		if (!con->write)
>  			continue;
> -		if (!cpu_online(smp_processor_id()) &&
> -		    !(con->flags & CON_ANYTIME))
> -			continue;
>  		if (con->flags & CON_EXTENDED)
>  			con->write(con, ext_text, ext_len);
>  		else
> @@ -1500,18 +1497,6 @@ static int have_callable_console(void)
>  }
>  
>  /*
> - * Can we actually use the console at this time on this cpu?
> - *
> - * Console drivers may assume that per-cpu resources have been allocated. So
> - * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
> - * call them until this CPU is officially up.
> - */
> -static inline int can_use_console(unsigned int cpu)
> -{
> -	return cpu_online(cpu) || have_callable_console();
> -}
> -
> -/*
>   * Try to get console ownership to actually show the kernel
>   * messages from a 'printk'. Return true (and with the
>   * console_lock held, and 'console_locked' set) if it
> @@ -1519,21 +1504,7 @@ static inline int can_use_console(unsigned int cpu)
>   */
>  static int console_trylock_for_printk(void)
>  {
> -	unsigned int cpu = smp_processor_id();
> -
> -	if (!console_trylock())
> -		return 0;
> -	/*
> -	 * If we can't use the console, we need to release the console
> -	 * semaphore by hand to avoid flushing the buffer. We need to hold the
> -	 * console semaphore in order to do this test safely.
> -	 */
> -	if (!can_use_console(cpu)) {
> -		console_locked = 0;
> -		up_console_sem();
> -		return 0;
> -	}
> -	return 1;
> +	return console_trylock();
>  }
>  
>  int printk_delay_msec __read_mostly;
> @@ -2177,10 +2148,23 @@ int is_console_locked(void)
>  	return console_locked;
>  }
>  
> -static void console_cont_flush(char *text, size_t size)
> +/*
> + * Can we actually use the console at this time on this cpu?
> + *
> + * Console drivers may assume that per-cpu resources have been allocated. So
> + * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
> + * call them until this CPU is officially up.
> + */
> +static inline int can_use_console(void)
> +{
> +	return cpu_online(smp_processor_id()) || have_callable_console();
> +}
> +
> +static bool console_cont_flush(char *text, size_t size)
>  {
>  	unsigned long flags;
>  	size_t len;
> +	bool ret = true;
>  
>  	raw_spin_lock_irqsave(&logbuf_lock, flags);
>  
> @@ -2195,15 +2179,21 @@ static void console_cont_flush(char *text, size_t size)
>  	if (console_seq < log_next_seq && !cont.cons)
>  		goto out;
>  
> +	if (!can_use_console()) {
> +		ret = false;
> +		goto out;
> +	}
> +
>  	len = cont_print_text(text, size);
>  	raw_spin_unlock(&logbuf_lock);
>  	stop_critical_timings();
>  	call_console_drivers(cont.level, NULL, 0, text, len);
>  	start_critical_timings();
>  	local_irq_restore(flags);
> -	return;
> +	return ret;
>  out:
>  	raw_spin_unlock_irqrestore(&logbuf_lock, flags);
> +	return ret;
>  }
>  
>  /**
> @@ -2227,7 +2217,7 @@ void console_unlock(void)
>  	static u64 seen_seq;
>  	unsigned long flags;
>  	bool wake_klogd = false;
> -	bool do_cond_resched, retry;
> +	bool do_cond_resched, retry, aborted = false;
>  
>  	if (console_suspended) {
>  		up_console_sem();
> @@ -2248,7 +2238,12 @@ void console_unlock(void)
>  	console_may_schedule = 0;
>  
>  	/* flush buffered message fragment immediately to console */
> -	console_cont_flush(text, sizeof(text));
> +	if (!console_cont_flush(text, sizeof(text))) {
> +		console_locked = 0;
> +		up_console_sem();
> +		return;
> +	}
> +
>  again:
>  	for (;;) {
>  		struct printk_log *msg;
> @@ -2257,6 +2252,12 @@ again:
>  		int level;
>  
>  		raw_spin_lock_irqsave(&logbuf_lock, flags);
> +
> +		if (!can_use_console()) {
> +			aborted = true;
> +			break;
> +		}
> +
>  		if (seen_seq != log_next_seq) {
>  			wake_klogd = true;
>  			seen_seq = log_next_seq;
> @@ -2337,7 +2338,7 @@ skip:
>  	 * flush, no worries.
>  	 */
>  	raw_spin_lock(&logbuf_lock);
> -	retry = console_seq != log_next_seq;
> +	retry = !aborted && (console_seq != log_next_seq);
>  	raw_spin_unlock_irqrestore(&logbuf_lock, flags);
>  
>  	if (retry && console_trylock())
> -- 
> 2.7.0
> 

[toc] | [next] | [standalone]

#1311867

Hello,
thanks for review.

On (01/18/16 16:42), Petr Mladek wrote:
> On Thu 2016-01-14 13:57:22, Sergey Senozhatsky wrote:
> > vprintk_emit() disables preemption around console_trylock_for_printk()
> > and console_unlock() calls for a strong reason -- can_use_console()
> > check. The thing is that vprintl_emit() can be called on a CPU that
> > is not fully brought up yet (!cpu_online()), which potentially can
> > cause problems if console driver accesses per-cpu data. A console
> > driver can explicitly state that it's safe to call it from !online
> > cpu by setting CON_ANYTIME bit in console ->flags. That's why for
> > !cpu_online() can_use_console() iterates all the console to find out
> > if there is a CON_ANYTIME console, otherwise console_unlock() must be
> > avoided.
> > 
> > call_console_drivers(), called from console_cont_flush() and
> > console_unlock(), does the same test during for_each_console() loop.
> > However, we can have the following corner case. Assume that we have 2
> > cpus -- CPU0 is online, CPU1 is !online; and no CON_ANYTIME consoles
> > available.
> > 
> > CPU0 online                        CPU1 !online
> >                                  console_trylock()
> >                                  ...
> >                                  console_unlock()
> 
> Please, where this console_unlock() comes from?

from UP* or DOWN* (_PREPARE f.e.) notifiers on this CPU, for example, we don't
know what's going on there. what prevents it from calling console_trylock(),
grabbing the console_sem and eventually doing console_unlock()? there is
a can_use_console() check, but it handles only one case -- printk().
there is also an extra '!cpu_online() && !CON_ANYTIME' test done for_each_console
in call_console_drivers(), but it's too late -- we already msg_print_text()
and advanced console_seq/console_idx/etc., the message will be lost, we
don't put it back.

> If I get this correctly, this CPU is not online and no CON_ANYTIME
> console exists
> => can_use_console() fails
>   => console_trylock() fails
>     => console_unlock() is not called from vprintk_emit().

the current flow is
vprintk_emit()
  console_trylock_for_printk
    can_use_console fails -- !cpu online and no CON_ANYTIME
      console_unlock() is not called from vprintk_emit()

the missing path
console_trylock
  console_unlock
    for (;;) {
      msg_print_text
      call_console_drivers
        !cpu_online && !CON_ANYTIME -- lost it and repeat again
    }


the new one is

vprintk_emit()
  console_trylock_for_printk  -- ok
    console_unlock
      can_use_console fails -- !cpu online and no CON_ANYTIME

and it also covers the case
console_trylock  -- detour can_use_console()
  console_unlock
    can_use_console fails -- !cpu online and no CON_ANYTIME, abort

	-ss

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

#1312237

On Tue 2016-01-19 09:42:36, Sergey Senozhatsky wrote:
> Hello,
> thanks for review.
> 
> On (01/18/16 16:42), Petr Mladek wrote:
> > On Thu 2016-01-14 13:57:22, Sergey Senozhatsky wrote:
> > > vprintk_emit() disables preemption around console_trylock_for_printk()
> > > and console_unlock() calls for a strong reason -- can_use_console()
> > > check. The thing is that vprintl_emit() can be called on a CPU that
> > > is not fully brought up yet (!cpu_online()), which potentially can
> > > cause problems if console driver accesses per-cpu data. A console
> > > driver can explicitly state that it's safe to call it from !online
> > > cpu by setting CON_ANYTIME bit in console ->flags. That's why for
> > > !cpu_online() can_use_console() iterates all the console to find out
> > > if there is a CON_ANYTIME console, otherwise console_unlock() must be
> > > avoided.
> > > 
> > > call_console_drivers(), called from console_cont_flush() and
> > > console_unlock(), does the same test during for_each_console() loop.
> > > However, we can have the following corner case. Assume that we have 2
> > > cpus -- CPU0 is online, CPU1 is !online; and no CON_ANYTIME consoles
> > > available.
> > > 
> > > CPU0 online                        CPU1 !online
> > >                                  console_trylock()
> > >                                  ...
> > >                                  console_unlock()
> > 
> > Please, where this console_unlock() comes from?
> 
> from UP* or DOWN* (_PREPARE f.e.) notifiers on this CPU, for example, we don't
> know what's going on there. what prevents it from calling console_trylock(),
> grabbing the console_sem and eventually doing console_unlock()? there is
> a can_use_console() check, but it handles only one case -- printk().

So, is it a theoretical problem or do you know about any
particular path where this happens?

Well, it might make sense to get rid of console_trylock_for_printk()
and do the can_use_console() check at the beginning of
unlock_console(). I mean to do

-	if (console_trylock_for_printk())
+	if (console_trylock())
		unlock_console();

But do we really need to repeat the check in every cycle?
can_use_console() checks available consoles and if the CPU
is online. Consoles could not get added or removed when
we own console_sem. It seems that CPUs get disabled
in a process context. Therefore it seems that it might happen
only when unlock_console() gets rescheduled. I guess that
it could not get scheduled back on an offline CPU. So, it
seems that it is enough to check can_use_console() only once at
the beginning.

Or did I miss anything?

Best Regards,
Petr

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

#1312315

Hello,

On (01/19/16 14:31), Petr Mladek wrote:
> > On (01/18/16 16:42), Petr Mladek wrote:
> > > On Thu 2016-01-14 13:57:22, Sergey Senozhatsky wrote:
> > > > vprintk_emit() disables preemption around console_trylock_for_printk()
> > > > and console_unlock() calls for a strong reason -- can_use_console()
> > > > check. The thing is that vprintl_emit() can be called on a CPU that
> > > > is not fully brought up yet (!cpu_online()), which potentially can
> > > > cause problems if console driver accesses per-cpu data. A console
> > > > driver can explicitly state that it's safe to call it from !online
> > > > cpu by setting CON_ANYTIME bit in console ->flags. That's why for
> > > > !cpu_online() can_use_console() iterates all the console to find out
> > > > if there is a CON_ANYTIME console, otherwise console_unlock() must be
> > > > avoided.
> > > > 
> > > > call_console_drivers(), called from console_cont_flush() and
> > > > console_unlock(), does the same test during for_each_console() loop.
> > > > However, we can have the following corner case. Assume that we have 2
> > > > cpus -- CPU0 is online, CPU1 is !online; and no CON_ANYTIME consoles
> > > > available.
> > > > 
> > > > CPU0 online                        CPU1 !online
> > > >                                  console_trylock()
> > > >                                  ...
> > > >                                  console_unlock()
> > > 
> > > Please, where this console_unlock() comes from?
> > 
> > from UP* or DOWN* (_PREPARE f.e.) notifiers on this CPU, for example, we don't
> > know what's going on there. what prevents it from calling console_trylock(),
> > grabbing the console_sem and eventually doing console_unlock()? there is
> > a can_use_console() check, but it handles only one case -- printk().
> 
> So, is it a theoretical problem or do you know about any
> particular path where this happens?

a theoretical one at this point.

> Well, it might make sense to get rid of console_trylock_for_printk()
> and do the can_use_console() check at the beginning of
> unlock_console(). I mean to do
> 
> -	if (console_trylock_for_printk())
> +	if (console_trylock())
> 		unlock_console();

agree, I almost removed it, but decided to keep for a while,
just in case if we would want to add any additional code there.


> But do we really need to repeat the check in every cycle?

well, on every iteration in the best case we check cpu_online()
only. which is what we would have done anyway in vprintk_emit(),
so no additional checks added. at the same time call_console_drivers
does not do '!cpu_online && !CON_ANYTIME' for each console now, so
in some sense there are less checks now (this is far even from a
micro-optimization, just noted).

> can_use_console() checks available consoles and if the CPU
> is online. Consoles could not get added or removed when
> we own console_sem. It seems that CPUs get disabled
> in a process context. Therefore it seems that it might happen
> only when unlock_console() gets rescheduled. I guess that
> it could not get scheduled back on an offline CPU. So, it
> seems that it is enough to check can_use_console() only once at
> the beginning.
> 
> Or did I miss anything?

It does sound interesting, thanks. I was trying to keep the existing
behaviour.

It almost works, there is one case.

console_unlock()    /* w/o can_use_console() in logbuf_lock section */
....
again:
	for (;;) {
		raw_spin_lock_irqsave logbuf_lock
		msg_print_text
		raw_spin_unlock logbuf_lock
		call_console_drivers
		local_irq_restore
	}

	up_console_sem

	raw_spin_lock logbuf_lock
	retry = console_seq != log_next_seq
	raw_spin_unlock_irqrestore logbuf_lock
	
	if retry && console_trylock()
		goto again;


when we up_console_sem(), consoles may appear and disappear, since we
don't keep the semaphore. Suppose that we have OFFLINE cpu and we had a
CON_ANYTIME console, but in between up_console_sem--console_trylock
that single CON_ANYTIME console was removed. So now we have !cpu_online
and !CON_ANYTIME.

So this is why I re-do the can_use_console() check. I can add a bool flag
and do the can_use_console() only once -- when the flag is false, set it
to true on successful can_use_console() and avoid can_use_console() during
this loop; and set the flag to false right after the 'again:' label, which
will imply that console semaphore has been re-acquired and we need to
can_use_console() again.

something like this, perhaps (to be folded.. or should it be a separate
commit -- optimization). will give a test tomorrow.

I reused `bool retry' flag (which is probably a bit hacky, it'll be
better to have a separate byte for this thing). And I moved
can_use_console() from console_cont_flush() to the top -- so if we
are executing the `for (;;)' loop, then can_use_console() was
successful in console_cont_flush() and we have to re-do
can_use_console() only if we released console_sem and jumped to
`again' label.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 28b2dec..c7a5ce8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2170,6 +2170,11 @@ static bool console_cont_flush(char *text, size_t size)
 
 	raw_spin_lock_irqsave(&logbuf_lock, flags);
 
+	if (!can_use_console()) {
+		ret = false;
+		goto out;
+	}
+
 	if (!cont.len)
 		goto out;
 
@@ -2181,11 +2186,6 @@ static bool console_cont_flush(char *text, size_t size)
 	if (console_seq < log_next_seq && !cont.cons)
 		goto out;
 
-	if (!can_use_console()) {
-		ret = false;
-		goto out;
-	}
-
 	len = cont_print_text(text, size);
 	raw_spin_unlock(&logbuf_lock);
 	stop_critical_timings();
@@ -2219,7 +2219,7 @@ void console_unlock(void)
 	static u64 seen_seq;
 	unsigned long flags;
 	bool wake_klogd = false;
-	bool do_cond_resched, retry, aborted = false;
+	bool do_cond_resched, retry = false, aborted = false;
 
 	if (console_suspended) {
 		up_console_sem();
@@ -2255,9 +2255,20 @@ again:
 
 		raw_spin_lock_irqsave(&logbuf_lock, flags);
 
-		if (!can_use_console()) {
-			aborted = true;
-			break;
+		/*
+		 * @retry == true implies that we have released console_sem
+		 * and, thus, someone could have added/removed console(-s).
+		 * we need to can_use_console() again. @retry intially set
+		 * to false, because console_cont_flush() does the
+		 * can_use_console() check and we can't execute this loop
+		 * in can_use_console() returned `false` there.
+		 */
+		if (retry) {
+			retry = false;
+			if (!can_use_console()) {
+				aborted = true;
+				break;
+			}
 		}
 
 		if (seen_seq != log_next_seq) {
-- 
2.7.0

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

#1312363

On Wed 2016-01-20 00:00:40, Sergey Senozhatsky wrote:
> > But do we really need to repeat the check in every cycle?
> 
> well, on every iteration in the best case we check cpu_online()
> only. which is what we would have done anyway in vprintk_emit(),
> so no additional checks added. at the same time call_console_drivers
> does not do '!cpu_online && !CON_ANYTIME' for each console now, so
> in some sense there are less checks now (this is far even from a
> micro-optimization, just noted).

Hmm, we need to keep the check in call_console_drivers(). It iterates
over all registered consoles. Only some of them could habe CON_ANYTIME
flag. We need to skip the others when the CPU is not online.


> console_unlock()    /* w/o can_use_console() in logbuf_lock section */
> ....
> again:
> 	for (;;) {
> 		raw_spin_lock_irqsave logbuf_lock
> 		msg_print_text
> 		raw_spin_unlock logbuf_lock
> 		call_console_drivers
> 		local_irq_restore
> 	}
> 
> 	up_console_sem
> 
> 	raw_spin_lock logbuf_lock
> 	retry = console_seq != log_next_seq
> 	raw_spin_unlock_irqrestore logbuf_lock
> 	
> 	if retry && console_trylock()
> 		goto again;
> 
> 
> when we up_console_sem(), consoles may appear and disappear, since we
> don't keep the semaphore. Suppose that we have OFFLINE cpu and we had a
> CON_ANYTIME console, but in between up_console_sem--console_trylock
> that single CON_ANYTIME console was removed. So now we have !cpu_online
> and !CON_ANYTIME.

Ah, I have missed that the console_sem is released/taken before goto
again. Hmm, your proposed solution adds even more twists into the code.
I think how to make it easier. I think that we could move the again:
target and call console_cont_flush() when there is some new content.
It is a corner case, anyway. Then we could do:


	do_cond_resched = console_may_schedule;
	console_may_schedule = 0;

+again:
+	if (!can_use_console()) {
+		console_locked = 0;
+		up_console_sem();
+		return;
	}

	/* flush buffered message fragment immediately to console */
	console_cont_flush(text, sizeof(text));
-again:
	for (;;) {
		struct printk_log *msg;


Then we remove this check from console_trylock_for_printk() and
eventually remove this function altogether.

It means that the code will be easier and protected against the
theoretical race.

How does that sound, please?

Best Regards,
Petr

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

#1312862

On (01/19/16 17:16), Petr Mladek wrote:
> On Wed 2016-01-20 00:00:40, Sergey Senozhatsky wrote:
> > > But do we really need to repeat the check in every cycle?
> > 
> > well, on every iteration in the best case we check cpu_online()
> > only. which is what we would have done anyway in vprintk_emit(),
> > so no additional checks added. at the same time call_console_drivers
> > does not do '!cpu_online && !CON_ANYTIME' for each console now, so
> > in some sense there are less checks now (this is far even from a
> > micro-optimization, just noted).
> 
> Hmm, we need to keep the check in call_console_drivers(). It iterates
> over all registered consoles. Only some of them could habe CON_ANYTIME
> flag. We need to skip the others when the CPU is not online.

oh... good point, you are right! my bad. so we basically need both. the
first one (can_use_console() before call_console_drivers()) ensures that
it's _generally_ OK to call call_console_drivers() later and that it will
not drain messages, the second one _in_ call_console_drivers() filters out
only CON_ANYTIME consoles if !cpu_online(), but by this time we are sure
that there is at least one CON_ANYTIME console.

[..]
> > when we up_console_sem(), consoles may appear and disappear, since we
> > don't keep the semaphore. Suppose that we have OFFLINE cpu and we had a
> > CON_ANYTIME console, but in between up_console_sem--console_trylock
> > that single CON_ANYTIME console was removed. So now we have !cpu_online
> > and !CON_ANYTIME.
> 
> Ah, I have missed that the console_sem is released/taken before goto
> again. Hmm, your proposed solution adds even more twists into the code.
> I think how to make it easier. I think that we could move the again:
> target and call console_cont_flush() when there is some new content.
> It is a corner case, anyway. Then we could do:
> 
> 
> 	do_cond_resched = console_may_schedule;
> 	console_may_schedule = 0;
> 
> +again:
> +	if (!can_use_console()) {
> +		console_locked = 0;
> +		up_console_sem();
> +		return;
> 	}
> 
> 	/* flush buffered message fragment immediately to console */
> 	console_cont_flush(text, sizeof(text));
> -again:
> 	for (;;) {
> 		struct printk_log *msg;
> 

looks better. we do extra IRQ disable/enable (spin lock irq) when we jump
to `again' label, but I don't think this introduces any significant overhead.
however, if it does, we always can avoid extra console_cont_flush() by simply
checking @retry -- it's `false' only once, when we execute this part for
the first time in the current console_unlock() call; all goto-jumps imply
that @retry is `true'.

IOW:

	bool retry = false;

again:
	can_use_console

	if (!retry)	/* if we jumped to again, retry is true */
		console_cont_flush
	
	for (;;) {
		...
	}

	retry = ...

	if retry && console_trylock()
		goto retry;


> Then we remove this check from console_trylock_for_printk() and
> eventually remove this function altogether.

yes, that was the plan :)

> It means that the code will be easier and protected against the
> theoretical race.
> 
> How does that sound, please?

sounds good, thanks.

> Best Regards,
> Petr

	-ss

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

#1313053

On Wed 2016-01-20 13:18:04, Sergey Senozhatsky wrote:
> On (01/19/16 17:16), Petr Mladek wrote:
> > 	do_cond_resched = console_may_schedule;
> > 	console_may_schedule = 0;
> > 
> > +again:
> > +	if (!can_use_console()) {
> > +		console_locked = 0;
> > +		up_console_sem();
> > +		return;
> > 	}
> > 
> > 	/* flush buffered message fragment immediately to console */
> > 	console_cont_flush(text, sizeof(text));
> > -again:
> > 	for (;;) {
> > 		struct printk_log *msg;
> > 
> 
> looks better. we do extra IRQ disable/enable (spin lock irq) when we jump
> to `again' label, but I don't think this introduces any significant overhead.
> however, if it does, we always can avoid extra console_cont_flush() by simply
> checking @retry -- it's `false' only once, when we execute this part for
> the first time in the current console_unlock() call; all goto-jumps imply
> that @retry is `true'.

IMHO, the extra spin_lock/unlock does not harm much. We do this
for each line in the for(;;) cycle anyway. But the check for retry
value is clever. We could use this if others would want to preserve
the existing behavior and call console_cont_flush() only once.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web