From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2E5E9EB64DD for ; Wed, 12 Jul 2023 19:48:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 68FB38E0005; Wed, 12 Jul 2023 15:48:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 63F888E0002; Wed, 12 Jul 2023 15:48:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4E01A8E0005; Wed, 12 Jul 2023 15:48:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 3BBFE8E0002 for ; Wed, 12 Jul 2023 15:48:25 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id F10D112036E for ; Wed, 12 Jul 2023 19:48:24 +0000 (UTC) X-FDA: 81003996528.07.CA97D13 Received: from mail-yb1-f171.google.com (mail-yb1-f171.google.com [209.85.219.171]) by imf11.hostedemail.com (Postfix) with ESMTP id 32EA94000E for ; Wed, 12 Jul 2023 19:48:22 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=Qsi8SCQ4; spf=pass (imf11.hostedemail.com: domain of surenb@google.com designates 209.85.219.171 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689191303; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ryI+iQYRW8e7m//aKp3sgPHutOkW3BfhlGtXfN8LbS4=; b=FWh278VWnwztisAcgSO43M5csPIrCRmQRNbD9TOfLLIktcbZlCa31CHdowusHuOjW5dOft 2PQInAPbf23Ano4hTWl05QBUKJC6qMUf2nl3f7/1xbZhxccrPC/cxovVxiBRiXPT3Sjeq9 zvydzrKVt2IjWPW60PQ8HqIuCaDjIXk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689191303; a=rsa-sha256; cv=none; b=h99ZY5yZozzx+BU712NnkyPlbWgtUKvZPdBsnr81Dsfq5OxUJrHz0sDE+puYZh/9DLyoS3 BTsids0KFONz7ygt6SIJTz+vxAgiN2jhErS/EBEW6naTNZTFiL7ZzZ8qUJjFo5PYAe+8lC +2lmu0jrBR2+/gu4EQnEPu0tzoXz+Z4= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=Qsi8SCQ4; spf=pass (imf11.hostedemail.com: domain of surenb@google.com designates 209.85.219.171 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-yb1-f171.google.com with SMTP id 3f1490d57ef6-cabf1dbafc4so489923276.2 for ; Wed, 12 Jul 2023 12:48:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689191302; x=1691783302; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ryI+iQYRW8e7m//aKp3sgPHutOkW3BfhlGtXfN8LbS4=; b=Qsi8SCQ48YygDhrcva/o0iP3wlKaP6ZcIPK/eKcRUiq3VgkvXOrGYimA6JEtj/VvKN sJFQPKeZROTZzAYxhZQzM2XyfFP6WNNmncflq26qGW+J/LPY6TQlOJbHjONpTIq/HP8h 4N75JSw0VwtGLUgRYTmyFt6ON5AWVoWNqabgOvImqv79jYQLgBsSvGojEpU0c0lwwM3R tYfnnjvbEvZU93JozTybjP5Pn9M1HkUcXy6df4SIp4eaXk0dXi0/WLpItxmixZ3nv8Lh tROBO+Zbr2k2+Vegac40t/s1aYjnUgzX0yIiz18Qe4QCJVgz74dH1A280yuxaGhPzCGg Vl6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689191302; x=1691783302; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ryI+iQYRW8e7m//aKp3sgPHutOkW3BfhlGtXfN8LbS4=; b=cxVK9TILKU/ANYjmT+t9an7anRbXAj/uAajWRqR+WT6ycODydkDiYKKyCNcHk2HedY KOHtS+3lH9WSJUI00F3vH8DxFavggc32ju9O7WaWGl1+IiR5EmrQN0jdirF2MKgpX/f9 py/JeP59hbjT2OoMGnlJNLBHH3E8Px3ToviM86C1wxBp0sDDlsP6drHm+Q4MO7vWjGN+ 9sdee1DySby9n6CcgMGGlNFid457gNmSEHVq9utvhgUVY7ceXqeix68fpO2LJBKcm5vR AN21N7GpOQ04IdTcUu4rCnq7WsvKAZBbEbJPQPwE6KO5H8RnYWYNFeUJzkAGnl14iZOp SDEw== X-Gm-Message-State: ABy/qLZyq3IQbCIe/kQNUoRIQs6C6v3YJHZ5JlG5UTr000haD3xiNJTp YSvRKwQ6WgAQhBxAqvo4fc2Lfel2JnGce3iOy5wqmQ== X-Google-Smtp-Source: APBJJlFe4hlnVLrjLKDsuThNmwwiow9w6l1Zw+FU1AovSr30LyjgL4ZexTHiEAa7BShm/IBrGvG/0PPHYoOzaqoWcuc= X-Received: by 2002:a25:2e4e:0:b0:c6f:bf96:28e3 with SMTP id b14-20020a252e4e000000b00c6fbf9628e3mr13122934ybn.35.1689191301994; Wed, 12 Jul 2023 12:48:21 -0700 (PDT) MIME-Version: 1.0 References: <9704a138-60e6-4ede-91f0-844e1df2ad84@moroto.mountain> <331201b2-5f13-8e81-b5d4-b17f8784d498@redhat.com> In-Reply-To: From: Suren Baghdasaryan Date: Wed, 12 Jul 2023 19:48:06 +0000 Message-ID: Subject: Re: [bug report] mm: replace vma->vm_flags direct modifications with modifier calls To: David Hildenbrand Cc: Matthew Wilcox , Dan Carpenter , linux-mm@kvack.org, Andrew Morton , "Liam R. Howlett" , Laurent Dufour , Michel Lespinasse , Jerome Glisse , Michal Hocko , Vlastimil Babka , Johannes Weiner , Peter Xu , Dimitri Sivanich , Mike Travis , Steve Wahl Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 32EA94000E X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: y47zmsqyxkiw3jeqwq4p4agtynassm1w X-HE-Tag: 1689191302-709571 X-HE-Meta: U2FsdGVkX19bho14bJt7YeEOOSg1pv6OcaUYJLS5v8d6xDQN3GHQSHqc3yhSP0MBjeZ1ZOju0COu9qrKPtGZSV+L9csTM4YnxvuJcgY6g+s/ZuJmsGYeuha7Sf2GrelnY1qQaNa8M1uJaFcBGO6buC1gZRla76yxZ5o277inzYv9AIjPQ9cXUFhQ3eIGAFpHXX37slZlmYzquQWi4H/9ia1espI0eaycLNDWp3xTPshTzVX2XAfLsuctWs1yzbsf/m2Lk8WXO6ctzLOrkoL/kBIALiRmCOli2PA9gczXT5lX0+i6TTKO8Uw7+00JEvm7yyO2fIjiHivMq4Ny9urF7h6erAIezwwUkXP/Xb+5E+KaKWMmELr7via8S0ODLMPN18en/aYdVELYs3XwndOYJElKu3f+b4Nxd7aMtJo4rl5MWrI3A9QvDIOBXQTIrI6sIhS27N8bmTsB7a0X2naQauHjdAuWWyFq+4XlV0VYCP9VD3XyhxAY41SaOe8ArJisHf/PgNRC5VC1oLKsbVFSR6plIWs1ikutaYs9sX82sOfjVn9cDQGVWWLpja9AYqGajLrGm1NP48oN0ppIkAUjhurDQDrO0bk9NeGOaR3Ek3HcNnQWHFHMGRdOSmHPWDVoWOGJ2LSXSauCnQsJYLOEhcowPD7fxyTKdMln0/PMiX/aIdnbAKKpieWhiLHeWqXstfGgKs1qbDFl2voDe6/0kIaZE5ou4kHudFNYVIW+I/rxcs3vFXi2476A3PKRlI9G6A1tYZiJLL4n8bagI/iiSNbbT+R3cV1HRsi8rDPy820C55dj0KDaebvChKX1xKyS+8QhSftV+uGPCMMuOnECFIVY76hBIbVupW8PAMP5bRD7lNZkO0tT6mcycA35Dglj01jkhPnvYuDqH+Isj01yaamfICy+RWNdwGdiwy3+csd9lMUP6sAnPlyT1H9wnBF4ZkkjmP/YLEOeDy42g8c tbkTRlI0 crRojfYvsv/TqHFxpYYyW6B+cu15tL747ywu2ajG4nG0PN5ogvn/FWpYLihJJhKwRlQrfEAflQd22vDOMn70e/arQzz5S09Q0B0pIUVkc4C87opJzx4vkF497zDl+nEi4mDD8TplFrZRyDabixiK6A0Pu5BSWa1uriiQXVYHTfUHWZkpCDP8PQIfenyzHRBgWFLe9LlXoQMlBXHyxYEAOrj9nYJOVIYNelgTDETH1tXnx0SAfJAhmfusb5kzBXnYSE7mAtOksVa0Rm3BwqJMU3ESA7Y8JOCUMGtiZiK2l0IR7REU= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Jul 12, 2023 at 6:52=E2=80=AFPM David Hildenbrand wrote: > > On 12.07.23 20:49, Matthew Wilcox wrote: > > On Wed, Jul 12, 2023 at 05:55:47PM +0200, David Hildenbrand wrote: > >> On 12.07.23 17:52, Matthew Wilcox wrote: > >>> On Wed, Jul 12, 2023 at 08:01:18AM -0700, Suren Baghdasaryan wrote: > >>>> Are you suggesting to break remap_pfn_range() into two stages > >>>> (remap_pfn_range_prepare() then remap_pfn_range())? > >>>> If so, there are many places remap_pfn_range() is called and IIUC al= l > >>>> of them would need to use that 2-stage approach (lots of code churn)= . > >>>> In addition, this is an exported function, so many more drivers migh= t > >>>> expect the current behavior. > >>> > >>> You do not understand correctly. > >>> > >>> When somebody calls mmap, there are two reasonable implementations. > >>> Here's one: > >>> > >>> .mmap =3D snd_dma_iram_mmap, > >>> > >>> static int snd_dma_iram_mmap(struct snd_dma_buffer *dmab, > >>> struct vm_area_struct *area) > >>> { > >>> area->vm_page_prot =3D pgprot_writecombine(area->vm_page_pr= ot); > >>> return remap_pfn_range(area, area->vm_start, > >>> dmab->addr >> PAGE_SHIFT, > >>> area->vm_end - area->vm_start, > >>> area->vm_page_prot); > >>> } > >>> > >>> This is _fine_. It is not called from the fault path, it is called i= n > >>> process context. Few locks are held (which ones aren't even > >>> documented!) > >>> > >>> The other way is to set vma->vm_ops. The fault handler in vm_ops > >>> should not be calling remap_pfn_range(). It should be calling > >>> set_ptes(). I almost have this driver fixed up, but I have another > >>> meeting to go to now. > >> > >> Just a note that we still have to make sure that the VMA flags will be= set > >> properly -- I guess at mmap time is the right time as I suggested abov= e. > > > > It actually does that already: > > > > static int gru_file_mmap(struct file *file, struct vm_area_struct *vma) > > { > > if ((vma->vm_flags & (VM_SHARED | VM_WRITE)) !=3D (VM_SHARED |= VM_WRITE)) > > return -EPERM; > > > > if (vma->vm_start & (GRU_GSEG_PAGESIZE - 1) || > > vma->vm_end & (GRU_GSEG_PAGESIZE - 1)) > > return -EINVAL; > > > > vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_LOCKED | > > VM_DONTCOPY | VM_DONTEXPAND | VM_DONTDUMP); > > > > > > This compiles, but obviously I don't have a spare HP supercomputer lyin= g > > around for me to test whether it works. Also set_ptes() was only just > > introduced to the mm tree, so doing something that needs backporting > > would take more effort (maybe having a private set_ptes() in the driver > > would be a good backport option that calls set_pte_at() in a loop). vm_flags_set() was introduced in 6.3, so not too many versions to backport to if needed. Private set_ptes() seems reasonable to me. > > > > > > diff --git a/drivers/misc/sgi-gru/grumain.c b/drivers/misc/sgi-gru/grum= ain.c > > index 4eb4b9455139..c21bcb528f12 100644 > > --- a/drivers/misc/sgi-gru/grumain.c > > +++ b/drivers/misc/sgi-gru/grumain.c > > @@ -951,6 +951,8 @@ vm_fault_t gru_fault(struct vm_fault *vmf) > > } > > > > if (!gts->ts_gru) { > > + pte_t *ptep, pte; > > + > > STAT(load_user_context); > > if (!gru_assign_gru_context(gts)) { > > preempt_enable(); > > @@ -964,9 +966,12 @@ vm_fault_t gru_fault(struct vm_fault *vmf) > > } > > gru_load_context(gts); > > paddr =3D gseg_physical_address(gts->ts_gru, gts->ts_ctxn= um); > > - remap_pfn_range(vma, vaddr & ~(GRU_GSEG_PAGESIZE - 1), > > - paddr >> PAGE_SHIFT, GRU_GSEG_PAGESIZE, > > - vma->vm_page_prot); > > + > > + pte =3D pfn_pte(paddr / PAGE_SIZE, vma->vm_page_prot); > > + ptep =3D vmf->pte - (vaddr % GRU_GSEG_PAGESIZE) / PAGE_SI= ZE; > > + set_ptes(vma->vm_mm, vaddr & ~(GRU_GSEG_PAGESIZE - 1), > > + ptep, pte_mkspecial(pte), > > + GRU_GSEG_PAGESIZE / PAGE_SIZE); > > } > > > > preempt_enable(); > > > > Would we be able to fix it in stable simply by not triggering the > vm_flags_set() in case these flags are already set? I think we can do that. gru_file_mmap() sets all the flags that are set by remap_pfn_range_notrack() (VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP), so we can check if all bits are already present and skip the vm_flags_set() call. > > -- > Cheers, > > David / dhildenb >