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:     Ralph Campbell <rcampbell@nvidia.com>
CC:     "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        =?iso-8859-1?Q?J=E9r=F4me_Glisse?= <jglisse@redhat.com>,
        Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 1/2] mm/hmm: a few more C style and comment clean ups
Thread-Topic: [PATCH 1/2] mm/hmm: a few more C style and comment clean ups
Thread-Index: AQHVQa6ccTnGbJ4iyUKRepNq/WEUcqbY4X2A
Date:   Tue, 23 Jul 2019 23:57:52 +0000
Message-ID: <20190723235747.GP15331@mellanox.com>
References: <20190723233016.26403-1-rcampbell@nvidia.com>
 <20190723233016.26403-2-rcampbell@nvidia.com>
In-Reply-To: <20190723233016.26403-2-rcampbell@nvidia.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: 2d870704-40c9-48e3-545d-08d70fc992a8
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:VI1PR05MB5037;
x-ms-traffictypediagnostic: VI1PR05MB5037:
x-microsoft-antispam-prvs: <VI1PR05MB50374C489F4CF0EB5003882ECFC70@VI1PR05MB5037.eurprd05.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:8882;
x-forefront-prvs: 0107098B6C
x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(366004)(396003)(136003)(39860400002)(346002)(376002)(189003)(199004)(7736002)(486006)(14454004)(305945005)(8936002)(81156014)(2616005)(14444005)(256004)(229853002)(1076003)(8676002)(71190400001)(36756003)(66066001)(5660300002)(71200400001)(6486002)(386003)(6916009)(316002)(446003)(11346002)(86362001)(54906003)(478600001)(99286004)(3846002)(68736007)(66946007)(66446008)(76176011)(64756008)(476003)(52116002)(6512007)(186003)(66476007)(66556008)(6436002)(53936002)(6116002)(102836004)(25786009)(53546011)(26005)(4326008)(6506007)(33656002)(2906002)(6246003)(81166006)(334744003);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR05MB5037;H:VI1PR05MB4141.eurprd05.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A: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: YsYw+TXrljd5KVznr5GNkrWU0koHjVRt9/Hg7Dq39UbN9i08nNtB4qlVNo5BNV2Bi6PST7LmG3GWRF3dMwZWG7SYj7JZqbFoG7TSfF5IVWM/1TGCmxTLL8RJM/3WOfCOosDRB4jycVYsNwvz+ucdl17NI+fS/SPOdgX5aiJ26PT6WsWRoF44wHlmfTD0zHMwteU1xssHMhXGqR3tvJmf3eYG86OzE6RIQBo6ma/NQarvxhQ+YtqO69xHV6/qE6edjCoNRUkj8wzAFRxbkSVl4aS+IItdEbSHHrgAUV1JS4D0XBFT5KCfZWVeoNhUYi7CUdg7gd0DOWFBYcJLYrk62/FFSeRPTjPOWZGMXJzOtEa+O+g5Jnil50fuu8sTzXdp6up+hya5n4eCq7zWhClGJsRuGPeR2wD9zKK06dYLH1g=
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <0ADC3FCB7798D64EAEAEF7472C17BC21@eurprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Mellanox.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d870704-40c9-48e3-545d-08d70fc992a8
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jul 2019 23:57:52.2021
 (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: VI1PR05MB5037
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 Tue, Jul 23, 2019 at 04:30:15PM -0700, Ralph Campbell wrote:
> -	if (pmd_huge(pmd) && (range->vma->vm_flags & VM_HUGETLB))
> +	if (pmd_huge(pmd) && is_vm_hugetlb_page(vma))
>  		return hmm_pfns_bad(start, end, walk);

This one is not a minor cleanup.. I think it should be done on its
own commit, and more comletely, maybe based on the below..

If vma is always the same as the the first vma, then your hunk above
here is much better than introducing a hugetlb flag as I did below..

Although I don't understand why we have this test when it does seem to
support huge pages, and the commit log suggests hugetlbfs was
deliberately supported. So a comment (or deletion) sure would be nice.

So maybe sequence this into your series?

Jason

From 6ea7cd2565b5b660d22a659b71b62614e66bc345 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Tue, 23 Jul 2019 12:28:32 -0300
Subject: [PATCH] mm/hmm: remove hmm_range vma

This value is only read inside hmm_vma_walk_pmd() and all the callers,
through walk_page_range(), always set the value. The proper place for
per-walk data is in hmm_vma_walk, and since the only usage is a vm_flags
test just precompute and store that.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/gpu/drm/nouveau/nouveau_svm.c |  7 +++----
 include/linux/hmm.h                   |  1 -
 mm/hmm.c                              | 11 ++++++-----
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouvea=
u/nouveau_svm.c
index a9c5c58d425b3d..4f4bec40b887a6 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -495,12 +495,12 @@ nouveau_range_fault(struct hmm_mirror *mirror, struct=
 hmm_range *range)
 				 range->start, range->end,
 				 PAGE_SHIFT);
 	if (ret) {
-		up_read(&range->vma->vm_mm->mmap_sem);
+		up_read(&range->hmm->mm->mmap_sem);
 		return (int)ret;
 	}
=20
 	if (!hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT)) {
-		up_read(&range->vma->vm_mm->mmap_sem);
+		up_read(&range->hmm->mm->mmap_sem);
 		return -EBUSY;
 	}
=20
@@ -508,7 +508,7 @@ nouveau_range_fault(struct hmm_mirror *mirror, struct h=
mm_range *range)
 	if (ret <=3D 0) {
 		if (ret =3D=3D 0)
 			ret =3D -EBUSY;
-		up_read(&range->vma->vm_mm->mmap_sem);
+		up_read(&range->hmm->mm->mmap_sem);
 		hmm_range_unregister(range);
 		return ret;
 	}
@@ -681,7 +681,6 @@ nouveau_svm_fault(struct nvif_notify *notify)
 			 args.i.p.addr + args.i.p.size, fn - fi);
=20
 		/* Have HMM fault pages within the fault window to the GPU. */
-		range.vma =3D vma;
 		range.start =3D args.i.p.addr;
 		range.end =3D args.i.p.addr + args.i.p.size;
 		range.pfns =3D args.phys;
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 9f32586684c9c3..d4b89f655817cd 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -164,7 +164,6 @@ enum hmm_pfn_value_e {
  */
 struct hmm_range {
 	struct hmm		*hmm;
-	struct vm_area_struct	*vma;
 	struct list_head	list;
 	unsigned long		start;
 	unsigned long		end;
diff --git a/mm/hmm.c b/mm/hmm.c
index 16b6731a34db79..3d8cdfb67a6ab8 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -285,8 +285,9 @@ struct hmm_vma_walk {
 	struct hmm_range	*range;
 	struct dev_pagemap	*pgmap;
 	unsigned long		last;
-	bool			fault;
-	bool			block;
+	bool			fault : 1;
+	bool			block : 1;
+	bool			hugetlb : 1;
 };
=20
 static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr,
@@ -635,7 +636,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
 	if (pmd_none(pmd))
 		return hmm_vma_walk_hole(start, end, walk);
=20
-	if (pmd_huge(pmd) && (range->vma->vm_flags & VM_HUGETLB))
+	if (pmd_huge(pmd) && hmm_vma_walk->hugetlb)
 		return hmm_pfns_bad(start, end, walk);
=20
 	if (thp_migration_supported() && is_pmd_migration_entry(pmd)) {
@@ -994,7 +995,7 @@ long hmm_range_snapshot(struct hmm_range *range)
 			return -EPERM;
 		}
=20
-		range->vma =3D vma;
+		hmm_vma_walk.hugetlb =3D vma->vm_flags & VM_HUGETLB;
 		hmm_vma_walk.pgmap =3D NULL;
 		hmm_vma_walk.last =3D start;
 		hmm_vma_walk.fault =3D false;
@@ -1090,7 +1091,7 @@ long hmm_range_fault(struct hmm_range *range, bool bl=
ock)
 			return -EPERM;
 		}
=20
-		range->vma =3D vma;
+		hmm_vma_walk.hugetlb =3D vma->vm_flags & VM_HUGETLB;
 		hmm_vma_walk.pgmap =3D NULL;
 		hmm_vma_walk.last =3D start;
 		hmm_vma_walk.fault =3D true;
--=20
2.22.0