Message-ID: <2153769.NgBsaNRSFp@suse>
Date:   Fri, 18 Nov 2022 20:26:13 +0100
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     netdev@...r.kernel.org,
        Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>
Cc:     Ira Weiny <ira.weiny@...el.com>,
        Edward Cree <ecree.xilinx@...il.com>,
        Martin Habets <habetsm.xilinx@...il.com>
Subject: Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

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).

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).

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

Thanks for helping,

Fabio  

Powered by blists - more mailing lists