Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
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 > > Subject: Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with > > kmap_local_page() > > > > Zhao Liu wrote: > > > From: Zhao Liu > > > > > > 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
Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
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 > Subject: Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with > kmap_local_page() > > Zhao Liu wrote: > > From: Zhao Liu > > > > 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/20220813220034.806698-1-ira.we...@intel.com > > --- > > 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 > > > >
Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
Zhao Liu wrote: > From: Zhao Liu > > 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/20220813220034.806698-1-ira.we...@intel.com > --- > 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 >
Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
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" > 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 > > > > 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
Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
On lunedì 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote: > From: Zhao Liu > > 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.we...@intel.com > --- > 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
