Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()

From: Fabio M. De Francesco
Date: Sat Oct 29 2022 - 03:12:49 EST


On lunedì 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@xxxxxxxxx>
>
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].

Some words to explain why kmap_atomic was deprecated won't hurt. Many
maintainers and reviewers, and also casual readers might not yet be aware of
the reasons behind that deprecation.

> In the following patches, we can convert the calls of kmap_atomic() /
> kunmap_atomic() to kmap_local_page() / kunmap_local(), which can
> instead do the mapping / unmapping regardless of the context.

Readers are probably much more interested in what you did in the following
patches and why you did it, instead of being informed about what "we can" do.

I would suggest something like "The following patches convert the calls to
kmap_atomic() to kmap_local_page() [the rest looks OK]".

This could also be the place to say something about why we prefer
kmap_local_page() to kmap_atomic().

Are you sure that the reasons that motivates your conversions are merely
summarized to kmap_local_page() being able to do mappings regardless of
context? I think you are missing the real reasons why.

What about avoiding the often unwanted side effect of unnecessary page faults
disables?

>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.

No news here. kmap_atomic() is "per thread, CPU local and not glocally
visible". I cannot see any difference here between kmap_atomic() and
kmap_local_page().

>
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@xxxxxxxxx
> ---
> Zhao Liu (9):
> drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
> drm/i915: Use kmap_local_page() in gem/i915_gem_pyhs.c
> drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c
> drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c
> drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c
> drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c
> drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c
> drm/i915: Use kmap_local_page() in i915_cmd_parser.c
> drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c
>
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++-----
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 8 ++++----
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++++--
> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 6 +++---
> .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c | 12 ++++--------
> .../gpu/drm/i915/gem/selftests/i915_gem_context.c | 8 ++++----
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 +----
> drivers/gpu/drm/i915/i915_cmd_parser.c | 4 ++--
> 9 files changed, 30 insertions(+), 37 deletions(-)

Thanks for helping with kmap_atomic() conversions to kmap_local_page().

Fabio