Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bf1f7a47-5d57-492a-03dd-e42afe186d47@oracle.com>
Date:   Thu, 10 Feb 2022 13:36:57 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Naoya Horiguchi <naoya.horiguchi@linux.dev>,
        David Hildenbrand <david@redhat.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mina Almasry <almasrymina@google.com>,
        Michal Hocko <mhocko@suse.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Shuah Khan <shuah@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
References: <20220202014034.182008-1-mike.kravetz@oracle.com>
 <20220202014034.182008-2-mike.kravetz@oracle.com>
 <YgSDcmXya7vTvvZE@xz-m1.local>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <YgSDcmXya7vTvvZE@xz-m1.local>
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?T2RRR0xmN2EyeEdaZWJoczhodVVoQTk5MS9NaHpXUnMrTE1tTVRVLy9pNUdK?=
 =?utf-8?B?bjhZZHNxNVVBZnNyczhuTjBjd2MwTUljT0pCU1NHUmlLbHFBNno3cG1rV3pN?=
 =?utf-8?B?OVlzSkNMdGdaMGYycWZwdmkraWlNRmFKempHZ2tYbnA0aU45N0dJNHJQZ01X?=
 =?utf-8?B?RmxpY3duVEhlcWxZV1YrUExXWDFvMXVyeDdLT0hRVmRVZ0J5azJsNGF4alBB?=
 =?utf-8?B?Rm11b1Z0aEhPMXB2dE9KTjViTkg4RDJwakJ6eDJ6bTJ2NWVPZytUN2FXWXJX?=
 =?utf-8?B?Qkx6TlhReVZNUm1JTW5PenF3aEdQNkdJOTRiRUlKMHlSNnZVU3EwdE5sWEtB?=
 =?utf-8?B?cWNNbWtCcVNFalVkNVFmNThhcW12SXNHRFpvRjFHTGcxV056cGxJZU5ScXcy?=
 =?utf-8?B?eXhObFVLUTBieXNlT1Q0WEZPQ3dLa3hrdDdDWkZzL2UydDBvbGgrUHUyQmQ1?=
 =?utf-8?B?eExQSU1KVzRsUGVzV2JmSWYxSDNmMS92SFA2ZFVxejJCNUpSaEJpWUMxUEox?=
 =?utf-8?B?czdVNzg5TUZTTVdLZmtVTTZScDBkV2wrQkYzaEh6WGhlZitGNHVxeWRYUUV2?=
 =?utf-8?B?SVhPTER5aDNrcW9FRDQyb0VaSXZubDF1TU1wQ2JPMnJpckJQOHJMcnQxbjJn?=
 =?utf-8?B?YmZyNHJBYkdCbnU0OTN1ZXJqVjhEL2dyeUljUlQ5Yk0rWWxSMHViUUoyeHdR?=
 =?utf-8?B?c2NWTGRrMUhDWGdoTHhvVUxBSTNZNnpSZjljTFBMNjV3d1IxTkxuN3hVeHJQ?=
 =?utf-8?B?dm16YjZuRUVmdWh6SjQ2eU9ZWmQ4ZzhHYXBIWUIyVkxFUHRCOWM0bzdzOUQ5?=
 =?utf-8?B?WHdCN2NXaW1ybUllMnNZQTIwemZEcm41N2RvbjRiWmhuQ3pORUxhTWhrOFQ4?=
 =?utf-8?B?UmtWWmh6ZklnNFEwbnV6V2s3TlYyRXptbzlCNTYvbUljVEJZOHJ6M3JDRENT?=
 =?utf-8?B?WVMyNTNmUFN3UkY5cEdzYXQ3WFVlOHRFbDdTUW5pNFlhTXo5NGUyZWxoY2M4?=
 =?utf-8?B?Y1llQzZxUHRZNTRMb3gySmZBc3UrVm14a1lWMkxGN0NWNzM1Z285RXRiS29F?=
 =?utf-8?B?SVpBejZoZExhdm50bzY2MEhTZHU2bnNRQlhWNGgrSFhialkxUHRaQ2R3d01I?=
 =?utf-8?B?TURJY2xpbVhKSm1IYTlEWUxKZHNPdm9QRldadnY0Ui91dmIzL3VNcEV6R25u?=
 =?utf-8?B?aFZ6aEJUSlQvbG1YUncrTnFaendyU0NNR2VBb290NEd2ME5IWk11bHo0ZVFr?=
 =?utf-8?B?SS83YWt6REMzTmZzZ1hMZ040amtsNUVEb0J5dXZPVGNZbkI5RXFUZlNpUFdn?=
 =?utf-8?B?NDRLczR0TFZCdSsvTE9RMVNNUG1WZUdBWFRpbG44TzNXU0cvdm1MWFd3T3ZS?=
 =?utf-8?B?R1FjUXMzNGQ3YXMxemQ4bmswTGdUcEhaY0RXc3RkdGs4dG5yVWhTQmNCUndQ?=
 =?utf-8?B?TjZZcDNOcjVOeE9JUFdrUWR5TXU5RVh2Mm5ZQ2xWMzMxK3NKdldVcGZqQTl2?=
 =?utf-8?B?WXgwM0hoaGlhTVBrRWU5QkdyOFlaek5NQ1k1enR6K05hZmRvNjZhNXBRTXBV?=
 =?utf-8?B?MkpWSlFwQXRRQ0lHb2R5Z0ZuODBEYXJVeXh0OGVSdm1LM0k3YUpCeTN0b1dG?=
 =?utf-8?B?ODNTUVp1TUh1TWhKSmRjWlVZb2JiSVkra1NFTjU0R3dUa1JNZVozdXFKckl1?=
 =?utf-8?B?QXhtR21xanR2SXBTMHpSdVI3MUhFek1zelB4Q3A5b3pjdHlyUWFad3p4SDNh?=
 =?utf-8?B?M2o0amcxSGxqZkg0NytEYSsyS093K252bWlNWm5CNzJsYkxWMnp6aUQ1RHZp?=
 =?utf-8?B?QkRqTkxpS1lhSDFIeTV4RmljdjlpSmpTaUZ2a0FKOWtkT0Q3eVdpRkV1VUJY?=
 =?utf-8?B?dUxyZ2Y2WEFnNFVSODJwcHVPdjExMURNNmViN1l4L1hrT3FVVmpEMVViZk9m?=
 =?utf-8?B?NGxZLzJvT0phOHN6Z0ZzUHlMenpKaTJMVEdLVlhmN3ZGWFl1N1FMK2swTTkv?=
 =?utf-8?B?WmxNdUJWTm1YeUVscERkWS94cE0zMXlvTEJYZEZoZnUrczNCOFJvVGljd2Jo?=
 =?utf-8?B?djlGUVh6bk9jQnNGWlhxRVYwWmZpb25zRlB5WVNvcFFlUm9OYnVVaCs4a3hE?=
 =?utf-8?B?ZzExOTJyRDJlR29nWVA2M3hSc2JycHM1dVRBUTZlZ0Y3b2czaXJsRXZtT0Qr?=
 =?utf-8?Q?YsRfOTcu7vJMSs3x2P6Rolo=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 168e6f1f-df3b-4dc2-fb80-08d9ecdd77ed
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Feb 2022 21:36:59.5366
 (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: xM7yXb1gtADUG0lO7c1HftfvPLmnof4X2QXVoz00RRKHoQ+VvO73QIaXhqrOKtuW7LnjhlQeKLhiyKDNfESfEg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5043
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10254 signatures=673431
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0
 suspectscore=0 adultscore=0 malwarescore=0 bulkscore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2202100112
X-Proofpoint-GUID: okgyLgmaHu0GQmLOOR1pjEjhSu2gnIHu
X-Proofpoint-ORIG-GUID: okgyLgmaHu0GQmLOOR1pjEjhSu2gnIHu
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham 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 2/9/22 19:21, Peter Xu wrote:
> (Sorry for the late comment)

Thanks for taking a look.

> 
> On Tue, Feb 01, 2022 at 05:40:32PM -0800, Mike Kravetz wrote:
>> MADV_DONTNEED is currently disabled for hugetlb mappings.  This
>> certainly makes sense in shared file mappings as the pagecache maintains
>> a reference to the page and it will never be freed.  However, it could
>> be useful to unmap and free pages in private mappings.
>>
>> The only thing preventing MADV_DONTNEED from working on hugetlb mappings
>> is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
>> create and use a new routine madvise_dontneed_free_valid_vma() that will
>> allow hugetlb mappings.  Also, before calling zap_page_range in the
>> DONTNEED case align start and size to huge page size for hugetlb vmas.
>> madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
>> requires huge page size alignment.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  mm/madvise.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 5604064df464..7ae891e030a4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
>>  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>>  					unsigned long start, unsigned long end)
>>  {
>> +	/*
>> +	 * start and size (end - start) must be huge page size aligned
>> +	 * for hugetlb vmas.
>> +	 */
>> +	if (is_vm_hugetlb_page(vma)) {
>> +		struct hstate *h = hstate_vma(vma);
>> +
>> +		start = ALIGN_DOWN(start, huge_page_size(h));
>> +		end = ALIGN(end, huge_page_size(h));
>> +	}
>> +
> 
> Maybe check the alignment before userfaultfd_remove()?  Otherwise there'll be a
> fake message generated to the tracer.

Yes, we should pass the aligned addresses to userfaultfd_remove.  We will
also need to potentially align again after the call.

> 
>>  	zap_page_range(vma, start, end - start);
>>  	return 0;
>>  }
>>  
>> +static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
>> +						int behavior)
>> +{
>> +	if (is_vm_hugetlb_page(vma))
>> +		return behavior == MADV_DONTNEED;
>> +	else
>> +		return can_madv_lru_vma(vma);
>> +}
> 
> can_madv_lru_vma() will check hugetlb again which looks a bit weird.  Would it
> look better to write it as:
> 
> madvise_dontneed_free_valid_vma()
> {
>     return !(vma->vm_flags & (VM_LOCKED|VM_PFNMAP));
> }
> 
> can_madv_lru_vma()
> {
>     return madvise_dontneed_free_valid_vma() && !is_vm_hugetlb_page(vma);
> }
> 
> ?

Yes, that would look better.

> 
> Another use case of DONTNEED upon hugetlbfs could be uffd-minor, because afaiu
> this is the only api that can force strip the hugetlb mapped pgtable without
> losing pagecache data.
> 

Correct.  However, I do not know if uffd-minor users would ever want to
do this.  Perhaps?
-- 
Mike Kravetz