Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <a0c54e91-dcb2-debd-a1ef-b4906fed8ab1@oracle.com>
Date:   Thu, 5 May 2022 09:48:34 -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>
 <f77412f1-ffe5-659d-8a7d-578e0e8c5e2c@oracle.com>
 <YnOEl6Qwp5jp7RHp@FVFYT0MHHV2J>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <YnOEl6Qwp5jp7RHp@FVFYT0MHHV2J>
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?WEdwZnZLeFZhTURvYUlsOXNIdExIVStiU3JTaFhtZFhYSWcxcllRRHZxOTVs?=
 =?utf-8?B?M2txajZjTldnUk9BeU1LaU9ZRGdWWHpvaHlUMFNJR2RhRjV2NFdwS0t2ZDZ6?=
 =?utf-8?B?bTBpcDZWM2JUZVltTnNucE9xWGZLcm1laThCcGF6Sk5EN0ozUTN3RmZGSUZw?=
 =?utf-8?B?SGwvOUlhRlBhOFRXdlVlb1pHSVBxNHhoeXN0TnZyejdxQ2JwS1dYMWtUbFk5?=
 =?utf-8?B?QlcrVnJmWVJKRXg5bEFUSGJyUUJQdEhMbjU5M1l6SHBsVDh6Yll3YWhUUXVN?=
 =?utf-8?B?cUwwRmJSbTVSdDgvT1pLYzNPWTE3NUpNcitxbm9PTHVJUWQyRzExcWpwU0dP?=
 =?utf-8?B?MXhWcGUzOHlTWFBNVHRRZGpCd0VLWXZRMWVpK2NpV0x1b0hzUisrT3JkajBt?=
 =?utf-8?B?bzRRSUZwY3ZkSlB1ZFJRbHcvaW03Y3ZFRzZwY0xhZk9xSWpubDNHUlRWL2lh?=
 =?utf-8?B?VWl0NGo4Sk5MclI1bE1EY004SHFnSnkrcUhwMytjQnJNY0U2a3Z1VUdrK2ps?=
 =?utf-8?B?cW11bWdNYzRveXZiVi81SHF2anhrdFpkZGFDdU9zT1RxRzJlN1BQcG95Tng0?=
 =?utf-8?B?WVNPTGs1eWJSTUdVNnJNTy9hQmQwSUVHNVNnb0wrc043NFhuYk1Uak1DTEt4?=
 =?utf-8?B?VUNrTFFXRThCNU9XYUMrUFVqRExlUEdlWWZjUW0yTGdxN0Y2QjlHQ0x5RHA1?=
 =?utf-8?B?TU9DbHdheE1BNFdlYmxlVjdNWUxiS3BDNVpqbEtXZ09OM25HckxqUExlWlJM?=
 =?utf-8?B?ZU5xeHEvY3FMRTZOS3JSTlg0eDlmNGExemtyNG9kc3ZESmVTVkZ0Z2VxQWR0?=
 =?utf-8?B?TzVmRERLZzJaZTNrMmovaGJObFhsRVgyV1RnTm1DYVVQUmt0cS9nTlA1QTZs?=
 =?utf-8?B?Nm8zQ2xvQXhMcHJuRjNIa2Jrb2djTWxBTEErNUlDMEs3eEdkcWptTWRRVTBB?=
 =?utf-8?B?dzVpSjMveWpPR0pJNjdYakNod0wyU2Zhc2gyeGdYS0c1ZjBsVnFDbzM2SXA5?=
 =?utf-8?B?cWVQL2lBTnB0UUQ2QlFxNDJIWEdxVWlqdkVjakgzaGtsQkhoK25EWjZUeitv?=
 =?utf-8?B?eGozTnkvVDh4L3NPU1BJOHZ2bW45NzRmV3hhSGlOc1UzZWJ1ZXlkSlBUYnpr?=
 =?utf-8?B?bFQwTjJFZzVITmVnc0EyZFBxM2dRL2pIcVZyYmpibDBNenA5cmNUK2NZUnJa?=
 =?utf-8?B?em5sUzM1NitnUEMvSS93VDk3S0JBWUYyc0dDT1R6UmJ6QktXaTIxaUtER1M0?=
 =?utf-8?B?K3NzdFlFdFpZWDMvNWR5VURZbW1qWGNTaCt0VGdlZklMNDNVaHpJbjZYRktG?=
 =?utf-8?B?dEZUZUFtTyt6SkswN29BbXdGTmVlNXZQRS9GWWhwRDZGUncwV0xKRkJhalAr?=
 =?utf-8?B?Y3JHUGV4NnltSmJQMkVxTG9iZkFPU0VWdWE5cWZFRityaFJDbXJFNkNPNzI0?=
 =?utf-8?B?QVMzWTh6K1phUmtBUEFlNyt3akQwYWNzbG5FOXRPTTdVdWtjWTY2MHNkaE5X?=
 =?utf-8?B?Q0dIaEc3Z2hxWkZEZ2hmQWI4ZDIvYjJCZm53NmxXRTdUR0w2SnJZbFFkbG96?=
 =?utf-8?B?SG4yYzJtNU12Ukc5cmhKcTNoSzQ1SnZmZlliVjExZGJ6eVc0dW5Qc2ZROUVY?=
 =?utf-8?B?K1VRRndNcERoczgvdUM2SUFQaUYraFN6c2hYZUNHSUl3d1p0dnBvMHQrOGFO?=
 =?utf-8?B?VWkzL2xjeGlXU09UNy9UUDFSZU42NFBYYlM5SDNlL3dFYUR0MXovQTFlN2ZQ?=
 =?utf-8?B?VnRkOUR3WCtCQXYxNnVNUGREVm9wb0dyQU9nUVBoQmZ4T0tXNUdZMUplbTA5?=
 =?utf-8?B?T2w4c2dzU3FwOXMxTGsxQTlvQ2Foc1JMVzRyeVk1bENOUXdjaHZ6ZjNmWXlv?=
 =?utf-8?B?MmpIWi9VWHZ5bkRRUFc1K2EvL3c2c3VHbUNzeHpsM1NQTGRucFY1dlNSRG1V?=
 =?utf-8?B?aEhnU1BwWXBrelhJSVRkY0tsbm9kUlhjeC9CTDlEeVFET284MDg5V3M5Rml2?=
 =?utf-8?B?M0VUZXJ2K0FQMjIvVTNja3BkNm9mMS9pSEtleTVnd1V6eU56R1V3NUR0ajZD?=
 =?utf-8?B?TDhxazJmNjNkeWkyKzhxY1luR3o0UDUxK1Juakx3aEZsZHFFZS9KeXNFVjRy?=
 =?utf-8?B?VElQd21EME9MNkJlOWRGK3l4dEUxWVRRN2NYUjN6ZEJ1WTFUZTZlK0tRQU5m?=
 =?utf-8?B?NG5Uamc0amcwR1owQ0VEZGZQS0Z1SGhJejYrbEFXbVhHZGoraVBXeHJTbC9L?=
 =?utf-8?B?SnhzU2owTlJRNlMrRlY0OUY2dDQzMzZUNlJGOWZSOHJBL2tQQkgzbjl2bXlp?=
 =?utf-8?B?ZzFnek9senFJUkNESEE5ZmgyQ0lVWEJPblk0OVdrYUhVUklKZ1QzN3drK3N0?=
 =?utf-8?Q?Jf8ClNg1rtXNsYOY=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 40e19501-9f7b-4793-68b5-08da2eb71968
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2022 16:48:37.0574
 (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: iLI3w1AoGOQD2FEq6NrN1NOOGmd1bJv0I3G1l/JM0biD0q8FEL5EFPgl9LlGCTBSspSUKQ98Rm8mqehZmV3nQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR10MB2463
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-05_06:2022-05-05,2022-05-05 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 mlxlogscore=999 adultscore=0 mlxscore=0 spamscore=0 malwarescore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205050116
X-Proofpoint-ORIG-GUID: 1QdUW0Nn-8t0D0Z2JV349APYSzm8X1xv
X-Proofpoint-GUID: 1QdUW0Nn-8t0D0Z2JV349APYSzm8X1xv
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/5/22 01:02, Muchun Song wrote:
> On Wed, May 04, 2022 at 08:36:00PM -0700, Mike Kravetz wrote:
>> 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 ???
> 
> Definitely right.
> 
>> 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
> 
> I'm wrong.
> 
>> to free all hugetlb pages with optimization before we can add new pages
>> without optimization?
>>
> 
> My bad. I think the following code would fix this.
> 
> Thanks for your review carefully.
> 
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 5820a681a724..997e192aeed7 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -105,7 +105,7 @@ void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
>         unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
> 
>         vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
> -       if (!vmemmap_pages)
> +       if (!vmemmap_pages || READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
>                 return;
> 
>         static_branch_inc(&hugetlb_optimize_vmemmap_key);
>  

If vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF is sufficient for turning
off optimizations, do we really need to static_branch_inc/dev for each
hugetlb page?

-- 
Mike Kravetz