Re: [PATCH v1 2/2] mm/hugetlb: support write-faults in shared mappings

From: David Hildenbrand <david@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Muchun Song <songmuchun@bytedance.com>,
	Peter Feiner <pfeiner@google.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH v1 2/2] mm/hugetlb: support write-faults in shared mappings
Date: Fri, 5 Aug 2022 20:20:52 +0200	[thread overview]
Message-ID: <4f644ac5-c40b-32d4-3234-c1dac3d09f83@redhat.com> (raw)
In-Reply-To: <Yu1dkZeBwv0SUQMq@xz-m1.local>

On 05.08.22 20:12, Peter Xu wrote:
> On Fri, Aug 05, 2022 at 01:03:29PM +0200, David Hildenbrand wrote:
>> Let's add a safety net if we ever get (again) a write-fault on a R/O-mapped
>> page in a shared mapping, in which case we simply have to map the
>> page writable.
>>
>> VM_MAYSHARE handling in hugetlb_fault() for FAULT_FLAG_WRITE
>> indicates that this was at least envisioned, but could never have worked
>> as expected. This theoretically paves the way for softdirty tracking
>> support in hugetlb.
>>
>> Tested without the fix for softdirty tracking.
>>
>> Note that there is no need to do any kind of reservation in hugetlb_fault()
>> in this case ... because we already have a hugetlb page mapped R/O
>> that we will simply map writable and we are not dealing with COW/unsharing.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  mm/hugetlb.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index a18c071c294e..bbab7aa9d8f8 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -5233,6 +5233,16 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
>>  	VM_BUG_ON(unshare && (flags & FOLL_WRITE));
>>  	VM_BUG_ON(!unshare && !(flags & FOLL_WRITE));
>>  
>> +	/* Let's take out shared mappings first, this should be a rare event. */
>> +	if (unlikely(vma->vm_flags & VM_MAYSHARE)) {
> 
> Should we check VM_SHARED instead?

Relying on VM_SHARED to detect MAP_PRIVATE vs. MAP_SHARED is
unfortunately wrong.

If you're curious, take a look at f83a275dbc5c ("mm: account for
MAP_SHARED mappings using VM_MAYSHARE and not VM_SHARED in hugetlbfs")
and mmap() code.

Long story short: if the file is read-only, we only have VM_MAYSHARE but
not VM_SHARED (and consequently also not VM_MAYWRITE).

> 
>> +		if (unshare)
>> +			return 0;
> 
> Curious when will this happen especially if we switch to VM_SHARED above.
> Shouldn't "unshare" not happen at all on a shared region?

FAULT_FLAG_UNSHARE is documented to behave like:

"FAULT_FLAG_UNSHARE is ignored and treated like an ordinary read fault
when no existing R/O-mapped anonymous page is encountered."

It should currently not happen. Focus on should ;)

> 
>> +		if (WARN_ON_ONCE(!(vma->vm_flags & VM_WRITE)))
>> +			return VM_FAULT_SIGSEGV;
> 
> I had a feeling that you just want to double check we have write
> permission, but IIUC this should be checked far earlier or we'll have
> problem.  No strong opinion if so, but I'd suggest dropping this one,
> otherwise we could add tons of WARN_ON_ONCE() in anywhere in the page fault
> stack and they mostly won't trigger at all.

Not quite. We usually (!hugetlb) have maybe_mkwrite() all over the
place. This is just an indication that we don't have maybe semantics
here. But as we also don't have it for hugetlb anon code below, maybe I
can just drop it. (or check it for both call paths)

> 
>> +		set_huge_ptep_writable(vma, haddr, ptep);
> 
> Do we wanna set dirty bits too?

set_huge_ptep_writable() handles that.

Thanks!

-- 
Thanks,

David / dhildenb