* [RFC v2 0/3] move PG_slab to page_type
@ 2022-11-06 14:03 Hyeonggon Yoo
2022-11-06 14:03 ` [RFC v2 1/3] mm: move PG_slab flag " Hyeonggon Yoo
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Hyeonggon Yoo @ 2022-11-06 14:03 UTC (permalink / raw)
To: linux-mm
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes,
Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox,
Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams,
Hugh Dickins, Muchun Song, David Hildenbrand, Hyeonggon Yoo
RFC v1: https://lore.kernel.org/all/Y0BpuxUb+Y8BKHIM@casper.infradead.org/T/
This series moves PG_slab from page->flags to page->page_type.
as page_type field is also used number of active objects in slab,
upper half (16 bits) are used as page type and lower half (16 bits) are
used as slab->active.
It simplifies checking page_mapped() and folio_mapped() and
frees a bit in page->flags.
This also adds new %pGt printf format that prints human-readable
page_type, and show_page_flags() (for tracepoints).
More tests are still needed, but I think it's worth to get some early
feedbacks.
TO HWPOISON DEVELOPERS:
I think it would be best to add a code that identifies a type of
page from page_type like page flags. but I'm not sure how to
properly test it.
v1 -> v2:
- use page flag policy for pages that uses page_type
(PF_NO_TAIL for slab and PF_ANY for others) (Matthew WilCox)
- store slab->active in negative form and use helpers to
access/modify it (Matthew WilCox)
- Fix logical errors and some cleanup in fs/proc/page.c and kernel/crash_core.c
- add show_page_flags() (patch 2) and %pGt format (patch 3)
Any feedbacks are appreciated.
Hyeonggon Yoo (3):
mm: move PG_slab flag to page_type
mm: introduce show_page_types() to provide human-readable page_type
mm, printk: introduce new format %pGt for page_type
Documentation/core-api/printk-formats.rst | 3 +-
fs/proc/page.c | 13 ++--
include/linux/mm_types.h | 11 ++--
include/linux/page-flags.h | 77 ++++++++++++++++-------
include/trace/events/mmflags.h | 13 +++-
include/trace/events/page_ref.h | 10 ++-
kernel/crash_core.c | 3 +-
lib/test_printf.c | 23 +++++++
lib/vsprintf.c | 24 +++++++
mm/debug.c | 7 +++
mm/internal.h | 1 +
mm/memory-failure.c | 8 ---
mm/slab.c | 44 ++++++++-----
mm/slab.h | 3 +-
14 files changed, 175 insertions(+), 65 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC v2 1/3] mm: move PG_slab flag to page_type 2022-11-06 14:03 [RFC v2 0/3] move PG_slab to page_type Hyeonggon Yoo @ 2022-11-06 14:03 ` Hyeonggon Yoo 2022-11-08 5:39 ` HORIGUCHI NAOYA(堀口 直也) 2022-11-06 14:03 ` [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type Hyeonggon Yoo 2 siblings, 1 reply; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-06 14:03 UTC (permalink / raw) To: linux-mm Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Hyeonggon Yoo For now, only SLAB uses _mapcount field as a number of active objects in a slab, and other slab allocators do not use it. As 16 bits are enough for that, use remaining 16 bits of _mapcount as page_type even when SLAB is used. And then move PG_slab flag to page_type. As suggested by Matthew, store number of active objects in negative form and use helper when accessing or modifying it. Note that page_type is always placed in upper 16 bits of _mapcount to avoid confusing normal _mapcount as page_type. As underflow (actually I mean, yeah, overflow) is not a concern anymore, use more lower bits. Add more folio helpers for PAGE_TYPE_OPS() not to break existing slab implementations. Remove PG_slab check from PAGE_FLAGS_CHECK_AT_FREE. buddy will still check if _mapcount is properly set at free. Exclude PG_slab from hwpoison and show_page_flags() for now. Note that with this patch, page_mapped() and folio_mapped() always return false for slab page. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Naoya Horiguchi <naoya.horiguchi@nec.com> Cc: Miaohe Lin <linmiaohe@huawei.com> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> Cc: Minchan Kim <minchan@kernel.org> Cc: Mel Gorman <mgorman@techsingularity.net> Cc: Andrea Arcangeli <aarcange@redhat.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Hugh Dickins <hughd@google.com> Cc: Muchun Song <songmuchun@bytedance.com> Cc: David Hildenbrand <david@redhat.com> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- fs/proc/page.c | 13 ++---- include/linux/mm_types.h | 11 +++-- include/linux/page-flags.h | 77 ++++++++++++++++++++++++---------- include/trace/events/mmflags.h | 1 - kernel/crash_core.c | 3 +- mm/memory-failure.c | 8 ---- mm/slab.c | 44 ++++++++++++------- mm/slab.h | 3 +- 8 files changed, 98 insertions(+), 62 deletions(-) diff --git a/fs/proc/page.c b/fs/proc/page.c index f2273b164535..101be8d5a74e 100644 --- a/fs/proc/page.c +++ b/fs/proc/page.c @@ -67,7 +67,7 @@ static ssize_t kpagecount_read(struct file *file, char __user *buf, */ ppage = pfn_to_online_page(pfn); - if (!ppage || PageSlab(ppage) || page_has_type(ppage)) + if (!ppage || page_has_type(ppage)) pcount = 0; else pcount = page_mapcount(ppage); @@ -124,11 +124,8 @@ u64 stable_page_flags(struct page *page) /* * pseudo flags for the well known (anonymous) memory mapped pages - * - * Note that page->_mapcount is overloaded in SLOB/SLUB/SLQB, so the - * simple test in page_mapped() is not enough. */ - if (!PageSlab(page) && page_mapped(page)) + if (page_mapped(page)) u |= 1 << KPF_MMAP; if (PageAnon(page)) u |= 1 << KPF_ANON; @@ -178,16 +175,14 @@ u64 stable_page_flags(struct page *page) u |= 1 << KPF_OFFLINE; if (PageTable(page)) u |= 1 << KPF_PGTABLE; + if (PageSlab(page)) + u |= 1 << KPF_SLAB; if (page_is_idle(page)) u |= 1 << KPF_IDLE; u |= kpf_copy_bit(k, KPF_LOCKED, PG_locked); - u |= kpf_copy_bit(k, KPF_SLAB, PG_slab); - if (PageTail(page) && PageSlab(compound_head(page))) - u |= 1 << KPF_SLAB; - u |= kpf_copy_bit(k, KPF_ERROR, PG_error); u |= kpf_copy_bit(k, KPF_DIRTY, PG_dirty); u |= kpf_copy_bit(k, KPF_UPTODATE, PG_uptodate); diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h index 834022721bc6..2f298d1b8cf5 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -196,10 +196,13 @@ struct page { atomic_t _mapcount; /* - * If the page is neither PageSlab nor mappable to userspace, - * the value stored here may help determine what this page - * is used for. See page-flags.h for a list of page types - * which are currently stored here. + * If the page is not mappable to userspace, the value + * stored here may help determine what this page is used for. + * See page-flags.h for a list of page types which are currently + * stored here. + * + * Note that only upper half is used for page types and lower + * half is reserved for SLAB. */ unsigned int page_type; }; diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index 0b0ae5084e60..31dda492cda5 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -107,7 +107,6 @@ enum pageflags { PG_workingset, PG_waiters, /* Page has waiters, check its waitqueue. Must be bit #7 and in the same byte as "PG_locked" */ PG_error, - PG_slab, PG_owner_priv_1, /* Owner use. If pagecache, fs may use*/ PG_arch_1, PG_reserved, @@ -484,7 +483,6 @@ PAGEFLAG(Active, active, PF_HEAD) __CLEARPAGEFLAG(Active, active, PF_HEAD) TESTCLEARFLAG(Active, active, PF_HEAD) PAGEFLAG(Workingset, workingset, PF_HEAD) TESTCLEARFLAG(Workingset, workingset, PF_HEAD) -__PAGEFLAG(Slab, slab, PF_NO_TAIL) __PAGEFLAG(SlobFree, slob_free, PF_NO_TAIL) PAGEFLAG(Checked, checked, PF_NO_COMPOUND) /* Used by some filesystems */ @@ -926,42 +924,72 @@ static inline bool is_page_hwpoison(struct page *page) } /* - * For pages that are never mapped to userspace (and aren't PageSlab), - * page_type may be used. Because it is initialised to -1, we invert the - * sense of the bit, so __SetPageFoo *clears* the bit used for PageFoo, and - * __ClearPageFoo *sets* the bit used for PageFoo. We reserve a few high and - * low bits so that an underflow or overflow of page_mapcount() won't be - * mistaken for a page type value. + * For pages that are never mapped to userspace, page_type may be used. + * Because it is initialised to -1, we invert the sense of the bit, + * so __SetPageFoo *clears* the bit used for PageFoo, and __ClearPageFoo + * *sets* the bit used for PageFoo. We reserve a few high and low bits + * so that an underflow or overflow of page_mapcount() won't be mistaken + * for a page type value. */ #define PAGE_TYPE_BASE 0xf0000000 -/* Reserve 0x0000007f to catch underflows of page_mapcount */ -#define PAGE_MAPCOUNT_RESERVE -128 -#define PG_buddy 0x00000080 -#define PG_offline 0x00000100 -#define PG_table 0x00000200 -#define PG_guard 0x00000400 +#define PG_buddy 0x00010000 +#define PG_offline 0x00020000 +#define PG_table 0x00040000 +#define PG_guard 0x00080000 +#define PG_slab 0x00100000 #define PageType(page, flag) \ ((page->page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE) -static inline int page_has_type(struct page *page) +#define PAGE_TYPE_MASK 0xffff0000 + +static inline bool page_type_has_type(unsigned int page_type) { - return (int)page->page_type < PAGE_MAPCOUNT_RESERVE; + return ((int)page_type < (int)PAGE_TYPE_MASK); } -#define PAGE_TYPE_OPS(uname, lname) \ +static inline bool page_has_type(struct page *page) +{ + return page_type_has_type(page->page_type); +} + + +#define PAGE_TYPE_OPS(uname, lname, policy) \ static __always_inline int Page##uname(struct page *page) \ { \ + page = policy(page, 0); \ + return PageType(page, PG_##lname); \ +} \ +static __always_inline int folio_test_##lname(struct folio *folio) \ +{ \ + struct page *page = &folio->page; \ + \ return PageType(page, PG_##lname); \ } \ static __always_inline void __SetPage##uname(struct page *page) \ { \ + page = policy(page, 1); \ + VM_BUG_ON_PAGE(!PageType(page, 0), page); \ + page->page_type &= ~PG_##lname; \ +} \ +static __always_inline void __folio_set_##lname(struct folio *folio) \ +{ \ + struct page *page = &folio->page; \ + \ VM_BUG_ON_PAGE(!PageType(page, 0), page); \ page->page_type &= ~PG_##lname; \ } \ static __always_inline void __ClearPage##uname(struct page *page) \ { \ + page = policy(page, 1); \ + VM_BUG_ON_PAGE(!Page##uname(page), page); \ + page->page_type |= PG_##lname; \ +} \ +static __always_inline void __folio_clear_##lname(struct folio *folio) \ +{ \ + struct page *page = &folio->page; \ + \ VM_BUG_ON_PAGE(!Page##uname(page), page); \ page->page_type |= PG_##lname; \ } @@ -970,7 +998,7 @@ static __always_inline void __ClearPage##uname(struct page *page) \ * PageBuddy() indicates that the page is free and in the buddy system * (see mm/page_alloc.c). */ -PAGE_TYPE_OPS(Buddy, buddy) +PAGE_TYPE_OPS(Buddy, buddy, PF_ANY) /* * PageOffline() indicates that the page is logically offline although the @@ -994,7 +1022,10 @@ PAGE_TYPE_OPS(Buddy, buddy) * pages should check PageOffline() and synchronize with such drivers using * page_offline_freeze()/page_offline_thaw(). */ -PAGE_TYPE_OPS(Offline, offline) +PAGE_TYPE_OPS(Offline, offline, PF_ANY) + +/* PageSlab() indicates that the page is used by slab subsystem. */ +PAGE_TYPE_OPS(Slab, slab, PF_NO_TAIL) extern void page_offline_freeze(void); extern void page_offline_thaw(void); @@ -1004,12 +1035,12 @@ extern void page_offline_end(void); /* * Marks pages in use as page tables. */ -PAGE_TYPE_OPS(Table, table) +PAGE_TYPE_OPS(Table, table, PF_ANY) /* * Marks guardpages used with debug_pagealloc. */ -PAGE_TYPE_OPS(Guard, guard) +PAGE_TYPE_OPS(Guard, guard, PF_ANY) extern bool is_free_buddy_page(struct page *page); @@ -1057,8 +1088,8 @@ static __always_inline void __ClearPageAnonExclusive(struct page *page) (1UL << PG_lru | 1UL << PG_locked | \ 1UL << PG_private | 1UL << PG_private_2 | \ 1UL << PG_writeback | 1UL << PG_reserved | \ - 1UL << PG_slab | 1UL << PG_active | \ - 1UL << PG_unevictable | __PG_MLOCKED | LRU_GEN_MASK) + 1UL << PG_active | 1UL << PG_unevictable | \ + __PG_MLOCKED | LRU_GEN_MASK) /* * Flags checked when a page is prepped for return by the page allocator. diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h index 11524cda4a95..72c11a16f771 100644 --- a/include/trace/events/mmflags.h +++ b/include/trace/events/mmflags.h @@ -112,7 +112,6 @@ {1UL << PG_lru, "lru" }, \ {1UL << PG_active, "active" }, \ {1UL << PG_workingset, "workingset" }, \ - {1UL << PG_slab, "slab" }, \ {1UL << PG_owner_priv_1, "owner_priv_1" }, \ {1UL << PG_arch_1, "arch_1" }, \ {1UL << PG_reserved, "reserved" }, \ diff --git a/kernel/crash_core.c b/kernel/crash_core.c index a0eb4d5cf557..f72437e4192f 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -479,13 +479,14 @@ static int __init crash_save_vmcoreinfo_init(void) VMCOREINFO_NUMBER(PG_private); VMCOREINFO_NUMBER(PG_swapcache); VMCOREINFO_NUMBER(PG_swapbacked); - VMCOREINFO_NUMBER(PG_slab); #ifdef CONFIG_MEMORY_FAILURE VMCOREINFO_NUMBER(PG_hwpoison); #endif VMCOREINFO_NUMBER(PG_head_mask); #define PAGE_BUDDY_MAPCOUNT_VALUE (~PG_buddy) VMCOREINFO_NUMBER(PAGE_BUDDY_MAPCOUNT_VALUE); +#define PAGE_SLAB_MAPCOUNT_VALUE (~PG_slab) + VMCOREINFO_NUMBER(PAGE_SLAB_MAPCOUNT_VALUE); #ifdef CONFIG_HUGETLB_PAGE VMCOREINFO_NUMBER(HUGETLB_PAGE_DTOR); #define PAGE_OFFLINE_MAPCOUNT_VALUE (~PG_offline) diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 779a426d2cab..9494f47c4cee 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1145,7 +1145,6 @@ static int me_huge_page(struct page_state *ps, struct page *p) #define mlock (1UL << PG_mlocked) #define lru (1UL << PG_lru) #define head (1UL << PG_head) -#define slab (1UL << PG_slab) #define reserved (1UL << PG_reserved) static struct page_state error_states[] = { @@ -1155,13 +1154,6 @@ static struct page_state error_states[] = { * PG_buddy pages only make a small fraction of all free pages. */ - /* - * Could in theory check if slab page is free or if we can drop - * currently unused objects without touching them. But just - * treat it as standard kernel for now. - */ - { slab, slab, MF_MSG_SLAB, me_kernel }, - { head, head, MF_MSG_HUGE, me_huge_page }, { sc|dirty, sc|dirty, MF_MSG_DIRTY_SWAPCACHE, me_swapcache_dirty }, diff --git a/mm/slab.c b/mm/slab.c index 59c8e28f7b6a..da12e82aba41 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -2265,6 +2265,21 @@ void __kmem_cache_release(struct kmem_cache *cachep) } } +static inline unsigned int slab_get_active(struct slab *slab) +{ + return ~(slab->page_type | PG_slab); +} + +static inline void slab_inc_active(struct slab *slab) +{ + slab->page_type--; +} + +static inline void slab_dec_active(struct slab *slab) +{ + slab->page_type++; +} + /* * Get the memory for a slab management obj. * @@ -2287,7 +2302,6 @@ static void *alloc_slabmgmt(struct kmem_cache *cachep, void *addr = slab_address(slab); slab->s_mem = addr + colour_off; - slab->active = 0; if (OBJFREELIST_SLAB(cachep)) freelist = NULL; @@ -2506,8 +2520,8 @@ static void *slab_get_obj(struct kmem_cache *cachep, struct slab *slab) { void *objp; - objp = index_to_obj(cachep, slab, get_free_obj(slab, slab->active)); - slab->active++; + objp = index_to_obj(cachep, slab, get_free_obj(slab, slab_get_active(slab))); + slab_inc_active(slab); return objp; } @@ -2520,7 +2534,7 @@ static void slab_put_obj(struct kmem_cache *cachep, unsigned int i; /* Verify double free bug */ - for (i = slab->active; i < cachep->num; i++) { + for (i = slab_get_active(slab); i < cachep->num; i++) { if (get_free_obj(slab, i) == objnr) { pr_err("slab: double free detected in cache '%s', objp %px\n", cachep->name, objp); @@ -2528,11 +2542,11 @@ static void slab_put_obj(struct kmem_cache *cachep, } } #endif - slab->active--; + slab_dec_active(slab); if (!slab->freelist) slab->freelist = objp + obj_offset(cachep); - set_free_obj(slab, slab->active, objnr); + set_free_obj(slab, slab_get_active(slab), objnr); } /* @@ -2631,14 +2645,14 @@ static void cache_grow_end(struct kmem_cache *cachep, struct slab *slab) spin_lock(&n->list_lock); n->total_slabs++; - if (!slab->active) { + if (!slab_get_active(slab)) { list_add_tail(&slab->slab_list, &n->slabs_free); n->free_slabs++; } else fixup_slab_list(cachep, n, slab, &list); STATS_INC_GROWN(cachep); - n->free_objects += cachep->num - slab->active; + n->free_objects += cachep->num - slab_get_active(slab); spin_unlock(&n->list_lock); fixup_objfreelist_debug(cachep, &list); @@ -2740,7 +2754,7 @@ static inline void fixup_slab_list(struct kmem_cache *cachep, { /* move slabp to correct slabp list: */ list_del(&slab->slab_list); - if (slab->active == cachep->num) { + if (slab_get_active(slab) == cachep->num) { list_add(&slab->slab_list, &n->slabs_full); if (OBJFREELIST_SLAB(cachep)) { #if DEBUG @@ -2779,7 +2793,7 @@ static noinline struct slab *get_valid_first_slab(struct kmem_cache_node *n, /* Move pfmemalloc slab to the end of list to speed up next search */ list_del(&slab->slab_list); - if (!slab->active) { + if (!slab_get_active(slab)) { list_add_tail(&slab->slab_list, &n->slabs_free); n->free_slabs++; } else @@ -2861,9 +2875,9 @@ static __always_inline int alloc_block(struct kmem_cache *cachep, * There must be at least one object available for * allocation. */ - BUG_ON(slab->active >= cachep->num); + BUG_ON(slab_get_active(slab) >= cachep->num); - while (slab->active < cachep->num && batchcount--) { + while (slab_get_active(slab) < cachep->num && batchcount--) { STATS_INC_ALLOCED(cachep); STATS_INC_ACTIVE(cachep); STATS_SET_HIGH(cachep); @@ -3158,7 +3172,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, STATS_INC_ACTIVE(cachep); STATS_SET_HIGH(cachep); - BUG_ON(slab->active == cachep->num); + BUG_ON(slab_get_active(slab) == cachep->num); obj = slab_get_obj(cachep, slab); n->free_objects--; @@ -3292,7 +3306,7 @@ static void free_block(struct kmem_cache *cachep, void **objpp, STATS_DEC_ACTIVE(cachep); /* fixup slab chains */ - if (slab->active == 0) { + if (slab_get_active(slab) == 0) { list_add(&slab->slab_list, &n->slabs_free); n->free_slabs++; } else { @@ -3347,7 +3361,7 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac) struct slab *slab; list_for_each_entry(slab, &n->slabs_free, slab_list) { - BUG_ON(slab->active); + BUG_ON(slab_get_active(slab); i++; } diff --git a/mm/slab.h b/mm/slab.h index 0202a8c2f0d2..f9df0fc3a918 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -18,7 +18,8 @@ struct slab { struct kmem_cache *slab_cache; void *freelist; /* array of free object indexes */ void *s_mem; /* first object */ - unsigned int active; + /* lower half of page_type is used as active objects counter */ + unsigned int page_type; #elif defined(CONFIG_SLUB) -- 2.32.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC v2 1/3] mm: move PG_slab flag to page_type 2022-11-06 14:03 ` [RFC v2 1/3] mm: move PG_slab flag " Hyeonggon Yoo @ 2022-11-08 5:39 ` HORIGUCHI NAOYA(堀口 直也) 2022-11-09 5:45 ` Hyeonggon Yoo 0 siblings, 1 reply; 15+ messages in thread From: HORIGUCHI NAOYA(堀口 直也) @ 2022-11-08 5:39 UTC (permalink / raw) To: Hyeonggon Yoo Cc: linux-mm@kvack.org, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand On Sun, Nov 06, 2022 at 11:03:53PM +0900, Hyeonggon Yoo wrote: > For now, only SLAB uses _mapcount field as a number of active objects in > a slab, and other slab allocators do not use it. As 16 bits are enough > for that, use remaining 16 bits of _mapcount as page_type even when > SLAB is used. And then move PG_slab flag to page_type. > > As suggested by Matthew, store number of active objects in negative > form and use helper when accessing or modifying it. > > Note that page_type is always placed in upper 16 bits of _mapcount to > avoid confusing normal _mapcount as page_type. As underflow (actually > I mean, yeah, overflow) is not a concern anymore, use more lower bits. > > Add more folio helpers for PAGE_TYPE_OPS() not to break existing > slab implementations. > > Remove PG_slab check from PAGE_FLAGS_CHECK_AT_FREE. buddy will still > check if _mapcount is properly set at free. > > Exclude PG_slab from hwpoison and show_page_flags() for now. > > Note that with this patch, page_mapped() and folio_mapped() always return > false for slab page. > ... > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 779a426d2cab..9494f47c4cee 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1145,7 +1145,6 @@ static int me_huge_page(struct page_state *ps, struct page *p) > #define mlock (1UL << PG_mlocked) > #define lru (1UL << PG_lru) > #define head (1UL << PG_head) > -#define slab (1UL << PG_slab) > #define reserved (1UL << PG_reserved) > > static struct page_state error_states[] = { > @@ -1155,13 +1154,6 @@ static struct page_state error_states[] = { > * PG_buddy pages only make a small fraction of all free pages. > */ > > - /* > - * Could in theory check if slab page is free or if we can drop > - * currently unused objects without touching them. But just > - * treat it as standard kernel for now. > - */ > - { slab, slab, MF_MSG_SLAB, me_kernel }, > - Hi Hyeonggon, Actually the above part is dead code now and it's harmless to remove this. identify_page_state() is never called when handling memory error event on a slab page because HWPoisonHandlable() returns false for it. If you remove it, a few other lines using MF_MSG_SLAB will be unneccessary, so could you remove them too? As for testing, you can test this case for example like below: - install page-types (available in tools/vm/page-types.c), - show the list of slab pages by "page-types -b slab -Nl" command (the first column is PFNs) and choose one PFNs as target, - call "page-types -a <PFN> -X" (requires hwpoison-inject module to be loaded) In my testing server, dmesg shows like below: [598746.497805] Injecting memory failure at pfn 0x16295b [598746.499208] Memory failure: 0x16295b: unhandlable page. [598746.500570] Memory failure: 0x16295b: recovery action for unknown page: Ignored And your patchset seems not to affect this behavior. Thanks, Naoya Horiguchi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 1/3] mm: move PG_slab flag to page_type 2022-11-08 5:39 ` HORIGUCHI NAOYA(堀口 直也) @ 2022-11-09 5:45 ` Hyeonggon Yoo 0 siblings, 0 replies; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-09 5:45 UTC (permalink / raw) To: HORIGUCHI NAOYA(堀口 直也) Cc: linux-mm@kvack.org, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand On Tue, Nov 08, 2022 at 05:39:24AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote: > On Sun, Nov 06, 2022 at 11:03:53PM +0900, Hyeonggon Yoo wrote: > > For now, only SLAB uses _mapcount field as a number of active objects in > > a slab, and other slab allocators do not use it. As 16 bits are enough > > for that, use remaining 16 bits of _mapcount as page_type even when > > SLAB is used. And then move PG_slab flag to page_type. > > > > As suggested by Matthew, store number of active objects in negative > > form and use helper when accessing or modifying it. > > > > Note that page_type is always placed in upper 16 bits of _mapcount to > > avoid confusing normal _mapcount as page_type. As underflow (actually > > I mean, yeah, overflow) is not a concern anymore, use more lower bits. > > > > Add more folio helpers for PAGE_TYPE_OPS() not to break existing > > slab implementations. > > > > Remove PG_slab check from PAGE_FLAGS_CHECK_AT_FREE. buddy will still > > check if _mapcount is properly set at free. > > > > Exclude PG_slab from hwpoison and show_page_flags() for now. > > > > Note that with this patch, page_mapped() and folio_mapped() always return > > false for slab page. > > > ... > > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > > index 779a426d2cab..9494f47c4cee 100644 > > --- a/mm/memory-failure.c > > +++ b/mm/memory-failure.c > > @@ -1145,7 +1145,6 @@ static int me_huge_page(struct page_state *ps, struct page *p) > > #define mlock (1UL << PG_mlocked) > > #define lru (1UL << PG_lru) > > #define head (1UL << PG_head) > > -#define slab (1UL << PG_slab) > > #define reserved (1UL << PG_reserved) > > > > static struct page_state error_states[] = { > > @@ -1155,13 +1154,6 @@ static struct page_state error_states[] = { > > * PG_buddy pages only make a small fraction of all free pages. > > */ > > > > - /* > > - * Could in theory check if slab page is free or if we can drop > > - * currently unused objects without touching them. But just > > - * treat it as standard kernel for now. > > - */ > > - { slab, slab, MF_MSG_SLAB, me_kernel }, > > - > > Hi Hyeonggon, > Hi Naoya. > Actually the above part is dead code now and it's harmless to remove this. > identify_page_state() is never called when handling memory error event > on a slab page because HWPoisonHandlable() returns false for it. Oh wasn't aware of it. Thanks. That makes it easier. > If you remove it, a few other lines using MF_MSG_SLAB will be unneccessary, > so could you remove them too? Sure. Will split this to separate patch then. > As for testing, you can test this case for example like below: > > - install page-types (available in tools/vm/page-types.c), > - show the list of slab pages by "page-types -b slab -Nl" command > (the first column is PFNs) and choose one PFNs as target, > - call "page-types -a <PFN> -X" (requires hwpoison-inject module to be loaded) > > In my testing server, dmesg shows like below: > > [598746.497805] Injecting memory failure at pfn 0x16295b > [598746.499208] Memory failure: 0x16295b: unhandlable page. > [598746.500570] Memory failure: 0x16295b: recovery action for unknown page: Ignored > > And your patchset seems not to affect this behavior. Cool! Will test it before sending next version. Thanks for looking at this :-) -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type 2022-11-06 14:03 [RFC v2 0/3] move PG_slab to page_type Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 1/3] mm: move PG_slab flag " Hyeonggon Yoo @ 2022-11-06 14:03 ` Hyeonggon Yoo 2022-11-06 18:23 ` Steven Rostedt 2022-11-06 14:03 ` [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type Hyeonggon Yoo 2 siblings, 1 reply; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-06 14:03 UTC (permalink / raw) To: linux-mm Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Hyeonggon Yoo, Steven Rostedt, Masami Hiramatsu, Andrey Konovalov, Marco Elver, Vasily Averin, NeilBrown Some page flags are not actually set in 'flags' field. To provide better understanding of tracepoint output, introduce show_page_types() that shows page flags from page_type. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andrey Konovalov <andreyknvl@gmail.com> Cc: Marco Elver <elver@google.com> Cc: Vasily Averin <vasily.averin@linux.dev> Cc: NeilBrown <neilb@suse.de> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- include/trace/events/mmflags.h | 12 ++++++++++++ include/trace/events/page_ref.h | 10 ++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h index 72c11a16f771..a8dfb98a4dd6 100644 --- a/include/trace/events/mmflags.h +++ b/include/trace/events/mmflags.h @@ -136,6 +136,18 @@ IF_HAVE_PG_SKIP_KASAN_POISON(PG_skip_kasan_poison, "skip_kasan_poison") __def_pageflag_names \ ) : "none" +#define __def_pagetype_names \ + {PG_slab, "slab" }, \ + {PG_offline, "offline" }, \ + {PG_guard, "guard" }, \ + {PG_table, "table" }, \ + {PG_buddy, "buddy" } + +#define show_page_types(page_type) \ + page_type_has_type(page_type) ? \ + __print_flags((~page_type), "|", __def_pagetype_names) \ + : "none" + #if defined(CONFIG_X86) #define __VM_ARCH_SPECIFIC_1 {VM_PAT, "pat" } #elif defined(CONFIG_PPC) diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h index 8a99c1cd417b..b00d23e90e93 100644 --- a/include/trace/events/page_ref.h +++ b/include/trace/events/page_ref.h @@ -21,6 +21,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, __field(unsigned long, flags) __field(int, count) __field(int, mapcount) + __field(unsigned int, page_type) __field(void *, mapping) __field(int, mt) __field(int, val) @@ -31,14 +32,16 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, __entry->flags = page->flags; __entry->count = page_ref_count(page); __entry->mapcount = page_mapcount(page); + __entry->page_type = page->page_type; __entry->mapping = page->mapping; __entry->mt = get_pageblock_migratetype(page); __entry->val = v; ), - TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", + TP_printk("pfn=0x%lx flags=%s page_type=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", __entry->pfn, show_page_flags(__entry->flags & PAGEFLAGS_MASK), + show_page_types(__entry->page_type & PAGE_TYPE_MASK), __entry->count, __entry->mapcount, __entry->mapping, __entry->mt, __entry->val) @@ -69,6 +72,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, __field(unsigned long, flags) __field(int, count) __field(int, mapcount) + __field(unsigned int, page_type) __field(void *, mapping) __field(int, mt) __field(int, val) @@ -80,15 +84,17 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, __entry->flags = page->flags; __entry->count = page_ref_count(page); __entry->mapcount = page_mapcount(page); + __entry->page_type = page->page_type; __entry->mapping = page->mapping; __entry->mt = get_pageblock_migratetype(page); __entry->val = v; __entry->ret = ret; ), - TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", + TP_printk("pfn=0x%lx flags=%s page_type=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", __entry->pfn, show_page_flags(__entry->flags & PAGEFLAGS_MASK), + show_page_types(__entry->page_type & PAGE_TYPE_MASK), __entry->count, __entry->mapcount, __entry->mapping, __entry->mt, __entry->val, __entry->ret) -- 2.32.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type 2022-11-06 14:03 ` [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type Hyeonggon Yoo @ 2022-11-06 18:23 ` Steven Rostedt 2022-11-09 6:19 ` Hyeonggon Yoo 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2022-11-06 18:23 UTC (permalink / raw) To: Hyeonggon Yoo Cc: linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Masami Hiramatsu, Andrey Konovalov, Marco Elver, Vasily Averin, NeilBrown On Sun, 6 Nov 2022 23:03:54 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > Some page flags are not actually set in 'flags' field. To provide > better understanding of tracepoint output, introduce show_page_types() > that shows page flags from page_type. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andrey Konovalov <andreyknvl@gmail.com> > Cc: Marco Elver <elver@google.com> > Cc: Vasily Averin <vasily.averin@linux.dev> > Cc: NeilBrown <neilb@suse.de> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > --- > include/trace/events/mmflags.h | 12 ++++++++++++ > include/trace/events/page_ref.h | 10 ++++++++-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h > index 72c11a16f771..a8dfb98a4dd6 100644 > --- a/include/trace/events/mmflags.h > +++ b/include/trace/events/mmflags.h > @@ -136,6 +136,18 @@ IF_HAVE_PG_SKIP_KASAN_POISON(PG_skip_kasan_poison, "skip_kasan_poison") > __def_pageflag_names \ > ) : "none" > > +#define __def_pagetype_names \ > + {PG_slab, "slab" }, \ > + {PG_offline, "offline" }, \ > + {PG_guard, "guard" }, \ > + {PG_table, "table" }, \ > + {PG_buddy, "buddy" } > + > +#define show_page_types(page_type) \ > + page_type_has_type(page_type) ? \ > + __print_flags((~page_type), "|", __def_pagetype_names) \ > + : "none" > + > #if defined(CONFIG_X86) > #define __VM_ARCH_SPECIFIC_1 {VM_PAT, "pat" } > #elif defined(CONFIG_PPC) > diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h > index 8a99c1cd417b..b00d23e90e93 100644 > --- a/include/trace/events/page_ref.h > +++ b/include/trace/events/page_ref.h > @@ -21,6 +21,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, > __field(unsigned long, flags) > __field(int, count) > __field(int, mapcount) > + __field(unsigned int, page_type) Be careful were you add int fields for 64 bit machines. The above is going to add 4 bytes of nothing in the ring buffer for each event. Please try to keep ints together by 2s, especially between long and pointer fields. > __field(void *, mapping) > __field(int, mt) > __field(int, val) > @@ -31,14 +32,16 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, > __entry->flags = page->flags; > __entry->count = page_ref_count(page); > __entry->mapcount = page_mapcount(page); > + __entry->page_type = page->page_type; > __entry->mapping = page->mapping; > __entry->mt = get_pageblock_migratetype(page); > __entry->val = v; > ), > > - TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", > + TP_printk("pfn=0x%lx flags=%s page_type=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", > __entry->pfn, > show_page_flags(__entry->flags & PAGEFLAGS_MASK), > + show_page_types(__entry->page_type & PAGE_TYPE_MASK), > __entry->count, > __entry->mapcount, __entry->mapping, __entry->mt, > __entry->val) > @@ -69,6 +72,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, > __field(unsigned long, flags) > __field(int, count) > __field(int, mapcount) > + __field(unsigned int, page_type) Here too. -- Steve > __field(void *, mapping) > __field(int, mt) > __field(int, val) > @@ -80,15 +84,17 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, > __entry->flags = page->flags; > __entry->count = page_ref_count(page); > __entry->mapcount = page_mapcount(page); > + __entry->page_type = page->page_type; > __entry->mapping = page->mapping; > __entry->mt = get_pageblock_migratetype(page); > __entry->val = v; > __entry->ret = ret; > ), > > - TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", > + TP_printk("pfn=0x%lx flags=%s page_type=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", > __entry->pfn, > show_page_flags(__entry->flags & PAGEFLAGS_MASK), > + show_page_types(__entry->page_type & PAGE_TYPE_MASK), > __entry->count, > __entry->mapcount, __entry->mapping, __entry->mt, > __entry->val, __entry->ret) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type 2022-11-06 18:23 ` Steven Rostedt @ 2022-11-09 6:19 ` Hyeonggon Yoo 0 siblings, 0 replies; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-09 6:19 UTC (permalink / raw) To: Steven Rostedt Cc: linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Masami Hiramatsu, Andrey Konovalov, Marco Elver, Vasily Averin, NeilBrown On Sun, Nov 06, 2022 at 01:23:15PM -0500, Steven Rostedt wrote: > On Sun, 6 Nov 2022 23:03:54 +0900 > Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > > Some page flags are not actually set in 'flags' field. To provide > > better understanding of tracepoint output, introduce show_page_types() > > that shows page flags from page_type. > > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Andrey Konovalov <andreyknvl@gmail.com> > > Cc: Marco Elver <elver@google.com> > > Cc: Vasily Averin <vasily.averin@linux.dev> > > Cc: NeilBrown <neilb@suse.de> > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > --- > > include/trace/events/mmflags.h | 12 ++++++++++++ > > include/trace/events/page_ref.h | 10 ++++++++-- > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h > > index 72c11a16f771..a8dfb98a4dd6 100644 > > --- a/include/trace/events/mmflags.h > > +++ b/include/trace/events/mmflags.h > > @@ -136,6 +136,18 @@ IF_HAVE_PG_SKIP_KASAN_POISON(PG_skip_kasan_poison, "skip_kasan_poison") > > __def_pageflag_names \ > > ) : "none" > > > > +#define __def_pagetype_names \ > > + {PG_slab, "slab" }, \ > > + {PG_offline, "offline" }, \ > > + {PG_guard, "guard" }, \ > > + {PG_table, "table" }, \ > > + {PG_buddy, "buddy" } > > + > > +#define show_page_types(page_type) \ > > + page_type_has_type(page_type) ? \ > > + __print_flags((~page_type), "|", __def_pagetype_names) \ > > + : "none" > > + > > #if defined(CONFIG_X86) > > #define __VM_ARCH_SPECIFIC_1 {VM_PAT, "pat" } > > #elif defined(CONFIG_PPC) > > diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h > > index 8a99c1cd417b..b00d23e90e93 100644 > > --- a/include/trace/events/page_ref.h > > +++ b/include/trace/events/page_ref.h > > @@ -21,6 +21,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, > > __field(unsigned long, flags) > > __field(int, count) > > __field(int, mapcount) > > + __field(unsigned int, page_type) > > Be careful were you add int fields for 64 bit machines. > > The above is going to add 4 bytes of nothing in the ring buffer for > each event. Please try to keep ints together by 2s, especially between > long and pointer fields. Oh, I wasn't aware of it. I think I can use the field 'mapcount' to represent both mapcount and page_type as they have same offset and size within the structure. Just wondering where this constraint came from... > > > __field(void *, mapping) > > __field(int, mt) > > __field(int, val) > > @@ -31,14 +32,16 @@ DECLARE_EVENT_CLASS(page_ref_mod_template, > > __entry->flags = page->flags; > > __entry->count = page_ref_count(page); > > __entry->mapcount = page_mapcount(page); > > + __entry->page_type = page->page_type; > > __entry->mapping = page->mapping; > > __entry->mt = get_pageblock_migratetype(page); > > __entry->val = v; > > ), > > > > - TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", > > + TP_printk("pfn=0x%lx flags=%s page_type=%s count=%d mapcount=%d mapping=%p mt=%d val=%d", > > __entry->pfn, > > show_page_flags(__entry->flags & PAGEFLAGS_MASK), > > + show_page_types(__entry->page_type & PAGE_TYPE_MASK), > > __entry->count, > > __entry->mapcount, __entry->mapping, __entry->mt, > > __entry->val) > > @@ -69,6 +72,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, > > __field(unsigned long, flags) > > __field(int, count) > > __field(int, mapcount) > > + __field(unsigned int, page_type) > > Here too. > > -- Steve > > > __field(void *, mapping) > > __field(int, mt) > > __field(int, val) > > @@ -80,15 +84,17 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template, > > __entry->flags = page->flags; > > __entry->count = page_ref_count(page); > > __entry->mapcount = page_mapcount(page); > > + __entry->page_type = page->page_type; > > __entry->mapping = page->mapping; > > __entry->mt = get_pageblock_migratetype(page); > > __entry->val = v; > > __entry->ret = ret; > > ), > > > > - TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", > > + TP_printk("pfn=0x%lx flags=%s page_type=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d", > > __entry->pfn, > > show_page_flags(__entry->flags & PAGEFLAGS_MASK), > > + show_page_types(__entry->page_type & PAGE_TYPE_MASK), > > __entry->count, > > __entry->mapcount, __entry->mapping, __entry->mt, > > __entry->val, __entry->ret) > -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-06 14:03 [RFC v2 0/3] move PG_slab to page_type Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 1/3] mm: move PG_slab flag " Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type Hyeonggon Yoo @ 2022-11-06 14:03 ` Hyeonggon Yoo 2022-11-06 18:04 ` Joe Perches 2022-11-07 11:18 ` Andy Shevchenko 2 siblings, 2 replies; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-06 14:03 UTC (permalink / raw) To: linux-mm Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Hyeonggon Yoo, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel dump_page() uses %pGp format to print 'flags' field of struct page. As some page flags (e.g. PG_buddy, see page-flags.h for more details) are set in page_type field, introduce %pGt format which provides human readable output of page_type. And use it in dump_page(). Note that the sense of bits are different in page_type. if page_type is 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, page_type is 0xffefffff. Clearing a bit means we set the bit. Bits in page_type are inverted when printing type names. Below is examples of dump_page(). One is just after alloc_pages() and the other is after __SetPageSlab(). [ 1.814728] page:ffffea000415e200 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x105788 [ 1.815961] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) [ 1.816443] page_type: 0xffffffff() [ 1.816704] raw: 0017ffffc0000000 0000000000000000 dead000000000122 0000000000000000 [ 1.817291] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000 [ 1.817870] page dumped because: Before __SetPageSlab() [ 1.818258] page:ffffea000415e200 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x105788 [ 1.818857] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) [ 1.819250] page_type: 0xffefffff(slab) [ 1.819483] raw: 0017ffffc0000000 0000000000000000 dead000000000122 0000000000000000 [ 1.819947] raw: 0000000000000000 0000000000000000 00000001ffefffff 0000000000000000 [ 1.820410] page dumped because: After __SetPageSlab() Cc: Petr Mladek <pmladek@suse.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-doc@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-mm@kvack.org Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- Documentation/core-api/printk-formats.rst | 3 ++- lib/test_printf.c | 23 ++++++++++++++++++++++ lib/vsprintf.c | 24 +++++++++++++++++++++++ mm/debug.c | 7 +++++++ mm/internal.h | 1 + 5 files changed, 57 insertions(+), 1 deletion(-) diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index dbe1aacc79d0..582e965508eb 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -575,12 +575,13 @@ The field width is passed by value, the bitmap is passed by reference. Helper macros cpumask_pr_args() and nodemask_pr_args() are available to ease printing cpumask and nodemask. -Flags bitfields such as page flags, gfp_flags +Flags bitfields such as page flags, page_type, gfp_flags --------------------------------------------- :: %pGp 0x17ffffc0002036(referenced|uptodate|lru|active|private|node=0|zone=2|lastcpupid=0x1fffff) + %pGt 0xffefffff(slab) %pGg GFP_USER|GFP_DMA32|GFP_NOWARN %pGv read|exec|mayread|maywrite|mayexec|denywrite diff --git a/lib/test_printf.c b/lib/test_printf.c index fe13de1bed5f..6b778a8ea44c 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -654,12 +654,26 @@ page_flags_test(int section, int node, int zone, int last_cpupid, test(cmp_buf, "%pGp", &flags); } +static void __init page_type_test(unsigned int page_type, const char *name, + char *cmp_buf) +{ + unsigned long size; + + size = scnprintf(cmp_buf, BUF_SIZE, "%#x(", page_type); + if (page_type_has_type(page_type)) + size += scnprintf(cmp_buf + size, BUF_SIZE - size, "%s", name); + + snprintf(cmp_buf + size, BUF_SIZE - size, ")"); + test(cmp_buf, "%pGt", &page_type); +} + static void __init flags(void) { unsigned long flags; char *cmp_buffer; gfp_t gfp; + unsigned int page_type; cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL); if (!cmp_buffer) @@ -699,6 +713,15 @@ flags(void) gfp |= __GFP_HIGH; test(cmp_buffer, "%pGg", &gfp); + page_type = ~0; + page_type_test(page_type, "", cmp_buffer); + + page_type = ~PG_slab; + page_type_test(page_type, "slab", cmp_buffer); + + page_type = ~(PG_slab | PG_table | PG_buddy); + page_type_test(page_type, "slab|table|buddy", cmp_buffer); + kfree(cmp_buffer); } diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 24f37bab8bc1..d855b40e5cfd 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2056,6 +2056,28 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) return buf; } +static +char *format_page_type(char *buf, char *end, unsigned int page_type) +{ + if (!(page_type & PAGE_TYPE_BASE)) + return string(buf, end, "no type for user-mapped page", default_str_spec); + + buf = number(buf, end, page_type, default_flag_spec); + + if (buf < end) + *buf = '('; + buf++; + + if (page_type_has_type(page_type)) + buf = format_flags(buf, end, ~page_type, pagetype_names); + + if (buf < end) + *buf = ')'; + buf++; + + return buf; +} + static noinline_for_stack char *flags_string(char *buf, char *end, void *flags_ptr, struct printf_spec spec, const char *fmt) @@ -2069,6 +2091,8 @@ char *flags_string(char *buf, char *end, void *flags_ptr, switch (fmt[1]) { case 'p': return format_page_flags(buf, end, *(unsigned long *)flags_ptr); + case 't': + return format_page_type(buf, end, *(unsigned int *)flags_ptr); case 'v': flags = *(unsigned long *)flags_ptr; names = vmaflag_names; diff --git a/mm/debug.c b/mm/debug.c index 0fd15ba70d16..bb7f2278abc5 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -36,6 +36,11 @@ const struct trace_print_flags pageflag_names[] = { {0, NULL} }; +const struct trace_print_flags pagetype_names[] = { + __def_pagetype_names, + {0, NULL} +}; + const struct trace_print_flags gfpflag_names[] = { __def_gfpflag_names, {0, NULL} @@ -114,6 +119,8 @@ static void __dump_page(struct page *page) pr_warn("%sflags: %pGp%s\n", type, &head->flags, page_cma ? " CMA" : ""); + pr_warn("page_type: %pGt\n", &head->page_type); + print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, sizeof(unsigned long), page, sizeof(struct page), false); diff --git a/mm/internal.h b/mm/internal.h index cb4c663a714e..956eaa9f12c0 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -773,6 +773,7 @@ static inline void flush_tlb_batched_pending(struct mm_struct *mm) #endif /* CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH */ extern const struct trace_print_flags pageflag_names[]; +extern const struct trace_print_flags pagetype_names[]; extern const struct trace_print_flags vmaflag_names[]; extern const struct trace_print_flags gfpflag_names[]; -- 2.32.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-06 14:03 ` [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type Hyeonggon Yoo @ 2022-11-06 18:04 ` Joe Perches 2022-11-09 6:14 ` Hyeonggon Yoo 2022-11-07 11:18 ` Andy Shevchenko 1 sibling, 1 reply; 15+ messages in thread From: Joe Perches @ 2022-11-06 18:04 UTC (permalink / raw) To: Hyeonggon Yoo, linux-mm Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Sun, 2022-11-06 at 23:03 +0900, Hyeonggon Yoo wrote: > dump_page() uses %pGp format to print 'flags' field of struct page. > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > are set in page_type field, introduce %pGt format which provides > human readable output of page_type. And use it in dump_page(). [] > diff --git a/lib/vsprintf.c b/lib/vsprintf.c [] > @@ -2056,6 +2056,28 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > return buf; > } > > +static noinline_for_stack ? > +char *format_page_type(char *buf, char *end, unsigned int page_type) > +{ > + if (!(page_type & PAGE_TYPE_BASE)) > + return string(buf, end, "no type for user-mapped page", default_str_spec); Might be better with something like '%pGt: no type..." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-06 18:04 ` Joe Perches @ 2022-11-09 6:14 ` Hyeonggon Yoo 2022-11-09 8:13 ` Petr Mladek 0 siblings, 1 reply; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-09 6:14 UTC (permalink / raw) To: Joe Perches Cc: linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Sun, Nov 06, 2022 at 10:04:25AM -0800, Joe Perches wrote: > On Sun, 2022-11-06 at 23:03 +0900, Hyeonggon Yoo wrote: > > dump_page() uses %pGp format to print 'flags' field of struct page. > > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > > are set in page_type field, introduce %pGt format which provides > > human readable output of page_type. And use it in dump_page(). > [] > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] > > @@ -2056,6 +2056,28 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > > return buf; > > } > > > > +static Thanks for looking at this. > > noinline_for_stack ? May I ask why, Does it have issues related to stack consumption? To Be Honest I'm a bit unsure what is purpose of this attribute. > > +char *format_page_type(char *buf, char *end, unsigned int page_type) > > +{ > > + if (!(page_type & PAGE_TYPE_BASE)) > > + return string(buf, end, "no type for user-mapped page", default_str_spec); > > Might be better with something like '%pGt: no type..." Will try something like "0x32()" when it has no type because it is mapped to userspace. -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-09 6:14 ` Hyeonggon Yoo @ 2022-11-09 8:13 ` Petr Mladek 0 siblings, 0 replies; 15+ messages in thread From: Petr Mladek @ 2022-11-09 8:13 UTC (permalink / raw) To: Hyeonggon Yoo Cc: Joe Perches, linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Wed 2022-11-09 15:14:05, Hyeonggon Yoo wrote: > On Sun, Nov 06, 2022 at 10:04:25AM -0800, Joe Perches wrote: > > On Sun, 2022-11-06 at 23:03 +0900, Hyeonggon Yoo wrote: > > > dump_page() uses %pGp format to print 'flags' field of struct page. > > > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > > > are set in page_type field, introduce %pGt format which provides > > > human readable output of page_type. And use it in dump_page(). > > [] > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > [] > > > @@ -2056,6 +2056,28 @@ char *format_page_flags(char *buf, char *end, unsigned long flags) > > > return buf; > > > } > > > > > > +static > > Thanks for looking at this. > > > > > noinline_for_stack ? Honestly, I do not like much adding this without numbers. It has been added to some functions in vsprintf.c long time ago because it reduced the stack usage. But I think that it is a compiler and an architecture specific. And it is not clear if it would really help in this particular case. Feel free to omit it. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-06 14:03 ` [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type Hyeonggon Yoo 2022-11-06 18:04 ` Joe Perches @ 2022-11-07 11:18 ` Andy Shevchenko 2022-11-07 14:20 ` Petr Mladek 1 sibling, 1 reply; 15+ messages in thread From: Andy Shevchenko @ 2022-11-07 11:18 UTC (permalink / raw) To: Hyeonggon Yoo Cc: linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Sun, Nov 06, 2022 at 11:03:55PM +0900, Hyeonggon Yoo wrote: > dump_page() uses %pGp format to print 'flags' field of struct page. > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > are set in page_type field, introduce %pGt format which provides > human readable output of page_type. And use it in dump_page(). > > Note that the sense of bits are different in page_type. if page_type is > 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, > page_type is 0xffefffff. Clearing a bit means we set the bit. Bits in > page_type are inverted when printing type names. > > Below is examples of dump_page(). One is just after alloc_pages() and > the other is after __SetPageSlab(). > > [ 1.814728] page:ffffea000415e200 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x105788 > [ 1.815961] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) > [ 1.816443] page_type: 0xffffffff() Why do we have empty parentheses? I would expect either something there, or no parentheses at all. > [ 1.816704] raw: 0017ffffc0000000 0000000000000000 dead000000000122 0000000000000000 > [ 1.817291] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000 > [ 1.817870] page dumped because: Before __SetPageSlab() > > [ 1.818258] page:ffffea000415e200 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x105788 > [ 1.818857] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) > [ 1.819250] page_type: 0xffefffff(slab) > [ 1.819483] raw: 0017ffffc0000000 0000000000000000 dead000000000122 0000000000000000 > [ 1.819947] raw: 0000000000000000 0000000000000000 00000001ffefffff 0000000000000000 > [ 1.820410] page dumped because: After __SetPageSlab() > Cc: Petr Mladek <pmladek@suse.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-doc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mm@kvack.org Can you utilize --cc parameter next time and avoid polluting commit message with this? We have archives where anybody can check this (and usually maintainers add a Link tag for that purpose). ... > + %pGt 0xffefffff(slab) No space before ( ? ... > +static > +char *format_page_type(char *buf, char *end, unsigned int page_type) > +{ > + if (!(page_type & PAGE_TYPE_BASE)) > + return string(buf, end, "no type for user-mapped page", default_str_spec); It's too long, can we make it shorten? > + buf = number(buf, end, page_type, default_flag_spec); > + > + if (buf < end) > + *buf = '('; > + buf++; > + if (page_type_has_type(page_type)) This should be check for the entire function. > + buf = format_flags(buf, end, ~page_type, pagetype_names); > + > + if (buf < end) > + *buf = ')'; > + buf++; > + > + return buf; > +} ... > @@ -36,6 +36,11 @@ const struct trace_print_flags pageflag_names[] = { > {0, NULL} > }; > > +const struct trace_print_flags pagetype_names[] = { > + __def_pagetype_names, > + {0, NULL} Hmm... I see it's already done like this above, but {} would suffice, why not to convert the rest first to the {} and use it here? > +}; ... > pr_warn("%sflags: %pGp%s\n", type, &head->flags, > page_cma ? " CMA" : ""); > + pr_warn("page_type: %pGt\n", &head->page_type); > + > print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, > sizeof(unsigned long), page, > sizeof(struct page), false); > diff --git a/mm/internal.h b/mm/internal.h > index cb4c663a714e..956eaa9f12c0 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -773,6 +773,7 @@ static inline void flush_tlb_batched_pending(struct mm_struct *mm) > #endif /* CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH */ > > extern const struct trace_print_flags pageflag_names[]; > +extern const struct trace_print_flags pagetype_names[]; > extern const struct trace_print_flags vmaflag_names[]; > extern const struct trace_print_flags gfpflag_names[]; I would split this to a separate change, but it's up to PRINTK maintainers. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-07 11:18 ` Andy Shevchenko @ 2022-11-07 14:20 ` Petr Mladek 2022-11-07 14:41 ` Andy Shevchenko 2022-11-09 6:04 ` Hyeonggon Yoo 0 siblings, 2 replies; 15+ messages in thread From: Petr Mladek @ 2022-11-07 14:20 UTC (permalink / raw) To: Andy Shevchenko Cc: Hyeonggon Yoo, linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Mon 2022-11-07 13:18:52, Andy Shevchenko wrote: > On Sun, Nov 06, 2022 at 11:03:55PM +0900, Hyeonggon Yoo wrote: > > dump_page() uses %pGp format to print 'flags' field of struct page. > > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > > are set in page_type field, introduce %pGt format which provides > > human readable output of page_type. And use it in dump_page(). > > > > Note that the sense of bits are different in page_type. if page_type is > > 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, > > page_type is 0xffefffff. Clearing a bit means we set the bit. Bits in > > page_type are inverted when printing type names. > > > > Below is examples of dump_page(). One is just after alloc_pages() and > > the other is after __SetPageSlab(). > > > > [ 1.814728] page:ffffea000415e200 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x105788 > > [ 1.815961] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) > > > [ 1.816443] page_type: 0xffffffff() > > Why do we have empty parentheses? I would expect either something there, or no > parentheses at all. This looks fine. format_page_flags() does the same for %pGp. > ... > > > + %pGt 0xffefffff(slab) > > No space before ( ? Also looks fine. %pGp does the same. > ... > > > +static > > +char *format_page_type(char *buf, char *end, unsigned int page_type) > > +{ > > + if (!(page_type & PAGE_TYPE_BASE)) > > + return string(buf, end, "no type for user-mapped page", default_str_spec); > > It's too long, can we make it shorten? I wonder if it would help to write the value. Something like: page_type: 0x0ace5768(no type) That said. I am not familiar with the page types and am not sure about the semantic of this value. MM people should decide what they want to see in this case. > ... > > > pr_warn("%sflags: %pGp%s\n", type, &head->flags, > > page_cma ? " CMA" : ""); > > + pr_warn("page_type: %pGt\n", &head->page_type); > > + > > print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, > > sizeof(unsigned long), page, > > sizeof(struct page), false); > > diff --git a/mm/internal.h b/mm/internal.h > > index cb4c663a714e..956eaa9f12c0 100644 > > --- a/mm/internal.h > > +++ b/mm/internal.h > > @@ -773,6 +773,7 @@ static inline void flush_tlb_batched_pending(struct mm_struct *mm) > > #endif /* CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH */ > > > > extern const struct trace_print_flags pageflag_names[]; > > +extern const struct trace_print_flags pagetype_names[]; > > extern const struct trace_print_flags vmaflag_names[]; > > extern const struct trace_print_flags gfpflag_names[]; > > I would split this to a separate change, but it's up to PRINTK maintainers. I guess that you are talking about the line: + pr_warn("page_type: %pGt\n", &head->page_type); Yes, it would be better to have implementation of %pGt modifier in one patch and add the user in another one. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-07 14:20 ` Petr Mladek @ 2022-11-07 14:41 ` Andy Shevchenko 2022-11-09 6:04 ` Hyeonggon Yoo 1 sibling, 0 replies; 15+ messages in thread From: Andy Shevchenko @ 2022-11-07 14:41 UTC (permalink / raw) To: Petr Mladek Cc: Hyeonggon Yoo, linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Mon, Nov 07, 2022 at 03:20:34PM +0100, Petr Mladek wrote: > On Mon 2022-11-07 13:18:52, Andy Shevchenko wrote: > > On Sun, Nov 06, 2022 at 11:03:55PM +0900, Hyeonggon Yoo wrote: > > > dump_page() uses %pGp format to print 'flags' field of struct page. ... > > > [ 1.816443] page_type: 0xffffffff() > > > > Why do we have empty parentheses? I would expect either something there, or no > > parentheses at all. > > This looks fine. format_page_flags() does the same for %pGp. ... > > > + %pGt 0xffefffff(slab) > > > > No space before ( ? > > Also looks fine. %pGp does the same. Maybe we can modify them before or after this change? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type 2022-11-07 14:20 ` Petr Mladek 2022-11-07 14:41 ` Andy Shevchenko @ 2022-11-09 6:04 ` Hyeonggon Yoo 1 sibling, 0 replies; 15+ messages in thread From: Hyeonggon Yoo @ 2022-11-09 6:04 UTC (permalink / raw) To: Petr Mladek Cc: Andy Shevchenko, linux-mm, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka, Naoya Horiguchi, Miaohe Lin, Matthew Wilcox, Minchan Kim, Mel Gorman, Andrea Arcangeli, Dan Williams, Hugh Dickins, Muchun Song, David Hildenbrand, Steven Rostedt, Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet, linux-doc, linux-kernel On Mon, Nov 07, 2022 at 03:20:34PM +0100, Petr Mladek wrote: > On Mon 2022-11-07 13:18:52, Andy Shevchenko wrote: > > On Sun, Nov 06, 2022 at 11:03:55PM +0900, Hyeonggon Yoo wrote: > > > dump_page() uses %pGp format to print 'flags' field of struct page. > > > As some page flags (e.g. PG_buddy, see page-flags.h for more details) > > > are set in page_type field, introduce %pGt format which provides > > > human readable output of page_type. And use it in dump_page(). > > > > > > Note that the sense of bits are different in page_type. if page_type is > > > 0xffffffff, no flags are set. if PG_slab (0x00100000) flag is set, > > > page_type is 0xffefffff. Clearing a bit means we set the bit. Bits in > > > page_type are inverted when printing type names. > > > > > > Below is examples of dump_page(). One is just after alloc_pages() and > > > the other is after __SetPageSlab(). > > > > > > [ 1.814728] page:ffffea000415e200 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x105788 > > > [ 1.815961] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff) > > > > > [ 1.816443] page_type: 0xffffffff() > > Thank you both for looking at this! :-) > > Why do we have empty parentheses? I would expect either something there, or no > > parentheses at all. > > This looks fine. format_page_flags() does the same for %pGp. > > > ... > > > > > + %pGt 0xffefffff(slab) > > > > No space before ( ? > > Also looks fine. %pGp does the same. > > > ... > > > > > +static > > > +char *format_page_type(char *buf, char *end, unsigned int page_type) > > > +{ > > > + if (!(page_type & PAGE_TYPE_BASE)) > > > + return string(buf, end, "no type for user-mapped page", default_str_spec); > > > > It's too long, can we make it shorten? > > I wonder if it would help to write the value. Something like: > > page_type: 0x0ace5768(no type) > > That said. I am not familiar with the page types and am not sure > about the semantic of this value. MM people should decide what they > want to see in this case. Hmm, then for consistency let's try: - 0xffefffff(slab) - 0xffffffff() - 0x0ace5768() this way. > > ... > > > > > pr_warn("%sflags: %pGp%s\n", type, &head->flags, > > > page_cma ? " CMA" : ""); > > > + pr_warn("page_type: %pGt\n", &head->page_type); > > > + > > > print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32, > > > sizeof(unsigned long), page, > > > sizeof(struct page), false); > > > diff --git a/mm/internal.h b/mm/internal.h > > > index cb4c663a714e..956eaa9f12c0 100644 > > > --- a/mm/internal.h > > > +++ b/mm/internal.h > > > @@ -773,6 +773,7 @@ static inline void flush_tlb_batched_pending(struct mm_struct *mm) > > > #endif /* CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH */ > > > > > > extern const struct trace_print_flags pageflag_names[]; > > > +extern const struct trace_print_flags pagetype_names[]; > > > extern const struct trace_print_flags vmaflag_names[]; > > > extern const struct trace_print_flags gfpflag_names[]; > > > > I would split this to a separate change, but it's up to PRINTK maintainers. > > I guess that you are talking about the line: > > + pr_warn("page_type: %pGt\n", &head->page_type); > > > Yes, it would be better to have implementation of %pGt modifier > in one patch and add the user in another one. Sounds reasonable. Will split its caller into another patch next to this one. -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-11-09 8:14 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-06 14:03 [RFC v2 0/3] move PG_slab to page_type Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 1/3] mm: move PG_slab flag " Hyeonggon Yoo 2022-11-08 5:39 ` HORIGUCHI NAOYA(堀口 直也) 2022-11-09 5:45 ` Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 2/3] mm: introduce show_page_types() to provide human-readable page_type Hyeonggon Yoo 2022-11-06 18:23 ` Steven Rostedt 2022-11-09 6:19 ` Hyeonggon Yoo 2022-11-06 14:03 ` [RFC v2 3/3] mm, printk: introduce new format %pGt for page_type Hyeonggon Yoo 2022-11-06 18:04 ` Joe Perches 2022-11-09 6:14 ` Hyeonggon Yoo 2022-11-09 8:13 ` Petr Mladek 2022-11-07 11:18 ` Andy Shevchenko 2022-11-07 14:20 ` Petr Mladek 2022-11-07 14:41 ` Andy Shevchenko 2022-11-09 6:04 ` Hyeonggon Yoo
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).