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 3/4] hfsplus: Convert kmap() to kmap_local_page() in
 bitmap.c
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <20220809203105.26183-4-fmdefrancesco@gmail.com>
Date:   Tue, 9 Aug 2022 15:18:05 -0700
Cc:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Ira Weiny <ira.weiny@intel.com>, Jens Axboe <axboe@kernel.dk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Bart Van Assche <bvanassche@acm.org>,
        Kees Cook <keescook@chromium.org>,
        Muchun Song <songmuchun@bytedance.com>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <55002DA9-757F-47C0-B1C7-F808002E6B34@dubeyko.com>
References: <20220809203105.26183-1-fmdefrancesco@gmail.com>
 <20220809203105.26183-4-fmdefrancesco@gmail.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Precedence: bulk


> On Aug 9, 2022, at 1:31 PM, Fabio M. De Francesco =
<fmdefrancesco@gmail.com> wrote:
>=20
> kmap() is being deprecated in favor of kmap_local_page().
>=20
> There are 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 are still valid.
>=20
> Since its use in bitmap.c is safe everywhere, it should be preferred.
>=20
> Therefore, replace kmap() with kmap_local_page() in bitmap.c.
>=20
> Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> HIGHMEM64GB enabled.
>=20
> Cc: Viacheslav Dubeyko <slava@dubeyko.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-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/hfsplus/bitmap.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>=20
> diff --git a/fs/hfsplus/bitmap.c b/fs/hfsplus/bitmap.c
> index cebce0cfe340..bd8dcea85588 100644
> --- a/fs/hfsplus/bitmap.c
> +++ b/fs/hfsplus/bitmap.c
> @@ -39,7 +39,7 @@ int hfsplus_block_allocate(struct super_block *sb, =
u32 size,
> 		start =3D size;
> 		goto out;
> 	}
> -	pptr =3D kmap(page);
> +	pptr =3D kmap_local_page(page);
> 	curr =3D pptr + (offset & (PAGE_CACHE_BITS - 1)) / 32;
> 	i =3D offset % 32;
> 	offset &=3D ~(PAGE_CACHE_BITS - 1);
> @@ -74,7 +74,7 @@ int hfsplus_block_allocate(struct super_block *sb, =
u32 size,
> 			}
> 			curr++;
> 		}
> -		kunmap(page);
> +		kunmap_local(pptr);
> 		offset +=3D PAGE_CACHE_BITS;
> 		if (offset >=3D size)
> 			break;
> @@ -84,7 +84,7 @@ int hfsplus_block_allocate(struct super_block *sb, =
u32 size,
> 			start =3D size;
> 			goto out;
> 		}
> -		curr =3D pptr =3D kmap(page);
> +		curr =3D pptr =3D kmap_local_page(page);
> 		if ((size ^ offset) / PAGE_CACHE_BITS)
> 			end =3D pptr + PAGE_CACHE_BITS / 32;
> 		else
> @@ -127,7 +127,7 @@ int hfsplus_block_allocate(struct super_block *sb, =
u32 size,
> 			len -=3D 32;
> 		}
> 		set_page_dirty(page);
> -		kunmap(page);
> +		kunmap_local(pptr);
> 		offset +=3D PAGE_CACHE_BITS;
> 		page =3D read_mapping_page(mapping, offset / =
PAGE_CACHE_BITS,
> 					 NULL);
> @@ -135,7 +135,7 @@ int hfsplus_block_allocate(struct super_block *sb, =
u32 size,
> 			start =3D size;
> 			goto out;
> 		}
> -		pptr =3D kmap(page);
> +		pptr =3D kmap_local_page(page);
> 		curr =3D pptr;
> 		end =3D pptr + PAGE_CACHE_BITS / 32;
> 	}
> @@ -151,7 +151,7 @@ int hfsplus_block_allocate(struct super_block *sb, =
u32 size,
> done:
> 	*curr =3D cpu_to_be32(n);
> 	set_page_dirty(page);
> -	kunmap(page);
> +	kunmap_local(pptr);
> 	*max =3D offset + (curr - pptr) * 32 + i - start;
> 	sbi->free_blocks -=3D *max;
> 	hfsplus_mark_mdb_dirty(sb);
> @@ -185,7 +185,7 @@ int hfsplus_block_free(struct super_block *sb, u32 =
offset, u32 count)
> 	page =3D read_mapping_page(mapping, pnr, NULL);
> 	if (IS_ERR(page))
> 		goto kaboom;
> -	pptr =3D kmap(page);
> +	pptr =3D kmap_local_page(page);
> 	curr =3D pptr + (offset & (PAGE_CACHE_BITS - 1)) / 32;
> 	end =3D pptr + PAGE_CACHE_BITS / 32;
> 	len =3D count;
> @@ -215,11 +215,11 @@ int hfsplus_block_free(struct super_block *sb, =
u32 offset, u32 count)
> 		if (!count)
> 			break;
> 		set_page_dirty(page);
> -		kunmap(page);
> +		kunmap_local(pptr);
> 		page =3D read_mapping_page(mapping, ++pnr, NULL);
> 		if (IS_ERR(page))
> 			goto kaboom;
> -		pptr =3D kmap(page);
> +		pptr =3D kmap_local_page(page);
> 		curr =3D pptr;
> 		end =3D pptr + PAGE_CACHE_BITS / 32;
> 	}
> @@ -231,7 +231,7 @@ int hfsplus_block_free(struct super_block *sb, u32 =
offset, u32 count)
> 	}
> out:
> 	set_page_dirty(page);
> -	kunmap(page);
> +	kunmap_local(pptr);
> 	sbi->free_blocks +=3D len;
> 	hfsplus_mark_mdb_dirty(sb);
> 	mutex_unlock(&sbi->alloc_mutex);
> --=20
> 2.37.1
>=20