Messages in this thread | | | From | David Howells <> | Subject | Re: [RFC][PATCH] mm: Split page_has_private() in two to better handle PG_private_2 | Date | Fri, 09 Apr 2021 09:14:24 +0100 |
| |
Linus Torvalds <torvalds@linux-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
| |