Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()

[Fabio M. De Francesco]

On sabato 17 giugno 2023 20:04:20 CEST Sumitra Sharma wrote:
> kmap() has been deprecated in favor of the kmap_local_page()
> due to high cost, restricted mapping space, the overhead of a
> global lock for synchronization, and making the process sleep
> in the absence of free slots.
> 
> kmap_local_page() is faster than kmap() and offers thread-local
> and CPU-local mappings, take pagefaults in a local kmap region

NIT: _can_ take pagefaults in a local kmap region

> and preserves preemption by saving the mappings of outgoing tasks
> and restoring those of the incoming one during a context switch.
> 
> The mapping is kept thread local in the function
> “i915_vma_coredump_create” in i915_gpu_error.c
> 
> Therefore, replace kmap() with kmap_local_page().
> 
> Suggested-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> Signed-off-by: Sumitra Sharma <sumitraartsy@xxxxxxxxx>
> ---
> 
> Changes in v2:
> 	- Replace kmap() with kmap_local_page().
> 	- Change commit subject and message.

With the changes that Ira suggested and the minor fix I'm proposing to the 
commit message, it looks good to me too, so this patch is... 

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>

However, as far as I'm concerned, our nits don't necessarily require any newer 
version, especially because Tvrtko has already sent this patch for their CI.

Thanks,

Fabio

P.S.: As Sumitra says both kmap() and kmap_local_page() allows preemption in 
non atomic context. 

Furthermore, Tvrtko confirmed that the pages can come from HIGHMEM, therefore 
kmap_local_page for local temporary mapping is unavoidable.

Last thing... Thomas thinks he wants to make it run atomically (if I 
understood one of his messages correctly). As I already responded, nothing 
prevents someone does another patch just to disable preemption (or to enter 
atomic context by other means) around the code marked by kmap_local_page() / 
kunmap_local() because these functions work perfectly _also_ in atomic context 
(including interrupts). But this is not something that Sumitra should be 
worried about.

> 
>  drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c index f020c0086fbc..bc41500eedf5
> 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
> 
>  			drm_clflush_pages(&page, 1);
> 
> -			s = kmap(page);
> +			s = kmap_local_page(page);
>  			ret = compress_page(compress, s, dst, false);
> -			kunmap(page);
> +			kunmap_local(s);
> 
>  			drm_clflush_pages(&page, 1);
> 
> --
> 2.25.1