Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <cbd20bc4-84ee-fd39-875b-75426bc61219@nvidia.com>
Date:   Tue, 26 Oct 2021 11:01:56 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [RFC 3/8] mm: Avoid using set_page_count() in
 set_page_recounted()
Content-Language: en-US
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
References: <20211026173822.502506-1-pasha.tatashin@soleen.com>
 <20211026173822.502506-4-pasha.tatashin@soleen.com>
 <7b131cb1-68d8-6746-f9c1-2b01d4838869@nvidia.com>
In-Reply-To: <7b131cb1-68d8-6746-f9c1-2b01d4838869@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UUVSN3Q2WStsTUpiUjMyaUpuT0tteDFidXl0M052bTUwMGZxNkVlU1lLZC81?=
 =?utf-8?B?ejB1d3BSaU1aQVcrWFJYWGl0cElEak01czRPU1ovYzArRlVNQXRmRWtOd1A2?=
 =?utf-8?B?UVREZDFJc3grVTRHam0yZDJwbU5ia2tTVStLellUSU9oUUh6K2lVVGdWOFEv?=
 =?utf-8?B?dGhFNlRhRmtCdXRHdW41NitqNk13MEQ0cmVyQzJvQkorSmlXSUo0b1lWY0dM?=
 =?utf-8?B?RFBmYm5oYStzZVh2MEl0Q0JxNGZHMmJuQ09pVjMyd2xmWDA0VUMxWkRFZUN4?=
 =?utf-8?B?S3RtNEZxZkMxazdmcy9KSEFiUFVBSm45ckNtVzRLTjgvcnF0U2swUFowRlov?=
 =?utf-8?B?eEJWM2xHWkM2aGxGNG81b1pvOElnSEFmSDVXWE9ucytqRy9tQ0FmY1dRVFFY?=
 =?utf-8?B?T05Xekl2Y3VMQnp4bFZHVDAxL21ZdlZSc29KRHNjZWhnVm94K0JLMFVhdHdn?=
 =?utf-8?B?ajZjU05LMStKWDFjMkk3dTdnWVRBQytVYVVhWFhheXN1bGRZeG1jaHhBKzhk?=
 =?utf-8?B?UTlCQVJNRTQ4cVpFQzVlQmMzTTlhMVphOXJINUF0Qkw4djAvcTVPc2VJci96?=
 =?utf-8?B?dXRzUE5rMjN3U2hTaTFmcm1HODFmUGtmVWtzak5EaE9OWmFvVXoyQVdCM1dN?=
 =?utf-8?B?cjJFem1MOHZ1SEYvaDBtR3lMWHNJaFJMc3A0aVZlcW1xVVZZUXpNRjE2TFU2?=
 =?utf-8?B?QzNtQnAwem1wMlFlemJJSzA1Nk92VW8rc1htS2ZrUXdWbEJsOUQ2bWphMFM5?=
 =?utf-8?B?ZGtyUkFCaGpqOXozVXZOeWhUSjZaak5pOC9URy9ZbFNiL1FEa240SlFZem9H?=
 =?utf-8?B?UHdGT1Y3czJ3TXhUU2xNRjBacW44dng2SDBwZ3pzajZVL2F3dzg4T25mTHFG?=
 =?utf-8?B?REloNFZaMFFBWW5lb2N2RWFTLzRRT292cDY5VGpMeEtSVDQ0bDhFMGVEVzZD?=
 =?utf-8?B?anZNWklZdDh6Z1RPTDRGbXRxbzdSdDB5V3ZIVXlOZElFVWhiQ3dUNVlSMWlP?=
 =?utf-8?B?M3prZUdGS2xDVEVLQjF3ZTJCMjFoVVVMZE1FY2dHUVNvN2NUeFJuV1d4Wkx2?=
 =?utf-8?B?T2dyUS9OOWliZlArVWFlMFNFNWVMU0dvQnJlbWhXdHhIb0Nld3QyREJIYi9a?=
 =?utf-8?B?OE1iZFlQWVgwMCt6dDg3anY2R3ZWUXljYXJVd3hNMUNiMEJWUHJhUENLUWZX?=
 =?utf-8?B?RE5oNWx0WXpMQUl1WE56RGV6dG8rMzN2OG81U0FLdTBMcURSRnIxbmdkR2hG?=
 =?utf-8?B?R09USzVjSk5SMldEWGkwS0VialNrZGNzcW9GeWNQVndBRDZUUEdSUkJyTkZZ?=
 =?utf-8?B?WHEzUGdlQU9WbUoxRmwvV1VBOXpXaS8rbDU4clU5akpXRjZQSDl0QVZIUjFX?=
 =?utf-8?B?S2xSZjl3Nk82Yng3enhLQ3NpYjNSK21pL1l5cHUvTDNTaWxmenBrTUcxRm5R?=
 =?utf-8?B?RFBaNGtUcGo3Y3NvUzRVNCtGbDN1Uis0TCtQZVhjL3Evdk10a0RHYStlOTJG?=
 =?utf-8?B?TmZ4cmF3UzVIVnhnUGM1SE9DMjZxS2tiK3pjazVWRlVKMkFuR1A0aDIycCt5?=
 =?utf-8?B?Qm9ma3BTNFZ4cGxDc0NzeFZBeUVSTVJuMUxQYlFlNFp3YjNUVHJEcDFJMk5J?=
 =?utf-8?B?bllZZUkyZGhzbUI1a3ZSNkZzVDBIaThnNDlFZk5YTnpzS1ZuL0NFRnNvdDFP?=
 =?utf-8?B?VzFIL0ZkeWhBRC9GYyttSHRSWllGeDRLVmkyeDMyU3hJY1BFMnB1QWdwQXpJ?=
 =?utf-8?B?SlZBdVBaTGtDMFRCZlYzRWtTUG9WTTNqNmk0TTQvQmdxdm1GbHg3dEtCa2E0?=
 =?utf-8?B?cDQ5dS9iTmE0RlM5dk9ESis1WkRoejNSdVMxZHBsV1Vuc2xVNzdtZUN1ZlpO?=
 =?utf-8?B?NE5pY1NKM05YMFc5K2pwcnFwUDBWTnhsNGRta3JXSnBMeTEvQ3o0UE9nSTVD?=
 =?utf-8?B?azN6WlpQempJbWpvTzlmUVNMRE02OHpQajN3QSs2cUplcCtsTFZjZU5VbXVL?=
 =?utf-8?B?Q0lDOVVqSjU4VzY5VlBzWXQzRkp5ZVNXZVI1MENENHNYQU03L0ErWHV5dkJj?=
 =?utf-8?B?ZnA3RnQyM2tIQXVTN0pqZzVmQzZQRHJEa2dJUVJFNWFjZ0pGRGxYUWpZcnRL?=
 =?utf-8?B?V1JCSWVmb2pxTEdtT2tzbzV3TEE4cDVMY2M2OXJVUkZUTnhBbmZDSC9PUWU3?=
 =?utf-8?Q?l3Sji/que92UDpuYh/ZD25s=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 99e99571-7f55-4729-2721-08d998aab47a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB4140.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Oct 2021 18:01:59.1154
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yYYo6pE6i9vp70RFZWq7ORV3GP6sTuoCZYZ4hdgSOE7UZnFR+veMpa/RxKxs1ZJTrqKplgRw1Qy7+HfB/dV+xA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4467
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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,