Messages in this thread | | | Date | Thu, 6 Apr 2023 11:46:36 +0200 | From | Petr Mladek <> | Subject | port lock: was: Re: [PATCH printk v1 11/18] printk: nobkl: Introduce printer threads |
| |
On Thu 2023-03-02 21:02:11, John Ogness wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Add the infrastructure to create a printer thread per console along > with the required thread function, which is takeover/handover aware.
> --- a/kernel/printk/printk_nobkl.c > +++ b/kernel/printk/printk_nobkl.c > +/** > + * cons_kthread_func - The printk thread function > + * @__console: Console to operate on > + */ > +static int cons_kthread_func(void *__console) > +{ > + struct console *con = __console; > + struct cons_write_context wctxt = { > + .ctxt.console = con, > + .ctxt.prio = CONS_PRIO_NORMAL, > + .ctxt.thread = 1, > + }; > + struct cons_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt); > + unsigned long flags; > + short con_flags; > + bool backlog; > + int cookie; > + int ret; > + > + for (;;) { > + atomic_inc(&con->kthread_waiting); > + > + /* > + * Provides a full memory barrier vs. cons_kthread_wake(). > + */ > + ret = rcuwait_wait_event(&con->rcuwait, > + cons_kthread_should_wakeup(con, ctxt), > + TASK_INTERRUPTIBLE); > + > + atomic_dec(&con->kthread_waiting); > + > + if (kthread_should_stop()) > + break; > + > + /* Wait was interrupted by a spurious signal, go back to sleep */ > + if (ret) > + continue; > + > + for (;;) { > + cookie = console_srcu_read_lock(); > + > + /* > + * Ensure this stays on the CPU to make handover and > + * takeover possible. > + */ > + if (con->port_lock) > + con->port_lock(con, true, &flags);
IMHO, we should use a more generic name. This should be a lock that provides full synchronization between con->write() and other operations on the device used by the console.
"port_lock" is specific for the serial consoles. IMHO, other consoles might use another lock. IMHO, tty uses "console_lock" internally for this purpose. netconsole seems to has "target_list_lock" that might possible have this purpose, s390 consoles are using sclp_con_lock, sclp_vt220_lock, or get_ccwdev_lock(raw->cdev).
Honestly, I expected that we could replace these locks by cons_acquire_lock(). I know that the new lock is special: sleeping, timeouting, allows hand-over by priorities.
But I think that we might implement cons_acquire_lock() that would always busy wait without any timeout. And use some "priority" that would never handover the lock a voluntary way at least not with a voluntary one. The only difference would be that it is sleeping. But it might be acceptable in many cases.
Using the new lock instead of port->lock would allow to remove the tricks with using spin_trylock() when oops_in_progress is set.
That said, I am not sure if this is possible without major changes. For example, in case of serial consoles, it would require touching the layer using port->lock.
Also it would requere 1:1 relation between struct console and the output device lock. I am not sure if it is always the case. On the other hand, adding some infrastructure for this 1:1 relationship would help to solve smooth transition from the boot to the real console driver.
OK, let's first define what the two locks are supposed to synchronize. My understanding is that this patchset uses them the following way:
+ The new lock (atomic_state) is used to serialize emiting messages between different write contexts. It replaces the functionality of console_lock.
It is a per-console sleeping lock, allows voluntary and hars hand-over using priorities and spinning with a timeout.
+ The port_lock is used to synchronize various operations of the console driver/device, like probe, init, exit, configuration update.
It is typically a per-console driver/device spin lock.
I guess that we would want to keep both locks:
+ it might help to do the rework manageable
+ the sleeping lock might complicate some operations; raw_spin_lock might be necessary at least on non-RT system.
Are there better names? What about?
+ emit_lock() or ctxt_lock() for the new special lock
+ device_lock() or driver_lock() as a generic name for the driver/device specific lock.
Sigh, the problem with the device_lock()/driver_lock() is that it might get confused with:
struct tty_driver *(*device)(struct console *co, int *index);
It would be really great to make it clear that this callback is about the connection to the tty layer. I would rename it to:
struct tty_driver *(*tty_drv)(struct console *co, int *index); or struct tty_driver *(*tty_driver)(struct console *co, int *index);
> + else > + migrate_disable(); > + > + /* > + * Try to acquire the console without attempting to > + * take over. If an atomic printer wants to hand > + * back to the thread it simply wakes it up. > + */ > + if (!cons_try_acquire(ctxt)) > + break; > +
Best Regards, Petr
| |