| Message ID | ba47efbe432cf33cb358a027a2266296e2cfe89e.1730133890.git.chris@chrisdown.name |
|---|---|
| State | New |
| Headers | show |
| Series | printk: console: Per-console loglevels | expand |
On Mon 2024-10-28 16:45:34, Chris Down wrote: > In preparation for supporting per-console loglevels, modify > printk_get_next_message() to accept the console itself instead of > individual arguments that mimic its fields. As part of the upcoming > per-console loglevel support we need the console object here anyway, so > it makes sense to amortise this now. > > devkmsg_read() has special behaviour here, but all other consoles follow > the same patterns and can have their extension/suppression states > determined from their struct console. > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2925,20 +2925,19 @@ void console_prepend_replay(struct printk_message *pmsg) > * @pmsg will contain the formatted result. @pmsg->pbufs must point to a > * struct printk_buffers. > * > + * @con is the console in question. Only @con->flags and @con->level are > + * guaranteed to be valid at this point. Note especially well that con->seq is > + * not yet guaranteed to be consistent with @seq. @con->level does not exist at this point. But more importantly, the read of @con->flags and @con->level is sychronized only by SRCU read lock. It means that the word "valid" is a bit misleading. SRCU just guarantees that the struct console can't disappear. I would write something like: <proposal> * @con might point to the console where the read message will be emitted. * It is used to determine the format of the message and whether it would get * suppressed. The sequence number stored in the struct console is updated * by the caller depending on whether the emission succeeds. * * @con might be NULL when the message is used for another purpose, * for example, devkmsg. </proposal> > + * > * @seq is the record to read and format. If it is not available, the next > * valid record is read. > * > - * @is_extended specifies if the message should be formatted for extended > - * console output. > - * > - * @may_supress specifies if records may be skipped based on loglevel. > - * > * Returns false if no record is available. Otherwise true and all fields > * of @pmsg are valid. (See the documentation of struct printk_message > * for information about the @pmsg fields.) > */ > -bool printk_get_next_message(struct printk_message *pmsg, u64 seq, > - bool is_extended, bool may_suppress) > +bool printk_get_next_message(struct printk_message *pmsg, struct console *con, > + u64 seq) > { > struct printk_buffers *pbufs = pmsg->pbufs; > const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf); > @@ -2948,6 +2947,14 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq, > struct printk_info info; > struct printk_record r; > size_t len = 0; > + bool is_extended; > + > + if (con) { > + is_extended = console_srcu_read_flags(con) & CON_EXTENDED; I guess that we would need to implement similar API also for reading the per-console loglevel. I mean that we should check the SRCU lock is held. And describe the potential data_race() if the value might be modified by a sysfs interface in parallel. Let's discuss this in the patch adding the read. I mention it here rather just as a mental note. The proposed comment says that it will be synchronized using SRCU. We need to make sure that it will be valid also for the loglevel stuff. > + } else { > + /* Used only by devkmsg_read(). */ > + is_extended = true; > + } > > /* > * Formatting extended messages requires a separate buffer, so use the Anyway, the change looks fine. With the updated comment: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On 2024-10-28, Chris Down <chris@chrisdown.name> wrote: > In preparation for supporting per-console loglevels, modify > printk_get_next_message() to accept the console itself instead of > individual arguments that mimic its fields. Sorry, this is not allowed. printk_get_next_message() was created specifically to locklessly retrieve and format arbitrary records. It must never be tied to a console because it has nothing to do with consoles (as can bee seen with the devkmsg_read() hack you added in the function). I recommend adding an extra argument specifying the level. The extra argument would be redundant if may_suppress=false. So perhaps as an alternative change "bool may_suppress" to "u32 supress_level". The loglevels are only 3 bits. So you could easily define a special value NO_SUPPRESS to represent the may_suppress=false case. #define NO_SUPPRESS BIT(31) bool printk_get_next_message(struct printk_message *pmsg, u64 seq, bool is_extended, u32 suppress_level); Then in devkmsg_read(): printk_get_next_message(&pmsg, atomic64_read(&user->seq), true, NO_SUPRRESS) John Ogness
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h index 3fcb48502adb..58ad209e0310 100644 --- a/kernel/printk/internal.h +++ b/kernel/printk/internal.h @@ -328,8 +328,8 @@ struct printk_message { }; bool other_cpu_in_panic(void); -bool printk_get_next_message(struct printk_message *pmsg, u64 seq, - bool is_extended, bool may_supress); +bool printk_get_next_message(struct printk_message *pmsg, struct console *con, + u64 seq); #ifdef CONFIG_PRINTK void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped); diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index fd12efcc4aed..5ae1155c34d3 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -974,7 +974,7 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a if (!nbcon_context_enter_unsafe(ctxt)) return false; - ctxt->backlog = printk_get_next_message(&pmsg, ctxt->seq, is_extended, true); + ctxt->backlog = printk_get_next_message(&pmsg, con, ctxt->seq); if (!ctxt->backlog) return nbcon_context_exit_unsafe(ctxt); diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index d6159f1c5b29..dfe7011b863a 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -833,7 +833,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf, if (ret) return ret; - if (!printk_get_next_message(&pmsg, atomic64_read(&user->seq), true, false)) { + if (!printk_get_next_message(&pmsg, NULL, atomic64_read(&user->seq))) { if (file->f_flags & O_NONBLOCK) { ret = -EAGAIN; goto out; @@ -850,8 +850,8 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf, * This pairs with __wake_up_klogd:A. */ ret = wait_event_interruptible(log_wait, - printk_get_next_message(&pmsg, atomic64_read(&user->seq), true, - false)); /* LMM(devkmsg_read:A) */ + printk_get_next_message(&pmsg, NULL, + atomic64_read(&user->seq))); /* LMM(devkmsg_read:A) */ if (ret) goto out; } @@ -2925,20 +2925,19 @@ void console_prepend_replay(struct printk_message *pmsg) * @pmsg will contain the formatted result. @pmsg->pbufs must point to a * struct printk_buffers. * + * @con is the console in question. Only @con->flags and @con->level are + * guaranteed to be valid at this point. Note especially well that con->seq is + * not yet guaranteed to be consistent with @seq. + * * @seq is the record to read and format. If it is not available, the next * valid record is read. * - * @is_extended specifies if the message should be formatted for extended - * console output. - * - * @may_supress specifies if records may be skipped based on loglevel. - * * Returns false if no record is available. Otherwise true and all fields * of @pmsg are valid. (See the documentation of struct printk_message * for information about the @pmsg fields.) */ -bool printk_get_next_message(struct printk_message *pmsg, u64 seq, - bool is_extended, bool may_suppress) +bool printk_get_next_message(struct printk_message *pmsg, struct console *con, + u64 seq) { struct printk_buffers *pbufs = pmsg->pbufs; const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf); @@ -2948,6 +2947,14 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq, struct printk_info info; struct printk_record r; size_t len = 0; + bool is_extended; + + if (con) { + is_extended = console_srcu_read_flags(con) & CON_EXTENDED; + } else { + /* Used only by devkmsg_read(). */ + is_extended = true; + } /* * Formatting extended messages requires a separate buffer, so use the @@ -2967,8 +2974,8 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq, pmsg->seq = r.info->seq; pmsg->dropped = r.info->seq - seq; - /* Skip record that has level above the console loglevel. */ - if (may_suppress && suppress_message_printing(r.info->level)) + /* Never suppress when used in devkmsg_read() */ + if (con && suppress_message_printing(r.info->level)) goto out; if (is_extended) { @@ -3044,7 +3051,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co *handover = false; - if (!printk_get_next_message(&pmsg, con->seq, is_extended, true)) + if (!printk_get_next_message(&pmsg, con, con->seq)) return false; con->dropped += pmsg.dropped;
In preparation for supporting per-console loglevels, modify printk_get_next_message() to accept the console itself instead of individual arguments that mimic its fields. As part of the upcoming per-console loglevel support we need the console object here anyway, so it makes sense to amortise this now. devkmsg_read() has special behaviour here, but all other consoles follow the same patterns and can have their extension/suppression states determined from their struct console. Signed-off-by: Chris Down <chris@chrisdown.name> --- kernel/printk/internal.h | 4 ++-- kernel/printk/nbcon.c | 2 +- kernel/printk/printk.c | 33 ++++++++++++++++++++------------- 3 files changed, 23 insertions(+), 16 deletions(-)