Re: [PATCH] mm/hugetlb: simplify refs in memfd_alloc_folio

[Steven Sistare <steven.sistare@oracle.com>]

On 9/4/2024 4:02 PM, Matthew Wilcox wrote:
> On Wed, Sep 04, 2024 at 12:41:08PM -0700, Steve Sistare wrote:
>> The folio_try_get in memfd_alloc_folio is not necessary.  Delete it, and
>> delete the matching folio_put in memfd_pin_folios.  This also avoids
>> leaking a ref if the memfd_alloc_folio call to hugetlb_add_to_page_cache
>> fails, which would otherwise need an additional folio_put.  This is a
>> continuation of the fix
>>    "mm/hugetlb: fix memfd_pin_folios free_huge_pages leak"
> 
> I think you're right, but don't we also need to get rid of the
> folio_put() call in the 'if (err)' case after calling
> hugetlb_add_to_page_cache()?

That folio_put drops the ref obtained by unfreeze:

memfd_alloc_folio()
   alloc_hugetlb_folio_nodemask()
     dequeue_hugetlb_folio_nodemask()
       dequeue_hugetlb_folio_node_exact()
         folio_ref_unfreeze(folio, 1);    ; adds 1 refcount

- Steve

>> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning memfd folios")
>>
>> Suggested-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   mm/gup.c   | 4 +---
>>   mm/memfd.c | 2 +-
>>   2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index bccabaa..947881ff 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -3618,7 +3618,7 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
>>   	pgoff_t start_idx, end_idx, next_idx;
>>   	struct folio *folio = NULL;
>>   	struct folio_batch fbatch;
>> -	struct hstate *h = NULL;
>> +	struct hstate *h;
>>   	long ret = -EINVAL;
>>   
>>   	if (start < 0 || start > end || !max_folios)
>> @@ -3662,8 +3662,6 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
>>   							     &fbatch);
>>   			if (folio) {
>>   				folio_put(folio);
>> -				if (h)
>> -					folio_put(folio);
>>   				folio = NULL;
>>   			}
>>   
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index bcb131d..f715301 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
>> @@ -89,7 +89,7 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>>   						    numa_node_id(),
>>   						    NULL,
>>   						    gfp_mask);
>> -		if (folio && folio_try_get(folio)) {
>> +		if (folio) {
>>   			err = hugetlb_add_to_page_cache(folio,
>>   							memfd->f_mapping,
>>   							idx);
>> -- 
>> 1.8.3.1
>>