Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jason Gunthorpe <jgg@mellanox.com>
To:     Souptick Joarder <jrdr.linux@gmail.com>
CC:     Christoph Hellwig <hch@lst.de>,
        =?iso-8859-1?Q?J=E9r=F4me_Glisse?= <jglisse@redhat.com>,
        Ben Skeggs <bskeggs@redhat.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        Linux-MM <linux-mm@kvack.org>,
        "nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Felix Kuehling <Felix.Kuehling@amd.com>
Subject: Re: [PATCH 1/6] mm: always return EBUSY for invalid ranges in
 hmm_range_{fault,snapshot}
Thread-Topic: [PATCH 1/6] mm: always return EBUSY for invalid ranges in
 hmm_range_{fault,snapshot}
Thread-Index: AQHVQHIRs6fGZEVop06hMQcrvsZ8NqbWtRqAgAGXHoA=
Date:   Tue, 23 Jul 2019 14:54:45 +0000
Message-ID: <20190723145441.GI15331@mellanox.com>
References: <20190722094426.18563-1-hch@lst.de>
 <20190722094426.18563-2-hch@lst.de>
 <CAFqt6zY8zWAmc-VTrZ1KxQPBCdbTxmZy_tq2-OkUi3TVrfp7Og@mail.gmail.com>
In-Reply-To: <CAFqt6zY8zWAmc-VTrZ1KxQPBCdbTxmZy_tq2-OkUi3TVrfp7Og@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [156.34.55.100]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: d966fda0-338e-478c-42a1-08d70f7db38f
x-ms-office365-filtering-ht: Tenant
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(4618075)(2017052603328)(7193020);SRVR:VI1PR05MB3472;
x-ms-traffictypediagnostic: VI1PR05MB3472:
x-microsoft-antispam-prvs: <VI1PR05MB347230E82CADE677CF76D105CFC70@VI1PR05MB3472.eurprd05.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:10000;
x-forefront-prvs: 0107098B6C
x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(346002)(396003)(366004)(376002)(136003)(39860400002)(199004)(189003)(3846002)(52116002)(8676002)(86362001)(4326008)(14444005)(6512007)(5660300002)(36756003)(6246003)(64756008)(54906003)(478600001)(66446008)(7416002)(53936002)(66556008)(66476007)(305945005)(7736002)(68736007)(25786009)(256004)(66946007)(6116002)(476003)(486006)(81166006)(33656002)(81156014)(11346002)(1076003)(6486002)(6916009)(2616005)(2906002)(76176011)(8936002)(6436002)(71200400001)(71190400001)(316002)(186003)(6506007)(14454004)(53546011)(386003)(446003)(66066001)(229853002)(99286004)(102836004)(26005)(142933001);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR05MB3472;H:VI1PR05MB4141.eurprd05.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1;
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: cJYSJS2lynI+8NSitY0FCPF3DUkNFAV9rRYTDScuv0IEZyNwhUTOooWDYj2xi0xGXE3/oMIvdp7ZHywr0NO2Zklirmi83UxUGZsHZLBriJog+mCFGxkkPNbNbYM8HynGZeKbNPuWrGdmNbJR1POj87XJ9HqDsIJ6+Rzk5U3sb5rBYCw7cXdsReBv4kLS2ipscGz1zpHloPKnWfrm5yasb+YL7Obs74wYRn868EhdOQwLte3hQNWWExU78jtuMPdy6MvtixZmTXbrheElthkZAmU45ShPGsC920dg8ldAyWFsfFo1gkyg8dMxngwKBEzc9PiCsk2pD3L1RCBmFzsdxujyg4fIQTTBqcOveNHDooi/ki2rqYBCv0xzGO5VkBzrRW1Y0X+tFjyZxl1Uy3QPkDxI6Es1212yFeCGnjy6s60=
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <84AEF1430B5CAF409CC0D62618B7407E@eurprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Mellanox.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d966fda0-338e-478c-42a1-08d70f7db38f
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jul 2019 14:54:45.5342
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: jgg@mellanox.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR05MB3472
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Jul 22, 2019 at 08:07:33PM +0530, Souptick Joarder wrote:
> On Mon, Jul 22, 2019 at 3:14 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > We should not have two different error codes for the same condition.  I=
n
> > addition this really complicates the code due to the special handling o=
f
> > EAGAIN that drops the mmap_sem due to the FAULT_FLAG_ALLOW_RETRY logic
> > in the core vm.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> >  Documentation/vm/hmm.rst |  2 +-
> >  mm/hmm.c                 | 10 ++++------
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
> > index 7d90964abbb0..710ce1c701bf 100644
> > +++ b/Documentation/vm/hmm.rst
> > @@ -237,7 +237,7 @@ The usage pattern is::
> >        ret =3D hmm_range_snapshot(&range);
> >        if (ret) {
> >            up_read(&mm->mmap_sem);
> > -          if (ret =3D=3D -EAGAIN) {
> > +          if (ret =3D=3D -EBUSY) {
> >              /*
> >               * No need to check hmm_range_wait_until_valid() return va=
lue
> >               * on retry we will get proper error with hmm_range_snapsh=
ot()
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index e1eedef129cf..16b6731a34db 100644
> > +++ b/mm/hmm.c
> > @@ -946,7 +946,7 @@ EXPORT_SYMBOL(hmm_range_unregister);
> >   * @range: range
> >   * Return: -EINVAL if invalid argument, -ENOMEM out of memory, -EPERM =
invalid
> >   *          permission (for instance asking for write and range is rea=
d only),
> > - *          -EAGAIN if you need to retry, -EFAULT invalid (ie either n=
o valid
> > + *          -EBUSY if you need to retry, -EFAULT invalid (ie either no=
 valid
> >   *          vma or it is illegal to access that range), number of vali=
d pages
> >   *          in range->pfns[] (from range start address).
> >   *
> > @@ -967,7 +967,7 @@ long hmm_range_snapshot(struct hmm_range *range)
> >         do {
> >                 /* If range is no longer valid force retry. */
> >                 if (!range->valid)
> > -                       return -EAGAIN;
> > +                       return -EBUSY;
> >
> >                 vma =3D find_vma(hmm->mm, start);
> >                 if (vma =3D=3D NULL || (vma->vm_flags & device_vma))
> > @@ -1062,10 +1062,8 @@ long hmm_range_fault(struct hmm_range *range, bo=
ol block)
> >
> >         do {
> >                 /* If range is no longer valid force retry. */
> > -               if (!range->valid) {
> > -                       up_read(&hmm->mm->mmap_sem);
> > -                       return -EAGAIN;
> > -               }
> > +               if (!range->valid)
> > +                       return -EBUSY;
>=20
> Is it fine to remove  up_read(&hmm->mm->mmap_sem) ?

It seems very subtle, but under the covers this calls
handle_mm_fault() with FAULT_FLAG_ALLOW_RETRY which will cause the
mmap sem to become unlocked along the -EAGAIN return path.

I think without the commit message I wouldn't have been able to
understand that, so Christoph, could you also add the comment below
please?

Otherwise

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Frankly, I find the 'int *locked' scheme of GUP easier to understand..

diff --git a/mm/hmm.c b/mm/hmm.c
index 16b6731a34db79..54b3a4162ae949 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -301,8 +301,10 @@ static int hmm_vma_do_fault(struct mm_walk *walk, unsi=
gned long addr,
 	flags |=3D hmm_vma_walk->block ? 0 : FAULT_FLAG_ALLOW_RETRY;
 	flags |=3D write_fault ? FAULT_FLAG_WRITE : 0;
 	ret =3D handle_mm_fault(vma, addr, flags);
-	if (ret & VM_FAULT_RETRY)
+	if (ret & VM_FAULT_RETRY) {
+		/* Note, handle_mm_fault did up_read(&mm->mmap_sem)) */
 		return -EAGAIN;
+	}
 	if (ret & VM_FAULT_ERROR) {
 		*pfn =3D range->values[HMM_PFN_ERROR];
 		return -EFAULT;