Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 14:03:31 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v2 27/38] printk: console_flush_all: use srcu
 console list iterator
Message-ID: <Y2kCI7pAiQBNefgi@alley>
References: <20221019145600.1282823-1-john.ogness@linutronix.de>
 <20221019145600.1282823-28-john.ogness@linutronix.de>
 <Y1f+Izra36qQ4ae6@alley>
 <87a6534osd.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87a6534osd.fsf@jogness.linutronix.de>
Precedence: bulk

On Mon 2022-11-07 01:06:02, John Ogness wrote:
> On 2022-10-25, Petr Mladek <pmladek@suse.com> wrote:
> >>   console_lock()
> >>   | mutex_acquire(&console_lock_dep_map)       <-- console lock
> >>   |
> >>   console_unlock()
> >>   | console_flush_all()
> >>   | | srcu_read_lock(&console_srcu)            <-- srcu lock
> >>   | | console_emit_next_record()
> >>   | | | console_lock_spinning_disable_and_check()
> >>   | | | | srcu_read_unlock(&console_srcu)      <-- srcu unlock
> >>   | | | | mutex_release(&console_lock_dep_map) <-- console unlock
> >>
> >> @@ -2819,12 +2827,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
> >>  				/* Extended consoles do not print "dropped messages". */
> >>  				progress = console_emit_next_record(con, &text[0],
> >>  								    &ext_text[0], NULL,
> >> -								    handover);
> >> +								    handover, cookie);
> >>  			} else {
> >>  				progress = console_emit_next_record(con, &text[0],
> >>  								    NULL, &dropped_text[0],
> >> -								    handover);
> >> +								    handover, cookie);
> >>  			}
> >> +
> >> +			/*
> >> +			 * If a handover has occurred, the SRCU read lock
> >> +			 * is already released.
> >> +			 */
> >>  			if (*handover)
> >>  				return false;
> >
> > Please, release the SRCU read lock here:
> >
> > 			if (*handover) {
> > 				console_srcu_read_unlock(cookie);
> > 				return false;
> > 			}
> >
> > The lock should be released in the same function where it was taken.
> > It does not require passing the cookie and looks more straightforward.
> 
> It looks more straight forward, but it is incorrect from a locking
> perspective.
> 
> The locking order was:
> 
> console_lock()
> console_srcu_read_lock()
> 
> But for a handover at this point in code, console_emit_next_record() has
> already released the console_lock (to the spinning context). The
> console_srcu_read_lock should have been released first.

Ah, I see. I should be read all the comments more carefully.
I do not know about any better solution. Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>


Note:

The complexity is caused by calling
mutex_release() in console_lock_spinning_disable_and_check() and
mutex_acquire() in console_trylock_spinning().

I wondered if we really need to do so. These functions actually do not
release or acquire the console_lock. But it seems that it is necessary
because lockdep is not able to track the lock when it was moved into
another process.

I even tried to replace mutex_acquire()/mutex_release() with
rwsem_acquire()/rwsem_release(). But it did not help. lockdep
still complained when I removed these calls from
the _spinning_() API. Sigh.

Best Regards,
Petr