Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v1 03/18] printk: Consolidate console deferred printing | Date | Fri, 17 Mar 2023 14:11:01 +0106 |
| |
On 2023-03-08, Petr Mladek <pmladek@suse.com> wrote: >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -2321,7 +2321,10 @@ asmlinkage int vprintk_emit(int facility, int level, >> preempt_enable(); >> } >> >> - wake_up_klogd(); >> + if (in_sched) >> + defer_console_output(); >> + else >> + wake_up_klogd(); > > Nit: I would add an empty line here. Or I would move this up into the > previous if (in_sched()) condition.
Empty line is ok. I do not want to move it up because the above condition gets more complicated later. IMHO a simple if/else for specifying what the irq_work should do is the most straight forward here.
>> @@ -3811,11 +3814,30 @@ static void __wake_up_klogd(int val) >> preempt_enable(); >> } >> >> +/** >> + * wake_up_klogd - Wake kernel logging daemon >> + * >> + * Use this function when new records have been added to the ringbuffer >> + * and the console printing for those records is handled elsewhere. In > > "elsewhere" is ambiguous. I would write: > > "and the console printing for those records maybe handled in this context".
The reason for using the word "elsewhere" is because in later patches it is also the printing threads that can handle it. I can change it to "this context" for this patch, but then after adding threads I will need to adjust the comment again. How about:
"and the console printing for those records should not be handled by the irq_work context because another context will handle it."
> Anyway, I like this change.
Thanks.
John
| |