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

Groups > linux.kernel > #1385135 > unrolled thread

[PATCH v12 3/3] printk: make printk.synchronous param rw

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

  [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-04-22 15:00 +0200
    Re: [PATCH v12 3/3] printk: make printk.synchronous param rw Jan Kara <jack@suse.cz> - 2016-04-23 09:00 +0200
      Re: [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-04-23 10:30 +0200
    [PATCH] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-04-23 16:10 +0200
      Re: [PATCH] printk: make printk.synchronous param rw Petr Mladek <pmladek@suse.com> - 2016-04-26 16:20 +0200
        Re: [PATCH] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-04-27 03:10 +0200

#1385135 — [PATCH v12 3/3] printk: make printk.synchronous param rw

Change `synchronous' printk param to be RW, so user space
can change printk mode back and forth to/from sync mode
(which is considered to be more reliable).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 51 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 89f5441..61e639a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -288,9 +288,6 @@ static u32 log_buf_len = __LOG_BUF_LEN;
 
 /* Control whether printing to console must be synchronous. */
 static bool __read_mostly printk_sync = true;
-module_param_named(synchronous, printk_sync, bool, S_IRUGO);
-MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
-
 /* Printing kthread for async printk */
 static struct task_struct *printk_kthread;
 /* When `true' printing thread has messages to print */
@@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * operate in sync mode once panic() occurred.
 		 */
 		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
-				printk_kthread) {
+				!printk_sync && printk_kthread) {
 			/* Offload printing to a schedulable context. */
 			printk_kthread_need_flush_console = true;
 			wake_up_process(printk_kthread);
@@ -2757,6 +2754,13 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
+/*
+ * Prevent starting printk_kthread from start_kernel()->parse_args().
+ * It's not possible at this stage. Instead, do it via the inticall
+ * or a sysfs knob.
+ */
+static bool printk_kthread_can_run;
+
 static int printk_kthread_func(void *data)
 {
 	while (1) {
@@ -2780,18 +2784,14 @@ static int printk_kthread_func(void *data)
 	return 0;
 }
 
-/*
- * Init async printk via late_initcall, after core/arch/device/etc.
- * initialization.
- */
-static int __init init_printk_kthread(void)
+static int __init_printk_kthread(void)
 {
 	struct task_struct *thread;
 	struct sched_param param = {
 		.sched_priority = MAX_RT_PRIO - 1,
 	};
 
-	if (printk_sync)
+	if (!printk_kthread_can_run || printk_sync || printk_kthread)
 		return 0;
 
 	thread = kthread_run(printk_kthread_func, NULL, "printk");
@@ -2805,6 +2805,35 @@ static int __init init_printk_kthread(void)
 	printk_kthread = thread;
 	return 0;
 }
+
+static int printk_sync_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (ret)
+		return ret;
+	return __init_printk_kthread();
+}
+
+static const struct kernel_param_ops param_ops_printk_sync = {
+	.set = printk_sync_set,
+	.get = param_get_bool,
+};
+
+module_param_cb(synchronous, &param_ops_printk_sync, &printk_sync,
+		S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
+
+/*
+ * Init async printk via late_initcall, after core/arch/etc.
+ * initialization.
+ */
+static __init int init_printk_kthread(void)
+{
+	printk_kthread_can_run = true;
+	return __init_printk_kthread();
+}
 late_initcall(init_printk_kthread);
 
 /*
@@ -2820,7 +2849,7 @@ 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_kthread) {
+		if (!printk_sync && printk_kthread) {
 			wake_up_process(printk_kthread);
 		} else {
 			/*
-- 
2.8.0

[toc] | [next] | [standalone]

#1385582

On Fri 22-04-16 22:53:00, Sergey Senozhatsky wrote:
> Change `synchronous' printk param to be RW, so user space
> can change printk mode back and forth to/from sync mode
> (which is considered to be more reliable).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

The patch looks good to me. One suggestion below:

> @@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 * operate in sync mode once panic() occurred.
>  		 */
>  		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
> -				printk_kthread) {
> +				!printk_sync && printk_kthread) {
>  			/* Offload printing to a schedulable context. */
>  			printk_kthread_need_flush_console = true;
>  			wake_up_process(printk_kthread);

It would seem more future-proof to hide '!printk_sync && printk_kthread'
into a wrapper function as it is somewhat subtle detail that printk_kthread
needn't exist while !printk_sync and I can imagine someone forgetting to
check that in the future. Something like 'can_print_async()'? But I don't
feel too strongly about that so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

regardless whether you change this or not.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

#1385596

Hello,

On (04/23/16 08:56), Jan Kara wrote:
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> The patch looks good to me. One suggestion below:
> 
> > @@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> >  		 * operate in sync mode once panic() occurred.
> >  		 */
> >  		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
> > -				printk_kthread) {
> > +				!printk_sync && printk_kthread) {
> >  			/* Offload printing to a schedulable context. */
> >  			printk_kthread_need_flush_console = true;
> >  			wake_up_process(printk_kthread);
> 
> It would seem more future-proof to hide '!printk_sync && printk_kthread'
> into a wrapper function as it is somewhat subtle detail that printk_kthread
> needn't exist while !printk_sync and I can imagine someone forgetting to
> check that in the future. Something like 'can_print_async()'? But I don't
> feel too strongly about that so feel free to add:

hm, yes. this is what I eventually do in "yet to be posted"
make-console_unlock()-async patch. I move printing kthread
wakeup-s and those async printing checks out of vprintk_emit()
and wake_up_klogd_work_func() to a special function:

static bool console_unlock_async_flush(void)
{
...
       if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
                       !printk_sync && printk_kthread) {
               /* Offload printing to a schedulable context. */
               printk_kthread_need_flush_console = true;
               console_locked = 0;
               up_console_sem();
               wake_up_process(printk_kthread);
               return true;
       }
       return false;
}


so async_printk flags live in one place (which makes it easier
to maintain) and vprintk_emit()/wake_up_klogd_work_func() simply
do:

	if (console_trylock())
		console_unlock();


console_unlock() is the one who decides if it can do async
printk or a 'direct printing' via console_flush_and_unlock().

void console_unlock(void)
{
       if (console_unlock_async_flush())
               return;
       console_flush_and_unlock();
}


console_flush_and_unlock() is what was previously known
as console_unlock() - emit the messages and call_console_drivers().


I guess I can send out an updated version of 0003 as a reply
to the initial patch and hide '!printk_sync && printk_kthread'.


> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> regardless whether you change this or not.

thanks.

	-ss

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

#1385678 — [PATCH] printk: make printk.synchronous param rw

Change `synchronous' printk param to be RW, so user space
can change printk mode back and forth to/from sync mode
(which is considered to be more reliable).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---

-- added Jan's Reviewed-by
-- factored out async printk checks to can_printk_async()

 kernel/printk/printk.c | 56 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 89f5441..9345a29 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -288,14 +288,16 @@ static u32 log_buf_len = __LOG_BUF_LEN;
 
 /* Control whether printing to console must be synchronous. */
 static bool __read_mostly printk_sync = true;
-module_param_named(synchronous, printk_sync, bool, S_IRUGO);
-MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
-
 /* Printing kthread for async printk */
 static struct task_struct *printk_kthread;
 /* When `true' printing thread has messages to print */
 static bool printk_kthread_need_flush_console;
 
+static inline bool can_printk_async(void)
+{
+	return !printk_sync && printk_kthread;
+}
+
 /* Return log buffer address */
 char *log_buf_addr_get(void)
 {
@@ -1785,7 +1787,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * operate in sync mode once panic() occurred.
 		 */
 		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
-				printk_kthread) {
+				can_printk_async()) {
 			/* Offload printing to a schedulable context. */
 			printk_kthread_need_flush_console = true;
 			wake_up_process(printk_kthread);
@@ -2757,6 +2759,13 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
+/*
+ * Prevent starting printk_kthread from start_kernel()->parse_args().
+ * It's not possible at this stage. Instead, do it via the inticall
+ * or a sysfs knob.
+ */
+static bool printk_kthread_can_run;
+
 static int printk_kthread_func(void *data)
 {
 	while (1) {
@@ -2780,18 +2789,14 @@ static int printk_kthread_func(void *data)
 	return 0;
 }
 
-/*
- * Init async printk via late_initcall, after core/arch/device/etc.
- * initialization.
- */
-static int __init init_printk_kthread(void)
+static int __init_printk_kthread(void)
 {
 	struct task_struct *thread;
 	struct sched_param param = {
 		.sched_priority = MAX_RT_PRIO - 1,
 	};
 
-	if (printk_sync)
+	if (!printk_kthread_can_run || printk_sync || printk_kthread)
 		return 0;
 
 	thread = kthread_run(printk_kthread_func, NULL, "printk");
@@ -2805,6 +2810,35 @@ static int __init init_printk_kthread(void)
 	printk_kthread = thread;
 	return 0;
 }
+
+static int printk_sync_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (ret)
+		return ret;
+	return __init_printk_kthread();
+}
+
+static const struct kernel_param_ops param_ops_printk_sync = {
+	.set = printk_sync_set,
+	.get = param_get_bool,
+};
+
+module_param_cb(synchronous, &param_ops_printk_sync, &printk_sync,
+		S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
+
+/*
+ * Init async printk via late_initcall, after core/arch/etc.
+ * initialization.
+ */
+static __init int init_printk_kthread(void)
+{
+	printk_kthread_can_run = true;
+	return __init_printk_kthread();
+}
 late_initcall(init_printk_kthread);
 
 /*
@@ -2820,7 +2854,7 @@ 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_kthread) {
+		if (can_printk_async()) {
 			wake_up_process(printk_kthread);
 		} else {
 			/*
-- 
2.8.0

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

#1387520 — Re: [PATCH] printk: make printk.synchronous param rw

On Sun 2016-04-24 00:01:05, Sergey Senozhatsky wrote:
> Change `synchronous' printk param to be RW, so user space
> can change printk mode back and forth to/from sync mode
> (which is considered to be more reliable).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
> 
> -- added Jan's Reviewed-by
> -- factored out async printk checks to can_printk_async()
> 
>  kernel/printk/printk.c | 56 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 89f5441..9345a29 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2757,6 +2759,13 @@ static int __init printk_late_init(void)
>  late_initcall(printk_late_init);
>  
>  #if defined CONFIG_PRINTK
> +/*
> + * Prevent starting printk_kthread from start_kernel()->parse_args().
> + * It's not possible at this stage. Instead, do it via the inticall
                                                              ^^^^^^^^

s/inticall/initcall/

Otherwise, it looks fine. I like the series and keep my fingers
crossed.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

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

#1388576 — Re: [PATCH] printk: make printk.synchronous param rw

Hello,

On (04/26/16 16:15), Petr Mladek wrote:
[..]
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 89f5441..9345a29 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2757,6 +2759,13 @@ static int __init printk_late_init(void)
> >  late_initcall(printk_late_init);
> >  
> >  #if defined CONFIG_PRINTK
> > +/*
> > + * Prevent starting printk_kthread from start_kernel()->parse_args().
> > + * It's not possible at this stage. Instead, do it via the inticall
>                                                               ^^^^^^^^
> 
> s/inticall/initcall/

geez, english is hard   :(


> Otherwise, it looks fine. I like the series and keep my fingers
> crossed.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

thanks!

my system was massively Oopsing (due to kcompactd direct freepage
allocation, I suspect), OOM panicking, etc. during zsmalloc/zram
stress tests yesterday. and async printk worked fine for me. just
for note.

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web