[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <184803.1617956064@warthog.procyon.org.uk>
Date: Fri, 09 Apr 2021 09:14:24 +0100
From: David Howells <dhowells@...hat.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: dhowells@...hat.com, Matthew Wilcox <willy@...radead.org>,
Jeff Layton <jlayton@...hat.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christoph Hellwig <hch@....de>,
Josef Bacik <josef@...icpanda.com>,
Linux-MM <linux-mm@...ck.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-cachefs@...hat.com, linux-afs@...ts.infradead.org,
"open list:NFS, SUNRPC, AND..." <linux-nfs@...r.kernel.org>,
CIFS <linux-cifs@...r.kernel.org>, ceph-devel@...r.kernel.org,
v9fs-developer@...ts.sourceforge.net,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] mm: Split page_has_private() in two to better handle PG_private_2
Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> > #define PAGE_FLAGS_PRIVATE \
> > (1UL << PG_private | 1UL << PG_private_2)
>
> I think this should be re-named to be PAGE_FLAGS_CLEANUP, because I
> don't think it makes any other sense to "combine" the two PG_private*
> bits any more. No?
Sure. Do we even want it still, or should I just fold it into
page_needs_cleanup()? It seems to be the only place it's used.
> > +static inline int page_private_count(struct page *page)
> > +{
> > + return test_bit(PG_private, &page->flags) ? 1 : 0;
> > +}
>
> Why is this open-coding the bit test, rather than just doing
>
> return PagePrivate(page) ? 1 : 0;
>
> instead? In fact, since test_bit() _should_ return a 'bool', I think even just
>
> return PagePrivate(page);
Sorry, yes, it should be that. I was looking at transforming the "1 <<
PG_private" and completely overlooked that this should be PagePrivate().
> should work and give the same result, but I could imagine that some
> architecture version of "test_bit()" might return some other non-zero
> value (although honestly, I think that should be fixed if so).
Yeah. I seem to recall that test_bit() on some arches used to return the
datum just with the other bits masked off, but I may be misremembering.
In asm-generic/bitops/non-atomic.h:
static inline int test_bit(int nr, const volatile unsigned long *addr)
{
return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
}
should perhaps return bool?
I wonder, should:
static __always_inline int PageTail(struct page *page)
static __always_inline int PageCompound(struct page *page)
static __always_inline int Page##uname(struct page *page)
static __always_inline int TestSetPage##uname(struct page *page)
static __always_inline int TestClearPage##uname(struct page *page)
also all return bool?
David
Powered by blists - more mailing lists