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

Groups > linux.kernel > #1603267 > unrolled thread

Re: [RFC][PATCH 2/4] printk: offload printing from wake_up_klogd_work_func()

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 2/4] printk: offload printing from  wake_up_klogd_work_func() Petr Mladek <pmladek@suse.com> - 2017-03-17 13:30 +0100
    Re: [RFC][PATCH 2/4] printk: offload printing from  wake_up_klogd_work_func() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-18 11:00 +0100
      Re: [RFC][PATCH 2/4] printk: offload printing from  wake_up_klogd_work_func() Petr Mladek <pmladek@suse.com> - 2017-03-20 17:30 +0100
        Re: [RFC][PATCH 2/4] printk: offload printing from  wake_up_klogd_work_func() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-21 05:10 +0100
        Re: [RFC][PATCH 2/4] printk: offload printing from  wake_up_klogd_work_func() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-23 10:10 +0100
          Re: [RFC][PATCH 2/4] printk: offload printing from  wake_up_klogd_work_func() Petr Mladek <pmladek@suse.com> - 2017-03-23 13:20 +0100

#1603267 — Re: [RFC][PATCH 2/4] printk: offload printing from wake_up_klogd_work_func()

On Mon 2017-03-06 21:45:52, Sergey Senozhatsky wrote:
> Offload printing of printk_deferred() messages from IRQ context
> to a schedulable printing kthread, when possible (the same way
> we do it in vprintk_emit()). Otherwise, console_unlock() can
> force the printing CPU to spend unbound amount of time flushing
> kernel messages from IRQ context.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  kernel/printk/printk.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 1c4232ca2e6a..6e00073a7331 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2735,9 +2735,16 @@ static void wake_up_klogd_work_func(struct irq_work *irq_work)
>  	int pending = __this_cpu_xchg(printk_pending, 0);
>  
>  	if (pending & PRINTK_PENDING_OUTPUT) {
> -		/* If trylock fails, someone else is doing the printing */
> -		if (console_trylock())
> -			console_unlock();
> +		if (printk_kthread_enabled()) {
> +			wake_up_process(printk_kthread);

I have just noticed a possible race. printk_deferred() does not set
printk_kthread_need_flush_console and there might stay a
pending job:

CPU0					CPU1

printk_kthread_func()

  printk_kthread_need_flush_console = false;

  console_lock()
  console_unlock()

					printk_deferred()
					  vprintk_emit()
					  irq_work_queue()


					<IRQ>
					 wake_up_klogd_work_func()
					   if (printk_kthread_enabled())
					     wake_up_process(printk_kthread);

  set_current_state(TASK_INTERRUPTIBLE);
  if (!printk_kthread_need_flush_console)
    schedule();

Result: printk_kthread goes to sleep even though there is
	a pending job.


A solution might be to rename the variable to something like
printk_pending_output, always set it in vprintk_emit() and
clear it in console_unlock() when there are no pending messages.

I think that we have already discussed this in the past.
This solution would also remove one extra cycle if more messages
are handled by one console_unlock() call:

CPU0					CPU1

printk()
  vprintk_emit()
    printk_kthread_need_flush_console = true;
    wake_up_process(printk_kthread)


					<printk_kthread>

					printk_kthread_need_flush_console
					= false;

					console_lock()

printk()
  vprintk_emit()
    printk_kthread_need_flush_console = true;
    wake_up_process(printk_kthread)

					console_unlock()

					set_current_state(TASK_INTERRUPTIBLE);
					if (!printk_kthread_need_flush_console)
					  <fail>

					  _set_current_state(TASK_RUNNING);

					  console_lock()
					  console_unlock()

Result: The second console_unlock() has nothing to do.


If I remember correctly, you were not much happy with this
solution because it did spread the logic. I think that you did not
believe that it was worth fixing the second problem. But fixing
the race might need to spread the logic as well.

I see it the following way. vprintk_emit() is a producer,
console_unlock() is a consumer, and printk_thread is a room
that allows consumer to do its job. The consumer has more
rooms available. The state variable is a flag showing that
there is a pending job, consumer is looking for a room,
and printk_kthread should offer it.

Of course, it is possible that you will find a better
solution.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1603754

Hello Petr,

On (03/17/17 13:19), Petr Mladek wrote:
[..]
> A solution might be to rename the variable to something like
> printk_pending_output, always set it in vprintk_emit() and
> clear it in console_unlock() when there are no pending messages.

believe it or not, I thought that I set printk_kthread_need_flush_console
to `true' unconditionally. probably I did so in one of the previous patch
set iterations. weird. I agree that doing this makes sense. thanks for
bringing this up.


....

I don't want that printk_kthread_need_flush_console to exist. instead,
I think, I want to move printk_pending out of per-cpu memory and use a
global printk_pending. set PRINTK_PENDING_OUTPUT bit to true in
vprintk_emit(), clear it in console_unlock(). and make both printk_kthread
scheduling condition and console_unlock() retry path depend on
`printk_pending == 0' being true.

something like below (the code is ugly and lacks a ton of barriers, etc.
etc.)


---

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 601a9ef6db89..a0b231f49052 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -439,8 +439,6 @@ static char *log_buf = __log_buf;
 static u32 log_buf_len = __LOG_BUF_LEN;
 
 static struct task_struct *printk_kthread __read_mostly;
-/* When `true' printing thread has messages to print. */
-static bool printk_kthread_need_flush_console;
 /*
  * We can't call into the scheduler (wake_up() printk kthread), for example,
  * during suspend/kexec. This temporarily switches printk to old behaviour.
@@ -451,6 +449,13 @@ static int printk_kthread_disable __read_mostly;
  * it doesn't go back to 0.
  */
 static bool printk_emergency __read_mostly;
+/*
+ * Delayed printk version, for scheduler-internal messages:
+ */
+#define PRINTK_PENDING_WAKEUP	0x01
+#define PRINTK_PENDING_OUTPUT	0x02
+
+static int printk_pending = 0;
 
 static inline bool printk_kthread_enabled(void)
 {
@@ -1806,6 +1811,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 
 	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
 
+	printk_pending |= PRINTK_PENDING_OUTPUT;
 	logbuf_unlock_irqrestore(flags);
 
 	/* If called from the scheduler, we can not call up(). */
@@ -1819,8 +1825,6 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * schedulable context.
 		 */
 		if (printk_kthread_enabled()) {
-			printk_kthread_need_flush_console = true;
-
 			printk_safe_enter_irqsave(flags);
 			wake_up_process(printk_kthread);
 			printk_safe_exit_irqrestore(flags);
@@ -2220,10 +2224,11 @@ void console_unlock(void)
 	static char text[LOG_LINE_MAX + PREFIX_MAX];
 	static u64 seen_seq;
 	unsigned long flags;
-	bool wake_klogd = false;
-	bool do_cond_resched, retry;
+	bool wake_klogd;
+	bool do_cond_resched, retry = false;
 
 	if (console_suspended) {
+		printk_pending &= ~PRINTK_PENDING_OUTPUT;
 		up_console_sem();
 		return;
 	}
@@ -2242,6 +2247,8 @@ void console_unlock(void)
 	console_may_schedule = 0;
 
 again:
+	wake_klogd = printk_pending & PRINTK_PENDING_WAKEUP;
+	printk_pending = 0;
 	/*
 	 * We released the console_sem lock, so we need to recheck if
 	 * cpu is online and (if not) is there at least one CON_ANYTIME
@@ -2330,15 +2337,16 @@ void console_unlock(void)
 	 * flush, no worries.
 	 */
 	raw_spin_lock(&logbuf_lock);
-	retry = console_seq != log_next_seq;
+	if (printk_pending != 0 || console_seq != log_next_seq)
+		retry = true;
 	raw_spin_unlock(&logbuf_lock);
 	printk_safe_exit_irqrestore(flags);
 
-	if (retry && console_trylock())
-		goto again;
-
 	if (wake_klogd)
 		wake_up_klogd();
+
+	if (retry && console_trylock())
+		goto again;
 }
 EXPORT_SYMBOL(console_unlock);
 
@@ -2722,19 +2730,9 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
-/*
- * Delayed printk version, for scheduler-internal messages:
- */
-#define PRINTK_PENDING_WAKEUP	0x01
-#define PRINTK_PENDING_OUTPUT	0x02
-
-static DEFINE_PER_CPU(int, printk_pending);
-
 static void wake_up_klogd_work_func(struct irq_work *irq_work)
 {
-	int pending = __this_cpu_xchg(printk_pending, 0);
-
-	if (pending & PRINTK_PENDING_OUTPUT) {
+	if (printk_pending & PRINTK_PENDING_OUTPUT) {
 		if (printk_kthread_enabled()) {
 			wake_up_process(printk_kthread);
 		} else {
@@ -2747,8 +2745,10 @@ static void wake_up_klogd_work_func(struct irq_work *irq_work)
 		}
 	}
 
-	if (pending & PRINTK_PENDING_WAKEUP)
+	if (printk_pending & PRINTK_PENDING_WAKEUP) {
+		printk_pending &= ~PRINTK_PENDING_WAKEUP;
 		wake_up_interruptible(&log_wait);
+	}
 }
 
 static DEFINE_PER_CPU(struct irq_work, wake_up_klogd_work) = {
@@ -2760,18 +2760,10 @@ static int printk_kthread_func(void *data)
 {
 	while (1) {
 		set_current_state(TASK_INTERRUPTIBLE);
-		if (!printk_kthread_need_flush_console)
+		if (!(printk_pending & PRINTK_PENDING_OUTPUT))
 			schedule();
 
 		__set_current_state(TASK_RUNNING);
-		/*
-		 * Avoid an infinite loop when console_unlock() cannot
-		 * access consoles, e.g. because console_suspended is
-		 * true. schedule(), someone else will print the messages
-		 * from resume_console().
-		 */
-		printk_kthread_need_flush_console = false;
-
 		console_lock();
 		console_unlock();
 	}
@@ -2802,7 +2794,7 @@ void wake_up_klogd(void)
 {
 	preempt_disable();
 	if (waitqueue_active(&log_wait)) {
-		this_cpu_or(printk_pending, PRINTK_PENDING_WAKEUP);
+		printk_pending |= PRINTK_PENDING_WAKEUP;
 		irq_work_queue(this_cpu_ptr(&wake_up_klogd_work));
 	}
 	preempt_enable();
@@ -2818,7 +2810,6 @@ int printk_deferred(const char *fmt, ...)
 	r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, 0, fmt, args);
 	va_end(args);
 
-	__this_cpu_or(printk_pending, PRINTK_PENDING_OUTPUT);
 	irq_work_queue(this_cpu_ptr(&wake_up_klogd_work));
 	preempt_enable();
 

---


[..]
> If I remember correctly, you were not much happy with this
> solution because it did spread the logic. I think that you did not
> believe that it was worth fixing the second problem.

hm, I think Jan Kara was the first one who said that we
are overcomplicating the whole thing... or may be it was me.
don't deny it either.

	-ss

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

#1604743

On Sat 2017-03-18 18:57:39, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (03/17/17 13:19), Petr Mladek wrote:
> [..]
> > A solution might be to rename the variable to something like
> > printk_pending_output, always set it in vprintk_emit() and
> > clear it in console_unlock() when there are no pending messages.
> 
> believe it or not, I thought that I set printk_kthread_need_flush_console
> to `true' unconditionally. probably I did so in one of the previous patch
> set iterations. weird. I agree that doing this makes sense. thanks for
> bringing this up.
> 
> ....
> 
> I don't want that printk_kthread_need_flush_console to exist. instead,
> I think, I want to move printk_pending out of per-cpu memory and use a
> global printk_pending. set PRINTK_PENDING_OUTPUT bit to true in
> vprintk_emit(), clear it in console_unlock(). and make both printk_kthread
> scheduling condition and console_unlock() retry path depend on
> `printk_pending == 0' being true.

I like the idea. The things closely related.
 
> something like below (the code is ugly and lacks a ton of barriers, etc.
> etc.)

Sigh, I wanted to add few comments and it got me deeper than I wanted.

I am sorry if some of my comments are obvious. I know that the
patch was a draft and you probably was aware of many of the
problems.

Anyway, it might make sense to do the change in more steps.
One thing is removing the per-cpu variable. Another thing is changing
the logic and setting/clearing the state variable in different
situation. Yet another thing is adding the support for
kthread, etc. I am afraid that it might be a patchset on its own.


> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 601a9ef6db89..a0b231f49052 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -439,8 +439,6 @@ static char *log_buf = __log_buf;
>  static u32 log_buf_len = __LOG_BUF_LEN;
>  
>  static struct task_struct *printk_kthread __read_mostly;
> -/* When `true' printing thread has messages to print. */
> -static bool printk_kthread_need_flush_console;
>  /*
>   * We can't call into the scheduler (wake_up() printk kthread), for example,
>   * during suspend/kexec. This temporarily switches printk to old behaviour.
> @@ -451,6 +449,13 @@ static int printk_kthread_disable __read_mostly;
>   * it doesn't go back to 0.
>   */
>  static bool printk_emergency __read_mostly;
> +/*
> + * Delayed printk version, for scheduler-internal messages:
> + */
> +#define PRINTK_PENDING_WAKEUP	0x01
> +#define PRINTK_PENDING_OUTPUT	0x02
> +
> +static int printk_pending = 0;

Something tells me that we need to use atomic_t. Otherwise, we could
not safely manipulate the bits withtout a lock.

Alternative solution would be to use two separate variables.
This might make the code easier to read. I think that they
were combined only to safe space in the per-CPU area.


>  static inline bool printk_kthread_enabled(void)
>  {
> @@ -1806,6 +1811,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  
>  	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
>  
> +	printk_pending |= PRINTK_PENDING_OUTPUT;
>  	logbuf_unlock_irqrestore(flags);
>
>  	/* If called from the scheduler, we can not call up(). */
> @@ -1819,8 +1825,6 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 * schedulable context.
>  		 */
>  		if (printk_kthread_enabled()) {
> -			printk_kthread_need_flush_console = true;
> -
>  			printk_safe_enter_irqsave(flags);
>  			wake_up_process(printk_kthread);
>  			printk_safe_exit_irqrestore(flags);
> @@ -2220,10 +2224,11 @@ void console_unlock(void)
>  	static char text[LOG_LINE_MAX + PREFIX_MAX];
>  	static u64 seen_seq;
>  	unsigned long flags;
> -	bool wake_klogd = false;
> -	bool do_cond_resched, retry;
> +	bool wake_klogd;
> +	bool do_cond_resched, retry = false;
>  
>  	if (console_suspended) {
> +		printk_pending &= ~PRINTK_PENDING_OUTPUT;

Hmm, this is pretty non-intuitive. I guess that it is needed to
avoid a busy cycle in the printk kthread?

I can't find a better solution. We should at least use a name
that makes more sense, e.g. PRINTK_POKE_CONSOLE.

>  		up_console_sem();
>  		return;
>  	}
> @@ -2242,6 +2247,8 @@ void console_unlock(void)
>  	console_may_schedule = 0;
>  
>  again:
> +	wake_klogd = printk_pending & PRINTK_PENDING_WAKEUP;
> +	printk_pending = 0;

This might be racy. PRINTK_PENDING_WAKEUP is set without
a lock in bust_spinlocks() via wake_up_klogd(). The above
code read and clears the state non-atomically.


>  	/*
>  	 * We released the console_sem lock, so we need to recheck if
>  	 * cpu is online and (if not) is there at least one CON_ANYTIME
> @@ -2330,15 +2337,16 @@ void console_unlock(void)
>  	 * flush, no worries.
>  	 */
>  	raw_spin_lock(&logbuf_lock);
> -	retry = console_seq != log_next_seq;
> +	if (printk_pending != 0 || console_seq != log_next_seq)

printk_pending != 0 also when PRINTK_PENDING_WAKEUP is set.
I would do it the other way. I would clear PRINTK_PENDING_OUTPUT
when console_seq == log_next_seq and keep the check as is here.

> +		retry = true;
>  	raw_spin_unlock(&logbuf_lock);
>  	printk_safe_exit_irqrestore(flags);
>  
> -	if (retry && console_trylock())
> -		goto again;
> -
>  	if (wake_klogd)
>  		wake_up_klogd();
> +
> +	if (retry && console_trylock())
> +		goto again;

Why do you actually modify the logic for klogd()?
It might make sense but it is questionable. For example,
klogd() will need logbuf_lock as well. It might fight over
it with the console when the again target is used.
I would do it in separate patch and probably not
in this patchset.


>  }
>  EXPORT_SYMBOL(console_unlock);
>  
> @@ -2722,19 +2730,9 @@ static int __init printk_late_init(void)
>  late_initcall(printk_late_init);
>  
>  #if defined CONFIG_PRINTK
> -/*
> - * Delayed printk version, for scheduler-internal messages:
> - */
> -#define PRINTK_PENDING_WAKEUP	0x01
> -#define PRINTK_PENDING_OUTPUT	0x02
> -
> -static DEFINE_PER_CPU(int, printk_pending);

BTW: wake_up_klogd_work does not need to be per-CPU as well.
irq_work infrastructure heavily uses per-CPU variables.
But a global struct irq_work is safe, see irq_work_claim().

> 
> 
> [..]
> > If I remember correctly, you were not much happy with this
> > solution because it did spread the logic. I think that you did not
> > believe that it was worth fixing the second problem.
> 
> hm, I think Jan Kara was the first one who said that we
> are overcomplicating the whole thing... or may be it was me.
> don't deny it either.

I do not remember as well :-) Anyway, it really looks more
complicated than I thought.

I think that some clean up and optimization of the printk_pending
stuff is needed and worth it. I am just not sure whether to do it
before or after the printk kthread patchset.

I would slightly prefer to clean the printk_pending stuff first.
It might delay printk kthread patchset a bit but it will be cleaner.

Best Regards,
Petr

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

#1605286

On (03/20/17 17:09), Petr Mladek wrote:
[..]
> > I don't want that printk_kthread_need_flush_console to exist. instead,
> > I think, I want to move printk_pending out of per-cpu memory and use a
> > global printk_pending. set PRINTK_PENDING_OUTPUT bit to true in
> > vprintk_emit(), clear it in console_unlock(). and make both printk_kthread
> > scheduling condition and console_unlock() retry path depend on
> > `printk_pending == 0' being true.
> 
> I like the idea. The things closely related.
>  
> > something like below (the code is ugly and lacks a ton of barriers, etc.
> > etc.)
> 
> Sigh, I wanted to add few comments and it got me deeper than I wanted.

no worries, Petr.


[..]
> Anyway, it might make sense to do the change in more steps.

yes, sure. "per-CPU -> global printk_pending" transition first,
and then printk kthread.


[..]
> > +#define PRINTK_PENDING_WAKEUP	0x01
> > +#define PRINTK_PENDING_OUTPUT	0x02
> > +
> > +static int printk_pending = 0;
> 
> Something tells me that we need to use atomic_t. Otherwise, we could
> not safely manipulate the bits withtout a lock.

yes, I'm doing atomic set_bit/test_bit/clear_bit in current (unpublished) version.


> Alternative solution would be to use two separate variables.
> This might make the code easier to read. I think that they
> were combined only to safe space in the per-CPU area.

hm. I think one variable still can work for us; but can split it.

as of rename. dunno. I'm kinda OK with its current name.
PENDING_OUTPUT looks a bit better that POKE_CONSOLE to me.


[..]
> >  	if (console_suspended) {
> > +		printk_pending &= ~PRINTK_PENDING_OUTPUT;
> 
> Hmm, this is pretty non-intuitive. I guess that it is needed to
> avoid a busy cycle in the printk kthread?

it absolutely is.
sorry, the "code" I posted was too cryptic.

> >  		up_console_sem();
> >  		return;
> >  	}
> > @@ -2242,6 +2247,8 @@ void console_unlock(void)
> >  	console_may_schedule = 0;
> >  
> >  again:
> > +	wake_klogd = printk_pending & PRINTK_PENDING_WAKEUP;
> > +	printk_pending = 0;
> 
> This might be racy. PRINTK_PENDING_WAKEUP is set without
> a lock in bust_spinlocks() via wake_up_klogd(). The above
> code read and clears the state non-atomically.

the patch I'm looking at right now does atomic set_bit() and a bunch of
atomic test_and_clear_bit/test_bit/etc.

> >  	/*
> >  	 * We released the console_sem lock, so we need to recheck if
> >  	 * cpu is online and (if not) is there at least one CON_ANYTIME
> > @@ -2330,15 +2337,16 @@ void console_unlock(void)
> >  	 * flush, no worries.
> >  	 */
> >  	raw_spin_lock(&logbuf_lock);
> > -	retry = console_seq != log_next_seq;
> > +	if (printk_pending != 0 || console_seq != log_next_seq)
> 
> printk_pending != 0 also when PRINTK_PENDING_WAKEUP is set.

yes.

> I would do it the other way. I would clear PRINTK_PENDING_OUTPUT
> when console_seq == log_next_seq and keep the check as is here.
[..]
> > +		retry = true;
> >  	raw_spin_unlock(&logbuf_lock);
> >  	printk_safe_exit_irqrestore(flags);
> >  
> > -	if (retry && console_trylock())
> > -		goto again;
> > -
> >  	if (wake_klogd)
> >  		wake_up_klogd();
> > +
> > +	if (retry && console_trylock())
> > +		goto again;
> 
> Why do you actually modify the logic for klogd()?
> It might make sense but it is questionable. For example,
> klogd() will need logbuf_lock as well. It might fight over
> it with the console when the again target is used.
> I would do it in separate patch and probably not
> in this patchset.

I just wanted to keep printk_prnding check simpler and I figured out
that klogd logbuf_lock contention will not be something new, because
of the while() loop in kthread_printk function

	printk_tkread func
		while (1) {
			if (!pending_output)
				schedule();

			console_lock()
			console_unlock()
				wake_up klogd	/*
						 * and may be do another
						 * console_lock() straight ahead if pending_output != 0
						 */
		}

but yes. I'll drop that part and will handle only PRINTK_PENDING_OUTPUT
bit in console_unlock(), leaving the PRINTK_PENDING_WAKEUP stuff to
irq work.

I'll try to send out a refreshed version soon.


> >  EXPORT_SYMBOL(console_unlock);
> >  
> > @@ -2722,19 +2730,9 @@ static int __init printk_late_init(void)
> >  late_initcall(printk_late_init);
> >  
> >  #if defined CONFIG_PRINTK
> > -/*
> > - * Delayed printk version, for scheduler-internal messages:
> > - */
> > -#define PRINTK_PENDING_WAKEUP	0x01
> > -#define PRINTK_PENDING_OUTPUT	0x02
> > -
> > -static DEFINE_PER_CPU(int, printk_pending);
> 
> BTW: wake_up_klogd_work does not need to be per-CPU as well.
> irq_work infrastructure heavily uses per-CPU variables.
> But a global struct irq_work is safe, see irq_work_claim().

interesting. need to look at it.

we also can move printk_kthread irq_work out of per-CPU and drop
the whole 'if (printk_safe_irq_ready) smp_rmb() ' thing in this case
and simplify printk_safe_init().

may be in a separate patch set, though. since this is not really
related to printk kthread.


> > [..]
> > > If I remember correctly, you were not much happy with this
> > > solution because it did spread the logic. I think that you did not
> > > believe that it was worth fixing the second problem.
> > 
> > hm, I think Jan Kara was the first one who said that we
> > are overcomplicating the whole thing... or may be it was me.
> > don't deny it either.
> 
> I do not remember as well :-) Anyway, it really looks more
> complicated than I thought.
> 
> I think that some clean up and optimization of the printk_pending
> stuff is needed and worth it. I am just not sure whether to do it
> before or after the printk kthread patchset.
> 
> I would slightly prefer to clean the printk_pending stuff first.
> It might delay printk kthread patchset a bit but it will be cleaner.

absolutely agree. and thanks for looking into it.

	-ss

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

#1607272

On (03/20/17 17:09), Petr Mladek wrote:
[..]
> 
> BTW: wake_up_klogd_work does not need to be per-CPU as well.
> irq_work infrastructure heavily uses per-CPU variables.
> But a global struct irq_work is safe, see irq_work_claim().

so I have a patch that turns wake_up_klogd_work into a global variable,
out of curiosity, but I'm not entire sure about it. the sort of a problem
is that queued irq_works still go into a per-CPU run_lists.  per-CPU
wake_up_klogd_work permits us to queue irq work on several CPUs so we
might have better chances to execute wake_up_klogd_work_func(), while
global wake_up_klogd_work will be only in one run_list. this can defer
wake_up_klogd_work processing until that particular single CPU handles
its interrupt and calls irq_work_run_list(). what do you think?

	-ss

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

#1607429

On Thu 2017-03-23 18:00:42, Sergey Senozhatsky wrote:
> On (03/20/17 17:09), Petr Mladek wrote:
> [..]
> > 
> > BTW: wake_up_klogd_work does not need to be per-CPU as well.
> > irq_work infrastructure heavily uses per-CPU variables.
> > But a global struct irq_work is safe, see irq_work_claim().
> 
> so I have a patch that turns wake_up_klogd_work into a global variable,
> out of curiosity, but I'm not entire sure about it. the sort of a problem
> is that queued irq_works still go into a per-CPU run_lists.  per-CPU
> wake_up_klogd_work permits us to queue irq work on several CPUs so we
> might have better chances to execute wake_up_klogd_work_func(), while
> global wake_up_klogd_work will be only in one run_list. this can defer
> wake_up_klogd_work processing until that particular single CPU handles
> its interrupt and calls irq_work_run_list(). what do you think?

Good question! I personally think that it should not cause a big harm
but I am not completely sure. It might need some more testing.

Let's postpone this change and do it alone in the future.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web