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(-)
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;
--
2.46.0On 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
John Ogness writes: >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) Petr, what do you think about this? I remember when we discussed this before we talked about either determining state via `struct console` (which seems to turn out not to be feasible) or passing another argument. Do you prefer to have another argument or do the bit dance? Personally I prefer the simpler solution with more arguments instead of bit stuffing if we have to go this way, but I'm up for whichever sounds good to you.
On Wed 2024-11-20 04:17:51, Chris Down wrote:
> John Ogness writes:
> > 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)
>
> Petr, what do you think about this? I remember when we discussed this before
> we talked about either determining state via `struct console` (which seems
> to turn out not to be feasible) or passing another argument.
>
> Do you prefer to have another argument or do the bit dance?
>
> Personally I prefer the simpler solution with more arguments instead of bit
> stuffing if we have to go this way, but I'm up for whichever sounds good to
> you.
Ah, I though that John's proposal was reasonable. But it is true that
the meaning of @supress_level is not clear.
I see two possibilities:
1. printk_get_next_message() and console_emit_next_record()
could pass con->level.
But then we would need to create the extra value for devkmsg_read().
2. printk_get_next_message() and console_emit_next_record()
could pass console_effective_loglevel().
devkmsg_read() could pass CONSOLE_LOGLEVEL_MOTORMOUTH.
Sigh, it seems that any solution is hairy, including the one which
passed @con.
I personally think that the 2nd variant, passing the effective
loglevel, is least ugly. I am just not sure about a good name
for the parameter. What about?
bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
bool is_extended, int con_eff_level);
Note that this would require passing the effective loglevel also
to suppress_message_printing() so that we would get:
static bool suppress_message_printing(int level, int con_eff_level)
{
return level >= con_eff_level;
}
Best Regards,
Petr
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
© 2016 - 2024 Red Hat, Inc.