Date:   Thu, 10 Mar 2022 15:34:25 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v1 11/13] printk: reimplement console_lock for
 proper kthread support

On Wed 2022-03-09 15:02:07, John Ogness wrote:
> On 2022-02-23, Petr Mladek <pmladek@...e.com> wrote:
> >> +/*
> >> + * A variant of console_trylock() that allows specifying if the context may
> >> + * sleep. If yes, a trylock on @console_sem is attempted and if successful,
> >> + * the threaded printers are paused. This is important to ensure that
> >> + * sleepable contexts do not become involved in console_lock handovers and
> >> + * will call cond_resched() during the printing loop.
> >> + */
> >> +static int console_trylock_sched(bool may_schedule)
> >> +{
> >> +	if (!may_schedule)
> >> +		return console_trylock();
> >> +
> >> +	might_sleep();
> >> +
> >> +	if (down_trylock_console_sem())
> >> +		return 0;
> >> +	if (console_suspended) {
> >> +		up_console_sem();
> >> +		return 0;
> >> +	}
> >> +	pause_all_consoles();
> >
> > This is weird. Any trylock function should be fast and non-blocking.
> > But pause_all_consoles() uses mutex_lock().
> >
> > My expectation would be that console_trylock_sched() behaves
> > excatly the same as console_trylock() except that it will
> > set console_may_schedule by the given parameter.
> >
> > I would do it the other way. Rename console_trylock() and
> > implement:
> >
> > int console_trylock(void)
> > {
> > 	return console_trylock_sched(false);
> > }
> >
> > LATER: I got it. It is used by console_trylock_sched() called
> >        in console_unlock() when "do_cond_resched == true". In this
> >        case, the trylock might wait for the mutexes. It will prevent
> >        transfering console_lock from schedulable to atomic context
> >        by the check in console_emit_next_record().
> 
> Yes!
> 
> >        Hmm, I would still prefer to keep console_trylock_sched()
> >        behavior sane: non-blocking in all situations. It means
> >        that we actually do not need it and console_trylock()
> >        is enough.
> >
> >        It will allow to steal console_lock() from schedulable
> >        context. But it is not a regression. And it is only
> >        a corner case when console_unlock() re-takes the semaphore
> >        after releasing it.
> 
> A console waiter must not wait on a schedulable context. The console
> waiter is burning the CPU waiting for a transfer. If the console owner
> gets scheduled away while still holding the console lock, that is bad.

There must be a confusion. Of course, preemption must be disabled
when the current console_lock() owner allows to take over the lock.
It is currently achieved by disabling interrupts:

	printk_safe_enter_irqsave(flags);
	console_lock_spinning_enable();

	call_console_driver(con, write_text, len, dropped_text);

	*handover = console_lock_spinning_disable_and_check();
	printk_safe_exit_irqrestore(flags);


I wanted to say that it is safe even when the preemption is otherwise
enabled around this code.

Also I wanted to say that it is not ideal when the current owner
is called with preemption enabled and the new owner will continue
handling consoles with preemption disabled. But the original code
worked this way.

It would be nice to avoid moving the lock from a preemptive context
to non-preemptive one. But I would prefer to do it separately
because the proposed console_trylock_sched() is really controversial.
IMHO, it is not worth delaying this patchset because of this.


> >        We could do the same optimization in console_unlock() by
> >        calling console_emit_next_record() with NULL handover pointer
> >        when do_cond_resched == true. But we should do it
> >        as a separate patch later.
> 
> It is not an optimization, it is needed. Passing a NULL handover pointer
> when do_cond_resched == true would handle it correctly, but this feels
> like a workaround to me.
>
> The reason for adding console_trylock_sched() is because a context that
> previously acquired the console lock via console_lock() wants to try to
> reacquire it. If it reacquires the console lock using the kthread
> mutexes, the locking scenario returns to the same as it was
> before... all kthreads are blocked via their mutex.
> 
> You are suggesting that a console_lock() context later tries to
> reacquire the console lock, but using the console_trylock() method
> (atomic counter) and keeping console_may_schedule=1.
> 
> IMHO, _this_ is a weird variant that requires passing in a NULL handover
> pointer as a workaround. It introduces a third locking scenario where a
> schedulable context is using functions created for atomic use.
>
> Also, as I mentioned in the percpu thread [0], I think we need to avoid
> console_trylock() usage in schedulable contexts. Functions need to be
> aware in what contexts they are running and call the appropriate
> functions for it.

Let's put the NULL parameter aside. It is an implementation detail.
It can be handled by another parameter or so.

IMHO, the main disagreement is:

   + I do not like the proposed console_trylock_sched() API because
     it uses mutex_lock().

     From my POV, it can obviously cause deadlock. It is something
     that nobody would expect from any *_trylock() API. IMHO,
     it is a ticking bomb.


   + You do not like that console_lock()/console_unlock() might
     re-take the lock using another approach (atomic counter
     vs. con->flags.

     From your POV, using non-blocking console_trylock() in
     console_unlock() would be 3rd locking scheme. Also you say that
     that the caller should know the context and use the appropriate
     function.


Let me try to persuade you ;-)

1. Regarding the 3rd locking scheme:

   It is exactly the opposite from my POV. console_trylock_sched()
   looks like a 3rd locking variant to me. We would have blocking
   console_lock(), non-blocking console_trylock(), and semi-blocking
   console_trylock_sched().

   And it is even worse. console_trylock_sched() behaves differently
   according to the parameter. It makes is even more hard to
   understand the behavior.


2. Regarding using appropriate function:

   Let's see how it looks in the code:

       printk()
           preempt_disable()

           console_trylock()
           console_unlock()
               // printing
               up_console_sem()
               retry = console_trylock_sched();

   It feels like using sleeping lock in a non-preemptive context.
   OK, we could make it more obvious:

   console_unlock()
   {
	  [...]
	  if (may_schedule)
	      retry = console_trylock_sched();
	  else
	      retry = console_trylock();
   }

   But it is yet another complexity in console_unlock().
   Is it really necessary?


3. Implementation detail:

  From my POV, the way how console_lock()/console_trylock() takes
  the lock is an implementation detail.

  console_lock() uses con->flags because it can and actually must
  wait/sleep. console_trylock() uses the atomic counter because
  it must not block.

  We hide this implementation detail in __console_unlock(). It
  releases the lock the same way as it was taken.


4. may_schedule vs. handover

  The information whether console_unlock() may schedule is stored in
  @console_may_schedule variable. console_lock() allows scheduling
  and console_trylock() disables scheduling.

  console_emit_next_record() allows handover according to
  the atomic counter. It is set by the way how console_lock
  was taken. console_lock() disables handover. console_trylock()
  enables handover.

  Now, console_trylock_sched() is needed to re-take the lock
  the same way as it was taken originally. Otherwise,
  the atomic counter will be used even when scheduling was enabled.
  As a result, console_emit_next_record() would work differently.

  By other word, console_trylock_sched() is used to encode
  @console_may_schedule into @console_lock_count.

  It looks like a hack similar to passing NULL *handover. But from my POV,
  it is much worse. The price is console_trylock_sched() API with
  very strange behavior. It is super tricky. It took me hours to
  understand the motivation and behavior.

  From, my POV, the most clear solution is to pass "may_schedule"
  parameter to console_emit_next_record().


Does it make sense, please?
Do I miss anything, please?

Best Regards,
Petr

Powered by blists - more mailing lists