Re: [PATCH v8 1/4] userfaultfd: Add UFFD WP Async support

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

On 1/31/23 2:27 AM, Peter Xu wrote:
> On Mon, Jan 30, 2023 at 01:38:16PM +0500, Muhammad Usama Anjum wrote:
>> On 1/27/23 8:32 PM, Peter Xu wrote:
>>> On Fri, Jan 27, 2023 at 11:47:14AM +0500, Muhammad Usama Anjum wrote:
>>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>>> index 4000e9f017e0..8c03b133d483 100644
>>>>>> --- a/mm/memory.c
>>>>>> +++ b/mm/memory.c
>>>>>> @@ -3351,6 +3351,18 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>>>>>>  
>>>>>>  	if (likely(!unshare)) {
>>>>>>  		if (userfaultfd_pte_wp(vma, *vmf->pte)) {
>>>>>> +			if (userfaultfd_wp_async(vma)) {
>>>>>> +				/*
>>>>>> +				 * Nothing needed (cache flush, TLB invalidations,
>>>>>> +				 * etc.) because we're only removing the uffd-wp bit,
>>>>>> +				 * which is completely invisible to the user. This
>>>>>> +				 * falls through to possible CoW.
>>>>>
>>>>> Here it says it falls through to CoW, but..
>>>>>
>>>>>> +				 */
>>>>>> +				pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> +				set_pte_at(vma->vm_mm, vmf->address, vmf->pte,
>>>>>> +					   pte_clear_uffd_wp(*vmf->pte));
>>>>>> +				return 0;
>>>>>
>>>>> ... it's not doing so.  The original lines should do:
>>>>>
>>>>> https://lore.kernel.org/all/Y8qq0dKIJBshua+X@x1n/
>>>
>>> [1]
>>>
>>>>>
>>>>> Side note: you cannot modify pgtable after releasing the pgtable lock.
>>>>> It's racy.
>>>> If I don't unlock and return after removing the UFFD_WP flag in case of
>>>> async wp, the target just gets stuck. Maybe the pte lock is not unlocked in
>>>> some path.
>>>>
>>>> If I unlock and don't return, the crash happens.
>>>>
>>>> So I'd put unlock and return from here. Please comment on the below patch
>>>> and what do you think should be done. I've missed something.
>>>
>>> Have you tried to just use exactly what I suggested in [1]?  I'll paste
>>> again:
>>>
>>> ---8<---
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 4000e9f017e0..09aab434654c 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -3351,8 +3351,20 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>>>
>>>         if (likely(!unshare)) {
>>>                 if (userfaultfd_pte_wp(vma, *vmf->pte)) {
>>> -                       pte_unmap_unlock(vmf->pte, vmf->ptl);
>>> -                       return handle_userfault(vmf, VM_UFFD_WP);
>>> +                       if (userfaultfd_uffd_wp_async(vma)) {
>>> +                               /*
>>> +                                * Nothing needed (cache flush, TLB
>>> +                                * invalidations, etc.) because we're only
>>> +                                * removing the uffd-wp bit, which is
>>> +                                * completely invisible to the user.
>>> +                                * This falls through to possible CoW.
>>> +                                */
>>> +                               set_pte_at(vma->vm_mm, vmf->address, vmf->pte,
>>> +                                          pte_clear_uffd_wp(*vmf->pte));
>>> +                       } else {
>>> +                               pte_unmap_unlock(vmf->pte, vmf->ptl);
>>> +                               return handle_userfault(vmf, VM_UFFD_WP);
>>> +                       }
>>>                 }
>>> ---8<---
>>>
>>> Note that there's no "return", neither the unlock.  The lock is used in the
>>> follow up write fault resolution and it's released later.
>> I've tried out the exact patch above. This doesn't work. The pages keep
>> their WP flag even after being resolved in do_wp_page() while is written on
>> the page.
>>
>> So I'd added pte_unmap_unlock() and return 0 from here. This makes the
>> patch to work. Maybe you can try this on your end to see what I'm seeing here?
> 
> Oh maybe it's because it didn't update orig_pte.  If you want, you can try
> again with doing so by changing:
> 
>   set_pte_at(vma->vm_mm, vmf->address, vmf->pte,
>              pte_clear_uffd_wp(*vmf->pte));
> 
> into:
> 
>   pte_t pte = pte_clear_uffd_wp(*vmf->pte);
>   set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
>   /* Update this to be prepared for following up CoW handling */
>   vmf->orig_pte = pte;
> 
It works.

>>
>>>
>>> Meanwhile please fully digest how pgtable lock is used in this path before
>>> moving forward on any of such changes.
>>>
>>>>
>>>>>
>>>>>> +			}
>>>>>>  			pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>>  			return handle_userfault(vmf, VM_UFFD_WP);
>>>>>>  		}
>>>>>> @@ -4812,8 +4824,21 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
>>>>>>  
>>>>>>  	if (vma_is_anonymous(vmf->vma)) {
>>>>>>  		if (likely(!unshare) &&
>>>>>> -		    userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd))
>>>>>> -			return handle_userfault(vmf, VM_UFFD_WP);
>>>>>> +		    userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) {
>>>>>> +			if (userfaultfd_wp_async(vmf->vma)) {
>>>>>> +				/*
>>>>>> +				 * Nothing needed (cache flush, TLB invalidations,
>>>>>> +				 * etc.) because we're only removing the uffd-wp bit,
>>>>>> +				 * which is completely invisible to the user. This
>>>>>> +				 * falls through to possible CoW.
>>>>>> +				 */
>>>>>> +				set_pmd_at(vmf->vma->vm_mm, vmf->address, vmf->pmd,
>>>>>> +					   pmd_clear_uffd_wp(*vmf->pmd));
>>>>>
>>>>> This is for THP, not hugetlb.
>>>>>
>>>>> Clearing uffd-wp bit here for the whole pmd is wrong to me, because we
>>>>> track writes in small page sizes only.  We should just split.
>>>> By detecting if the fault is async wp, just splitting the PMD doesn't work.
>>>> The below given snippit is working right now. But definately, the fault of
>>>> the whole PMD is being resolved which if we can bypass by correctly
>>>> splitting would be highly desirable. Can you please take a look on UFFD
>>>> side and suggest the changes? It would be much appreciated. I'm attaching
>>>> WIP v9 patches for you to apply on next(next-20230105) and pagemap_ioctl
>>>> selftest can be ran to test things after making changes.
>>>
>>> Can you elaborate why thp split didn't work?  Or if you want, I can look
>>> into this and provide the patch to enable uffd async mode.
>> Sorry, I was doing the wrong way. Splitting the page does work. What do you
>> think about the following:
>>
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3351,6 +3351,17 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>>
>>  	if (likely(!unshare)) {
>>  		if (userfaultfd_pte_wp(vma, *vmf->pte)) {
>> +			if (userfaultfd_wp_async(vma)) {
>> +				/*
>> +				 * Nothing needed (cache flush, TLB invalidations,
>> +				 * etc.) because we're only removing the uffd-wp bit,
>> +				 * which is completely invisible to the user.
>> +				 */
>> +				set_pte_at(vma->vm_mm, vmf->address, vmf->pte,
>> +					   pte_clear_uffd_wp(*vmf->pte));
>> +				pte_unmap_unlock(vmf->pte, vmf->ptl);
>> +				return 0;
> 
> Please give it a shot with above to see whether we can avoid the "return 0"
> here.
> 
>> +			}
>>  			pte_unmap_unlock(vmf->pte, vmf->ptl);
>>  			return handle_userfault(vmf, VM_UFFD_WP);
>>  		}
>> @@ -4812,8 +4823,13 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault
>> *vmf)
>>
>>  	if (vma_is_anonymous(vmf->vma)) {
>>  		if (likely(!unshare) &&
>> -		    userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd))
>> +		    userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) {
>> +			if (userfaultfd_wp_async(vmf->vma)) {
>> +				__split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL);
>> +				return 0;
> 
> Same here, I hope it'll work for you if you just goto __split_huge_pmd()
> right below and return with VM_FAULT_FALLBACK.  It avoids one more round of
> fault just like the pte case above.
> 
It works as well.

>> +			}
>>  			return handle_userfault(vmf, VM_UFFD_WP);
>> +		}
>>  		return do_huge_pmd_wp_page(vmf);
>>  	}
> 

-- 
BR,
Muhammad Usama Anjum