Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 18 May 2021 12:30:01 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Christoph Hellwig <hch@lst.de>,
        Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH v10 18/33] mm/filemap: Add folio_unlock
Message-ID: <YKOlOfFBrVu36g1q@casper.infradead.org>
References: <20210511214735.1836149-1-willy@infradead.org>
 <20210511214735.1836149-19-willy@infradead.org>
 <e3869efd-b4a3-93a2-b510-21142db91603@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e3869efd-b4a3-93a2-b510-21142db91603@suse.cz>
Precedence: bulk

On Tue, May 18, 2021 at 12:06:42PM +0200, Vlastimil Babka wrote:
> >  /**
> > - * unlock_page - unlock a locked page
> > - * @page: the page
> > + * folio_unlock - Unlock a locked folio.
> > + * @folio: The folio.
> >   *
> > - * Unlocks the page and wakes up sleepers in wait_on_page_locked().
> > - * Also wakes sleepers in wait_on_page_writeback() because the wakeup
> > - * mechanism between PageLocked pages and PageWriteback pages is shared.
> > - * But that's OK - sleepers in wait_on_page_writeback() just go back to sleep.
> > + * Unlocks the folio and wakes up any thread sleeping on the page lock.
> >   *
> > - * Note that this depends on PG_waiters being the sign bit in the byte
> > - * that contains PG_locked - thus the BUILD_BUG_ON(). That allows us to
> > - * clear the PG_locked bit and test PG_waiters at the same time fairly
> > - * portably (architectures that do LL/SC can test any bit, while x86 can
> > - * test the sign bit).
> 
> Was it necessary to remove the comments about wait_on_page_writeback() and
> PG_waiters etc?

I think so.  This kernel-doc is for the person who wants to understand
how to use the function, not for the person who wants to understand why
the function is written the way it is.  For that person, we have git log
messages and other comments dotted throughout, eg the comment on
clear_bit_unlock_is_negative_byte() in mm/filemap.c and the comment
on PG_waiters in include/linux/page-flags.h.

> > + * Context: May be called from interrupt or process context.  May not be
> > + * called from NMI context.
> 
> Where did the NMI part come from?

If you're in NMI context and call unlock_page() and the page has a
waiter on it, we call folio_wake_bit(), which calls spin_lock_irqsave()
on the wait_queue_head_t lock, which I believe cannot be done safely 
from NMI context (as the NMI may have interrupted us while holding
that lock).