Received-SPF: pass (google.com: domain of linux-kernel+bounces-197255-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <25dd4f6b-5c2c-4d29-b088-e8308e1e300c@oracle.com>
Date: Fri, 31 May 2024 10:39:00 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] mm/hugetlb: mm/memory_hotplug: use a folio in
 scan_movable_pages()
To: David Hildenbrand <david@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Cc: akpm@linux-foundation.org, vishal.moola@oracle.com, muchun.song@linux.dev,
        osalvador@suse.de, willy@infradead.org
References: <20240530171427.242018-1-sidhartha.kumar@oracle.com>
 <fe37643c-93a0-4220-b547-a5cae36b3231@redhat.com>
 <dec62086-8170-4fea-987a-9cc514cc4b27@redhat.com>
Content-Language: en-US
From: Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <dec62086-8170-4fea-987a-9cc514cc4b27@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?MHcrdis0WW00anFpY0dJTFpManFZT3RwQVhZSktUdnk2MW1Vc3ZwaU51dSty?=
 =?utf-8?B?NFRMZUVNR2ZYK3FVb1FYVnNZSkNGOHR0UGFtVEhUSmtoRXRMQVZiajV1ZlJq?=
 =?utf-8?B?bWtoUGtYeENJSUh0VHJXN3VGS1FHVlpwVGdxd1Z2NHp2bStGTDlONWpHZ0ZJ?=
 =?utf-8?B?K3hURmQ1ZW1QNldxRHZ3amZGZlovQW9uam5jVlFCYzBpa3QrSDBQeDFDM05F?=
 =?utf-8?B?NmsxRTVodVRPc3J0eDFmNjlWdUZjYk83U1orL25CUTB2SWxsY0hYQVJKVEpY?=
 =?utf-8?B?eDNsM0ltL1JKQy9FMnBSWjhtTi83SHcrMm81bXEyZTRhTDBHd0xCZ0IydERl?=
 =?utf-8?B?TUFXY2pBQllpYUFjOHhTajdQc0l5aVlxMFNnT3R1OEFCalMwSFJqYVkvM2ht?=
 =?utf-8?B?MHV3MmtwUHhCTFlDS0QrUE8yWW5HNnBUK0NZcmNkS0ZiSnhrSGNxMVR4SUVz?=
 =?utf-8?B?d2RYdDQyR1Y2RnNEYi9hbTc3VkhHMDZwRTh4ZEFEZTBmZ3YzbWRSWVJ4dTUw?=
 =?utf-8?B?RktUWmg2YkhvOUk4VVRqaWpOaXVJaGZMMndZYkJ0eEJyVmlyQnVkZitYbElu?=
 =?utf-8?B?cDlybHUyNG1mY1V2bERUZEdGUUk2YUU2K3AxR1dSLzErWFhKZFpEbUxRRHBu?=
 =?utf-8?B?ZHYva1pKcVlweHlKbGw0WDkvQi9Fa2YvRGc2RTIxQXlzbDFiMGFQcXJxbSsx?=
 =?utf-8?B?TlZrYlphZDRuRG1oa0JYZXFkamlDczZkRHp2bmZWK2phMytaamJNcnZNVkQx?=
 =?utf-8?B?ZDM3RkZYV0Flak1nb3FUMS9QYUxJc0t5cUVPYWpaOGJMd2tvcVdraGhxVDZP?=
 =?utf-8?B?aCtIOVR2Z0ZwcHdSL0RzME1TRnI0TmZEZjhKSTBITlJmMkVMWXdLaklrK0Qy?=
 =?utf-8?B?SC9uMzZLd3FvKzBZTE5qTVBOc3JybFFDQXFlUGJETnViak1MTDl5QVNWNERv?=
 =?utf-8?B?NDhNNXF4eDJ6MG80MkhTL1VVVzdQaDRxa3BtcjdoOTFwLzB3YkRUb0lEbjlp?=
 =?utf-8?B?OGZoWGFJc21BeTlBWGxLc1hIU256MGREa0s3MkJLMnN5RGtaZDR6M2tIVjdD?=
 =?utf-8?B?OEFnS3ExYVQvc2dRTUdCNGROYStucGtVYlU1RkVaTlVneWJ0dEthdStsRzFZ?=
 =?utf-8?B?a2JoTVJETS9weWV4Tk1tNFgyNS9CSys1LytrK3V2c0g1NTgwcGlmV25wQVZT?=
 =?utf-8?B?RFFGS0FUcUd5MzR6UDR4dU1wSlhXN2p2ejlvcmY3TXJkQ254RXpyNTNYbW5S?=
 =?utf-8?B?eTg5QUhvaGVYTVNzbjhRaGdiODg0Vk5zQzl1bVByYjd1Nkdtc21ITW5Nb3N4?=
 =?utf-8?B?c09ZK2FVNTVsRCt2Sy9id0EzQU5ieWR1dkJqU3BTdFBWVlIwakF3bU9WZkJI?=
 =?utf-8?B?OFNZTEFHV0NCcTBDTDB3MGxLMVdoNFBLUnFTZW8xcGVqQWFJc0hhMldkTlpo?=
 =?utf-8?B?TUhDSGRBam5HeSswcTZuZTV5M2MveXBHLytlMDRBQmZuM2YvRFJNMmkrQjBU?=
 =?utf-8?B?VWZ3cC9zU2ZKNkQ4TlNpdXBadGNxSGhJaVo0V1VhQjFyLzZkY2dpRWFUTjFF?=
 =?utf-8?B?a0YvV3k4UUVUaDc1L2xJZnJobkV2c29pZlJhZCtpdHVnZWZ3RzBTTUk2N1B2?=
 =?utf-8?B?L3NmK1ROeTVQUXZzeUJza3Q4TEMvOHZ4akJGVlZZSlAxVlZvUXlrUGc1K0Uz?=
 =?utf-8?B?dWFpZlNPaFR2UWhvRk00ZjRMbFZ4ZlpRUnZRSEc0S28zQmR1b0JWQUw5S3hW?=
 =?utf-8?B?K3hMWDhWVks0OFV0dHJrcGhwRC9EdlNJWTExWk9DWjh6cnhpWnFyYnhFKzUr?=
 =?utf-8?B?NjdCWGZrY3ArUmtKRXJsMloyaFExTk9Sa2FKeXM4aFE2cDJtUXZ2bHB0eVFW?=
 =?utf-8?B?UXkrbzVLcmxTVTVvdFpac0pBdXA4Q3FqZGR5M3FxNnllanZubmZUY1Q4am1Z?=
 =?utf-8?B?Ny8vMGc3d3FWTjZPY2p4WVZYMlZnYkFFL2NuSzVEWFNiNHlwUW1JSW9oR2ZU?=
 =?utf-8?B?R1NkL1kwTjYvcThoWHVUVzJBTzJnWE40eHQvZ1lIT25HRHJBTUNSUTZzcGpj?=
 =?utf-8?B?QktNOVpjVk9uOWhlUld2Rkt5blpJYnlBN0svMi9LVnU5bytGeU1EeWg2TnZw?=
 =?utf-8?B?RVp2c2lDOTJpcWpQM2RucmprU1NRdytVckZIdmhFKytHVXpCdTZBODN5d3VK?=
 =?utf-8?Q?cXasri37f1uyeyoycdz3fS8=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	7VUPSFdrhMiLyTZSv6IuLf09s6SmG4eWJYYnfwXFWWfF1W4N03jc8mm4PCI3zbRquO81XVWe2RosRCvNfSk0jxK747lHArug+sVfeevAquTD7ypuuuk6KoKcmRH4wJT5RpxbKuXiBeyJW1l9YrAwusGuBdGlGvj7ndsungf8oa5Qq9rr39eMFdSzV7+uLsUUK6wuvsUF1BK6y9BF41GGyKjT5Dc+X1hzwZJVvqSzvpCxJoAYlI7W0933N1QZgicFpVo0G8ticWLZDmFTmcm1CZJa4boPkhDJl1omsgMIhrSIGyW7IXzTLSrH63/Bo4iI53XNiHIsBgBqtE7ugeMeJzOUSrL73Ve5q6gCwCJ6Ts7awZY0fcf4L9ktbIA2F5PGjWXP0+V9Iv/OAIIMxcHB4qCnLmb/PMRMDb7B4aS2Ai4WGNmIYv67wWT6rP3XUXPZUKIR+gH+eb+/tgBP7yZYhDS1Mu1spwks9ec93HuzPlgdYNlYFUCk/OKRFPVmjSH2xwB6cpvlq5uz1/3Dx2bbenHyrC6+5+9D83VGoIGb9hhep4qCJrNRxSESHXIIp/TgMwK6urcmFcYM+yOiE2qy2HvacP+hGZa/P9igTQ5c8JI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 47a91b26-f629-408e-a383-08dc81988fdf
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2024 17:39:03.1144
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: BWVnzwTsCcGAZTv/r8+21EhhJptZMXYcBwLnDGcU0VbsclRz3WMJjmS0s/LPaZza4+elWOxQqvbObCIQ8+MfqEVhZw6edSxY5y/9RQHc8f8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5167
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-31_12,2024-05-30_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0 spamscore=0
 bulkscore=0 mlxlogscore=999 phishscore=0 mlxscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2405310134
X-Proofpoint-GUID: fjevafYN3EQL0gTNL3cqHqqwQkg38hb-
X-Proofpoint-ORIG-GUID: fjevafYN3EQL0gTNL3cqHqqwQkg38hb-

On 5/31/24 6:05 AM, David Hildenbrand wrote:
> On 31.05.24 15:04, David Hildenbrand wrote:
>> On 30.05.24 19:14, Sidhartha Kumar wrote:
>>> By using a folio in scan_movable_pages() we convert the last user of the
>>> page-based hugetlb information macro functions to the folio version.
>>> After this conversion, we can safely remove the page-based definitions
>>> from include/linux/hugetlb.h.
>>>
>>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>> ---
>>>
>>> v1 -> v2:
>>>     simplify pfn skipping logic with pfn |= folio_nr_pages(folio) - 1
>>>     per Matthew
>>>
>>>    include/linux/hugetlb.h |  6 +-----
>>>    mm/memory_hotplug.c     | 11 +++++------
>>>    2 files changed, 6 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>>> index 15a58f69782c..279aca379b95 100644
>>> --- a/include/linux/hugetlb.h
>>> +++ b/include/linux/hugetlb.h
>>> @@ -616,9 +616,7 @@ static __always_inline                        \
>>>    bool folio_test_hugetlb_##flname(struct folio *folio)        \
>>>        {    void *private = &folio->private;        \
>>>            return test_bit(HPG_##flname, private);        \
>>> -    }                            \
>>> -static inline int HPage##uname(struct page *page)        \
>>> -    { return test_bit(HPG_##flname, &(page->private)); }
>>> +    }
>>>    #define SETHPAGEFLAG(uname, flname)                \
>>>    static __always_inline                        \
>>> @@ -637,8 +635,6 @@ void folio_clear_hugetlb_##flname(struct folio 
>>> *folio)        \
>>>    #define TESTHPAGEFLAG(uname, flname)                \
>>>    static inline bool                        \
>>>    folio_test_hugetlb_##flname(struct folio *folio)        \
>>> -    { return 0; }                        \
>>> -static inline int HPage##uname(struct page *page)        \
>>>        { return 0; }
>>>    #define SETHPAGEFLAG(uname, flname)                \
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 431b1f6753c0..9c36eb3bbd3b 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1731,8 +1731,8 @@ static int scan_movable_pages(unsigned long start, 
>>> unsigned long end,
>>>        unsigned long pfn;
>>>        for (pfn = start; pfn < end; pfn++) {
>>> -        struct page *page, *head;
>>> -        unsigned long skip;
>>> +        struct page *page;
>>> +        struct folio *folio;
>>>            if (!pfn_valid(pfn))
>>>                continue;
>>> @@ -1753,7 +1753,7 @@ static int scan_movable_pages(unsigned long start, 
>>> unsigned long end,
>>>            if (!PageHuge(page))
>>>                continue;
>>> -        head = compound_head(page);
>>> +        folio = page_folio(page);
>>>            /*
>>>             * This test is racy as we hold no reference or lock.  The
>>>             * hugetlb page could have been free'ed and head is no longer
>>> @@ -1761,10 +1761,9 @@ static int scan_movable_pages(unsigned long start, 
>>> unsigned long end,
>>>             * cases false positives and negatives are possible.  Calling
>>>             * code must deal with these scenarios.
>>>             */
>>> -        if (HPageMigratable(head))
>>> +        if (folio_test_hugetlb_migratable(folio))
>>>                goto found;
>>> -        skip = compound_nr(head) - (pfn - page_to_pfn(head));
>>> -        pfn += skip - 1;
>>> +        pfn |= folio_nr_pages(folio) - 1;
>>
>> Likely not exactly what we want?
>>
>> pfn |= folio_nr_pages(folio);
>>
>> Would make sure that we are "one PFN before the start of the next
>> folio". The pfn++ before the next loop iteration would move us to the
>> next folio.
>>
>> Or am I missing something?
> 
> Okay, I got it wrong.
> 
> "folio_nr_pages(folio) - 1" gives us the bitmask to land one PFN before the end.

ya because folio_nr_pages() will be a power of 2, subtracting 1 turns it into a 
bitmask.

> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks for taking a look at this.

>>
>> It might be cleaner if we would handle the "pfn++;" on the "continue;"
>> paths inmstead, and simply here do something like
>>
>>     pfn = ALIGN(pfn + 1, folio_nr_pages(folio));
>>
>> instead.
>>
>