| Message ID | 20210817033032.76089-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1) | expand |
On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > PAGEFLAGS_MASK to make the code clear to get the page flags. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > include/linux/page-flags.h | 4 +++- > include/trace/events/page_ref.h | 4 ++-- > lib/test_printf.c | 2 +- > lib/vsprintf.c | 2 +- > 4 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index 54c4af35c628..1f951ac24a5e 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -180,6 +180,8 @@ enum pageflags { > PG_reported = PG_uptodate, > }; > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) Hm, isn't it better to invert it? Like #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) It feels more usual and will simplify the rest of the patch. Thanks! > + > #ifndef __GENERATING_BOUNDS_H > > static inline unsigned long _compound_head(const struct page *page) > @@ -975,7 +977,7 @@ static inline void ClearPageSlabPfmemalloc(struct page *page) > * alloc-free cycle to prevent from reusing the page. > */ > #define PAGE_FLAGS_CHECK_AT_PREP \ > - (((1UL << NR_PAGEFLAGS) - 1) & ~__PG_HWPOISON) > + (~(PAGEFLAGS_MASK | __PG_HWPOISON)) > > #define PAGE_FLAGS_PRIVATE \ > (1UL << PG_private | 1UL << PG_private_2) > diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h > index 5d2ea93956ce..643b1b4e9f27 100644 > --- a/include/trace/events/page_ref.h > +++ b/include/trace/events/page_ref.h > @@ -38,7 +38,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, > > TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", > __entry->pfn, > - show_page_flags(__entry->flags & ((1UL << NR_PAGEFLAGS) - 1)), > + show_page_flags(__entry->flags & ~PAGEFLAGS_MASK), > __entry->count, > __entry->mapcount, __entry->mapping, __entry->mt, > __entry->val) > @@ -88,7 +88,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, > > TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", > __entry->pfn, > - show_page_flags(__entry->flags & ((1UL << NR_PAGEFLAGS) - 1)), > + show_page_flags(__entry->flags & ~PAGEFLAGS_MASK), > __entry->count, > __entry->mapcount, __entry->mapping, __entry->mt, > __entry->val, __entry->ret) > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 8ac71aee46af..eadcc6458b10 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -614,7 +614,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > bool append = false; > int i; > > - flags &= BIT(NR_PAGEFLAGS) - 1; > + flags &= ~PAGEFLAGS_MASK; > if (flags) { > page_flags |= flags; > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index dd006adfe853..ff7c180a2971 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -2019,7 +2019,7 @@ static const struct page_flags_fields pff[] = { > static > char *format_page_flags(char *buf, char *end, unsigned long flags) > { > - unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1); > + unsigned long main_flags = flags & ~PAGEFLAGS_MASK; > bool append = false; > int i; > > -- > 2.11.0 >
On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > include/linux/page-flags.h | 4 +++- > > include/trace/events/page_ref.h | 4 ++-- > > lib/test_printf.c | 2 +- > > lib/vsprintf.c | 2 +- > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > index 54c4af35c628..1f951ac24a5e 100644 > > --- a/include/linux/page-flags.h > > +++ b/include/linux/page-flags.h > > @@ -180,6 +180,8 @@ enum pageflags { > > PG_reported = PG_uptodate, > > }; > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > Hm, isn't it better to invert it? Like > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > It feels more usual and will simplify the rest of the patch. Actually, I learned from PAGE_MASK. So I thought the macro like xxx_MASK should be the format of 0xff...ff00...00. I don't know if it is an unwritten rule. I can invert PAGEFLAGS_MASK if it's not a rule. Thanks. > > Thanks! > > > + > > #ifndef __GENERATING_BOUNDS_H > > > > static inline unsigned long _compound_head(const struct page *page) > > @@ -975,7 +977,7 @@ static inline void ClearPageSlabPfmemalloc(struct page *page) > > * alloc-free cycle to prevent from reusing the page. > > */ > > #define PAGE_FLAGS_CHECK_AT_PREP \ > > - (((1UL << NR_PAGEFLAGS) - 1) & ~__PG_HWPOISON) > > + (~(PAGEFLAGS_MASK | __PG_HWPOISON)) > > > > #define PAGE_FLAGS_PRIVATE \ > > (1UL << PG_private | 1UL << PG_private_2) > > diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h > > index 5d2ea93956ce..643b1b4e9f27 100644 > > --- a/include/trace/events/page_ref.h > > +++ b/include/trace/events/page_ref.h > > @@ -38,7 +38,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, > > > > TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", > > __entry->pfn, > > - show_page_flags(__entry->flags & ((1UL << NR_PAGEFLAGS) - 1)), > > + show_page_flags(__entry->flags & ~PAGEFLAGS_MASK), > > __entry->count, > > __entry->mapcount, __entry->mapping, __entry->mt, > > __entry->val) > > @@ -88,7 +88,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, > > > > TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", > > __entry->pfn, > > - show_page_flags(__entry->flags & ((1UL << NR_PAGEFLAGS) - 1)), > > + show_page_flags(__entry->flags & ~PAGEFLAGS_MASK), > > __entry->count, > > __entry->mapcount, __entry->mapping, __entry->mt, > > __entry->val, __entry->ret) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index 8ac71aee46af..eadcc6458b10 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -614,7 +614,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid, > > bool append = false; > > int i; > > > > - flags &= BIT(NR_PAGEFLAGS) - 1; > > + flags &= ~PAGEFLAGS_MASK; > > if (flags) { > > page_flags |= flags; > > snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index dd006adfe853..ff7c180a2971 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -2019,7 +2019,7 @@ static const struct page_flags_fields pff[] = { > > static > > char *format_page_flags(char *buf, char *end, unsigned long flags) > > { > > - unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1); > > + unsigned long main_flags = flags & ~PAGEFLAGS_MASK; > > bool append = false; > > int i; > > > > -- > > 2.11.0 > >
On Wed, Aug 18, 2021 at 12:35:08PM +0800, Muchun Song wrote: > On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > include/linux/page-flags.h | 4 +++- > > > include/trace/events/page_ref.h | 4 ++-- > > > lib/test_printf.c | 2 +- > > > lib/vsprintf.c | 2 +- > > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > > index 54c4af35c628..1f951ac24a5e 100644 > > > --- a/include/linux/page-flags.h > > > +++ b/include/linux/page-flags.h > > > @@ -180,6 +180,8 @@ enum pageflags { > > > PG_reported = PG_uptodate, > > > }; > > > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > > > Hm, isn't it better to invert it? Like > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > It feels more usual and will simplify the rest of the patch. > > Actually, I learned from PAGE_MASK. So I thought the macro > like xxx_MASK should be the format of 0xff...ff00...00. I don't > know if it is an unwritten rule. I can invert PAGEFLAGS_MASK > if it's not a rule. There are many examples of both approached in the kernel tree, however I'd say the more common is without "~" (out of my head). It's definitely OK to define it like #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) Thanks!
On Wed, Aug 18, 2021 at 12:44 PM Roman Gushchin <guro@fb.com> wrote: > > On Wed, Aug 18, 2021 at 12:35:08PM +0800, Muchun Song wrote: > > On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > --- > > > > include/linux/page-flags.h | 4 +++- > > > > include/trace/events/page_ref.h | 4 ++-- > > > > lib/test_printf.c | 2 +- > > > > lib/vsprintf.c | 2 +- > > > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > > > index 54c4af35c628..1f951ac24a5e 100644 > > > > --- a/include/linux/page-flags.h > > > > +++ b/include/linux/page-flags.h > > > > @@ -180,6 +180,8 @@ enum pageflags { > > > > PG_reported = PG_uptodate, > > > > }; > > > > > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > > > > > Hm, isn't it better to invert it? Like > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > It feels more usual and will simplify the rest of the patch. > > > > Actually, I learned from PAGE_MASK. So I thought the macro > > like xxx_MASK should be the format of 0xff...ff00...00. I don't > > know if it is an unwritten rule. I can invert PAGEFLAGS_MASK > > if it's not a rule. > > There are many examples of both approached in the kernel tree, > however I'd say the more common is without "~" (out of my head). > > It's definitely OK to define it like > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) Got it. Thanks Roman. > > Thanks!
On Tue, 17 Aug 2021 21:44:36 -0700 Roman Gushchin <guro@fb.com> wrote: > On Wed, Aug 18, 2021 at 12:35:08PM +0800, Muchun Song wrote: > > On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > --- > > > > include/linux/page-flags.h | 4 +++- > > > > include/trace/events/page_ref.h | 4 ++-- > > > > lib/test_printf.c | 2 +- > > > > lib/vsprintf.c | 2 +- > > > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > > > index 54c4af35c628..1f951ac24a5e 100644 > > > > --- a/include/linux/page-flags.h > > > > +++ b/include/linux/page-flags.h > > > > @@ -180,6 +180,8 @@ enum pageflags { > > > > PG_reported = PG_uptodate, > > > > }; > > > > > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > > > > > Hm, isn't it better to invert it? Like > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > It feels more usual and will simplify the rest of the patch. > > > > Actually, I learned from PAGE_MASK. So I thought the macro > > like xxx_MASK should be the format of 0xff...ff00...00. I don't > > know if it is an unwritten rule. I can invert PAGEFLAGS_MASK > > if it's not a rule. > > There are many examples of both approached in the kernel tree, > however I'd say the more common is without "~" (out of my head). > > It's definitely OK to define it like > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > PAGE_MASK has always seemed weird to me but I figured that emulating it would be the approach of least surprise. Might be wrong about that...
On Wed, Aug 18, 2021 at 3:39 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 17 Aug 2021 21:44:36 -0700 Roman Gushchin <guro@fb.com> wrote: > > > On Wed, Aug 18, 2021 at 12:35:08PM +0800, Muchun Song wrote: > > > On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > > > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > > > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > > --- > > > > > include/linux/page-flags.h | 4 +++- > > > > > include/trace/events/page_ref.h | 4 ++-- > > > > > lib/test_printf.c | 2 +- > > > > > lib/vsprintf.c | 2 +- > > > > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > > > > index 54c4af35c628..1f951ac24a5e 100644 > > > > > --- a/include/linux/page-flags.h > > > > > +++ b/include/linux/page-flags.h > > > > > @@ -180,6 +180,8 @@ enum pageflags { > > > > > PG_reported = PG_uptodate, > > > > > }; > > > > > > > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > > > > > > > Hm, isn't it better to invert it? Like > > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > > > It feels more usual and will simplify the rest of the patch. > > > > > > Actually, I learned from PAGE_MASK. So I thought the macro > > > like xxx_MASK should be the format of 0xff...ff00...00. I don't > > > know if it is an unwritten rule. I can invert PAGEFLAGS_MASK > > > if it's not a rule. > > > > There are many examples of both approached in the kernel tree, > > however I'd say the more common is without "~" (out of my head). > > > > It's definitely OK to define it like > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > PAGE_MASK has always seemed weird to me but I figured that emulating it > would be the approach of least surprise. Might be wrong about that... IIUC, you seem to agree with the current approach. Right?
On Thu, Aug 19, 2021 at 02:33:37PM +0800, Muchun Song wrote: > On Wed, Aug 18, 2021 at 3:39 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Tue, 17 Aug 2021 21:44:36 -0700 Roman Gushchin <guro@fb.com> wrote: > > > > > On Wed, Aug 18, 2021 at 12:35:08PM +0800, Muchun Song wrote: > > > > On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > > > > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > > > > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > > > --- > > > > > > include/linux/page-flags.h | 4 +++- > > > > > > include/trace/events/page_ref.h | 4 ++-- > > > > > > lib/test_printf.c | 2 +- > > > > > > lib/vsprintf.c | 2 +- > > > > > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > > > > > index 54c4af35c628..1f951ac24a5e 100644 > > > > > > --- a/include/linux/page-flags.h > > > > > > +++ b/include/linux/page-flags.h > > > > > > @@ -180,6 +180,8 @@ enum pageflags { > > > > > > PG_reported = PG_uptodate, > > > > > > }; > > > > > > > > > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > > > > > > > > > Hm, isn't it better to invert it? Like > > > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > > > > > It feels more usual and will simplify the rest of the patch. > > > > > > > > Actually, I learned from PAGE_MASK. So I thought the macro > > > > like xxx_MASK should be the format of 0xff...ff00...00. I don't > > > > know if it is an unwritten rule. I can invert PAGEFLAGS_MASK > > > > if it's not a rule. > > > > > > There are many examples of both approached in the kernel tree, > > > however I'd say the more common is without "~" (out of my head). > > > > > > It's definitely OK to define it like > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > > PAGE_MASK has always seemed weird to me but I figured that emulating it > > would be the approach of least surprise. Might be wrong about that... > > IIUC, you seem to agree with the current approach. Right? We do use both schemes. PAGE_MASK, as you pointed out. PMD_MASK, PUD_MASK etc. also *hide* rather than extract the named bits. However, we already have a series of masks for page->flags in mm.h that follow the more common scheme of *extracting* the named bits: #define ZONES_MASK ((1UL << ZONES_WIDTH) - 1) #define NODES_MASK ((1UL << NODES_WIDTH) - 1) #define SECTIONS_MASK ((1UL << SECTIONS_WIDTH) - 1) #define LAST_CPUPID_MASK ((1UL << LAST_CPUPID_SHIFT) - 1) #define KASAN_TAG_MASK ((1UL << KASAN_TAG_WIDTH) - 1) #define ZONEID_MASK ((1UL << ZONEID_SHIFT) - 1) Because of that, I would agree with Roman here and say it probably makes sense to put the PAGEFLAGS_MASK definition next to those and follow the "mask everything but" semantics.
On Thu, Aug 19, 2021 at 7:16 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Thu, Aug 19, 2021 at 02:33:37PM +0800, Muchun Song wrote: > > On Wed, Aug 18, 2021 at 3:39 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > On Tue, 17 Aug 2021 21:44:36 -0700 Roman Gushchin <guro@fb.com> wrote: > > > > > > > On Wed, Aug 18, 2021 at 12:35:08PM +0800, Muchun Song wrote: > > > > > On Wed, Aug 18, 2021 at 10:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > > > > > On Tue, Aug 17, 2021 at 11:30:32AM +0800, Muchun Song wrote: > > > > > > > Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing > > > > > > > PAGEFLAGS_MASK to make the code clear to get the page flags. > > > > > > > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > > > > --- > > > > > > > include/linux/page-flags.h | 4 +++- > > > > > > > include/trace/events/page_ref.h | 4 ++-- > > > > > > > lib/test_printf.c | 2 +- > > > > > > > lib/vsprintf.c | 2 +- > > > > > > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > > > > > > index 54c4af35c628..1f951ac24a5e 100644 > > > > > > > --- a/include/linux/page-flags.h > > > > > > > +++ b/include/linux/page-flags.h > > > > > > > @@ -180,6 +180,8 @@ enum pageflags { > > > > > > > PG_reported = PG_uptodate, > > > > > > > }; > > > > > > > > > > > > > > +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) > > > > > > > > > > > > Hm, isn't it better to invert it? Like > > > > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > > > > > > > It feels more usual and will simplify the rest of the patch. > > > > > > > > > > Actually, I learned from PAGE_MASK. So I thought the macro > > > > > like xxx_MASK should be the format of 0xff...ff00...00. I don't > > > > > know if it is an unwritten rule. I can invert PAGEFLAGS_MASK > > > > > if it's not a rule. > > > > > > > > There are many examples of both approached in the kernel tree, > > > > however I'd say the more common is without "~" (out of my head). > > > > > > > > It's definitely OK to define it like > > > > #define PAGEFLAGS_MASK ((1UL << NR_PAGEFLAGS) - 1) > > > > > > > > > > PAGE_MASK has always seemed weird to me but I figured that emulating it > > > would be the approach of least surprise. Might be wrong about that... > > > > IIUC, you seem to agree with the current approach. Right? > > We do use both schemes. PAGE_MASK, as you pointed out. PMD_MASK, > PUD_MASK etc. also *hide* rather than extract the named bits. > > However, we already have a series of masks for page->flags in mm.h > that follow the more common scheme of *extracting* the named bits: > > #define ZONES_MASK ((1UL << ZONES_WIDTH) - 1) > #define NODES_MASK ((1UL << NODES_WIDTH) - 1) > #define SECTIONS_MASK ((1UL << SECTIONS_WIDTH) - 1) > #define LAST_CPUPID_MASK ((1UL << LAST_CPUPID_SHIFT) - 1) > #define KASAN_TAG_MASK ((1UL << KASAN_TAG_WIDTH) - 1) > #define ZONEID_MASK ((1UL << ZONEID_SHIFT) - 1) > > Because of that, I would agree with Roman here and say it probably > makes sense to put the PAGEFLAGS_MASK definition next to those and > follow the "mask everything but" semantics. Thanks for your suggestions. I'll send a v2 patch.
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index 54c4af35c628..1f951ac24a5e 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -180,6 +180,8 @@ enum pageflags { PG_reported = PG_uptodate, }; +#define PAGEFLAGS_MASK (~((1UL << NR_PAGEFLAGS) - 1)) + #ifndef __GENERATING_BOUNDS_H static inline unsigned long _compound_head(const struct page *page) @@ -975,7 +977,7 @@ static inline void ClearPageSlabPfmemalloc(struct page *page) * alloc-free cycle to prevent from reusing the page. */ #define PAGE_FLAGS_CHECK_AT_PREP \ - (((1UL << NR_PAGEFLAGS) - 1) & ~__PG_HWPOISON) + (~(PAGEFLAGS_MASK | __PG_HWPOISON)) #define PAGE_FLAGS_PRIVATE \ (1UL << PG_private | 1UL << PG_private_2) diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h index 5d2ea93956ce..643b1b4e9f27 100644 --- a/include/trace/events/page_ref.h +++ b/include/trace/events/page_ref.h @@ -38,7 +38,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", __entry->pfn, - show_page_flags(__entry->flags & ((1UL << NR_PAGEFLAGS) - 1)), + show_page_flags(__entry->flags & ~PAGEFLAGS_MASK), __entry->count, __entry->mapcount, __entry->mapping, __entry->mt, __entry->val) @@ -88,7 +88,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", __entry->pfn, - show_page_flags(__entry->flags & ((1UL << NR_PAGEFLAGS) - 1)), + show_page_flags(__entry->flags & ~PAGEFLAGS_MASK), __entry->count, __entry->mapcount, __entry->mapping, __entry->mt, __entry->val, __entry->ret) diff --git a/lib/test_printf.c b/lib/test_printf.c index 8ac71aee46af..eadcc6458b10 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -614,7 +614,7 @@ page_flags_test(int section, int node, int zone, int last_cpupid, bool append = false; int i; - flags &= BIT(NR_PAGEFLAGS) - 1; + flags &= ~PAGEFLAGS_MASK; if (flags) { page_flags |= flags; snprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index dd006adfe853..ff7c180a2971 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2019,7 +2019,7 @@ static const struct page_flags_fields pff[] = { static char *format_page_flags(char *buf, char *end, unsigned long flags) { - unsigned long main_flags = flags & (BIT(NR_PAGEFLAGS) - 1); + unsigned long main_flags = flags & ~PAGEFLAGS_MASK; bool append = false; int i;
Instead of hard-coding ((1UL << NR_PAGEFLAGS) - 1) everywhere, introducing PAGEFLAGS_MASK to make the code clear to get the page flags. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/page-flags.h | 4 +++- include/trace/events/page_ref.h | 4 ++-- lib/test_printf.c | 2 +- lib/vsprintf.c | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-)