Message-ID: <79e1f37f-3ffa-0195-860b-08cc890d810e@shipmail.org>
Date:   Wed, 21 Jun 2023 11:06:51 +0200
From:   Thomas Hellström (Intel) 
        <thomas_os@...pmail.org>
To:     Sumitra Sharma <sumitraartsy@...il.com>,
        Ira Weiny <ira.weiny@...el.com>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, Fabio <fmdefrancesco@...il.com>,
        Deepak R Varma <drv@...lo.com>
Subject: 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 <ira.weiny@...el.com>
>>>>>
>>>> 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@...il.com>
>>>>> ---
>>>>>
>>>>> 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@sumitra.com/
>>>>
>>>> All that said the code looks good to me.  So with the above changes.
>>>>
>>>> Reviewed-by: Ira Weiny <ira.weiny@...el.com>
>>>>
>>> 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@linux.intel.com/
>>
>> <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
>>>>>
>>>>
>>

Powered by blists - more mailing lists