| Message ID | 20241105-printk-loud-con-v2-0-bd3ecdf7b0e4@suse.com |
|---|---|
| Headers | show |
| Series | printk: Add force_con printk flag to not suppress sysrq header msgs | expand |
On 2024-11-05, Marcos Paulo de Souza <mpdesouza@suse.com> wrote: > @@ -2947,6 +2953,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq, > struct printk_info info; > struct printk_record r; > size_t len = 0; > + bool force_con; > > /* > * Formatting extended messages requires a separate buffer, so use the > @@ -2965,9 +2972,13 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq, > > pmsg->seq = r.info->seq; > pmsg->dropped = r.info->seq - seq; > + force_con = r.info->flags & LOG_FORCE_CON; > > - /* Skip record that has level above the console loglevel. */ > - if (may_suppress && suppress_message_printing(r.info->level)) > + /* > + * Skip records that are not forced to be printed on consoles and that > + * has level above the console loglevel. > + */ > + if (!force_con && may_suppress && suppress_message_printing(r.info->level)) > goto out; Rather than adding a new local variable, setting it, and expanding the condition, it might be cleaner to just update @may_suppress before the condition check? /* Records forced to be printed on consoles must not be skipped. */ may_suppress &= !(r.info->flags & LOG_FORCE_CON); Feel free to ignore this suggestion if you think having an extra variable is easier to follow. With or without suggested change: Reviewed-by: John Ogness <john.ogness@linutronix.de>
On Tue, 2024-11-05 at 22:40 +0106, John Ogness wrote: > On 2024-11-05, Marcos Paulo de Souza <mpdesouza@suse.com> wrote: > > @@ -2947,6 +2953,7 @@ bool printk_get_next_message(struct > > printk_message *pmsg, u64 seq, > > struct printk_info info; > > struct printk_record r; > > size_t len = 0; > > + bool force_con; > > > > /* > > * Formatting extended messages requires a separate > > buffer, so use the > > @@ -2965,9 +2972,13 @@ bool printk_get_next_message(struct > > printk_message *pmsg, u64 seq, > > > > pmsg->seq = r.info->seq; > > pmsg->dropped = r.info->seq - seq; > > + force_con = r.info->flags & LOG_FORCE_CON; > > > > - /* Skip record that has level above the console loglevel. > > */ > > - if (may_suppress && suppress_message_printing(r.info- > > >level)) > > + /* > > + * Skip records that are not forced to be printed on > > consoles and that > > + * has level above the console loglevel. > > + */ > > + if (!force_con && may_suppress && > > suppress_message_printing(r.info->level)) > > goto out; > > Rather than adding a new local variable, setting it, and expanding > the > condition, it might be cleaner to just update @may_suppress before > the > condition check? > > /* Records forced to be printed on consoles must not be > skipped. */ > may_suppress &= !(r.info->flags & LOG_FORCE_CON); Well, your suggestion seems clever than what I did :) IHMO, I would prefer the new variable as it's easier to read (for me at least), but I can change if you think it's better.. > > Feel free to ignore this suggestion if you think having an extra > variable is easier to follow. > > With or without suggested change: > > Reviewed-by: John Ogness <john.ogness@linutronix.de> Thanks John!
On Tue 2024-11-05 16:45:07, Marcos Paulo de Souza wrote: > Hello, > > This is the second version of the patchset. It now addresses comments > from John and Petr, while also mentioning that the current work solves > one issue on handle_sysrq when the printk messages are deferred. > > The original cover-letter in is the v1. > > Please review! > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > Changes in v2: > - Mentioned that it fixes a bug related to loglevel= dance (suggested by John) > - Changed to loud_con to FORCE_CON (John, Petr) > - Don't skip printk delay if FORCE_CON is specified (John) > - Set FORCE_CON when LOG_CONT is handled (John) > - Changed force_con from a per-CPU variable to a global variable because > we can't disable migration on the callsites. (John, Petr) > - Used is_printk_force_console() on boot_delay_msec(), since it's used > when the message is stored, instead of setting is as an argument. > - Link to v1: https://lore.kernel.org/r/20241016-printk-loud-con-v1-0-065e4dad6632@suse.com > --- > Marcos Paulo de Souza (2): > printk: Introduce FORCE_CON flag > tty: sysrq: Use printk_force_console context on __handle_sysrq The patchset looks ready for linux-next from my POV. I am going to push it there tomorrow or on Monday unless anyone complains. There was some bike-shedding about the code style in the reviews. But the proposals did not look like a big win. I think that it is not worth a respin. Best Regards, Petr
On Thu, Nov 07, 2024 at 04:57:36PM +0100, Petr Mladek wrote: > On Tue 2024-11-05 16:45:07, Marcos Paulo de Souza wrote: > > Hello, > > > > This is the second version of the patchset. It now addresses comments > > from John and Petr, while also mentioning that the current work solves > > one issue on handle_sysrq when the printk messages are deferred. > > > > The original cover-letter in is the v1. > > > > Please review! > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > --- > > Changes in v2: > > - Mentioned that it fixes a bug related to loglevel= dance (suggested by John) > > - Changed to loud_con to FORCE_CON (John, Petr) > > - Don't skip printk delay if FORCE_CON is specified (John) > > - Set FORCE_CON when LOG_CONT is handled (John) > > - Changed force_con from a per-CPU variable to a global variable because > > we can't disable migration on the callsites. (John, Petr) > > - Used is_printk_force_console() on boot_delay_msec(), since it's used > > when the message is stored, instead of setting is as an argument. > > - Link to v1: https://lore.kernel.org/r/20241016-printk-loud-con-v1-0-065e4dad6632@suse.com > > --- > > Marcos Paulo de Souza (2): > > printk: Introduce FORCE_CON flag > > tty: sysrq: Use printk_force_console context on __handle_sysrq > > The patchset looks ready for linux-next from my POV. I am going to > push it there tomorrow or on Monday unless anyone complains. > > There was some bike-shedding about the code style in the reviews. > But the proposals did not look like a big win. I think that it > is not worth a respin. No objection from me! Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Thu 2024-11-07 16:57:40, Petr Mladek wrote: > On Tue 2024-11-05 16:45:07, Marcos Paulo de Souza wrote: > > Hello, > > > > This is the second version of the patchset. It now addresses comments > > from John and Petr, while also mentioning that the current work solves > > one issue on handle_sysrq when the printk messages are deferred. > > > > The original cover-letter in is the v1. > > > > Please review! > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > --- > > Changes in v2: > > - Mentioned that it fixes a bug related to loglevel= dance (suggested by John) > > - Changed to loud_con to FORCE_CON (John, Petr) > > - Don't skip printk delay if FORCE_CON is specified (John) > > - Set FORCE_CON when LOG_CONT is handled (John) > > - Changed force_con from a per-CPU variable to a global variable because > > we can't disable migration on the callsites. (John, Petr) > > - Used is_printk_force_console() on boot_delay_msec(), since it's used > > when the message is stored, instead of setting is as an argument. > > - Link to v1: https://lore.kernel.org/r/20241016-printk-loud-con-v1-0-065e4dad6632@suse.com > > --- > > Marcos Paulo de Souza (2): > > printk: Introduce FORCE_CON flag > > tty: sysrq: Use printk_force_console context on __handle_sysrq > > The patchset looks ready for linux-next from my POV. I am going to > push it there tomorrow or on Monday unless anyone complains. > > There was some bike-shedding about the code style in the reviews. > But the proposals did not look like a big win. I think that it > is not worth a respin. JFYI, the patchset has been committed into printk/linux.git, branch for-6.13-force-console. Best Regards, Petr
Hello, This is the second version of the patchset. It now addresses comments from John and Petr, while also mentioning that the current work solves one issue on handle_sysrq when the printk messages are deferred. The original cover-letter in is the v1. Please review! Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- Changes in v2: - Mentioned that it fixes a bug related to loglevel= dance (suggested by John) - Changed to loud_con to FORCE_CON (John, Petr) - Don't skip printk delay if FORCE_CON is specified (John) - Set FORCE_CON when LOG_CONT is handled (John) - Changed force_con from a per-CPU variable to a global variable because we can't disable migration on the callsites. (John, Petr) - Used is_printk_force_console() on boot_delay_msec(), since it's used when the message is stored, instead of setting is as an argument. - Link to v1: https://lore.kernel.org/r/20241016-printk-loud-con-v1-0-065e4dad6632@suse.com --- Marcos Paulo de Souza (2): printk: Introduce FORCE_CON flag tty: sysrq: Use printk_force_console context on __handle_sysrq drivers/tty/sysrq.c | 18 ++++++++---------- include/linux/printk.h | 3 +++ kernel/printk/internal.h | 3 +++ kernel/printk/printk.c | 21 ++++++++++++++++----- kernel/printk/printk_safe.c | 18 ++++++++++++++++++ 5 files changed, 48 insertions(+), 15 deletions(-) --- base-commit: d9f4b813ae5c9bcdc9fcbaa1f4c9829bdc272532 change-id: 20241016-printk-loud-con-03b393d5f6fb Best regards,