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 2/3] hfs: Replace kmap() with kmap_local_page() in bnode.c
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <20220809152004.9223-3-fmdefrancesco@gmail.com>
Date:   Tue, 9 Aug 2022 11:04:34 -0700
Cc:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        Jeff Layton <jlayton@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Muchun Song <songmuchun@bytedance.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ira Weiny <ira.weiny@intel.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <001D72B9-CB6A-4025-99CA-B3C02E1D5E0A@dubeyko.com>
References: <20220809152004.9223-1-fmdefrancesco@gmail.com>
 <20220809152004.9223-3-fmdefrancesco@gmail.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Precedence: bulk


> On Aug 9, 2022, at 8:20 AM, Fabio M. De Francesco =
<fmdefrancesco@gmail.com> wrote:
>=20
> kmap() is being deprecated in favor of kmap_local_page().
>=20
> Two main problems with kmap(): (1) It comes with an overhead as =
mapping
> space is restricted and protected by a global lock for synchronization =
and
> (2) it also requires global TLB invalidation when the kmap=E2=80=99s =
pool wraps
> and it might block when the mapping space is fully utilized until a =
slot
> becomes available.
>=20
> With kmap_local_page() the mappings are per thread, CPU local, can =
take
> page faults, and can be called from any context (including =
interrupts).
> It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> the tasks can be preempted and, when they are scheduled to run again, =
the
> kernel virtual addresses are restored and still valid.
>=20
> Since its use in bnode.c is safe everywhere, it should be preferred.
>=20
> Therefore, replace kmap() with kmap_local_page() in bnode.c. Where
> possible, use the suited standard helpers (memzero_page(), =
memcpy_page())
> instead of open coding kmap_local_page() plus memset() or memcpy().
>=20
> Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> HIGHMEM64GB enabled.
>=20
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.


> fs/hfs/bnode.c | 32 ++++++++++++--------------------
> 1 file changed, 12 insertions(+), 20 deletions(-)
>=20
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index c83fd0e8404d..2015e42e752a 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -21,7 +21,6 @@ void hfs_bnode_read(struct hfs_bnode *node, void =
*buf, int off, int len)
> 	int pagenum;
> 	int bytes_read;
> 	int bytes_to_read;
> -	void *vaddr;
>=20
> 	off +=3D node->page_offset;
> 	pagenum =3D off >> PAGE_SHIFT;
> @@ -33,9 +32,7 @@ void hfs_bnode_read(struct hfs_bnode *node, void =
*buf, int off, int len)
> 		page =3D node->page[pagenum];
> 		bytes_to_read =3D min_t(int, len - bytes_read, PAGE_SIZE =
- off);
>=20
> -		vaddr =3D kmap_atomic(page);
> -		memcpy(buf + bytes_read, vaddr + off, bytes_to_read);
> -		kunmap_atomic(vaddr);
> +		memcpy_from_page(buf + bytes_read, page, off, =
bytes_to_read);
>=20
> 		pagenum++;
> 		off =3D 0; /* page offset only applies to the first page =
*/
> @@ -80,8 +77,7 @@ void hfs_bnode_write(struct hfs_bnode *node, void =
*buf, int off, int len)
> 	off +=3D node->page_offset;
> 	page =3D node->page[0];
>=20
> -	memcpy(kmap(page) + off, buf, len);
> -	kunmap(page);
> +	memcpy_to_page(page, off, buf, len);
> 	set_page_dirty(page);
> }
>=20
> @@ -105,8 +101,7 @@ void hfs_bnode_clear(struct hfs_bnode *node, int =
off, int len)
> 	off +=3D node->page_offset;
> 	page =3D node->page[0];
>=20
> -	memset(kmap(page) + off, 0, len);
> -	kunmap(page);
> +	memzero_page(page, off, len);
> 	set_page_dirty(page);
> }
>=20
> @@ -123,9 +118,7 @@ void hfs_bnode_copy(struct hfs_bnode *dst_node, =
int dst,
> 	src_page =3D src_node->page[0];
> 	dst_page =3D dst_node->page[0];
>=20
> -	memcpy(kmap(dst_page) + dst, kmap(src_page) + src, len);
> -	kunmap(src_page);
> -	kunmap(dst_page);
> +	memcpy_page(dst_page, dst, src_page, src, len);
> 	set_page_dirty(dst_page);
> }
>=20
> @@ -140,9 +133,9 @@ void hfs_bnode_move(struct hfs_bnode *node, int =
dst, int src, int len)
> 	src +=3D node->page_offset;
> 	dst +=3D node->page_offset;
> 	page =3D node->page[0];
> -	ptr =3D kmap(page);
> +	ptr =3D kmap_local_page(page);
> 	memmove(ptr + dst, ptr + src, len);
> -	kunmap(page);
> +	kunmap_local(ptr);
> 	set_page_dirty(page);
> }
>=20
> @@ -346,13 +339,14 @@ struct hfs_bnode *hfs_bnode_find(struct =
hfs_btree *tree, u32 num)
> 	if (!test_bit(HFS_BNODE_NEW, &node->flags))
> 		return node;
>=20
> -	desc =3D (struct hfs_bnode_desc *)(kmap(node->page[0]) + =
node->page_offset);
> +	desc =3D (struct hfs_bnode_desc =
*)(kmap_local_page(node->page[0]) +
> +					 node->page_offset);
> 	node->prev =3D be32_to_cpu(desc->prev);
> 	node->next =3D be32_to_cpu(desc->next);
> 	node->num_recs =3D be16_to_cpu(desc->num_recs);
> 	node->type =3D desc->type;
> 	node->height =3D desc->height;
> -	kunmap(node->page[0]);
> +	kunmap_local(desc);
>=20
> 	switch (node->type) {
> 	case HFS_NODE_HEADER:
> @@ -436,14 +430,12 @@ struct hfs_bnode *hfs_bnode_create(struct =
hfs_btree *tree, u32 num)
> 	}
>=20
> 	pagep =3D node->page;
> -	memset(kmap(*pagep) + node->page_offset, 0,
> -	       min((int)PAGE_SIZE, (int)tree->node_size));
> +	memzero_page(*pagep, node->page_offset,
> +		     min((int)PAGE_SIZE, (int)tree->node_size));
> 	set_page_dirty(*pagep);
> -	kunmap(*pagep);
> 	for (i =3D 1; i < tree->pages_per_bnode; i++) {
> -		memset(kmap(*++pagep), 0, PAGE_SIZE);
> +		memzero_page(*++pagep, 0, PAGE_SIZE);
> 		set_page_dirty(*pagep);
> -		kunmap(*pagep);
> 	}
> 	clear_bit(HFS_BNODE_NEW, &node->flags);
> 	wake_up(&node->lock_wq);
> --=20
> 2.37.1
>=20