Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
