| Message ID | 20210115061349.67386-3-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: improve the output of print_page_info() | expand |
On 15.01.21 07:13, Yafang Shao wrote: > There're totally __NR_PAGEFLAGS page flags, but the type of page->flags is > unsigned long, that makes the value of page->flags a little misleading when > it is printed to the user. We'd better print the real pages flags, instead > of the whole 64bits including the random values in the useless high bits. No, these are *not* random values. They include the nid, zid, and section_nr - which are helpful to have at hand when debugging, or detecting that something might be messed up there. > > There're two choices to achieve that, one of which is clear the useless Again, not useless. > high bits when we initlize the page->flags, the other is don't print the > high bits when it is showed to the user. The latter one is better because > it is in the slow path and the performance won't be impacted. > > Before that change, the output is, > [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head) > > After that change, the output is, > [ 8843.757770] INFO: Slab 0x00000000f0e98335 objects=33 used=3 fp=0x00000000b643c7d8 flags=0x10200(slab|head) > Nack to the current approach. If you're going to strip this information, you should expose it differently. E.g., printing page_zonenum() or page_to_nid(). But still, then we might lose valuable information of bits stored in there that shouldn't have been set. > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > include/linux/page-flags.h | 2 ++ > mm/slub.c | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index ec5d029..db5c017 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -175,6 +175,8 @@ enum pageflags { > PG_reported = PG_uptodate, > }; > > +#define PAGE_FLAGS(flags) (flags & ((1 << __NR_PAGEFLAGS) - 1)) > + > #ifndef __GENERATING_BOUNDS_H > > struct page; /* forward declaration */ > diff --git a/mm/slub.c b/mm/slub.c > index 901a45a..a93a03c 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -643,7 +643,7 @@ static void print_page_info(struct page *page) > { > pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", > page, page->objects, page->inuse, page->freelist, > - page->flags, &page->flags); > + PAGE_FLAGS(page->flags), &page->flags); > > } > >
On Fri, Jan 15, 2021 at 4:39 PM David Hildenbrand <david@redhat.com> wrote: > > On 15.01.21 07:13, Yafang Shao wrote: > > There're totally __NR_PAGEFLAGS page flags, but the type of page->flags is > > unsigned long, that makes the value of page->flags a little misleading when > > it is printed to the user. We'd better print the real pages flags, instead > > of the whole 64bits including the random values in the useless high bits. > > No, these are *not* random values. They include the nid, zid, and > section_nr - which are helpful to have at hand when debugging, or > detecting that something might be messed up there. > Thanks for the explanation. I just noticed the page-flags layout in page-flags-layout.h. > > > > There're two choices to achieve that, one of which is clear the useless > > Again, not useless. > > > high bits when we initlize the page->flags, the other is don't print the > > high bits when it is showed to the user. The latter one is better because > > it is in the slow path and the performance won't be impacted. > > > > Before that change, the output is, > > [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head) > > > > After that change, the output is, > > [ 8843.757770] INFO: Slab 0x00000000f0e98335 objects=33 used=3 fp=0x00000000b643c7d8 flags=0x10200(slab|head) > > > > Nack to the current approach. If you're going to strip this information, > you should expose it differently. E.g., printing page_zonenum() or > page_to_nid(). But still, then we might lose valuable information of > bits stored in there that shouldn't have been set. > How about changing the implementation of pGp in printk() ? In the new implementation of pGp we can dump the full information of page->flags, rather than the flag's name only. For example, 0xXXXXXXXX(node n, nid n, ..., slab|head) That will make it easier to understand, as it is not easy to look into the detail of page-flags layout. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > include/linux/page-flags.h | 2 ++ > > mm/slub.c | 2 +- > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > index ec5d029..db5c017 100644 > > --- a/include/linux/page-flags.h > > +++ b/include/linux/page-flags.h > > @@ -175,6 +175,8 @@ enum pageflags { > > PG_reported = PG_uptodate, > > }; > > > > +#define PAGE_FLAGS(flags) (flags & ((1 << __NR_PAGEFLAGS) - 1)) > > + > > #ifndef __GENERATING_BOUNDS_H > > > > struct page; /* forward declaration */ > > diff --git a/mm/slub.c b/mm/slub.c > > index 901a45a..a93a03c 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -643,7 +643,7 @@ static void print_page_info(struct page *page) > > { > > pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", > > page, page->objects, page->inuse, page->freelist, > > - page->flags, &page->flags); > > + PAGE_FLAGS(page->flags), &page->flags); > > > > } > > > > > > > -- > Thanks, > > David / dhildenb >
On 15.01.21 11:10, Yafang Shao wrote: > On Fri, Jan 15, 2021 at 4:39 PM David Hildenbrand <david@redhat.com> wrote: >> >> On 15.01.21 07:13, Yafang Shao wrote: >>> There're totally __NR_PAGEFLAGS page flags, but the type of page->flags is >>> unsigned long, that makes the value of page->flags a little misleading when >>> it is printed to the user. We'd better print the real pages flags, instead >>> of the whole 64bits including the random values in the useless high bits. >> >> No, these are *not* random values. They include the nid, zid, and >> section_nr - which are helpful to have at hand when debugging, or >> detecting that something might be messed up there. >> > > Thanks for the explanation. I just noticed the page-flags layout in > page-flags-layout.h. > >>> >>> There're two choices to achieve that, one of which is clear the useless >> >> Again, not useless. >> >>> high bits when we initlize the page->flags, the other is don't print the >>> high bits when it is showed to the user. The latter one is better because >>> it is in the slow path and the performance won't be impacted. >>> >>> Before that change, the output is, >>> [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head) >>> >>> After that change, the output is, >>> [ 8843.757770] INFO: Slab 0x00000000f0e98335 objects=33 used=3 fp=0x00000000b643c7d8 flags=0x10200(slab|head) >>> >> >> Nack to the current approach. If you're going to strip this information, >> you should expose it differently. E.g., printing page_zonenum() or >> page_to_nid(). But still, then we might lose valuable information of >> bits stored in there that shouldn't have been set. >> > > How about changing the implementation of pGp in printk() ? > In the new implementation of pGp we can dump the full information of > page->flags, rather than the flag's name only. > > For example, > 0xXXXXXXXX(node n, nid n, ..., slab|head) > > That will make it easier to understand, as it is not easy to look into > the detail of page-flags layout. Not completely opposed to this. Just keep in mind that the information stored in the high bits differs per configuration. See include/linux/page-flags-layout.h So to dump reliably, you would have to consider all different flavors, looking at SECTIONS_WIDTH, NODES_WIDTH, ZONES_WIDTH, LAST_CPUPID_WIDTH, KASAN_TAG_WIDTH Might be helpful for other users as well, like in mm/memory-failure.c
On Fri, Jan 15, 2021 at 6:15 PM David Hildenbrand <david@redhat.com> wrote: > > On 15.01.21 11:10, Yafang Shao wrote: > > On Fri, Jan 15, 2021 at 4:39 PM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 15.01.21 07:13, Yafang Shao wrote: > >>> There're totally __NR_PAGEFLAGS page flags, but the type of page->flags is > >>> unsigned long, that makes the value of page->flags a little misleading when > >>> it is printed to the user. We'd better print the real pages flags, instead > >>> of the whole 64bits including the random values in the useless high bits. > >> > >> No, these are *not* random values. They include the nid, zid, and > >> section_nr - which are helpful to have at hand when debugging, or > >> detecting that something might be messed up there. > >> > > > > Thanks for the explanation. I just noticed the page-flags layout in > > page-flags-layout.h. > > > >>> > >>> There're two choices to achieve that, one of which is clear the useless > >> > >> Again, not useless. > >> > >>> high bits when we initlize the page->flags, the other is don't print the > >>> high bits when it is showed to the user. The latter one is better because > >>> it is in the slow path and the performance won't be impacted. > >>> > >>> Before that change, the output is, > >>> [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head) > >>> > >>> After that change, the output is, > >>> [ 8843.757770] INFO: Slab 0x00000000f0e98335 objects=33 used=3 fp=0x00000000b643c7d8 flags=0x10200(slab|head) > >>> > >> > >> Nack to the current approach. If you're going to strip this information, > >> you should expose it differently. E.g., printing page_zonenum() or > >> page_to_nid(). But still, then we might lose valuable information of > >> bits stored in there that shouldn't have been set. > >> > > > > How about changing the implementation of pGp in printk() ? > > In the new implementation of pGp we can dump the full information of > > page->flags, rather than the flag's name only. > > > > For example, > > 0xXXXXXXXX(node n, nid n, ..., slab|head) > > > > That will make it easier to understand, as it is not easy to look into > > the detail of page-flags layout. > > Not completely opposed to this. Just keep in mind that the information > stored in the high bits differs per configuration. See > include/linux/page-flags-layout.h > > So to dump reliably, you would have to consider all different flavors, > looking at SECTIONS_WIDTH, NODES_WIDTH, ZONES_WIDTH, LAST_CPUPID_WIDTH, > KASAN_TAG_WIDTH > > Might be helpful for other users as well, like in mm/memory-failure.c > Thanks for the suggestion. I will think about it.
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index ec5d029..db5c017 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -175,6 +175,8 @@ enum pageflags { PG_reported = PG_uptodate, }; +#define PAGE_FLAGS(flags) (flags & ((1 << __NR_PAGEFLAGS) - 1)) + #ifndef __GENERATING_BOUNDS_H struct page; /* forward declaration */ diff --git a/mm/slub.c b/mm/slub.c index 901a45a..a93a03c 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -643,7 +643,7 @@ static void print_page_info(struct page *page) { pr_err("INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", page, page->objects, page->inuse, page->freelist, - page->flags, &page->flags); + PAGE_FLAGS(page->flags), &page->flags); }
There're totally __NR_PAGEFLAGS page flags, but the type of page->flags is unsigned long, that makes the value of page->flags a little misleading when it is printed to the user. We'd better print the real pages flags, instead of the whole 64bits including the random values in the useless high bits. There're two choices to achieve that, one of which is clear the useless high bits when we initlize the page->flags, the other is don't print the high bits when it is showed to the user. The latter one is better because it is in the slow path and the performance won't be impacted. Before that change, the output is, [ 8846.517809] INFO: Slab 0x00000000f42a2c60 objects=33 used=3 fp=0x0000000060d32ca8 flags=0x17ffffc0010200(slab|head) After that change, the output is, [ 8843.757770] INFO: Slab 0x00000000f0e98335 objects=33 used=3 fp=0x00000000b643c7d8 flags=0x10200(slab|head) Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/linux/page-flags.h | 2 ++ mm/slub.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)