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;
Date:   Mon, 31 Oct 2022 18:48:37 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Nadav Amit <nadav.amit@gmail.com>
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>,
        "# 5 . 10+" <stable@vger.kernel.org>
Subject: Re: [PATCH v6] hugetlb: don't delete vma_lock in hugetlb
 MADV_DONTNEED processing
Message-ID: <Y2B69ViNMYHvPXYk@monkey>
References: <20221031223440.285187-1-mike.kravetz@oracle.com>
 <8D2D2F0F-9A53-42B5-8A9D-936E06E4A4E9@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8D2D2F0F-9A53-42B5-8A9D-936E06E4A4E9@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?clhZeGtrdG0rVTNMa0gzdkxvVy9hZlkyWlliT0oxZGVybkVTalYzbTV5dW8z?=
 =?utf-8?B?K3BQcWt5WlB0M05WS3k5SDdhMjhEemZpNHJua1Rsb1NSWGtNaThRcWZDbzVh?=
 =?utf-8?B?STVsZEFkSDQreDYxOVlaZE4rclBWQlFlNUk2RlpYdjAvR1B6a1d6RGFva1A0?=
 =?utf-8?B?MXpKTXlZQWd3RVkwYlgwMndIQjFvTkJHVUtVSFRpbU10YUZCRWFhKzQwdVpq?=
 =?utf-8?B?VkFnS1dtWlFoTUM5TGYrWXVqcHQ1L3RQejIxMzNxMU1hZW0yTjZmZ2krbTdJ?=
 =?utf-8?B?OTExdGVMUnhGbUFaeEIzeUlHOGtrZFRqUy8rTkx4ZVlzZnI0cEJiS0V2azFz?=
 =?utf-8?B?SGZWek0xdHFBclhMRTVaQXpGcCsxSlVpbkdmdlVzbmI5ajA0UVhmMkFkWkpo?=
 =?utf-8?B?elpPZFRwZmxkUnBvRjMwSGpKd25YZWtNU1NvdkJiTzNHYVNDOVhDN1V5WHQz?=
 =?utf-8?B?SGxJbkFBRXE3cjYwK2JpNklrS1A4aS9jUnRUWWFqV2ZXUWhZRzZ5b21DV241?=
 =?utf-8?B?RmkwUk9mYkVQRTRsT2pEcmI1ZnYySVNabmttMWpabXVEMkpuRitocGNSSVNO?=
 =?utf-8?B?RFVuWktheGRtZXh5cEJiN3NUR29rRHdseGplSmtpaVBpT0hmOTg3Kys5U2tM?=
 =?utf-8?B?eGUraDNyV2hqSWRBK1EzaHJldFRLNGdDODI1N2kyODFRUkxFOGpkZkdwaWdq?=
 =?utf-8?B?SVQvelAyZDgvNWRjemQ0SlNuKzFwVkVXU2lOWTRxUkQvbEdUdDZsSk1yQnhV?=
 =?utf-8?B?RjZrYkg4d3FlOFMwNXBMUVZYcktoWnVUY3lCb0R2b2FrcjlCdGtRTWtDVlk3?=
 =?utf-8?B?Y3kwVDhoRlNWc0lGVTVQMmwwZ21sc2tJb2s3ODVYbTNyTk1xSmdvOWwxUkpT?=
 =?utf-8?B?Z3pmSUJiendMZk9CcEVUK2hVZXBmeDNCVkhhV0QrQWRJS2FEaFlRQkExczR6?=
 =?utf-8?B?MXdqdEdYSXhFM3dQQ2JHY2wvWjIycW1rVzQ5Zi9kYktRcUZXMHhHU2FqV3FD?=
 =?utf-8?B?c3VpUlV2SzVHOTUzaFZaRllyTDdWYjlHeEtZa1pRQ2ZrdFpkNHljRFFOVW5C?=
 =?utf-8?B?d0dNQ08wKy82UjZHdklzeUFvUlR3U0VZUzdSUVFXQkxxellZOVFhRG5JNXRD?=
 =?utf-8?B?Nm5FbWNsT0I1dGtrRXV2RTV5YVpETTIrUnk2Q3VYd21hUTBvbFhwRFNCSkRR?=
 =?utf-8?B?RVVDazBFU0w2VTdNSU9DejhXb2V2UDJaMjdjOG9nR21lRGtBNnFQangranRR?=
 =?utf-8?B?aTJsR3hxZzBQKzc1MEpDVDArWUhzTXgvSmdBY0FHM0VrYStuTnhyeGM3M2lj?=
 =?utf-8?B?MjBMSCtHU3VxeDRPRkhHeHF6KzVzV3NpYnlOc0FaUWVqNDJDWnJZQ2tOYVR1?=
 =?utf-8?B?WGZKRTR5RGRnQkNTKzBKWGJZb21xUTNDT2ovbVVqTE52SDVRcFhxUzBmTW9V?=
 =?utf-8?B?TGtVcEZHM1Uvb0daWmo0aDMvYU1rUm1UNDBlYWsyQlhaT2JLZzhDNlVmdzRo?=
 =?utf-8?B?T0wwc05SS3BEMGQ3NHp3ZjlSenN3eUhHOGw2azA3OTE1Nkozbkt5M1dMUTVl?=
 =?utf-8?B?b3FvSUtKVHhTWjIvdUtNYXhxRC9hbGVQUnl2V0NqWE9xazNLMFA3cVdVc1lK?=
 =?utf-8?B?ZEpmUXRaZWtHZHhSWDdLcGN5MWRHVHpEQkxZN3B3ZDM3OElGM3NYZnlmQkhT?=
 =?utf-8?B?dzd5OUc3RW41ZkVYQjVQdmRmbFRmQWhqQXI1aUlmUjE1UHlKSVcremVseDQr?=
 =?utf-8?B?d3hic3M0bnVuelQrUWlKdGVlNnNxWmt1UGNSQXpLSmw0UDNYL01veGdhZTRC?=
 =?utf-8?B?L3poekpmM21hN3NwMVpHWGhKU2tEOE5vS040ZFQvaytGUVpKL0lkcmUyOEJF?=
 =?utf-8?B?UDd4ZTRqVFFxSzdpelB1V1VzVkRjMkZuQ1gweHgrNkg5QngyYVZqeWYzdnJh?=
 =?utf-8?B?NGNSYzhkcHE4cGdNeFVyOW5kdnE5TW1TWDlQZldqWlZra2lXeEozYlZ6RzBs?=
 =?utf-8?B?dGc0eEhEdStyUFB4MkpBNFl3ZEpKQ3V2UXdtQkxMNlFBM2hENHdLUS96ZzZi?=
 =?utf-8?B?Zmxwb0Q5dzNRSXN1bjg4ZXZvTGRCdEZsN0RFZkMwZktCeGFzV3JranByekw1?=
 =?utf-8?B?WXpuV1A2cVV1dnBWNWo3czNRRGpCSXJHM3ZjTGhiN2NlTTczQU5tTkx0aG5p?=
 =?utf-8?B?QUE9PQ==?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 99c66aa6-9396-4d3d-7520-08dabbab3538
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Nov 2022 01:48:43.6008
 (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: +dQ3dnIEPdr7s8Ap2TVvdWA4snzE/15jeBN1fs8C15Hp/o9/SI1lp6wMD2okLFXwcRb481M48mxeLDm9Q7sQlQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4352
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-31_22,2022-10-31_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 mlxscore=0
 suspectscore=0 malwarescore=0 bulkscore=0 phishscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211010012
X-Proofpoint-ORIG-GUID: ISptwYJcFumDZYZhOVFWUeDPV97BZo-f
X-Proofpoint-GUID: ISptwYJcFumDZYZhOVFWUeDPV97BZo-f
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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 10/31/22 17:54, Nadav Amit wrote:
> On Oct 31, 2022, at 3:34 PM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> 
> > madvise(MADV_DONTNEED) ends up calling zap_page_range() to clear the 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.
> > 
> 
> [snip]
> 
> > index 978c17df053e..517c8cc8ccb9 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -3464,4 +3464,7 @@ madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> >  */
> > #define  ZAP_FLAG_DROP_MARKER        ((__force zap_flags_t) BIT(0))
> > 
> > +/* Set in unmap_vmas() to indicate an unmap call.  Only used by hugetlb */
> > +#define  ZAP_FLAG_UNMAP              ((__force zap_flags_t) BIT(1))
> 
> PeterZ wants to add ZAP_FLAG_FORCE_FLUSH that would be set on
> zap_pte_range(). Not sure you would want to combine them both together, but
> at least be aware of potential conflict.
> 
> https://lore.kernel.org/all/Y1f7YvKuwOl1XEwU@hirez.programming.kicks-ass.net/
> 

Ok, noted

> [snip]
> 
> > +#ifdef CONFIG_ADVISE_SYSCALLS
> > +/*
> > + * Similar setup as in zap_page_range().  madvise(MADV_DONTNEED) can not call
> > + * zap_page_range for hugetlb vmas as __unmap_hugepage_range_final will delete
> > + * the associated vma_lock.
> > + */
> > +void clear_hugetlb_page_range(struct vm_area_struct *vma, unsigned long start,
> > +				unsigned long end)
> > +{
> > +	struct mmu_notifier_range range;
> > +	struct mmu_gather tlb;
> > +
> > +	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 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);
> > +	update_hiwater_rss(vma->vm_mm);
> > +	mmu_notifier_invalidate_range_start(&range);
> > +
> > +	__unmap_hugepage_range_locking(&tlb, vma, start, end, NULL, 0);
> > +
> > +	mmu_notifier_invalidate_range_end(&range);
> > +	tlb_finish_mmu(&tlb);
> > }
> > +#endif
> 
> I hate ifdef’s. And the second definition of clear_hugetlb_page_range() is
> confusing since it does not have an ifdef at all. . How about moving the
> ifdef’s into the function like being done in io_madvise_prep()? I think it
> is less confusing.

Dang!!!  I sent the wrong version of the patch.  clear_hugetlb_page_range
is not even used here.  I used Peter's idea to eliminate the need for
this separate routine.  And, this was a development version.

Really sorry about that.

Andrew, if you can please drop the version of the patch.
-- 
Mike Kravetz