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 <[email protected]>
Signed-off-by: Sumitra Sharma <[email protected]>
---
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(&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
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 <[email protected]>
>
NIT: No need for the line break between Suggested-by and your signed off line.
> Signed-off-by: Sumitra Sharma <[email protected]>
> ---
>
> 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/[email protected]/
All that said the code looks good to me. So with the above changes.
Reviewed-by: Ira Weiny <[email protected]>
> - 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
>
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 <[email protected]>
>>
> NIT: No need for the line break between Suggested-by and your signed off line.
>
>> Signed-off-by: Sumitra Sharma <[email protected]>
>> ---
>>
>> 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/[email protected]/
>
> All that said the code looks good to me. So with the above changes.
>
> Reviewed-by: Ira Weiny <[email protected]>
LGTM. Reviewed-by: Thomas Hellström <[email protected]>
>
>> - 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
>>
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 <[email protected]>
>>>
>> NIT: No need for the line break between Suggested-by and your signed
>> off line.
>>
>>> Signed-off-by: Sumitra Sharma <[email protected]>
>>> ---
>>>
>>> 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/[email protected]/
>>
>> All that said the code looks good to me. So with the above changes.
>>
>> Reviewed-by: Ira Weiny <[email protected]>
>
> LGTM. Reviewed-by: Thomas Hellström <[email protected]>
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
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 <[email protected]>
> >
>
> 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 <[email protected]>
> > ---
> >
> > 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/[email protected]/
>
> All that said the code looks good to me. So with the above changes.
>
> Reviewed-by: Ira Weiny <[email protected]>
>
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(&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
> >
>
>
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 <[email protected]>
> > >
> >
> > 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 <[email protected]>
> > > ---
> > >
> > > 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/[email protected]/
> >
> > All that said the code looks good to me. So with the above changes.
> >
> > Reviewed-by: Ira Weiny <[email protected]>
> >
>
> 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/[email protected]/
<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
> > >
> >
> >
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 <[email protected]>
> > > >
> > >
> > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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/[email protected]/
> > >
> > > All that said the code looks good to me. So with the above changes.
> > >
> > > Reviewed-by: Ira Weiny <[email protected]>
> > >
> >
> > 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/[email protected]/
>
> <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
> > > >
> > >
> > >
>
>
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 <[email protected]>
>>>>>
>>>> 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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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/[email protected]/
>>>>
>>>> All that said the code looks good to me. So with the above changes.
>>>>
>>>> Reviewed-by: Ira Weiny <[email protected]>
>>>>
>>> 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/[email protected]/
>>
>> <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
>>>>>
>>>>
>>
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
>
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
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
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
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 <[email protected]>
>
> Signed-off-by: Sumitra Sharma <[email protected]>
> ---
>
> 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 <[email protected]>
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
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 <[email protected]>
>>>>
>>>
>>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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/[email protected]/
>>>
>>> All that said the code looks good to me. So with the above changes.
>>>
>>> Reviewed-by: Ira Weiny <[email protected]>
>>>
>>
>> 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/[email protected]/
>
> <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>
Pushed to drm-intel-gt-next, thanks for the patch and reviews!
Regards,
Tvrtko