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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH v8 1/2] hugetlb: don't delete vma_lock in hugetlb
 MADV_DONTNEED processing
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <20221108011910.350887-2-mike.kravetz@oracle.com>
Date:   Thu, 10 Nov 2022 12:59:45 -0800
Cc:     Linux-MM <linux-mm@kvack.org>,
        kernel list <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>,
        Peter Xu <peterx@redhat.com>, Rik van Riel <riel@surriel.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Wei Chen <harperchen1110@gmail.com>, stable@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <9BB0EA0C-6E7C-462B-8374-5BFEC34E8415@gmail.com>
References: <20221108011910.350887-1-mike.kravetz@oracle.com>
 <20221108011910.350887-2-mike.kravetz@oracle.com>
To:     Mike Kravetz <mike.kravetz@oracle.com>
Precedence: bulk

On Nov 7, 2022, at 5:19 PM, Mike Kravetz <mike.kravetz@oracle.com> =
wrote:

> madvise(MADV_DONTNEED) ends up calling zap_page_range() to clear page
> tables associated with the address range.  For hugetlb vmas,
> zap_page_range will call __unmap_hugepage_range_final.  However,
> __unmap_hugepage_range_final assumes the passed vma is about to be =
removed
> and deletes the vma_lock to prevent pmd sharing as the vma is on the =
way
> out.  In the case of madvise(MADV_DONTNEED) the vma remains, but the
> missing vma_lock prevents pmd sharing and could potentially lead to =
issues
> with truncation/fault races.

I understand the problem in general. Please consider my feedback as =
partial
though.


> @@ -5203,32 +5194,50 @@ void __unmap_hugepage_range_final(struct =
mmu_gather *tlb,
> 			  unsigned long end, struct page *ref_page,
> 			  zap_flags_t zap_flags)
> {
> +	bool final =3D zap_flags & ZAP_FLAG_UNMAP;
> +

Not sure why caching final in local variable helps.

>=20
> void unmap_hugepage_range(struct vm_area_struct *vma, unsigned long =
start,
> 			  unsigned long end, struct page *ref_page,
> 			  zap_flags_t zap_flags)
> {
> +	struct mmu_notifier_range range;
> 	struct mmu_gather tlb;
>=20
> +	mmu_notifier_range_init(&range, MMU_NOTIFY_UNMAP, 0, vma, =
vma->vm_mm,
> +				start, end);
> +	adjust_range_if_pmd_sharing_possible(vma, &range.start, =
&range.end);
> 	tlb_gather_mmu(&tlb, vma->vm_mm);
> +
> 	__unmap_hugepage_range(&tlb, vma, start, end, ref_page, =
zap_flags);

Is there a reason for not using range.start and range.end?

It is just that every inconsistency is worrying=E2=80=A6

>=20
> @@ -1734,6 +1734,9 @@ static void zap_page_range_single(struct =
vm_area_struct *vma, unsigned long addr
> 	lru_add_drain();
> 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, =
vma->vm_mm,
> 				address, address + size);
> +	if (is_vm_hugetlb_page(vma))
> +		adjust_range_if_pmd_sharing_possible(vma, &range.start,
> +							&range.end);
> 	tlb_gather_mmu(&tlb, vma->vm_mm);
> 	update_hiwater_rss(vma->vm_mm);
> 	mmu_notifier_invalidate_range_start(&range);
> @@ -1742,6 +1745,12 @@ static void zap_page_range_single(struct =
vm_area_struct *vma, unsigned long addr
> 	tlb_finish_mmu(&tlb);
> }
>=20
> +void zap_vma_range(struct vm_area_struct *vma, unsigned long address,
> +		unsigned long size)
> +{
> +	__zap_page_range_single(vma, address, size, NULL);

Ugh. So zap_vma_range() would actually be emitted as a wrapper function =
that
only calls __zap_page_range_single() (or worse __zap_page_range_single()
which is large would be inlined), unless you use LTO.

Another option is to declare __zap_page_range_size() in the header and =
move
this one to the header as inline wrapper.