Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On 20/06/2023 14:23, 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 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. Thank you. I will take care about the line breaks. Signed-off-by: Sumitra Sharma --- 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.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny 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. Thanks! Ira [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7...@linux.intel.com/ 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 Pushed to drm-intel-gt-next, thanks for the patch and reviews! Regards, Tvrtko
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 > > Signed-off-by: Sumitra Sharma > --- > > 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 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(, 1); > > - s = kmap(page); > + s = kmap_local_page(page); > ret = compress_page(compress, s, dst, false); > - kunmap(page); > + kunmap_local(s); > > drm_clflush_pages(, 1); > > -- > 2.25.1
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On 21/06/2023 19:51, Thomas Hellström (Intel) wrote: On 6/21/23 18:35, Ira Weiny wrote: Thomas Hellström (Intel) wrote: 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. What you say is all true. But remember the mappings are only actually created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also they must suffer such performance issues because there is just no other way around supporting them. Also Sumitra, and our kmap conversion project in general, is focusing on not using kmap* if at all possible. Thus the reason V1 tried to use page_address(). Could we guarantee the i915 driver is excluded from all HIGHMEM systems? The i915 maintainers might want to chime in here, but I would say no, we can't, although we don't care much about optimizing for them. Same for the new xe driver. AFAIK i915 works on such systems so I don't think we can drop support just like that. Not sure what the process would be. Perhaps as part of a wider kernel deprecation would make most sense. Regards, Tvrtko
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On 6/21/23 18:35, Ira Weiny wrote: Thomas Hellström (Intel) wrote: 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. What you say is all true. But remember the mappings are only actually created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also they must suffer such performance issues because there is just no other way around supporting them. Also Sumitra, and our kmap conversion project in general, is focusing on not using kmap* if at all possible. Thus the reason V1 tried to use page_address(). Could we guarantee the i915 driver is excluded from all HIGHMEM systems? The i915 maintainers might want to chime in here, but I would say no, we can't, although we don't care much about optimizing for them. Same for the new xe driver. Thanks, /Thomas With that said, my Reviewed-by: still stands. Thanks! Ira
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Thomas Hellström (Intel) wrote: > > 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. What you say is all true. But remember the mappings are only actually created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also they must suffer such performance issues because there is just no other way around supporting them. Also Sumitra, and our kmap conversion project in general, is focusing on not using kmap* if at all possible. Thus the reason V1 tried to use page_address(). Could we guarantee the i915 driver is excluded from all HIGHMEM systems? > > With that said, my Reviewed-by: still stands. Thanks! Ira
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On mercoledì 21 giugno 2023 11:06:51 CEST Thomas Hellström (Intel) wrote: > > 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(), Migration is disabled. > so while I haven't seen any guidelines around the usage > of this api for long-held mappings, It would be advisable to not use it for long term mappings, if possible. These "local" mappings should better be help for not too long duration. > I figure it's wise to keep the > mapping duration short, or at least avoid sleeping with a > kmap_local_page() map active. Nothing prevents a call of preempt_disable() around the section of code between kmap_local_page() / kunmap_local(). If it is needed, local mappings could also be acquired under spinlocks and/or with disabled interrupts. I don't know the code, however, everything cited above could be the subject of a subsequent patch. Regards, Fabio > 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 >
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 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 --- 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.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny 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-7ccfdd2f7...@linux.intel.com/ 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 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(, 1); - s = kmap(page); + s = kmap_local_page(page); ret = compress_page(compress, s, dst, false); - kunmap(page); + kunmap_local(s); drm_clflush_pages(, 1); -- 2.25.1
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 > > > > > > > > > > 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 > > > > --- > > > > > > > > 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.ga381...@sumitra.com/ > > > > > > All that said the code looks good to me. So with the above changes. > > > > > > Reviewed-by: Ira Weiny > > > > > > > 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 > > Thanks! > Ira > > [*] > https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7...@linux.intel.com/ > > > 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 > > > > > > > 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(, 1); > > > > > > > > - s = kmap(page); > > > > + s = kmap_local_page(page); > > > > ret = compress_page(compress, s, dst, false); > > > > - kunmap(page); > > > > + kunmap_local(s); > > > > > > > > drm_clflush_pages(, 1); > > > > > > > > -- > > > > 2.25.1 > > > > > > > > > > > >
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 > > > > > > > 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. > > Thank you. I will take care about the line breaks. > > > > Signed-off-by: Sumitra Sharma > > > --- > > > > > > 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.ga381...@sumitra.com/ > > > > All that said the code looks good to me. So with the above changes. > > > > Reviewed-by: Ira Weiny > > > > 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. Thanks! Ira [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7...@linux.intel.com/ 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 > > 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(, 1); > > > > > > - s = kmap(page); > > > + s = kmap_local_page(page); > > > ret = compress_page(compress, s, dst, false); > > > - kunmap(page); > > > + kunmap_local(s); > > > > > > drm_clflush_pages(, 1); > > > > > > -- > > > 2.25.1 > > > > > > >
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 > > > > NIT: No need for the line break between Suggested-by and your signed off line. > Hi Ira, What does NIT stand for? Thank you. I will take care about the line breaks. > > Signed-off-by: Sumitra Sharma > > --- > > > > 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.ga381...@sumitra.com/ > > All that said the code looks good to me. So with the above changes. > > Reviewed-by: Ira Weiny > 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? 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(, 1); > > > > - s = kmap(page); > > + s = kmap_local_page(page); > > ret = compress_page(compress, s, dst, false); > > - kunmap(page); > > + kunmap_local(s); > > > > drm_clflush_pages(, 1); > > > > -- > > 2.25.1 > > > >
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On 19/06/2023 08:59, Thomas Hellström (Intel) wrote: On 6/18/23 20:11, 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 NIT: No need for the line break between Suggested-by and your signed off line. Signed-off-by: Sumitra Sharma --- 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. Thanks. Also a link to V1 is nice. B4 formats it like this: - Link to v1: https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny LGTM. Reviewed-by: Thomas Hellström 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
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On 6/18/23 20:11, 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 NIT: No need for the line break between Suggested-by and your signed off line. Signed-off-by: Sumitra Sharma --- 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. Thanks. Also a link to V1 is nice. B4 formats it like this: - Link to v1: https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny LGTM. Reviewed-by: Thomas Hellström - 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(, 1); - s = kmap(page); + s = kmap_local_page(page); ret = compress_page(compress, s, dst, false); - kunmap(page); + kunmap_local(s); drm_clflush_pages(, 1); -- 2.25.1
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 > NIT: No need for the line break between Suggested-by and your signed off line. > Signed-off-by: Sumitra Sharma > --- > > 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.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny > - 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(, 1); > > - s = kmap(page); > + s = kmap_local_page(page); > ret = compress_page(compress, s, dst, false); > - kunmap(page); > + kunmap_local(s); > > drm_clflush_pages(, 1); > > -- > 2.25.1 >
[PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
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 Signed-off-by: Sumitra Sharma --- Changes in v2: - Replace kmap() with kmap_local_page(). - 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(, 1); - s = kmap(page); + s = kmap_local_page(page); ret = compress_page(compress, s, dst, false); - kunmap(page); + kunmap_local(s); drm_clflush_pages(, 1); -- 2.25.1
[PATCH V2] drm/i915: Replace kmap() with kmap_local_page()
From: Ira Weiny
kmap() is being deprecated and these usages are all local to the thread
so there is no reason kmap_local_page() can't be used.
Replace kmap() calls with kmap_local_page().
Signed-off-by: Ira Weiny
---
NOTE: I'm sending as a follow on to the V1 patch. Please let me know if you
prefer the entire series instead.
Changes for V2:
From Christoph Helwig
Prefer the use of memcpy_*_page() where appropriate.
---
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 6 ++
drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8
drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++--
drivers/gpu/drm/i915/gt/shmem_utils.c | 7 ++-
drivers/gpu/drm/i915/i915_gem.c| 8
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
6 files changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index d77da59fae04..842e0895 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -589,7 +589,7 @@ i915_gem_object_create_shmem_from_data(struct
drm_i915_private *dev_priv,
do {
unsigned int len = min_t(typeof(size), size, PAGE_SIZE);
struct page *page;
- void *pgdata, *vaddr;
+ void *pgdata;
err = pagecache_write_begin(file, file->f_mapping,
offset, len, 0,
@@ -597,9 +597,7 @@ i915_gem_object_create_shmem_from_data(struct
drm_i915_private *dev_priv,
if (err < 0)
goto fail;
- vaddr = kmap(page);
- memcpy(vaddr, data, len);
- kunmap(page);
+ memcpy_to_page(page, 0, data, len);
err = pagecache_write_end(file, file->f_mapping,
offset, len, len,
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 6d30cdfa80f3..e59e1725e29d 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -144,7 +144,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
intel_gt_flush_ggtt_writes(_i915(obj->base.dev)->gt);
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu,
row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to
page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -162,7 +162,7 @@ static int check_partial_mapping(struct drm_i915_gem_object
*obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
out:
__i915_vma_put(vma);
@@ -237,7 +237,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
intel_gt_flush_ggtt_writes(_i915(obj->base.dev)->gt);
p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
- cpu = kmap(p) + offset_in_page(offset);
+ cpu = kmap_local_page(p) + offset_in_page(offset);
drm_clflush_virt_range(cpu, sizeof(*cpu));
if (*cpu != (u32)page) {
pr_err("Partial view for %lu [%u] (offset=%llu, size=%u
[%llu, row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected
write to page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
@@ -255,7 +255,7 @@ static int check_partial_mappings(struct
drm_i915_gem_object *obj,
}
*cpu = 0;
drm_clflush_virt_range(cpu, sizeof(*cpu));
- kunmap(p);
+ kunmap_local(cpu);
if (err)
return err;
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
index f8948de72036..743a414f86f3 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
@@ -743,7 +743,7 @@ static void swizzle_page(struct page *page)
char *vaddr;
int i;
- vaddr = kmap(page);
+ vaddr = kmap_local_page(page);
for (i = 0; i < PAGE_SIZE; i += 128) {
memcpy(temp, [i], 64);
@@ -751,7 +751,7 @@ static void swizzle_page(struct page *page)
memcpy([i + 64], temp, 64);
}
- kunmap(page);
+ kunmap_local(vaddr);
}
/**
diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c
b/drivers/gpu/drm/i915/gt/shmem_utils.c
index 0683b27a3890..d47f262d2f07 100644
--- a/drivers/gpu/drm/i915/gt/shmem_utils.c
+++
