Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>]

On 11/18/2022 11:26 AM, Fabio M. De Francesco wrote:
> On venerdì 18 novembre 2022 18:47:52 CET Anirudh Venkataramanan wrote:
>> On 11/18/2022 12:23 AM, Fabio M. De Francesco wrote:
>>> On giovedì 17 novembre 2022 23:25:54 CET Anirudh Venkataramanan wrote:
>>>> kmap_atomic() is being deprecated in favor of kmap_local_page().
>>>> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
>>>> and kunmap_local() respectively.
>>>>
>>>> Note that kmap_atomic() disables preemption and page-fault processing,
> but
>>>> kmap_local_page() doesn't. Converting the former to the latter is safe
> only
>>>> if there isn't an implicit dependency on preemption and page-fault
> handling
>>>> being disabled, which does appear to be the case here.
>>>
>>> NIT: It is always possible to disable explicitly along with the
> conversion.
>>
>> Fair enough. I suppose "convert" is broader than "replace". How about this:
>>
>> "Replacing the former with the latter is safe only if there isn't an
>> implicit dependency on preemption and page-fault handling being
>> disabled, which does appear to be the case here."
>>
>> Ani
> 
> Let's start with 2/5 because it looks that here we are talking of a sensitive
> subject. Yesterday something triggered the necessity to make a patch for
> highmem.rst for clarifying that these conversions can _always_ be addressed.
> 
> I sent it to Ira and I'm waiting for his opinion before submitting it.
> 
> The me explain better... the point is that all kmap_atomic(), despite the
> differences, _can_ be converted to kmap_local_page().
> 
> What I care of is the safety of the conversions. I trust your commit message
> where you say that you inspected the code and that "there isn't an implicit
> dependency on preemption and page-fault handling being disabled".
> 
> I was talking about something very different: what if the code between mapping
> and unmapping was relying on implicit page-faults and/or preemption disable? I
> read between the lines that you consider a conversion of that kind something
> that cannot be addressed because "kmap_atomic() disables preemption and page-
> fault processing, but kmap_local_page() doesn't" (which is true).

No, I wasn't saying (or implying) this at all, nor did I think it 
could/would be interpreted this way.

I was trying to say that a straight-up replacing kmap_atomic() with 
kmap_local_page() would not be functionally safe if the code in between 
the mapping and unmapping relied on page-faults and/or preemption being 
disabled.

> 
> The point is that you have the possibility to convert also in this
> hypothetical case by doing something like the following.
> 
> Old code:
> 
> ptr = kmap_atomic(page);
> do something with ptr;
> kunmap_atomic(ptr);
> 
> You checked the code and understood that that "something" can only be carried
> out with page-faults disabled (just an example). Conversion:
> 
> pagefault_disable();
> ptr = kmap_local_page(page);
> do something with ptr;
> kunmap_local(ptr);
> pagefault_enable();
> 
> I'm not asking to reword your commit message only for the purpose to clear
> that your changes are "safe" because you checked the code and can reasonably
> affirm that the conversion doesn't depend on further disables.
> 
> I just said it to make you notice that every kmap_atomic() conversion to
> kmap_local_page() is "safe", but only if you really understand the code and
> act accordingly.
> 
> I'm too wordy, Ira said it so many times. Unfortunately, I'm not able to
> optimize English text and need to improve. I'm sorry.
> 
> Does my long explanation make any sense to you?
> 
> If so, I'm happy. I'm not asking to send v2. I just desired that you realize
> (1) how tricky these conversions may be and therefore how much important is
> not to do them mechanically (2) how to better craft your next commit message
> (if you want to keep on helping with these conversions).

Appreciate the explanation, but as I explained above, what you read 
between the lines isn't what I was saying. I am most certainly not 
making these changes mechanically.

> 
> I'm OK with this patch. Did you see my tag? :-)

I did, and thanks!

FYI, I will be sending a v2 for the series anyway (with the 
memcpy_from_page()) changes, and so I am planning to update the commit 
messages to hopefully be a little clearer to you.

Ani