From: Zhao Liu <[email protected]>
The use of kmap_atomic() is being deprecated in favor of
kmap_local_page()[1].
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.
With kmap_local_page(), the mapping is per thread, CPU local and not
globally visible.
[1]: https://lore.kernel.org/all/[email protected]
---
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(-)
--
2.34.1
On luned? 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote:
> From: Zhao Liu <[email protected]>
>
> 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/[email protected]
> ---
> 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
On Sat, Oct 29, 2022 at 09:12:27AM +0200, Fabio M. De Francesco wrote:
> Date: Sat, 29 Oct 2022 09:12:27 +0200
> From: "Fabio M. De Francesco" <[email protected]>
> Subject: Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with
> kmap_local_page()
Hi Fabio, thanks for your review!! (I'm sorry I missed the previous mails).
>
> On luned? 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote:
> > From: Zhao Liu <[email protected]>
> >
> > 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.
Thanks for your reminder, I'll emphasize the motivation here.
> What about avoiding the often unwanted side effect of unnecessary page faults
> disables?
Good suggestion! I'll add this into this cover message.
What I think is that we have two reasons to do the replacement work:
1. (main motication) Avoid unnessary pagefaulta and preemption disabling to gain
performance benefits.
2. We are trying to deprecate the old kmap/kmap_atomic interface. Some maintainer
said it's also a good reason especially for the case that the performance is not
critical [1].
In addition, also from [1], I find in some case people chooses kmap_atomic() for
the consideration that they want the atomic context. So, the explaination about
why the atomic context is not needed is also a reasion? I understand that I need
to make special explaination in each commit depending on the situation (In this
case, it is not suitable to describe in the cover?).
[1]: https://lore.kernel.org/lkml/YzRVaJA0EyfcVisW@liuwe-devbox-debian-v2/#t
>
> >
> > 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().
What about the below description which refers to your doc?
"kmap_atomic() in the kernel creates a non-preemptible section
and disable pagefaults. This could be a source of unwanted latency.
And kmap_local_page effectively overcomes this issue because it doesn't
disable pagefault and preemption."
Thanks,
Zhao
Zhao Liu wrote:
> From: Zhao Liu <[email protected]>
>
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].
Zhao,
Was there ever a v2 of this series? I'm not finding it on Lore.
Thanks,
Ira
>
> 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.
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.
>
> [1]: https://lore.kernel.org/all/[email protected]
> ---
> 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(-)
>
> --
> 2.34.1
>
On Tue, Feb 14, 2023 at 08:25:08PM -0800, Ira Weiny wrote:
> Date: Tue, 14 Feb 2023 20:25:08 -0800
> From: Ira Weiny <[email protected]>
> Subject: Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with
> kmap_local_page()
>
> Zhao Liu wrote:
> > From: Zhao Liu <[email protected]>
> >
> > The use of kmap_atomic() is being deprecated in favor of
> > kmap_local_page()[1].
>
> Zhao,
>
> Was there ever a v2 of this series? I'm not finding it on Lore.
Sorry Ira, my delay is too long, I was busy with other patch work,
I will refresh v2 soon, and push this forward!
Best Regards,
Zhao
>
> Thanks,
> Ira
>
> >
> > 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.
> >
> > With kmap_local_page(), the mapping is per thread, CPU local and not
> > globally visible.
> >
> > [1]: https://lore.kernel.org/all/[email protected]
> > ---
> > 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(-)
> >
> > --
> > 2.34.1
> >
>
>
Zhao Liu wrote:
> On Tue, Feb 14, 2023 at 08:25:08PM -0800, Ira Weiny wrote:
> > Date: Tue, 14 Feb 2023 20:25:08 -0800
> > From: Ira Weiny <[email protected]>
> > Subject: Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with
> > kmap_local_page()
> >
> > Zhao Liu wrote:
> > > From: Zhao Liu <[email protected]>
> > >
> > > The use of kmap_atomic() is being deprecated in favor of
> > > kmap_local_page()[1].
> >
> > Zhao,
> >
> > Was there ever a v2 of this series? I'm not finding it on Lore.
>
> Sorry Ira, my delay is too long, I was busy with other patch work,
> I will refresh v2 soon, and push this forward!
Awesome! Thanks!
Ira
>
> Best Regards,
> Zhao