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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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:
> > 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
> > the driver doesn't know if the page is in normal memory. This is the 
reason
> > 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 <ira.weiny@intel.com>
> > Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> > ---
> > 
> >  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,
> 
> > 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),
> 
> >  					     blen, cookies + nc,
> 
> ncookies - nc,
> 
> >  					     map_perm);
> > 
> > -			kunmap_atomic(vaddr);
> > +			kunmap_local(vaddr);
> > 
> >  		} else {
> >  		
> >  			err = -EMSGSIZE;
> >  		
> >  		}
> > 
> > --
> > 2.37.2
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> 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