| Message ID | 20221117222557.2196195-3-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: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. However, you are noticing that we don't need to do it. I'm noticing the same wording in other of your patches, but there are no problems with them. > > 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> > Cc: Edward Cree <ecree.xilinx@gmail.com> > Cc: Martin Habets <habetsm.xilinx@gmail.com> > Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> > --- > drivers/net/ethernet/sfc/tx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c > index c5f88f7..4ed4082 100644 > --- a/drivers/net/ethernet/sfc/tx.c > +++ b/drivers/net/ethernet/sfc/tx.c > @@ -207,11 +207,11 @@ static void efx_skb_copy_bits_to_pio(struct efx_nic > *efx, struct sk_buff *skb, skb_frag_t *f = &skb_shinfo(skb)->frags[i]; > u8 *vaddr; > > - vaddr = kmap_atomic(skb_frag_page(f)); > + vaddr = kmap_local_page(skb_frag_page(f)); > > efx_memcpy_toio_aligned_cb(efx, piobuf, vaddr + skb_frag_off(f), > skb_frag_size(f), copy_buf); > - kunmap_atomic(vaddr); > + kunmap_local(vaddr); > } > > EFX_WARN_ON_ONCE_PARANOID(skb_shinfo(skb)->frag_list); > -- > 2.37.2 Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> Thanks, Fabio
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
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
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
On Thu, Nov 17, 2022 at 02:25:54PM -0800, Venkataramanan, Anirudh 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. Oh reading this here I see you meant that 'there is not an implicit dependency'... :-/ Ok read that too quick. Still it is not an appearance it is true right? Ok Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > 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> > Cc: Edward Cree <ecree.xilinx@gmail.com> > Cc: Martin Habets <habetsm.xilinx@gmail.com> > Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> > --- > drivers/net/ethernet/sfc/tx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c > index c5f88f7..4ed4082 100644 > --- a/drivers/net/ethernet/sfc/tx.c > +++ b/drivers/net/ethernet/sfc/tx.c > @@ -207,11 +207,11 @@ static void efx_skb_copy_bits_to_pio(struct efx_nic *efx, struct sk_buff *skb, > skb_frag_t *f = &skb_shinfo(skb)->frags[i]; > u8 *vaddr; > > - vaddr = kmap_atomic(skb_frag_page(f)); > + vaddr = kmap_local_page(skb_frag_page(f)); > > efx_memcpy_toio_aligned_cb(efx, piobuf, vaddr + skb_frag_off(f), > skb_frag_size(f), copy_buf); > - kunmap_atomic(vaddr); > + kunmap_local(vaddr); > } > > EFX_WARN_ON_ONCE_PARANOID(skb_shinfo(skb)->frag_list); > -- > 2.37.2 >
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c index c5f88f7..4ed4082 100644 --- a/drivers/net/ethernet/sfc/tx.c +++ b/drivers/net/ethernet/sfc/tx.c @@ -207,11 +207,11 @@ static void efx_skb_copy_bits_to_pio(struct efx_nic *efx, struct sk_buff *skb, skb_frag_t *f = &skb_shinfo(skb)->frags[i]; u8 *vaddr; - vaddr = kmap_atomic(skb_frag_page(f)); + vaddr = kmap_local_page(skb_frag_page(f)); efx_memcpy_toio_aligned_cb(efx, piobuf, vaddr + skb_frag_off(f), skb_frag_size(f), copy_buf); - kunmap_atomic(vaddr); + kunmap_local(vaddr); } EFX_WARN_ON_ONCE_PARANOID(skb_shinfo(skb)->frag_list);
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> Cc: Edward Cree <ecree.xilinx@gmail.com> Cc: Martin Habets <habetsm.xilinx@gmail.com> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> --- drivers/net/ethernet/sfc/tx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)