| Message ID | 20221117222557.2196195-6-anirudh.venkataramanan@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Remove uses of kmap_atomic() | expand |
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
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
On 11/18/2022 12:45 PM, Fabio M. De Francesco wrote: > On venerdì 18 novembre 2022 10:11:12 CET Fabio M. De Francesco wrote: > > 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. Hi Fabio, I'll be posting v2 next week. I've incorporated feedback from our discussion, and hopefully the things I've added make sense. If not, let's continue talking. Ani
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; }
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(-)