* [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
@ 2021-08-17 3:30 Muchun Song
2021-08-18 2:16 ` Roman Gushchin
0 siblings, 1 reply; 9+ messages in thread
From: Muchun Song @ 2021-08-17 3:30 UTC (permalink / raw)
To: guro, hannes, mhocko, akpm, shakeelb, vdavydov.dev, willy
Cc: linux-kernel, linux-mm, Muchun Song
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))
+
#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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-17 3:30 [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1) Muchun Song
@ 2021-08-18 2:16 ` Roman Gushchin
2021-08-18 4:35 ` Muchun Song
0 siblings, 1 reply; 9+ messages in thread
From: Roman Gushchin @ 2021-08-18 2:16 UTC (permalink / raw)
To: Muchun Song
Cc: hannes, mhocko, akpm, shakeelb, vdavydov.dev, willy,
linux-kernel, linux-mm
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-18 2:16 ` Roman Gushchin
@ 2021-08-18 4:35 ` Muchun Song
2021-08-18 4:44 ` Roman Gushchin
0 siblings, 1 reply; 9+ messages in thread
From: Muchun Song @ 2021-08-18 4:35 UTC (permalink / raw)
To: Roman Gushchin
Cc: Johannes Weiner, Michal Hocko, Andrew Morton, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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
> >
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-18 4:35 ` Muchun Song
@ 2021-08-18 4:44 ` Roman Gushchin
2021-08-18 5:32 ` Muchun Song
2021-08-18 7:39 ` Andrew Morton
0 siblings, 2 replies; 9+ messages in thread
From: Roman Gushchin @ 2021-08-18 4:44 UTC (permalink / raw)
To: Muchun Song
Cc: Johannes Weiner, Michal Hocko, Andrew Morton, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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!
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-18 4:44 ` Roman Gushchin
@ 2021-08-18 5:32 ` Muchun Song
2021-08-18 7:39 ` Andrew Morton
1 sibling, 0 replies; 9+ messages in thread
From: Muchun Song @ 2021-08-18 5:32 UTC (permalink / raw)
To: Roman Gushchin
Cc: Johannes Weiner, Michal Hocko, Andrew Morton, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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!
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-18 4:44 ` Roman Gushchin
2021-08-18 5:32 ` Muchun Song
@ 2021-08-18 7:39 ` Andrew Morton
2021-08-19 6:33 ` Muchun Song
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-08-18 7:39 UTC (permalink / raw)
To: Roman Gushchin
Cc: Muchun Song, Johannes Weiner, Michal Hocko, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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...
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-18 7:39 ` Andrew Morton
@ 2021-08-19 6:33 ` Muchun Song
2021-08-19 11:18 ` Johannes Weiner
0 siblings, 1 reply; 9+ messages in thread
From: Muchun Song @ 2021-08-19 6:33 UTC (permalink / raw)
To: Andrew Morton
Cc: Roman Gushchin, Johannes Weiner, Michal Hocko, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-19 6:33 ` Muchun Song
@ 2021-08-19 11:18 ` Johannes Weiner
2021-08-19 15:05 ` Muchun Song
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Weiner @ 2021-08-19 11:18 UTC (permalink / raw)
To: Muchun Song
Cc: Andrew Morton, Roman Gushchin, Michal Hocko, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1)
2021-08-19 11:18 ` Johannes Weiner
@ 2021-08-19 15:05 ` Muchun Song
0 siblings, 0 replies; 9+ messages in thread
From: Muchun Song @ 2021-08-19 15:05 UTC (permalink / raw)
To: Johannes Weiner
Cc: Andrew Morton, Roman Gushchin, Michal Hocko, Shakeel Butt,
Vladimir Davydov, Matthew Wilcox, LKML,
Linux Memory Management List
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-19 15:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-17 3:30 [PATCH] mm: introduce PAGEFLAGS_MASK to replace ((1UL << NR_PAGEFLAGS) - 1) Muchun Song
2021-08-18 2:16 ` Roman Gushchin
2021-08-18 4:35 ` Muchun Song
2021-08-18 4:44 ` Roman Gushchin
2021-08-18 5:32 ` Muchun Song
2021-08-18 7:39 ` Andrew Morton
2021-08-19 6:33 ` Muchun Song
2021-08-19 11:18 ` Johannes Weiner
2021-08-19 15:05 ` Muchun Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).