Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Matthew Wilcox <willy@infradead.org>]

On Thu, Mar 26, 2020 at 10:11:14AM -0700, Christoph Hellwig wrote:
> On Thu, Mar 26, 2020 at 05:24:29AM -0700, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > By moving PG_writeback down into the low bits of the page flags, we can
> > use clear_bit_unlock_is_negative_byte() for writeback as well as the
> > lock bit.  wake_up_page() then has no more callers.  Given the other
> > code being executed between the clear and the test, this is not going
> > to be as dramatic a win as it was for PageLocked, but symmetry between
> > the two is nice and lets us remove some code.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  include/linux/page-flags.h |  6 +++---
> >  mm/filemap.c               | 19 ++++++-------------
> >  mm/page-writeback.c        | 37 ++++++++++++++++++++-----------------
> >  3 files changed, 29 insertions(+), 33 deletions(-)
> > 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 222f6f7b2bb3..96c7d220c8cf 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -103,13 +103,14 @@
> >   */
> >  enum pageflags {
> >  	PG_locked,		/* Page is locked. Don't touch. */
> > +	PG_writeback,		/* Page is under writeback */
> 
> Do we need a comment why these need to be in the low bits?

There's some nice build time assertions that they are, next to all the
documentation about why and how it all works:

@@ -1266,6 +1259,7 @@ EXPORT_SYMBOL_GPL(add_page_wait_queue);
 void unlock_page(struct page *page)
 {
        BUILD_BUG_ON(PG_waiters != 7);
+       BUILD_BUG_ON(PG_locked > 7);
        page = compound_head(page);
        VM_BUG_ON_PAGE(!PageLocked(page), page);
        if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
@@ -1279,23 +1273,22 @@ EXPORT_SYMBOL(unlock_page);
  */
 void end_page_writeback(struct page *page)
 {
+       BUILD_BUG_ON(PG_writeback > 7);

so if anyone moves them out of the bottom byte, they'll see those
assertions fail and hopefully read this comment:

 * 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).

I'd expect any reviewer to notice a BUILD_BUG_ON() being removed and
query it if not explained in the changelog.