Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <f77412f1-ffe5-659d-8a7d-578e0e8c5e2c@oracle.com>
Date:   Wed, 4 May 2022 20:36:00 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v9 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap
 sysctl
Content-Language: en-US
To:     Muchun Song <songmuchun@bytedance.com>
Cc:     corbet@lwn.net, akpm@linux-foundation.org, mcgrof@kernel.org,
        keescook@chromium.org, yzaikin@google.com, osalvador@suse.de,
        david@redhat.com, masahiroy@kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        duanxiongchun@bytedance.com, smuchun@gmail.com
References: <20220429121816.37541-1-songmuchun@bytedance.com>
 <20220429121816.37541-5-songmuchun@bytedance.com>
 <eadec7de-2e1a-2fb3-3317-c7b492a84e2b@oracle.com>
 <YnM4DRFhdD6iZIs1@FVFYT0MHHV2J.googleapis.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <YnM4DRFhdD6iZIs1@FVFYT0MHHV2J.googleapis.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cnZUM3dtaytGYXh5UVFKeVVLNlIrdFFXQ2JCc0U4UHhqMHgza2NoY2JDS2Jo?=
 =?utf-8?B?cEhKdFZYZ05CS1h0VTZiRHdWbTZOUER2OWNPdnpTTXMvSlU4YUE2NHVWREI2?=
 =?utf-8?B?bkhSWERZWStLTTdNQkc2M1BWS0xFUzNmcUduWW5OWGFTQzRrdDZOd2ZjMVND?=
 =?utf-8?B?K29hbjRORysrUm9EMjkydk94bVlqT1kyYkpzUGVOemowZHZpNTdUUGZsYUo5?=
 =?utf-8?B?bVpBbVp0bmh1bk1pdzlyV0xJSEp6MCt4U1lWb2xEQWhRUjM5N0YyT3ErSkdP?=
 =?utf-8?B?K1p1cmNPaGV5bjZXaDZIT1JmeWlsOGNMRUh5WVN1M0FhUjlvQU5zNHlEeGdT?=
 =?utf-8?B?OEhjcVc1dWh6QSs0bExWT3BLLzRQZEp5djR5aGxzbHM3anNiY2ZZV3VIQUw1?=
 =?utf-8?B?V3pNbnE5MDB6R0pGWW9QY0g4SjBPaDB4eFEzMEVJQ0JQdHBTTXV6TW41Uyt4?=
 =?utf-8?B?T3plT2VieWpBZGZMYXhMUDlhZWl3RmRFUDAxZkE1NXJscjMvcDBSWHZpMU9L?=
 =?utf-8?B?SkRDd0tqcHlHc2R0bFN6ZElsa2s1NjBVUUNpSExMYkZwTkgvTGUwM25ySy9O?=
 =?utf-8?B?Nm8wbWttOXhZNWV5RUFjUHNsOUVESVJ5eVdLb2NybFAzb29XRGEvYzlCekFC?=
 =?utf-8?B?a2FKQTUwZUdvaWJEVDcwRzVuRjNNSEtmOTI5STYxSy96aWpIYUMxMnVHTkNM?=
 =?utf-8?B?TWxJZFNoTE5LK0Y4ZU5KS2NuRkU3TE4zZStMSjk2NmhINi9GNzJqc1JEb3dl?=
 =?utf-8?B?N254T0VGUjNEZFJCdjZ0clk2YWhlc2NJUHFtTEorTkhDQXpSbEg5eU0wTWl1?=
 =?utf-8?B?NHhBb1dPUExTSzdIRi9UL2tnVVJGb1laN0VRc2NIcG1HN0g0WmV4cXdvTkVl?=
 =?utf-8?B?VitaaVA2bnJvYzJiZnJuWUd5UTVTSzZXaDVOZjIwZVY0Zmd6REdIVGVSbkVG?=
 =?utf-8?B?c1dOUVdyVThZOUl3UWZwbW9ZdUdZaGN3dkJaeTJMdmhhTzNxclpjUitMcVJp?=
 =?utf-8?B?T2U2akQvWllwY1NDY0RnNDJ1VngyN1dQeTFzMWJaaTdUY3dSaThnV0RlTGJM?=
 =?utf-8?B?b1UrVE8rWFJ6TWRSWU5tOEl1QnBUdHJIand2OTBHcTVnSkZXK0pZb2VJUCs2?=
 =?utf-8?B?cUxCcjhkSEVGN0Z4cGFtOXdabjI0d2F6dEpaTHQ1YTg0U3QyZVdRRTFpcVlX?=
 =?utf-8?B?UVZCWFJPN3RXTzVFUFZDUzRRSFYrL0JOa3hpSWt0MGJCbmdiODNlTEZPa205?=
 =?utf-8?B?cGt3SXJkcW4rN0FLbGh5b05qT2pRNVJJSTZNaGxvdlg4M29FUCtINXVpYVhr?=
 =?utf-8?B?bU0vQTBqL01DV09IKzR0TUNBL0pOU3pmbVpjS3c5RnBFMFVZb0h5VjhIdEsz?=
 =?utf-8?B?YzBMV0h4bzJqR3RFM1BhRGNJdzk2YzFCWjQ0NFArQlRpMDA5VzdWU0w4VzY1?=
 =?utf-8?B?b1hHWmx4TWN2ekFwNjNKeVhBUmZMdFAxeDJERjlPNUgzL0tQcmE3b0RPb3lh?=
 =?utf-8?B?YUhaNTRMVk16T214UHpncEpYZlhWK2JzandVcEdiVWJJR0JXTEEwL1lEVDE2?=
 =?utf-8?B?NFBHSHBjOHBBWkJvSTkxcUlHMmpvOWhnaHBmbGJtUlFobEdPTUx1Z0ErbmFX?=
 =?utf-8?B?NGtERDRBZnBZUE9tblMrblVWWC9FZ0U2UjBERlhIYXc0V0tIMVZKZXVSV1c4?=
 =?utf-8?B?SU50bHcxTDFERExHRS9ZYmZlcm1oT3htVFdRazFYRGhodXlpNml2bHJSY3Z4?=
 =?utf-8?B?RTZTTjFmT1VWYlcyczc1RUROTE10UzdWb0FkTm5wRzFsbmxXMGFIcjI0cjJ1?=
 =?utf-8?B?SnBOODB0UWZYbDlmUHcxSDgzcGVlMjZrYTA5cWJIRFpRRHdWMWl3QW0wUFpG?=
 =?utf-8?B?cXo3WVVFS3JTeUE1ZElNK2M0YnJJMmh4dFVFdHZWeVJ4NFN0YnhablkraC9a?=
 =?utf-8?B?dnV2ekdhdWtyeHNwU1ZrOCtPbHdWS3NoYkR1eGtoS1Z6VXN3WHczQWlZSTFR?=
 =?utf-8?B?U2FjNXdTdDhsUWc1QXNFdW5jUTdVZkFqU3hBN2xibXhCb1htcTNDek5JWkpC?=
 =?utf-8?B?T0htMmVjcjh2cWpmMDNtR2xmTXZnb21QOW5ucDdyaXptc2FvY2U3YVByZG1H?=
 =?utf-8?B?N2dyQXUzM280bTJBQ2dqN1Y2RVRwZmdrdGdmTEFQYVlZSmlRKzRMRkVSSFNY?=
 =?utf-8?B?bTFLOHhzUnNOY2U1VGVRTjVLV2xxQ1lJSENoMDJvb1krcEdZSVhFaDc0dEFy?=
 =?utf-8?B?K05qbDZzNjNlYlppWEhzdnh2cHdpMnVxb1U0WnJjbnRGQ1dncVlKV3RXd01B?=
 =?utf-8?B?QnQ1SjNUVlgvcXp5VHFBVkhsQnF6VjRLVXFlRXN6bHVESXIwajRTa0F1MzRC?=
 =?utf-8?Q?SAQZjVI/i9pDUZeA=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1870a15b-d7b7-4983-ed6f-08da2e4860fc
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2022 03:36:02.7932
 (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: +HNi0IDj8SdYmN6MkOqgVnJafhIgYJTLHj9Ijn1AD5NTBdtBMlFJRvQtr89swhJflGf72DqgkonfQwxQXDoeJA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR10MB2674
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-05_01:2022-05-04,2022-05-05 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 malwarescore=0
 spamscore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205050025
X-Proofpoint-ORIG-GUID: euVHL2JYeDTGUL8vCGJP-229MK4IzCi9
X-Proofpoint-GUID: euVHL2JYeDTGUL8vCGJP-229MK4IzCi9
X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 5/4/22 19:35, Muchun Song wrote:
> On Wed, May 04, 2022 at 03:12:39PM -0700, Mike Kravetz wrote:
>> On 4/29/22 05:18, Muchun Song wrote:
>>> +static void vmemmap_optimize_mode_switch(enum vmemmap_optimize_mode to)
>>> +{
>>> +	if (vmemmap_optimize_mode == to)
>>> +		return;
>>> +
>>> +	if (to == VMEMMAP_OPTIMIZE_OFF)
>>> +		static_branch_dec(&hugetlb_optimize_vmemmap_key);
>>> +	else
>>> +		static_branch_inc(&hugetlb_optimize_vmemmap_key);
>>> +	vmemmap_optimize_mode = to;
>>> +}
>>> +
>>>  static int __init hugetlb_vmemmap_early_param(char *buf)
>>>  {
>>>  	bool enable;
>>> +	enum vmemmap_optimize_mode mode;
>>>  
>>>  	if (kstrtobool(buf, &enable))
>>>  		return -EINVAL;
>>>  
>>> -	if (enable)
>>> -		static_branch_enable(&hugetlb_optimize_vmemmap_key);
>>> -	else
>>> -		static_branch_disable(&hugetlb_optimize_vmemmap_key);
>>> +	mode = enable ? VMEMMAP_OPTIMIZE_ON : VMEMMAP_OPTIMIZE_OFF;
>>> +	vmemmap_optimize_mode_switch(mode);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -60,6 +80,8 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
>>>  	vmemmap_end	= vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
>>>  	vmemmap_reuse	= vmemmap_addr - PAGE_SIZE;
>>>  
>>> +	VM_BUG_ON_PAGE(!vmemmap_pages, head);
>>> +
>>>  	/*
>>>  	 * The pages which the vmemmap virtual address range [@vmemmap_addr,
>>>  	 * @vmemmap_end) are mapped to are freed to the buddy allocator, and
>>> @@ -69,8 +91,10 @@ int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
>>>  	 */
>>>  	ret = vmemmap_remap_alloc(vmemmap_addr, vmemmap_end, vmemmap_reuse,
>>>  				  GFP_KERNEL | __GFP_NORETRY | __GFP_THISNODE);
>>> -	if (!ret)
>>> +	if (!ret) {
>>>  		ClearHPageVmemmapOptimized(head);
>>> +		static_branch_dec(&hugetlb_optimize_vmemmap_key);
>>> +	}
>>>  
>>>  	return ret;
>>>  }
>>> @@ -84,6 +108,8 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
>>>  	if (!vmemmap_pages)
>>>  		return;
>>>  
>>> +	static_branch_inc(&hugetlb_optimize_vmemmap_key);
>>
>> Can you explain the reasoning behind doing the static_branch_inc here in free,
>> and static_branch_dec in alloc?
>> IIUC, they may not be absolutely necessary but you could use the count to
>> know how many optimized pages are in use?  Or, I may just be missing
>> something.
>>
> 
> Partly right. One 'count' is not enough. I have implemented this with similar
> approach in v6 [1]. Except the 'count', we also need a lock to do synchronization.
> However, both count and synchronization are included in static_key_inc/dec
> infrastructure. It is simpler to use static_key_inc/dec directly, right? 
> 
> [1] https://lore.kernel.org/all/20220330153745.20465-5-songmuchun@bytedance.com/
> 

Sorry, but I am a little confused.

vmemmap_optimize_mode_switch will static_key_inc to enable and static_key_dec
to disable.  In addition each time we optimize (allocate) a hugetlb page after
enabling we will static_key_inc.

Suppose we have 1 hugetlb page optimized.  So static count == 2 IIUC.
The someone turns off optimization via sysctl.  static count == 1 ???
If we then add another hugetlb page via nr_hugepages it seems that it
would be optimized as static count == 1.  Is that correct?  Do we need
to free all hugetlb pages with optimization before we can add new pages
without optimization?

-- 
Mike Kravetz