From: John Hubbard <jhubbard@nvidia.com>
To: Pasha Tatashin <pasha.tatashin@soleen.com>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
linux-m68k@lists.linux-m68k.org, anshuman.khandual@arm.com,
willy@infradead.org, akpm@linux-foundation.org,
william.kucharski@oracle.com, mike.kravetz@oracle.com,
vbabka@suse.cz, geert@linux-m68k.org, schmitzmic@gmail.com,
rostedt@goodmis.org, mingo@redhat.com, hannes@cmpxchg.org,
guro@fb.com, songmuchun@bytedance.com, weixugc@google.com,
gthelen@google.com
Subject: Re: [RFC 3/8] mm: Avoid using set_page_count() in set_page_recounted()
Date: Tue, 26 Oct 2021 11:01:56 -0700 [thread overview]
Message-ID: <cbd20bc4-84ee-fd39-875b-75426bc61219@nvidia.com> (raw)
In-Reply-To: <7b131cb1-68d8-6746-f9c1-2b01d4838869@nvidia.com>
On 10/26/21 10:53, John Hubbard wrote:
> On 10/26/21 10:38, Pasha Tatashin wrote:
>> set_page_refcounted() converts a non-refcounted page that has
>> (page->_refcount == 0) into a refcounted page by setting _refcount to 1,
>>
>> Use page_ref_inc_return() instead to avoid unconditionally overwriting
>> the _refcount value.
>>
>> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
>> ---
>> mm/internal.h | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index cf3cb933eba3..cf345fac6894 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -91,9 +91,12 @@ static inline bool page_evictable(struct page *page)
>> */
>> static inline void set_page_refcounted(struct page *page)
>> {
>> + int refcnt;
>> +
>> VM_BUG_ON_PAGE(PageTail(page), page);
>> VM_BUG_ON_PAGE(page_ref_count(page), page);
>> - set_page_count(page, 1);
>> + refcnt = page_ref_inc_return(page);
>> + VM_BUG_ON_PAGE(refcnt != 1, page);
>
> Hi Pavel,
ohhh, s/Pavel/Pasha/ !
Huge apologies for the name mixup, I had just seen another email...
very sorry about that mistake.
thanks,
--
John Hubbard
NVIDIA
>
> I am acutely uncomfortable with this change, because it changes the
> meaning and behavior of the function to something completely different,
> while leaving the function name unchanged. Furthermore, in relies upon
> debug assertions, rather than a return value (for example) to verify
> that all is well.
>
> I understand where this patchset is going, but this intermediate step is
> not a good move.
>
> Also, for the overall series, if you want to change from
> "set_page_count()" to "inc_and_verify_val_equals_one()", then the way to
> do that is *not* to depend solely on VM_BUG*() to verify. Instead,
> return something like -EBUSY if incrementing the value results in a
> surprise, and let the caller decide how to handle it.
>
> thanks,
next prev parent reply other threads:[~2021-10-26 18:02 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-26 17:38 [RFC 0/8] Hardening page _refcount Pasha Tatashin
2021-10-26 17:38 ` [RFC 1/8] mm: add overflow and underflow checks for page->_refcount Pasha Tatashin
2021-10-26 19:48 ` Matthew Wilcox
2021-10-26 21:34 ` Pasha Tatashin
2021-10-27 1:21 ` Pasha Tatashin
2021-10-27 3:04 ` Matthew Wilcox
2021-10-27 18:22 ` Pasha Tatashin
2021-10-27 7:46 ` Muchun Song
2021-10-27 18:22 ` Pasha Tatashin
2021-10-28 4:08 ` Muchun Song
2021-10-26 17:38 ` [RFC 2/8] mm/hugetlb: remove useless set_page_count() Pasha Tatashin
2021-10-26 18:44 ` Mike Kravetz
2021-10-26 18:50 ` Pasha Tatashin
2021-10-26 21:19 ` Mike Kravetz
2021-10-26 17:38 ` [RFC 3/8] mm: Avoid using set_page_count() in set_page_recounted() Pasha Tatashin
2021-10-26 17:53 ` John Hubbard
2021-10-26 18:01 ` John Hubbard [this message]
2021-10-26 18:14 ` Pasha Tatashin
2021-10-26 18:21 ` Pasha Tatashin
2021-10-27 5:12 ` John Hubbard
2021-10-27 18:27 ` Pasha Tatashin
2021-10-28 1:20 ` John Hubbard
2021-10-28 1:35 ` John Hubbard
2021-11-01 14:30 ` Pasha Tatashin
2021-11-01 19:35 ` John Hubbard
2021-11-01 14:22 ` Pasha Tatashin
2021-11-01 19:31 ` John Hubbard
2021-11-01 19:42 ` John Hubbard
2021-10-26 17:38 ` [RFC 4/8] mm: remove set_page_count() from page_frag_alloc_align Pasha Tatashin
2021-10-26 17:38 ` [RFC 5/8] mm: avoid using set_page_count() when pages are freed into allocator Pasha Tatashin
2021-10-26 17:38 ` [RFC 6/8] mm: rename init_page_count() -> page_ref_init() Pasha Tatashin
2021-10-27 6:46 ` Geert Uytterhoeven
2021-10-26 17:38 ` [RFC 7/8] mm: remove set_page_count() Pasha Tatashin
2021-10-26 17:38 ` [RFC 8/8] mm: simplify page_ref_* functions Pasha Tatashin
2021-10-26 18:23 ` [RFC 0/8] Hardening page _refcount Matthew Wilcox
2021-10-26 18:30 ` Pasha Tatashin
2021-10-26 20:13 ` Matthew Wilcox
2021-10-26 21:24 ` Pasha Tatashin
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=cbd20bc4-84ee-fd39-875b-75426bc61219@nvidia.com \
--to=jhubbard@nvidia.com \
--cc=akpm@linux-foundation.org \
--cc=anshuman.khandual@arm.com \
--cc=geert@linux-m68k.org \
--cc=gthelen@google.com \
--cc=guro@fb.com \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linux-mm@kvack.org \
--cc=mike.kravetz@oracle.com \
--cc=mingo@redhat.com \
--cc=pasha.tatashin@soleen.com \
--cc=rostedt@goodmis.org \
--cc=schmitzmic@gmail.com \
--cc=songmuchun@bytedance.com \
--cc=vbabka@suse.cz \
--cc=weixugc@google.com \
--cc=william.kucharski@oracle.com \
--cc=willy@infradead.org \
/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 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).