Re: [PATCH v2 2/3] printk: console_flush_one_record() code cleanup
From: Andrew Murray
Date: Wed Oct 01 2025 - 12:26:49 EST
On Wed, 1 Oct 2025 at 10:53, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
>
> On 2025-09-30, Andrew Murray <amurray@xxxxxxxxxxxxxxxxxxxx> wrote:
> >> On 2025-09-27, Andrew Murray <amurray@xxxxxxxxxxxxxxxxxxxx> wrote:
> >> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> > index 060d4919de320fe21fd7aca73ba497e27c4ff334..e2c1cacdb4164489c60fe38f1e2837eb838107d6 100644
> >> > --- a/kernel/printk/printk.c
> >> > +++ b/kernel/printk/printk.c
> >> > @@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> >> > bool any_progress;
> >> > int cookie;
> >> >
> >> > + *any_usable = false;
> >>
> >> Since it is expected that @next_seq and @handover are initialized by
> >> their callers (if their callers are interested in the values), then I
> >> would expect @any_usable to be initialized by the
> >> caller. console_flush_one_record() never reads this variable.
> >
> > Yes, that's correct. Perhaps the comments for the parameters should
> > indicate otherwise?
>
> They should clarify. For example, @next_seq is "valid only when this
> function returns true" but @handover validitiy is not specified and is
> also not related to the return value.
>
> I would like to see the helper function provide clear usage. If the
> caller is expected to initialize the parameters (which IMHO it should be
> the case for all of the pointer parameters), then that should be
> specified.
OK.
>
> >> > @@ -3280,21 +3284,16 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> >> > */
> >> > static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
> >> > {
> >> > - bool any_usable = false;
> >> > + bool any_usable;
> >>
> >> Since console_flush_all() does read @any_usable, I would expect it to
> >> initialize @any_usable. So I would not remove this definition initialization.
> >
> > Prior to this series, console_flush_all would set any_usable to false.
> > It would be set to true if at any point a usable console is found,
> > that value would be returned, or otherwise false if handover or panic.
> > When the first patch split out this function, any_usable was kept as
> > it was, leading to any_usable being true, even if a handover or panic
> > had happened (hence additional checks needed, which are removed in
> > this patch).
> >
> > By setting any_usable at the start of flush_one_record, it allows for
> > any_usable to revert back to false, in the case where a once usable
> > console is no longer usable. Thus representing the situation for the
> > last record printed. It also makes console_flush_one_record easier to
> > understand, as the any_usable flag will always be set, rather than
> > only changing from false to true.
>
> OK. But then just have console_flush_all() set @any_usable in the loop:
>
> static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
> {
> bool any_progress;
> bool any_usable;
>
> *next_seq = 0;
> *handover = false;
>
> do {
> any_usable = false;
> any_progress = console_flush_one_record(do_cond_resched, next_seq,
> handover, &any_usable);
> } while (any_progress);
>
> return any_usable;
> }
Yes, that seems like common sense, I have no idea why I didn't think of that :|
>
> > Alternatively, it may be possible for console_flush_one_record to
> > return any_usable, thus dropping it as an argument and removing the
> > return of any_progress. Instead the caller could keep calling
> > console_flush_one_record until it returns false or until next_seq
> > stops increasing? Thus semantically, the return value of
> > console_flush_one_record tells you that nothing bad happened and you
> > can call it again, and the benefit of calling it again depends on if
> > progress is being made (as determined by the caller through the
> > existing seq argument).
>
> Sorry, I could not follow how this would work. It sounds like a
> simplification. If you can make it work, go for it.
Against my patches, something like this:
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2c9b9383df76..f38295cc3645 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3186,16 +3186,13 @@ static inline void
printk_kthreads_check_locked(void) { }
*
* Requires the console_lock.
*/
-static bool console_flush_one_record(bool do_cond_resched, u64
*next_seq, bool *handover,
- bool *any_usable)
+static bool console_flush_one_record(bool do_cond_resched, u64
*next_seq, bool *handover)
{
struct console_flush_type ft;
struct console *con;
- bool any_progress;
int cookie;
- *any_usable = false;
- any_progress = false;
+ bool any_usable = false;
printk_get_console_flush_type(&ft);
@@ -3215,7 +3212,7 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
if (!console_is_usable(con, flags, !do_cond_resched))
continue;
- *any_usable = true;
+ any_usable = true;
if (flags & CON_NBCON) {
progress = nbcon_legacy_emit_next_record(con,
handover, cookie,
@@ -3239,7 +3236,6 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
if (!progress)
continue;
- any_progress = true;
/* Allow panic_cpu to take over the consoles safely. */
if (other_cpu_in_panic())
@@ -3250,12 +3246,11 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
}
console_srcu_read_unlock(cookie);
- return any_progress;
+ return any_usable;
unusable_srcu:
console_srcu_read_unlock(cookie);
unusable:
- *any_usable = false;
return false;
}
@@ -3286,15 +3281,15 @@ static bool console_flush_all(bool
do_cond_resched, u64 *next_seq, bool *handove
{
bool any_usable;
bool any_progress;
+ u64 last_seq;
*next_seq = 0;
*handover = false;
do {
- any_progress =
console_flush_one_record(do_cond_resched, next_seq,
- handover, &any_usable);
-
- } while (any_progress);
+ last_seq = *next_seq;
+ any_usable = console_flush_one_record(do_cond_resched,
next_seq, handover);
+ } while (*next_seq > last_seq);
return any_usable;
}
@@ -3674,21 +3669,20 @@ static int legacy_kthread_func(void *unused)
wait_event_interruptible(legacy_wait,
legacy_kthread_should_wakeup());
+ u64 last_seq, next_seq = 0;
do {
- bool any_usable;
bool handover = false;
- u64 next_seq;
if (kthread_should_stop())
return 0;
console_lock();
- any_progress = console_flush_one_record(true, &next_seq,
- &handover, &any_usable);
+ last_seq = next_seq;
+ console_flush_one_record(true, &next_seq, &handover);
if (!handover)
__console_unlock();
- } while (any_progress);
+ } while (next_seq > last_seq);
goto wait_for_event;
}
--
2.34.1
This also has the benefit of removing code (and more could be removed
if the progress variable in console_flush_one_record is removed - i.e.
we could unconditionally bail on panic and resched each time).
Thanks,
Andrew Murray