Messages in this thread | | | Date | Fri, 31 Mar 2023 12:36:02 +0200 | From | Petr Mladek <> | Subject | semantic: Re: [PATCH printk v1 10/18] printk: nobkl: Add emit function and callback functions for atomic printing |
| |
On Thu 2023-03-02 21:02:10, John Ogness wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Implement an emit function for non-BKL consoles to output printk > messages. It utilizes the lockless printk_get_next_message() and > console_prepend_dropped() functions to retrieve/build the output > message. The emit function includes the required safety points to > check for handover/takeover and calls a new write_atomic callback > of the console driver to output the message. It also includes proper > handling for updating the non-BKL console sequence number. > > --- a/kernel/printk/printk_nobkl.c > +++ b/kernel/printk/printk_nobkl.c > +/** > + * cons_emit_record - Emit record in the acquired context > + * @wctxt: The write context that will be handed to the write function > + * > + * Returns: False if the operation was aborted (takeover or handover). > + * True otherwise > + * > + * When false is returned, the caller is not allowed to touch console state. > + * The console is owned by someone else. If the caller wants to print more > + * it has to reacquire the console first. > + * > + * When true is returned, @wctxt->ctxt.backlog indicates whether there are > + * still records pending in the ringbuffer,
This is inconsistent and a bit confusing. This seems to be the only function returning "true" when there is no pending output.
All the other functions cons_get_record(), console_emit_next_record(), and printk_get_next_message() return false in this case.
It has to distinguish 3 different return states anyway, same as console_emit_next_record(). I suggest to use the same semantic and distinguish "no pending records" and "handed over lock" via a "handover" flag. Or maybe the caller should just check if it still owns the lock.
> + */ > +static int __maybe_unused cons_emit_record(struct cons_write_context *wctxt) > +{ > + struct cons_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt); > + struct console *con = ctxt->console; > + bool done = false; > + > + /* > + * @con->dropped is not protected in case of hostile takeovers so > + * the update below is racy. Annotate it accordingly. > + */ > + ctxt->dropped = data_race(READ_ONCE(con->dropped)); > + > + /* Fill the output buffer with the next record */ > + ctxt->backlog = cons_get_record(wctxt); > + if (!ctxt->backlog) > + return true; > + > + /* Safety point. Don't touch state in case of takeover */ > + if (!console_can_proceed(wctxt)) > + return false; > + > + /* Counterpart to the read above */ > + WRITE_ONCE(con->dropped, ctxt->dropped); > + > + /* > + * In case of skipped records, Update sequence state in @con. > + */ > + if (!wctxt->outbuf) > + goto update; > + > + /* Tell the driver about potential unsafe state */ > + wctxt->unsafe = ctxt->state.unsafe; > + > + if (!ctxt->thread && con->write_atomic) {
I would expect this check in console_is_usable(), same as for legacy consoles.
And what is actually the difference between con->write_atomic() and con->write_thread(), where write_thread() is added later in 11th patch?
I guess that the motivation is that the kthread variant might sleep. But I do not see it described anywhere.
Do we really need two callbacks? I would expect that the code would be basically the same.
Maybe, the callback could call cond_resched() when running in kthread but this information might be passed via a flag.
Or is this a preparation for tty code where the implementation would be really different?
> + done = con->write_atomic(con, wctxt); > + } else { > + cons_release(ctxt); > + WARN_ON_ONCE(1); > + return false; > + }
Best Regards, Petr
| |