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: <e199085c-d89d-093b-4257-0102980861bc@oracle.com>
Date:   Fri, 6 May 2022 09:50:53 -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>
 <a0c54e91-dcb2-debd-a1ef-b4906fed8ab1@oracle.com>
 <YnSM0Wd/lEc9wnwJ@FVFYT0MHHV2J.googleapis.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <YnSM0Wd/lEc9wnwJ@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?QUdhOWZRMWE2cGt1UjEwM1NtZDZVVjQwYS9aRDhiWUhwTnQwY3ljbUVWcVBD?=
 =?utf-8?B?NjRvZyt2dElGUmM5SkZMWXRXbEx3RC84elVuUmtHLysySS9TR1FaSnhSVW11?=
 =?utf-8?B?U3c2cEtwV0hBTnJDYjI1MmducnVUVzlQakF4Yk01ZHVYM2JUSTVxZkNSdXFv?=
 =?utf-8?B?dStDUmNPV1l0N3hXRzczaHNXVExRWUk4Ui9DZnRiYkRTekVsWkpLWUJJZXhN?=
 =?utf-8?B?V0FZbkVKQzI4Ti92aUdpZXZ0UEpKMzlIT2Q2MUdkeUlRWk4yb3pyZFYySjQy?=
 =?utf-8?B?SjJVOEgyalpndjI2Mm1WVXhvQ3lmMWVIMXN0am8xYVoySGFRdnFwbzEzTito?=
 =?utf-8?B?OEJIUG9uZGJxU1grbUhZS0cvSDh0OWZRVmc3bXVWUW5TRHoxZFlhVkI2VHBN?=
 =?utf-8?B?MXFmTWNyYkdDL1NNTTJlLzFwM09mNW41c3M2cEZCV0ZWYVJyb2dWdWE2bHN4?=
 =?utf-8?B?bHdvdTBoREcrdzU3b2orSEhuaHFKbThCZHgvQ2JiS3A1eEgvYSs0WXhpMjh0?=
 =?utf-8?B?dkVEcDhXeXFDQXBNR0UvR2E5UTVHZ2V1RDNKdFlmU2pLV1Z0RjV1alMzLzli?=
 =?utf-8?B?L0xRMlo0V01PVWw5d2J1TmszaTY0dXJsQm1yZU9KZkhkNXo5U1dQclZwSGRB?=
 =?utf-8?B?UHBMczUxS3lLWXFXUG1pYW9sY1NMdm1DODg1Um43V2s0MmF6TWZtY0VnQ1hE?=
 =?utf-8?B?bWsrT2xvbzRJZFI0eUNtLzZORytybm4vL2ZGd1FMbzNva3Z2c2ZRZDRQQnIx?=
 =?utf-8?B?VkJaQXhIMVEyVW0wRlpXanZPU0JNRTlsK25TWXIwVU5yeFlOeklkT2JnMklP?=
 =?utf-8?B?RTRKVFdYMkhXTllRQlVXM2thU2ZucE01Q0t0WVRoeVdIL2xIY0JYY0RXOFFj?=
 =?utf-8?B?RXAvcll1alN0L1B2bW5iVEZ3REt1VEpCaTlpTWRqaWhHTHZMWUpYYXI2MzdR?=
 =?utf-8?B?aEV6aW04bjRTUml2c2l4NHRra3FqZnRhOTRBNDRKdy82ZjA4TitxVFJucVVI?=
 =?utf-8?B?T3IxTDhKMTZYU1VUSzE4TFY1Z29PQkpSOExoektKVVg1UEJBNE54RXU1bkFP?=
 =?utf-8?B?MGpOYzBrdGd6cVo2dTJHd2NkS3cxWjBWdkttbC9WUk1iNWk3Q2RLejdVUGZE?=
 =?utf-8?B?MEoxMFJxQmlBWjdwTE90c2FQaU04d2hQMVZDbXk5OWVESDhUeXhZY2ZmTUl6?=
 =?utf-8?B?MWZPcHpKUUNQRHpQblF1L2JVMFB6WjAxZ1RKTGpGTDI1QStjcEY3WGZOQ01V?=
 =?utf-8?B?Tk1TR0VNWjRTcmhoVm5oVTkyTkJtSUFZYzhvMDg0US9HM2p5WWVWai9wZThh?=
 =?utf-8?B?UlExaS9KZVlobVNaL2M5aXRlZ3VDMnozRUo0NjlnUkVkWWt5WGJ5MTh5QlNq?=
 =?utf-8?B?YlNORFd0QisvaURtMno3dkhSTEYvMkd4QkVBS2lFdEQ5Y0ZiQ29jb1o1czRn?=
 =?utf-8?B?WjA5bUFnblJzd3IwQkNuVTdXVjdNZDFnanpVT2FQdUxpUHdiSmpBbVVwZEhV?=
 =?utf-8?B?N3FYNVV6Q1Q3Y09pRkNiSUVOZWp5QWVFQjcyRDU2WUI5L2M3dHE1RjVxa3hK?=
 =?utf-8?B?NU90a252NXp5Qll1VGJxR0pObHZOckV0aGZMcFZwcHdsTXZWdlMweEJjQy9t?=
 =?utf-8?B?TFIrS2syV29RN0xMUWp0NHVOQUUwTFhJKzQ2WkYvY0MrQXhVZGZhYWhTZnJ6?=
 =?utf-8?B?NC85VjdhR28yS092UDhXYnBDWXdVRVBkV0UvcFFkQk1DVkZxTjFxYzgwVXBC?=
 =?utf-8?B?dHZPMUc1N2ROZjFVcnNtbjBlV0lLOVl0ZmQ3WER2VmhrRnVhaHRNdkpsRUVH?=
 =?utf-8?B?eHVCRFhQVkoyYUt4dEJtcXZCOEd6citUL1ZOVmtyLzRQdWlrbElvUUtvdEFo?=
 =?utf-8?B?Ty9wUW4wRUhRMEpzc2JpMDlxZTR3WmtFUjlJcm1yci9oWnYwWmRvNkRTU3JI?=
 =?utf-8?B?T0o5dkVFTC9NSldXdTh1VFlOcGgrWkd0S0lnek4yQnBMQkVPQko3V01rQ1cr?=
 =?utf-8?B?bFN0S1REM3JTSktFOVR0ZldFTHlXSElkc21VTURwWG55MzNaYTVXYzdad252?=
 =?utf-8?B?c2V6c1IzalNOWUxOdU9kN1ZGeklzcVp6U1hHTTZyV09wenZteEtCTGZaZ0lE?=
 =?utf-8?B?dkFjL2dHUDZEZ2NxdmdwbVVueDI1VEkxd3VsT3lYa1JkN21GckJRazNzODdT?=
 =?utf-8?B?eU90SWlCc1NzSWFnQUZDMXl2THRIUm1mTTQxcnZJKzltVi9xTU1oTkRBUHlG?=
 =?utf-8?B?QXdERXJ6UU45MmJUYnZyKzdlMkZnaUhqZ2JBM2wxUk9sWUdMVFR1SmphZDJD?=
 =?utf-8?B?bzUzdkFiNE5RLzZlaTE5TkxhY0ExdVVhd21PUVErTEhRVTJ1d3RwMHV5Yi9T?=
 =?utf-8?Q?qU4RjcEUNMrub8QU=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a761dc5d-0e13-47e2-c806-08da2f809716
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4201.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2022 16:50:56.5706
 (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: Bd10e6KBRVNa+Ko4fG0C/hVH4CnBtbSoUAuVk9sheiclIFUzYQvOGN92mKyfPIESNDA1ZpjGj0gOnvGxk2BdmQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR10MB2021
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-06_06:2022-05-05,2022-05-06 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-2205060085
X-Proofpoint-GUID: p4CeherCt5KHTB3-MngVhF1RlTObVa4Z
X-Proofpoint-ORIG-GUID: p4CeherCt5KHTB3-MngVhF1RlTObVa4Z
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 19:49, Muchun Song wrote:
> On Thu, May 05, 2022 at 09:48:34AM -0700, Mike Kravetz wrote:
>> 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?
>>
> 
> static_branch_inc/dec is necessary since the user could change
> vmemmap_optimize_mode to off after the 'if' judgement.
> 
> CPU0:				CPU1:
> // Assume vmemmap_optimize_mode == 1
> // and static_key_count == 1
> if (vmemmap_optimize_mode == VMEMMAP_OPTIMIZE_OFF)
> 	return;
> 				hugetlb_optimize_vmemmap_handler();
> 					vmemmap_optimize_mode = 0;
> 					static_branch_dec();
> 					// static_key_count == 0
> // Enable static_key if necessary
> static_branch_inc();
> 
> Does this make sense for you?

Yes, it makes sense and is require because hugetlb_optimize_vmemmap_pages()
performs two functions:
1) It determines if vmemmap_optimization is enabled
2) It specifies how many vmemmap pages can be saved with optimization
hugetlb_optimize_vmemmap_pages returns 0 if static_key_count == 0, so this
would cause problems in places such as hugetlb free path (hugetlb_vmemmap_alloc).  I hope my understanding is correct?

Would it make the code more clear if we did not do the check for
vmemmap_optimization in hugetlb_optimize_vmemmap_pages()?  Instead:
- hugetlb_optimize_vmemmap_pages ALWAYS returns the number of vmemmap pages
  that can be freed/optimized
- At hugetlb allocation time (hugetlb_vmemmap_free) we only check
  hugetlb_optimize_vmemmap_enabled() to determine if optimization should
  be performed.
- After hugetlb_vmemmap_free, we can use HPageVmemmapOptimized to determine
  if vmemap pages need to be allocated in hugetlb freeing paths.

Perhaps, there is something wrong with the above suggestion?

I know you have always had hugetlb_optimize_vmemmap_pages perform the two
functions.  So, splitting functionality may not be more clear for you.  I am
OK leaving code as is (key inc/dec for each page).  Just wanted to get your
(and perhaps other) thoughts on splitting functionality as described above.   
-- 
Mike Kravetz