Message-ID: <5c40ed66-1e51-78fa-193c-0eb0db814b01@oracle.com>
Date:   Mon, 23 Jan 2023 13:08:49 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 1/2] mm/memory_hotplug: remove head page reference in
 do_migrate_range
Content-Language: en-US
To:     Matthew Wilcox <willy@infradead.org>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, david@redhat.com, osalvador@suse.de
References: <20230123202347.317065-1-sidhartha.kumar@oracle.com>
 <Y87wJ6ERhdujjo6P@casper.infradead.org>
From:   Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <Y87wJ6ERhdujjo6P@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?em1tWVZRVzErMS9CZmtNclVkaWt2UzJpdDBsNzhvTEJEbEdYQXV3YW1nV1lt?=
 =?utf-8?B?a2d6NWY1cGlzZTRaUFZSZXBqYkV3SUFZM1hyZ1ZnZEFEdzRxblFkU3BsaXJK?=
 =?utf-8?B?STAwZjBqbGk2WDBzZFRkL0hGbldObGZSVDE4N041cEkwcHhHVHZCallQUXBT?=
 =?utf-8?B?SlFTdjFuQVJWdXlIUnRsQko3Tkc4c0lLR0RyTFZSVnQ0dnJhNzR5d0NEdDZL?=
 =?utf-8?B?VXcvWGxwTFBleVZ3YUl6MWRRd1lpVGhlZUd0MEJmcG4rUndQdkk0Rlg5UGNy?=
 =?utf-8?B?eEt4dEg0elI2eng4c2xReG9ldjdnZW9uVGVLeGE4eGlHelpqSTJkNE16TkQ4?=
 =?utf-8?B?MitQak5tckRFREFjdk9LK0JhR3Yrc2VsdlcvYTlTQTRIVTdpSUlIeGFTOTNF?=
 =?utf-8?B?cGdQODlpQkpwMHBZR3RwWGd1SFROUFlwVGxmQy9yeXBrVWFZYkc2bWROZ3NL?=
 =?utf-8?B?bUQ3NG5taWJrU3VwVEkvbnZsZkY5a0RRWHBXNWIxUEg3VkdKVncvazE2a1pz?=
 =?utf-8?B?Wjl2bnliWFVpdlFiejlmYTQvY04zZWdvRU1HbDFNVzh1QlZLbytlMy81a0RT?=
 =?utf-8?B?VXp4SGZla3JRTzgwaGErS1F1bnB1Lzl6bFhVdWpndnk4UzVWMENreDZDa052?=
 =?utf-8?B?WGQzdzhFMHVSL1l2bzZCdVdSdVZIZU5nMUJ6YWNyelRJNk9acEtPNStacis3?=
 =?utf-8?B?T0xuUCtoaVVDVzRCUUxyeitYVmxHN1Y2U2czOGtmQkVmei8rSy9PNkZ5SnJ3?=
 =?utf-8?B?Q3hPeE1Jb3BWeDRqYVpldlIyREovNjZGdE5tM3ZLUVNiYXh0UktvbUdDQkZ4?=
 =?utf-8?B?NDFJMCtROXVoanFFdFZOSEVwUzM3UkxBTjl5SnhhTXExSlRMclBxMDVpY1Ev?=
 =?utf-8?B?bjRnUFZCVmhBcmZtK2JFUGxkZGVzOUdPYTNnb2ZkUDExdW1UZ2oxL1RhRENX?=
 =?utf-8?B?RWw3a09SRHRyNFllTnU3NEVnNjExM1orR3VtaExLdUtscXVQMGlBaE55VStj?=
 =?utf-8?B?ajRMRjZuVGpoTkMvU3BjdE0zQVFWSVZXaE5zdlA4WmxRVlZScnR2WElUQmE0?=
 =?utf-8?B?aVFEeGhkMUtBeWhRdmpORmJralpCdUNkd0FuYWM0NHVMZEhYK2RCNDlzOTk3?=
 =?utf-8?B?eTladi91azI1Q2pkZU5Xby82VXZHUExhL1NNaTZUZCsvQTB2TjlySDN6Uk0v?=
 =?utf-8?B?cTdYamM3ek1QL0w2OGJHYmVpWUxoL1ZLUmY3cnZ0QUtkdGZXQmNLZ3FZZ3pk?=
 =?utf-8?B?YTVEQTR2RmdhN0FPU284RjJrbU1EVmQ4elV5bnZsT3dYcHBLVnlRT2dYSy9o?=
 =?utf-8?B?VHE5ckxiTnczVEJRV2hTeS8wNS9QOFpZNWxacjlGemE1YkZkUkt4bmpqY28r?=
 =?utf-8?B?Znd4ay9rb2dnMHR2V2hYNmdYaFhrMC9pSUt6WU03cDBmM2YzTzRwVEFtTG1O?=
 =?utf-8?B?MUZpNGwxREk2MjNHN0pTaEsyRjJWai9aSUF1allpTlhJcGxIWVJodFNCSTJK?=
 =?utf-8?B?ZU5nZDFBWGRrRkJOZTF2anNWNWtmVENwNE5TVFNieFJyNjdCSmozUGFlamIv?=
 =?utf-8?B?aWlDSWJRUUlLZ09pVHdNNVZSYTNTOXRsMlhGcFhKK1dZK1RMR2xrQlBGZXV4?=
 =?utf-8?B?Ykc4MjhHaGkxNTdTQ2tHZzNUcUJYOFU5dEplK25QdWtaWGVuMHdSS09uUzhJ?=
 =?utf-8?B?ZFB1SzRpU1pNU01pamdDck9QajJZOGIrMGNKeUlTUzY3REJDL2h3cGZabTNQ?=
 =?utf-8?B?VFRUeE04SWlGZnRLTTJEaHBhdm82citzYTUvSDZqOEhidVMvSHRnSlU1VHZO?=
 =?utf-8?B?NGQ1S0JQc2NYa3NyT05vallJeElnQVU2ZFVVelpoUjVqd3RKV3NDcGQrODZv?=
 =?utf-8?B?TXdCUjVJWXhtSDk3ME1aMnBaSjRHZDNqRUIyY1RYS0xuck1xY0U4Rnk3NWtB?=
 =?utf-8?B?MnBwa0dSNjc2dnhERUlQZWpWc005WlpCYk8vdDIrRWprYkNuSm81c1FTaTBW?=
 =?utf-8?B?Y0RvSUJyN1BLMXlTY0JCc0dhZ0N4aVBHODV4OHZ0b3ZMdmZ4aFRtVFE3MWhW?=
 =?utf-8?B?eDdkVzFsekwwY2tuYllFMFE5SVJqaHpFY0hFS3hNRmpJTUE5UWtWd1RpRVpr?=
 =?utf-8?B?azB1bDNPYW8xTDFoTVowMTJqZUk1b2tDVmFPRXFFL3BrZWk5NlBwMU9LdGg1?=
 =?utf-8?B?YlE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?eTJSOHhxZE5lODgvWkZPR1Q1cmZzc0s5SlU1dnFMaFFuNmQyaURJRUUvZDN5?=
 =?utf-8?B?djF5OHBoQTF3dVpWRnB4K1gvdzJhQ1lFRjllMVFTT1cxck1WRmNQb0c3NWZI?=
 =?utf-8?B?UGRjai91dEQ1bDBONjArSjNBc2ZqbDVxak5pazE3NGlPdVNjWlMzMmJ5OHND?=
 =?utf-8?B?R1FBcGNncEpHRVN6UXJVM2JOY2R6U2xGUHd0N2g0R1pSNE9BZ0trVjl4U0hl?=
 =?utf-8?B?Nk56cUc5bUhRRkdTSVJabFNRajczbnowNnhsczR3QXR3TmdDMUtJaGNQQUcx?=
 =?utf-8?B?eUY2NXhhS1V5NSs1U3lkbjQyREN6aUNnQkFEZHFtZ2ppTHpzaEZqNzJ2Z3Fp?=
 =?utf-8?B?cmkvOExCY2M2cDl0UDNBQlB1bDEvVUlmbHpvc3Y2SUlKakJXTmsrMEhuSkJG?=
 =?utf-8?B?NTVuVlVtbitFQTdCSDZlVFBMbW5sRUxCOXRZZTJFZFROdTJkZUhZdE5nbFFi?=
 =?utf-8?B?aFIxQXJ1RXAyUFpFSmZzeEV0cXRnRFpTTGJUQ3hIZnBjOUNkV1gzL3cxSHEz?=
 =?utf-8?B?WXJvbXV4eklYcWFBUnlScTFNWkFvOXFMelk5eWlMSnlCTE1FY1dsRTNQbDFL?=
 =?utf-8?B?QTBmSzlTT1cxK1FjLzRybjM2QXdWejA1bUJKRkpuTjBGOEgxYTMxTkhEWi9n?=
 =?utf-8?B?eis4bDYzUXJXZ0RMK2tBTDZwS1p4UTJNSlExQTA2QzErc3VrNHVlY01mSHpJ?=
 =?utf-8?B?UXBhL2psUllCb1dHbEpPdk1PT3ZZamY0NDRIVUo4Rmo1TUVLbE1hSHhua1Rt?=
 =?utf-8?B?TklXU0ZQQU53QVIwSjhNdkhqZlBUZmMzbjR6d2RoVnFablIyL3FKL0orTjJ1?=
 =?utf-8?B?Qm8yNWt1aGd6azMrNXUyYTBvamg3azErRGljb0o2dU9BTUZZcENrYzY1TzhK?=
 =?utf-8?B?WGU4L1JybTgyTkNrYkJnYkp6VUVsSzJLSFNZb1NtQTU1RWpCS3F1YU9IWUFa?=
 =?utf-8?B?NHNIWWJVR0FXMXpvWkRpOTd2Z0QzcU1jRE9kSHdQYUtPRllpNHJlVm95cDli?=
 =?utf-8?B?d2ZzTGJ1VHFwZ2dhSWsrSWhBU3NwTGx1NWd1alVhRmZtSDJWRnMvdGVhdURs?=
 =?utf-8?B?SUFQcGhzZE9jeDVzMXBYeXlMa0kvanYzeXNLYXF0MTNOSXFLWldQWW5HOTZi?=
 =?utf-8?B?Y0RIRmk2UTFCQ3JSUzY3SzQ4RFFKS25aN0FzRTFiU2hOT0t5QjUvM1ZnR0Zp?=
 =?utf-8?B?Z3pwMnEwK1Z5UVF4Z3AzaERad1g3TXU0eml6K2kwOWFPRjlkejVSY2dZbXda?=
 =?utf-8?Q?aIbsTH2tVN9TZ7l?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2a639623-1491-45f1-592a-08dafd860776
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB5106.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2023 21:08:52.6157
 (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: IFIlp9abLzN+byztQN2S7S5Ou4GwvI8K4M7waoql/XxMdM0RxKwARD77VeI0mQUOeawz6ACtaO2d5QyspACfgWmHhYfwsALxSn7gzg22z9o=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5548
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-01-23_12,2023-01-23_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 spamscore=0
 bulkscore=0 mlxlogscore=906 malwarescore=0 adultscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2301230201
X-Proofpoint-GUID: O4dt8u9lszxXS-TYPocvawRVVTr7FlBM
X-Proofpoint-ORIG-GUID: O4dt8u9lszxXS-TYPocvawRVVTr7FlBM
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/23/23 12:37 PM, Matthew Wilcox wrote:
> On Mon, Jan 23, 2023 at 12:23:46PM -0800, Sidhartha Kumar wrote:
>> @@ -1637,14 +1637,13 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   			continue;
>>   		page = pfn_to_page(pfn);
>>   		folio = page_folio(page);
>> -		head = &folio->page;
>>   
>> -		if (PageHuge(page)) {
>> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> +		if (folio_test_hugetlb(folio)) {
>> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>   			isolate_hugetlb(folio, &source);
>>   			continue;
>> -		} else if (PageTransHuge(page))
>> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> +		} else if (folio_test_transhuge(folio))
>> +			pfn = folio_pfn(folio) + thp_nr_pages(page) - 1;
> 
> I'm pretty sure those two lines should be...
> 
> 		} else if (folio_test_large(folio) > 			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> 
> But, erm ... we're doing this before we have a refcount on the page,
> right?  So this is unsafe because the page might change which folio
> it is in.  And the folio we found earlier might become a tail page
> of a different folio.  (As the comment below explains, HWPoison pages
> won't, so it's not unsafe for them).
> 

Thanks for the explanation of why this is unsafe. Would it be worth to 
put this code block inside the

		if (!get_page_unless_zero(page))
			continue;

		put_page(page);

block found lower? My motivation for this series is the HPageMigratable 
call in patch 2 is the last user of the huge page flag test macros so a 
conversion would allow for the removal of the macro. I thought I could 
also remove the head page references found in this function, but if that 
would cause too much churn in a complicated sub-system it can be dropped.

Thanks,
Sidhartha Kumar
> Also, thp_nr_pages(page) is going to return 1 for tail pages.  So this
> is a noop, unless page is a head page.
> 
> It's all a bit confusing, and being memory-hotplug, it's not well
> tested.  More thought needed.