On 6/20/23 20:07, Sumitra Sharma wrote:
On Tue, Jun 20, 2023 at 06:23:38AM -0700, Ira Weiny wrote:
Sumitra Sharma wrote:
On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote:
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
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>
NIT: No need for the line break between Suggested-by and your signed off line.
Hi Ira,
What does NIT stand for?
Shorthand for 'nitpicking'.
"giving too much attention to details that are not important, especially
as a way of criticizing: "
- https://dictionary.cambridge.org/dictionary/english/nitpicking
Via email this is a way for authors of an email to indicate something is
technically wrong but while nicely acknowledging that it is not very
significant and could be seen as overly critical.
For this particular comment I'm showing something to pay attention to next
time but that was not a big deal this time around.
Hi Ira,
Thank for your explanation on NIT.
Thank you. I will take care about the line breaks.
Signed-off-by: Sumitra Sharma <sumitraartsy@xxxxxxxxx>
---
Changes in v2:
- Replace kmap() with kmap_local_page().
Generally it is customary to attribute a change like this to those who
suggested it in a V1 review.
For example:
- Tvrtko/Thomas: Use kmap_local_page() instead of page_address()
Also I don't see Thomas on the new email list. Since he took the time to
review V1 he might want to check this version out. I've added him to the
'To:' list.
Also a link to V1 is nice. B4 formats it like this:
- Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@xxxxxxxxxxx/
All that said the code looks good to me. So with the above changes.
Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
I have noted down the points mentioned above. Thank you again.
I am not supposed to create another version of this patch for
adding the above mentions, as you and Thomas both gave this patch
a reviewed-by tag. Right?
Based on this response[*] from Tvrtko I think this version can move
through without a v3.
Okay!
Thanks & regards
Sumitra
I think one thing worth mentioning in the context of this patch is that
IIRC kmap_local_page() will block offlining of the mapping CPU until
kunmap_local(), so while I haven't seen any guidelines around the usage
of this api for long-held mappings, I figure it's wise to keep the
mapping duration short, or at least avoid sleeping with a
kmap_local_page() map active.
I figured, while page compression is probably to be considered "slow"
it's probably not slow enough to motivate kmap() instead of
kmap_local_page(), but if anyone feels differently, perhaps it should be
considered.
With that said, my Reviewed-by: still stands.
/Thomas
Thanks!
Ira
[*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7b38@xxxxxxxxxxxxxxx/
<quote>
Thanks all! I'll just re-send the patch for our CI, since it didn't get
picked up automatically (stuck in moderation perhaps), with all r-b tags
added and extra line space removed and merge it if results will be green.
Regards,
Tvrtko
</quote>
Thanks & regards
Sumitra
PS: I am new to the open source vocabulary terms.
- Change commit subject and message.
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
