From: Vlastimil Babka <vbabka@suse.cz>
To: js1304@gmail.com, Andrew Morton <akpm@linux-foundation.org>
Cc: Hugh Dickins <hughd@google.com>,
Johannes Berg <johannes@sipsolutions.net>,
"David S. Miller" <davem@davemloft.net>,
Sunil Goutham <sgoutham@cavium.com>,
Chris Metcalf <cmetcalf@mellanox.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH v2 2/2] mm: rename _count, field of the struct page, to _refcount
Date: Tue, 29 Mar 2016 11:27:47 +0200 [thread overview]
Message-ID: <56FA4A93.6090502@suse.cz> (raw)
In-Reply-To: <1459146601-11448-2-git-send-email-iamjoonsoo.kim@lge.com>
On 03/28/2016 08:30 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> Many developer already know that field for reference count of
> the struct page is _count and atomic type. They would try to handle it
> directly and this could break the purpose of page reference count
> tracepoint. To prevent direct _count modification, this patch rename it
> to _refcount and add warning message on the code. After that, developer
> who need to handle reference count will find that field should not be
> accessed directly.
>
> v2: change more _count usages to _refcount
There's also
Documentation/vm/transhuge.txt talking about ->_count
include/linux/mm.h: * requires to already have an elevated page->_count.
include/linux/mm_types.h: * Keep _count separate from slub cmpxchg_double data.
include/linux/mm_types.h: * slab_lock but _count is not.
include/linux/pagemap.h: * If the page is free (_count == 0), then _count is untouched, and 0
include/linux/pagemap.h: * is returned. Otherwise, _count is incremented by 1 and 1 is returned.
include/linux/pagemap.h: * this allows allocators to use a synchronize_rcu() to stabilize _count.
include/linux/pagemap.h: * Remove-side that cares about stability of _count (eg. reclaim) has the
mm/huge_memory.c: * tail_page->_count is zero and not changing from under us. But
mm/huge_memory.c: /* Prevent deferred_split_scan() touching ->_count */
mm/internal.h: * Turn a non-refcounted page (->_count == 0) into refcounted with
mm/page_alloc.c: bad_reason = "nonzero _count";
mm/page_alloc.c: bad_reason = "nonzero _count";
mm/page_alloc.c: * because their page->_count is zero at all time.
mm/slub.c: * as page->_count. If we assign to ->counters directly
mm/slub.c: * we run the risk of losing updates to page->_count, so
mm/vmscan.c: * load is not satisfied before that of page->_count.
mm/vmscan.c: * The downside is that we have to touch page->_count against each page.
I've arrived at the following command to find this:
git grep "[^a-zA-Z0-9_]_count[^_]"
Not that many false positives in the output :)
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
> arch/tile/mm/init.c | 2 +-
> include/linux/mm_types.h | 8 ++++++--
> include/linux/page_ref.h | 26 +++++++++++++-------------
> kernel/kexec_core.c | 2 +-
> 4 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/arch/tile/mm/init.c b/arch/tile/mm/init.c
> index a0582b7..adce254 100644
> --- a/arch/tile/mm/init.c
> +++ b/arch/tile/mm/init.c
> @@ -679,7 +679,7 @@ static void __init init_free_pfn_range(unsigned long start, unsigned long end)
> * Hacky direct set to avoid unnecessary
> * lock take/release for EVERY page here.
> */
> - p->_count.counter = 0;
> + p->_refcount.counter = 0;
> p->_mapcount.counter = -1;
> }
> init_page_count(page);
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 944b2b3..9e8eb5a 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,7 +97,11 @@ struct page {
> };
> int units; /* SLOB */
> };
> - atomic_t _count; /* Usage count, see below. */
> + /*
> + * Usage count, *USE WRAPPER FUNCTION*
> + * when manual accounting. See page_ref.h
> + */
> + atomic_t _refcount;
> };
> unsigned int active; /* SLAB */
> };
> @@ -248,7 +252,7 @@ struct page_frag_cache {
> __u32 offset;
> #endif
> /* we maintain a pagecount bias, so that we dont dirty cache line
> - * containing page->_count every time we allocate a fragment.
> + * containing page->_refcount every time we allocate a fragment.
> */
> unsigned int pagecnt_bias;
> bool pfmemalloc;
> diff --git a/include/linux/page_ref.h b/include/linux/page_ref.h
> index e596d5d9..8b5e0a9 100644
> --- a/include/linux/page_ref.h
> +++ b/include/linux/page_ref.h
> @@ -63,17 +63,17 @@ static inline void __page_ref_unfreeze(struct page *page, int v)
>
> static inline int page_ref_count(struct page *page)
> {
> - return atomic_read(&page->_count);
> + return atomic_read(&page->_refcount);
> }
>
> static inline int page_count(struct page *page)
> {
> - return atomic_read(&compound_head(page)->_count);
> + return atomic_read(&compound_head(page)->_refcount);
> }
>
> static inline void set_page_count(struct page *page, int v)
> {
> - atomic_set(&page->_count, v);
> + atomic_set(&page->_refcount, v);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_set))
> __page_ref_set(page, v);
> }
> @@ -89,35 +89,35 @@ static inline void init_page_count(struct page *page)
>
> static inline void page_ref_add(struct page *page, int nr)
> {
> - atomic_add(nr, &page->_count);
> + atomic_add(nr, &page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, nr);
> }
>
> static inline void page_ref_sub(struct page *page, int nr)
> {
> - atomic_sub(nr, &page->_count);
> + atomic_sub(nr, &page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, -nr);
> }
>
> static inline void page_ref_inc(struct page *page)
> {
> - atomic_inc(&page->_count);
> + atomic_inc(&page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, 1);
> }
>
> static inline void page_ref_dec(struct page *page)
> {
> - atomic_dec(&page->_count);
> + atomic_dec(&page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, -1);
> }
>
> static inline int page_ref_sub_and_test(struct page *page, int nr)
> {
> - int ret = atomic_sub_and_test(nr, &page->_count);
> + int ret = atomic_sub_and_test(nr, &page->_refcount);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_and_test))
> __page_ref_mod_and_test(page, -nr, ret);
> @@ -126,7 +126,7 @@ static inline int page_ref_sub_and_test(struct page *page, int nr)
>
> static inline int page_ref_dec_and_test(struct page *page)
> {
> - int ret = atomic_dec_and_test(&page->_count);
> + int ret = atomic_dec_and_test(&page->_refcount);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_and_test))
> __page_ref_mod_and_test(page, -1, ret);
> @@ -135,7 +135,7 @@ static inline int page_ref_dec_and_test(struct page *page)
>
> static inline int page_ref_dec_return(struct page *page)
> {
> - int ret = atomic_dec_return(&page->_count);
> + int ret = atomic_dec_return(&page->_refcount);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_and_return))
> __page_ref_mod_and_return(page, -1, ret);
> @@ -144,7 +144,7 @@ static inline int page_ref_dec_return(struct page *page)
>
> static inline int page_ref_add_unless(struct page *page, int nr, int u)
> {
> - int ret = atomic_add_unless(&page->_count, nr, u);
> + int ret = atomic_add_unless(&page->_refcount, nr, u);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_unless))
> __page_ref_mod_unless(page, nr, ret);
> @@ -153,7 +153,7 @@ static inline int page_ref_add_unless(struct page *page, int nr, int u)
>
> static inline int page_ref_freeze(struct page *page, int count)
> {
> - int ret = likely(atomic_cmpxchg(&page->_count, count, 0) == count);
> + int ret = likely(atomic_cmpxchg(&page->_refcount, count, 0) == count);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_freeze))
> __page_ref_freeze(page, count, ret);
> @@ -165,7 +165,7 @@ static inline void page_ref_unfreeze(struct page *page, int count)
> VM_BUG_ON_PAGE(page_count(page) != 0, page);
> VM_BUG_ON(count == 0);
>
> - atomic_set(&page->_count, count);
> + atomic_set(&page->_refcount, count);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_unfreeze))
> __page_ref_unfreeze(page, count);
> }
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index f826e11..e0e95b0 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1410,7 +1410,7 @@ static int __init crash_save_vmcoreinfo_init(void)
> VMCOREINFO_STRUCT_SIZE(list_head);
> VMCOREINFO_SIZE(nodemask_t);
> VMCOREINFO_OFFSET(page, flags);
> - VMCOREINFO_OFFSET(page, _count);
> + VMCOREINFO_OFFSET(page, _refcount);
> VMCOREINFO_OFFSET(page, mapping);
> VMCOREINFO_OFFSET(page, lru);
> VMCOREINFO_OFFSET(page, _mapcount);
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Vlastimil Babka <vbabka@suse.cz>
To: js1304@gmail.com, Andrew Morton <akpm@linux-foundation.org>
Cc: Hugh Dickins <hughd@google.com>,
Johannes Berg <johannes@sipsolutions.net>,
"David S. Miller" <davem@davemloft.net>,
Sunil Goutham <sgoutham@cavium.com>,
Chris Metcalf <cmetcalf@mellanox.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH v2 2/2] mm: rename _count, field of the struct page, to _refcount
Date: Tue, 29 Mar 2016 11:27:47 +0200 [thread overview]
Message-ID: <56FA4A93.6090502@suse.cz> (raw)
In-Reply-To: <1459146601-11448-2-git-send-email-iamjoonsoo.kim@lge.com>
On 03/28/2016 08:30 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> Many developer already know that field for reference count of
> the struct page is _count and atomic type. They would try to handle it
> directly and this could break the purpose of page reference count
> tracepoint. To prevent direct _count modification, this patch rename it
> to _refcount and add warning message on the code. After that, developer
> who need to handle reference count will find that field should not be
> accessed directly.
>
> v2: change more _count usages to _refcount
There's also
Documentation/vm/transhuge.txt talking about ->_count
include/linux/mm.h: * requires to already have an elevated page->_count.
include/linux/mm_types.h: * Keep _count separate from slub cmpxchg_double data.
include/linux/mm_types.h: * slab_lock but _count is not.
include/linux/pagemap.h: * If the page is free (_count == 0), then _count is untouched, and 0
include/linux/pagemap.h: * is returned. Otherwise, _count is incremented by 1 and 1 is returned.
include/linux/pagemap.h: * this allows allocators to use a synchronize_rcu() to stabilize _count.
include/linux/pagemap.h: * Remove-side that cares about stability of _count (eg. reclaim) has the
mm/huge_memory.c: * tail_page->_count is zero and not changing from under us. But
mm/huge_memory.c: /* Prevent deferred_split_scan() touching ->_count */
mm/internal.h: * Turn a non-refcounted page (->_count == 0) into refcounted with
mm/page_alloc.c: bad_reason = "nonzero _count";
mm/page_alloc.c: bad_reason = "nonzero _count";
mm/page_alloc.c: * because their page->_count is zero at all time.
mm/slub.c: * as page->_count. If we assign to ->counters directly
mm/slub.c: * we run the risk of losing updates to page->_count, so
mm/vmscan.c: * load is not satisfied before that of page->_count.
mm/vmscan.c: * The downside is that we have to touch page->_count against each page.
I've arrived at the following command to find this:
git grep "[^a-zA-Z0-9_]_count[^_]"
Not that many false positives in the output :)
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
> arch/tile/mm/init.c | 2 +-
> include/linux/mm_types.h | 8 ++++++--
> include/linux/page_ref.h | 26 +++++++++++++-------------
> kernel/kexec_core.c | 2 +-
> 4 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/arch/tile/mm/init.c b/arch/tile/mm/init.c
> index a0582b7..adce254 100644
> --- a/arch/tile/mm/init.c
> +++ b/arch/tile/mm/init.c
> @@ -679,7 +679,7 @@ static void __init init_free_pfn_range(unsigned long start, unsigned long end)
> * Hacky direct set to avoid unnecessary
> * lock take/release for EVERY page here.
> */
> - p->_count.counter = 0;
> + p->_refcount.counter = 0;
> p->_mapcount.counter = -1;
> }
> init_page_count(page);
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 944b2b3..9e8eb5a 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,7 +97,11 @@ struct page {
> };
> int units; /* SLOB */
> };
> - atomic_t _count; /* Usage count, see below. */
> + /*
> + * Usage count, *USE WRAPPER FUNCTION*
> + * when manual accounting. See page_ref.h
> + */
> + atomic_t _refcount;
> };
> unsigned int active; /* SLAB */
> };
> @@ -248,7 +252,7 @@ struct page_frag_cache {
> __u32 offset;
> #endif
> /* we maintain a pagecount bias, so that we dont dirty cache line
> - * containing page->_count every time we allocate a fragment.
> + * containing page->_refcount every time we allocate a fragment.
> */
> unsigned int pagecnt_bias;
> bool pfmemalloc;
> diff --git a/include/linux/page_ref.h b/include/linux/page_ref.h
> index e596d5d9..8b5e0a9 100644
> --- a/include/linux/page_ref.h
> +++ b/include/linux/page_ref.h
> @@ -63,17 +63,17 @@ static inline void __page_ref_unfreeze(struct page *page, int v)
>
> static inline int page_ref_count(struct page *page)
> {
> - return atomic_read(&page->_count);
> + return atomic_read(&page->_refcount);
> }
>
> static inline int page_count(struct page *page)
> {
> - return atomic_read(&compound_head(page)->_count);
> + return atomic_read(&compound_head(page)->_refcount);
> }
>
> static inline void set_page_count(struct page *page, int v)
> {
> - atomic_set(&page->_count, v);
> + atomic_set(&page->_refcount, v);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_set))
> __page_ref_set(page, v);
> }
> @@ -89,35 +89,35 @@ static inline void init_page_count(struct page *page)
>
> static inline void page_ref_add(struct page *page, int nr)
> {
> - atomic_add(nr, &page->_count);
> + atomic_add(nr, &page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, nr);
> }
>
> static inline void page_ref_sub(struct page *page, int nr)
> {
> - atomic_sub(nr, &page->_count);
> + atomic_sub(nr, &page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, -nr);
> }
>
> static inline void page_ref_inc(struct page *page)
> {
> - atomic_inc(&page->_count);
> + atomic_inc(&page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, 1);
> }
>
> static inline void page_ref_dec(struct page *page)
> {
> - atomic_dec(&page->_count);
> + atomic_dec(&page->_refcount);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod))
> __page_ref_mod(page, -1);
> }
>
> static inline int page_ref_sub_and_test(struct page *page, int nr)
> {
> - int ret = atomic_sub_and_test(nr, &page->_count);
> + int ret = atomic_sub_and_test(nr, &page->_refcount);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_and_test))
> __page_ref_mod_and_test(page, -nr, ret);
> @@ -126,7 +126,7 @@ static inline int page_ref_sub_and_test(struct page *page, int nr)
>
> static inline int page_ref_dec_and_test(struct page *page)
> {
> - int ret = atomic_dec_and_test(&page->_count);
> + int ret = atomic_dec_and_test(&page->_refcount);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_and_test))
> __page_ref_mod_and_test(page, -1, ret);
> @@ -135,7 +135,7 @@ static inline int page_ref_dec_and_test(struct page *page)
>
> static inline int page_ref_dec_return(struct page *page)
> {
> - int ret = atomic_dec_return(&page->_count);
> + int ret = atomic_dec_return(&page->_refcount);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_and_return))
> __page_ref_mod_and_return(page, -1, ret);
> @@ -144,7 +144,7 @@ static inline int page_ref_dec_return(struct page *page)
>
> static inline int page_ref_add_unless(struct page *page, int nr, int u)
> {
> - int ret = atomic_add_unless(&page->_count, nr, u);
> + int ret = atomic_add_unless(&page->_refcount, nr, u);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_mod_unless))
> __page_ref_mod_unless(page, nr, ret);
> @@ -153,7 +153,7 @@ static inline int page_ref_add_unless(struct page *page, int nr, int u)
>
> static inline int page_ref_freeze(struct page *page, int count)
> {
> - int ret = likely(atomic_cmpxchg(&page->_count, count, 0) == count);
> + int ret = likely(atomic_cmpxchg(&page->_refcount, count, 0) == count);
>
> if (page_ref_tracepoint_active(__tracepoint_page_ref_freeze))
> __page_ref_freeze(page, count, ret);
> @@ -165,7 +165,7 @@ static inline void page_ref_unfreeze(struct page *page, int count)
> VM_BUG_ON_PAGE(page_count(page) != 0, page);
> VM_BUG_ON(count == 0);
>
> - atomic_set(&page->_count, count);
> + atomic_set(&page->_refcount, count);
> if (page_ref_tracepoint_active(__tracepoint_page_ref_unfreeze))
> __page_ref_unfreeze(page, count);
> }
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index f826e11..e0e95b0 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1410,7 +1410,7 @@ static int __init crash_save_vmcoreinfo_init(void)
> VMCOREINFO_STRUCT_SIZE(list_head);
> VMCOREINFO_SIZE(nodemask_t);
> VMCOREINFO_OFFSET(page, flags);
> - VMCOREINFO_OFFSET(page, _count);
> + VMCOREINFO_OFFSET(page, _refcount);
> VMCOREINFO_OFFSET(page, mapping);
> VMCOREINFO_OFFSET(page, lru);
> VMCOREINFO_OFFSET(page, _mapcount);
>
next prev parent reply other threads:[~2016-03-29 9:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-28 6:30 [PATCH v2 1/2] mm/page_ref: use page_ref helper instead of direct modification of _count js1304
2016-03-28 6:30 ` js1304
2016-03-28 6:30 ` [PATCH v2 2/2] mm: rename _count, field of the struct page, to _refcount js1304
2016-03-28 6:30 ` js1304
2016-03-29 9:27 ` Vlastimil Babka [this message]
2016-03-29 9:27 ` Vlastimil Babka
2016-03-29 19:23 ` Andrew Morton
2016-03-29 19:23 ` Andrew Morton
2016-03-30 8:27 ` Joonsoo Kim
2016-03-30 8:27 ` Joonsoo Kim
2016-03-30 8:29 ` Vlastimil Babka
2016-03-30 8:29 ` Vlastimil Babka
2016-03-29 9:12 ` [PATCH v2 1/2] mm/page_ref: use page_ref helper instead of direct modification of _count Vlastimil Babka
2016-03-29 9:12 ` Vlastimil Babka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56FA4A93.6090502@suse.cz \
--to=vbabka@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=cmetcalf@mellanox.com \
--cc=davem@davemloft.net \
--cc=hughd@google.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=johannes@sipsolutions.net \
--cc=js1304@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=sgoutham@cavium.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.