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

On venerdì 18 novembre 2022 10:11:12 CET Fabio M. De Francesco wrote:

On giovedì 17 novembre 2022 23:25:57 CET Anirudh Venkataramanan wrote:

quoted

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.

Also note that the page being mapped is not allocated by the driver, and 

so

quoted

the driver doesn't know if the page is in normal memory. This is the 

reason

quoted

kmap_local_page() is used as opposed to page_address().

I don't have hardware, so this change has only been compile tested.

Cc: Ira Weiny <redacted>
Cc: Fabio M. De Francesco <redacted>
Signed-off-by: Anirudh Venkataramanan <redacted>
---

 drivers/net/ethernet/sun/sunvnet_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet_common.c

b/drivers/net/ethernet/sun/sunvnet_common.c index 80fde5f..a6211b9 100644

--- a/drivers/net/ethernet/sun/sunvnet_common.c
+++ b/drivers/net/ethernet/sun/sunvnet_common.c

@@ -1085,13 +1085,13 @@ static inline int vnet_skb_map(struct ldc_channel

*lp,

quoted

struct sk_buff *skb, u8 *vaddr;

 		if (nc < ncookies) {

-			vaddr = kmap_atomic(skb_frag_page(f));
+			vaddr = kmap_local_page(skb_frag_page(f));

 			blen = skb_frag_size(f);
 			blen += 8 - (blen & 7);
 			err = ldc_map_single(lp, vaddr +

skb_frag_off(f),

quoted

 					     blen, cookies + nc,

ncookies - nc,

quoted

 					     map_perm);

-			kunmap_atomic(vaddr);
+			kunmap_local(vaddr);

 		} else {
 		
 			err = -EMSGSIZE;
 		
 		}

--
2.37.2

Reviewed-by: Fabio M. De Francesco <redacted>

Thanks,

Fabio

Now that we are at 5/5 again. I'd like to point again to what worries me:

"Converting the former to the latter is safe only if there isn't an implicit 
dependency on preemption and page-fault handling being disabled, ...".

If I was able to convey my thoughts this is what you should get from my long 
email: 

"Converting the former to the latter is _always_ safe if there isn't an 
implicit dependency on preemption and page-fault handling being disabled and 
also when the above-mentioned implicit dependency is present, but in the 
latter case only if calling pagefault_disable() and/or preempt_disable() with 
kmap_local_page(). These disables are not required here because...".

As you demonstrated none of your nine patches need any explicit disable along 
with kmap_local_page().

Do my two emails make any sense to you?
However, your patches are good. If you decide to make them perfect use those 
helpers we've been talking about.

Again thanks,

Fabio


Fabio