* DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry [not found] <20210518125443.34148-1-someguy@effective-light.com> @ 2021-09-09 3:33 ` Jeremy Linton 2021-09-09 21:16 ` Ioana Ciornei ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Jeremy Linton @ 2021-09-09 3:33 UTC (permalink / raw) To: Hamza Mahfooz, linux-kernel Cc: Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu, Dan Williams, Ioana Ciornei, netdev@vger.kernel.org +DPAA2, netdev maintainers Hi, On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > Since, overlapping mappings are not supported by the DMA API we should > report an error if active_cacheline_insert returns -EEXIST. It seems this patch found a victim. I was trying to run iperf3 on a honeycomb (5.14.0, fedora 35) and the console is blasting this error message at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which is attached below. [ 151.839693] cacheline tracking EEXIST, overlapping mappings aren't supported ... [ 151.924397] Hardware name: SolidRun Ltd. SolidRun CEX7 Platform, BIOS EDK II Aug 9 2021 [ 151.932481] pstate: 40400005 (nZcv daif +PAN -UAO -TCO BTYPE=--) [ 151.938483] pc : add_dma_entry+0x218/0x240 [ 151.942575] lr : add_dma_entry+0x218/0x240 [ 151.946666] sp : ffff8000101e2f20 [ 151.949975] x29: ffff8000101e2f20 x28: ffffaf317ac85000 x27: ffff3d0366ecb3a0 [ 151.957116] x26: 0000040000000000 x25: 0000000000000001 x24: ffffaf317bbe8908 [ 151.964257] x23: 0000000000000001 x22: ffffaf317bbe8810 x21: 0000000000000000 [ 151.971397] x20: 0000000082e48000 x19: ffffaf317be6e000 x18: ffffffffffffffff [ 151.978537] x17: 646574726f707075 x16: 732074276e657261 x15: ffff8000901e2c2f [ 151.985676] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [ 151.992816] x11: ffffaf317bb4c4c0 x10: 00000000ffffe000 x9 : ffffaf3179708060 [ 151.999956] x8 : 00000000ffffdfff x7 : ffffaf317bb4c4c0 x6 : 0000000000000001 [ 152.007096] x5 : ffff3d0a9af66e30 x4 : 0000000000000000 x3 : 0000000000000027 [ 152.014236] x2 : 0000000000000023 x1 : ffff3d0360aac000 x0 : 0000000000000040 [ 152.021376] Call trace: [ 152.023816] add_dma_entry+0x218/0x240 [ 152.027561] debug_dma_map_sg+0x118/0x17c [ 152.031566] dma_map_sg_attrs+0x70/0xb0 [ 152.035397] dpaa2_eth_build_sg_fd+0xac/0x2f0 [fsl_dpaa2_eth] [ 152.041150] __dpaa2_eth_tx+0x3ec/0x570 [fsl_dpaa2_eth] [ 152.046377] dpaa2_eth_tx+0x74/0x110 [fsl_dpaa2_eth] [ 152.051342] dev_hard_start_xmit+0xe8/0x1a4 [ 152.055523] sch_direct_xmit+0x8c/0x1e0 [ 152.059355] __dev_xmit_skb+0x484/0x6a0 [ 152.063186] __dev_queue_xmit+0x380/0x744 [ 152.067190] dev_queue_xmit+0x20/0x2c [ 152.070848] neigh_hh_output+0xb4/0x130 [ 152.074679] ip_finish_output2+0x494/0x8f0 [ 152.078770] __ip_finish_output+0x12c/0x230 [ 152.082948] ip_finish_output+0x40/0xe0 [ 152.086778] ip_output+0xe4/0x2d4 [ 152.090088] __ip_queue_xmit+0x1b4/0x5c0 [ 152.094006] ip_queue_xmit+0x20/0x30 [ 152.097576] __tcp_transmit_skb+0x3b8/0x7b4 [ 152.101755] tcp_write_xmit+0x350/0x8e0 [ 152.105586] __tcp_push_pending_frames+0x48/0x110 [ 152.110286] tcp_rcv_established+0x338/0x690 [ 152.114550] tcp_v4_do_rcv+0x1c0/0x29c [ 152.118294] tcp_v4_rcv+0xd14/0xe3c [ 152.121777] ip_protocol_deliver_rcu+0x88/0x340 [ 152.126302] ip_local_deliver_finish+0xc0/0x184 [ 152.130827] ip_local_deliver+0x7c/0x23c [ 152.134744] ip_rcv_finish+0xb4/0x100 [ 152.138400] ip_rcv+0x54/0x210 [ 152.141449] deliver_skb+0x74/0xdc [ 152.144846] __netif_receive_skb_core.constprop.0+0x250/0x81c [ 152.150588] __netif_receive_skb_list_core+0x94/0x264 [ 152.155635] netif_receive_skb_list_internal+0x1d0/0x3bc [ 152.160942] netif_receive_skb_list+0x38/0x70 [ 152.165295] dpaa2_eth_poll+0x168/0x350 [fsl_dpaa2_eth] [ 152.170521] __napi_poll.constprop.0+0x40/0x19c [ 152.175047] net_rx_action+0x2c4/0x360 [ 152.178792] __do_softirq+0x1b0/0x394 [ 152.182450] run_ksoftirqd+0x68/0xa0 [ 152.186023] smpboot_thread_fn+0x13c/0x270 [ 152.190115] kthread+0x138/0x140 PS, it might not hurt to rate limit/_once this somehow to avoid a runtime problem if it starts to trigger. Thanks, > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Hamza Mahfooz <someguy@effective-light.com> > --- > kernel/dma/debug.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c > index 14de1271463f..dadae6255d05 100644 > --- a/kernel/dma/debug.c > +++ b/kernel/dma/debug.c > @@ -566,11 +566,9 @@ static void add_dma_entry(struct dma_debug_entry *entry) > if (rc == -ENOMEM) { > pr_err("cacheline tracking ENOMEM, dma-debug disabled\n"); > global_disable = true; > + } else if (rc == -EEXIST) { > + pr_err("cacheline tracking EEXIST, overlapping mappings aren't supported\n"); > } > - > - /* TODO: report -EEXIST errors here as overlapping mappings are > - * not supported by the DMA API > - */ > } > > static int dma_debug_create_entries(gfp_t gfp) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-09-09 3:33 ` DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry Jeremy Linton @ 2021-09-09 21:16 ` Ioana Ciornei 2021-09-10 10:23 ` Christoph Hellwig 2021-09-14 15:45 ` Ioana Ciornei 2 siblings, 0 replies; 15+ messages in thread From: Ioana Ciornei @ 2021-09-09 21:16 UTC (permalink / raw) To: Jeremy Linton Cc: Hamza Mahfooz, linux-kernel@vger.kernel.org, Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, Dan Williams, netdev@vger.kernel.org On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > +DPAA2, netdev maintainers > Hi, > > On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > > Since, overlapping mappings are not supported by the DMA API we should > > report an error if active_cacheline_insert returns -EEXIST. > > It seems this patch found a victim. I was trying to run iperf3 on a > honeycomb (5.14.0, fedora 35) and the console is blasting this error message > at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which > is attached below. > Thanks for the report. I don't have access to hardware at the moment to actually see what's happening since I'm on vacation. I'll work on it in a few days. Ioana ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-09-09 3:33 ` DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry Jeremy Linton 2021-09-09 21:16 ` Ioana Ciornei @ 2021-09-10 10:23 ` Christoph Hellwig 2021-09-14 15:45 ` Ioana Ciornei 2 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2021-09-10 10:23 UTC (permalink / raw) To: Jeremy Linton Cc: Hamza Mahfooz, linux-kernel, Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu, Dan Williams, Ioana Ciornei, netdev@vger.kernel.org On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > PS, it might not hurt to rate limit/_once this somehow to avoid a runtime > problem if it starts to trigger. Yes, that might be a good idea. Care to prepare a patch? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-09-09 3:33 ` DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry Jeremy Linton 2021-09-09 21:16 ` Ioana Ciornei 2021-09-10 10:23 ` Christoph Hellwig @ 2021-09-14 15:45 ` Ioana Ciornei 2021-09-30 13:37 ` Karsten Graul 2021-10-01 4:19 ` Christoph Hellwig 2 siblings, 2 replies; 15+ messages in thread From: Ioana Ciornei @ 2021-09-14 15:45 UTC (permalink / raw) To: Jeremy Linton Cc: Hamza Mahfooz, linux-kernel@vger.kernel.org, Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, Dan Williams, netdev@vger.kernel.org On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > +DPAA2, netdev maintainers > Hi, > > On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > > Since, overlapping mappings are not supported by the DMA API we should > > report an error if active_cacheline_insert returns -EEXIST. > > It seems this patch found a victim. I was trying to run iperf3 on a > honeycomb (5.14.0, fedora 35) and the console is blasting this error message > at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which > is attached below. > > > [ 151.839693] cacheline tracking EEXIST, overlapping mappings aren't > supported > ... > [ 151.924397] Hardware name: SolidRun Ltd. SolidRun CEX7 Platform, BIOS > EDK II Aug 9 2021 > [ 151.932481] pstate: 40400005 (nZcv daif +PAN -UAO -TCO BTYPE=--) > [ 151.938483] pc : add_dma_entry+0x218/0x240 > [ 151.942575] lr : add_dma_entry+0x218/0x240 > [ 151.946666] sp : ffff8000101e2f20 > [ 151.949975] x29: ffff8000101e2f20 x28: ffffaf317ac85000 x27: > ffff3d0366ecb3a0 > [ 151.957116] x26: 0000040000000000 x25: 0000000000000001 x24: > ffffaf317bbe8908 > [ 151.964257] x23: 0000000000000001 x22: ffffaf317bbe8810 x21: > 0000000000000000 > [ 151.971397] x20: 0000000082e48000 x19: ffffaf317be6e000 x18: > ffffffffffffffff > [ 151.978537] x17: 646574726f707075 x16: 732074276e657261 x15: > ffff8000901e2c2f > [ 151.985676] x14: 0000000000000000 x13: 0000000000000000 x12: > 0000000000000000 > [ 151.992816] x11: ffffaf317bb4c4c0 x10: 00000000ffffe000 x9 : > ffffaf3179708060 > [ 151.999956] x8 : 00000000ffffdfff x7 : ffffaf317bb4c4c0 x6 : > 0000000000000001 > [ 152.007096] x5 : ffff3d0a9af66e30 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 152.014236] x2 : 0000000000000023 x1 : ffff3d0360aac000 x0 : > 0000000000000040 > [ 152.021376] Call trace: > [ 152.023816] add_dma_entry+0x218/0x240 > [ 152.027561] debug_dma_map_sg+0x118/0x17c > [ 152.031566] dma_map_sg_attrs+0x70/0xb0 > [ 152.035397] dpaa2_eth_build_sg_fd+0xac/0x2f0 [fsl_dpaa2_eth] > [ 152.041150] __dpaa2_eth_tx+0x3ec/0x570 [fsl_dpaa2_eth] > [ 152.046377] dpaa2_eth_tx+0x74/0x110 [fsl_dpaa2_eth] > [ 152.051342] dev_hard_start_xmit+0xe8/0x1a4 > [ 152.055523] sch_direct_xmit+0x8c/0x1e0 > [ 152.059355] __dev_xmit_skb+0x484/0x6a0 > [ 152.063186] __dev_queue_xmit+0x380/0x744 > [ 152.067190] dev_queue_xmit+0x20/0x2c > [ 152.070848] neigh_hh_output+0xb4/0x130 > [ 152.074679] ip_finish_output2+0x494/0x8f0 > [ 152.078770] __ip_finish_output+0x12c/0x230 > [ 152.082948] ip_finish_output+0x40/0xe0 > [ 152.086778] ip_output+0xe4/0x2d4 > [ 152.090088] __ip_queue_xmit+0x1b4/0x5c0 > [ 152.094006] ip_queue_xmit+0x20/0x30 > [ 152.097576] __tcp_transmit_skb+0x3b8/0x7b4 > [ 152.101755] tcp_write_xmit+0x350/0x8e0 > [ 152.105586] __tcp_push_pending_frames+0x48/0x110 > [ 152.110286] tcp_rcv_established+0x338/0x690 > [ 152.114550] tcp_v4_do_rcv+0x1c0/0x29c > [ 152.118294] tcp_v4_rcv+0xd14/0xe3c > [ 152.121777] ip_protocol_deliver_rcu+0x88/0x340 > [ 152.126302] ip_local_deliver_finish+0xc0/0x184 > [ 152.130827] ip_local_deliver+0x7c/0x23c > [ 152.134744] ip_rcv_finish+0xb4/0x100 > [ 152.138400] ip_rcv+0x54/0x210 > [ 152.141449] deliver_skb+0x74/0xdc > [ 152.144846] __netif_receive_skb_core.constprop.0+0x250/0x81c > [ 152.150588] __netif_receive_skb_list_core+0x94/0x264 > [ 152.155635] netif_receive_skb_list_internal+0x1d0/0x3bc > [ 152.160942] netif_receive_skb_list+0x38/0x70 > [ 152.165295] dpaa2_eth_poll+0x168/0x350 [fsl_dpaa2_eth] > [ 152.170521] __napi_poll.constprop.0+0x40/0x19c > [ 152.175047] net_rx_action+0x2c4/0x360 > [ 152.178792] __do_softirq+0x1b0/0x394 > [ 152.182450] run_ksoftirqd+0x68/0xa0 > [ 152.186023] smpboot_thread_fn+0x13c/0x270 > [ 152.190115] kthread+0x138/0x140 > I got some time to look at this and I am not sure if it's an actual problem or not. First of all, I added some more debug prints when any overlapping happens so that I can actually see the entries. [ 245.927020] fsl_dpaa2_eth dpni.3: scather-gather idx 0 P=20a7320000 N=20a7320 D=20a7320000 L=30 DMA_BIDIRECTIONAL dma map error check not applicable· [ 245.927048] fsl_dpaa2_eth dpni.3: scather-gather idx 1 P=20a7320030 N=20a7320 D=20a7320030 L=5a8 DMA_BIDIRECTIONAL dma map error check not applicable [ 245.927062] DMA-API: cacheline tracking EEXIST, overlapping mappings aren't supported The first line is the dump of the dma_debug_entry which is already present in the radix tree and the second one is the entry which just triggered the EEXIST. As we can see, they are not actually overlapping, at least from my understanding. The first one starts at 0x20a7320000 with a size 0x30 and the second one at 0x20a7320030. I wanted to see where these mappings are originating so I added some traces around the dma_[un]map_single, dma_[un]map_sg operations in dpaa2-eth. I can see the following: - There are two S/G skbs being sent one after another (no cleanup of the Tx confirmation is done between, so not kfree or dma_unmap is happening). - Skb#1 has 3 frags and skb#2 just 2 frags. - The skb#1 3rd frag will get into the same cacheline as the skb#2 2nd frag. skb#1: 245.926981: dpaa2_eth:dpaa2_dma_map_sg: [eth4] scl=0x0xffff4cf3ac188200 num_sg=3 245.926984: dpaa2_eth:dpaa2_dma_map_sg_entry: [eth4] size=0x42 dma_addr=0x20b66e58fe 245.926987: dpaa2_eth:dpaa2_dma_map_sg_entry: [eth4] size=0x578 dma_addr=0x20ab9c7a88 245.926989: dpaa2_eth:dpaa2_dma_map_sg_entry: [eth4] size=0x30 dma_addr=0x20a7320000 ^ | | This skb frag will land in the same cacheline number with the 2nd frag from skb#2. skb#2: 245.934933: dpaa2_eth:dpaa2_dma_map_sg: [eth4] scl=0x0xffff4cf3ac188300 num_sg=2 245.934936: dpaa2_eth:dpaa2_dma_map_sg_entry: [eth4] size=0x42 dma_addr=0x20b66e60fe 245.934939: dpaa2_eth:dpaa2_dma_map_sg_entry: [eth4] size=0x5a8 dma_addr=0x20a7320030 ^ | These frags are allocated by the stack, transformed into a scatterlist by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the dpaa2-eth's decision to use two fragments from the same page (that will also end un in the same cacheline) in two different in-flight skbs. Is this behavior normal? Ioana ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-09-14 15:45 ` Ioana Ciornei @ 2021-09-30 13:37 ` Karsten Graul 2021-10-01 12:52 ` Gerald Schaefer 2021-10-01 4:19 ` Christoph Hellwig 1 sibling, 1 reply; 15+ messages in thread From: Karsten Graul @ 2021-09-30 13:37 UTC (permalink / raw) To: Ioana Ciornei, Jeremy Linton Cc: Hamza Mahfooz, linux-kernel@vger.kernel.org, Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, Dan Williams, netdev@vger.kernel.org, Gerald Schaefer, linux-s390 On 14/09/2021 17:45, Ioana Ciornei wrote: > On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: >> +DPAA2, netdev maintainers >> Hi, >> >> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: >>> Since, overlapping mappings are not supported by the DMA API we should >>> report an error if active_cacheline_insert returns -EEXIST. >> >> It seems this patch found a victim. I was trying to run iperf3 on a >> honeycomb (5.14.0, fedora 35) and the console is blasting this error message >> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which >> is attached below. >> > > These frags are allocated by the stack, transformed into a scatterlist > by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the > dpaa2-eth's decision to use two fragments from the same page (that will > also end un in the same cacheline) in two different in-flight skbs. > > Is this behavior normal? > We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. The CI has panic_on_warn enabled so we see the panic every day now. Its always the same pattern: module SMC calls dma_map_sg_attrs() which ends up in the EEXIST warning sooner or later. It would be better to revert this patch now and start to better understand the checking logic for overlapping areas. Thank you. The call trace for reference: [ 864.189864] DMA-API: mlx5_core 0662:00:00.0: cacheline tracking EEXIST, overlapping mappings aren't supported [ 864.189883] WARNING: CPU: 0 PID: 33720 at kernel/dma/debug.c:570 add_dma_entry+0x208/0x2c8 ... [ 864.190747] CPU: 0 PID: 33720 Comm: smcapp Not tainted 5.15.0-20210928.rc3.git0.a59bf04db7bb.300.fc34.s390x+debug #1 [ 864.190758] Hardware name: IBM 8561 T01 701 (z/VM 7.2.0) [ 864.190766] Krnl PSW : 0704d00180000000 00000000fa6239fc (add_dma_entry+0x20c/0x2c8) [ 864.190783] R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 RI:0 EA:3 [ 864.190795] Krnl GPRS: c0000000ffffbfff 0000000080000000 0000000000000061 0000000000000000 [ 864.190804] 0000000000000001 0000000000000001 0000000000000001 0000000000000001 [ 864.190813] 0700000000000001 000000000020ff00 00000000ffffffff 000000008137b300 [ 864.190822] 0000000020020100 0000000000000001 00000000fa6239f8 00000380074536f8 [ 864.190837] Krnl Code: 00000000fa6239ec: c020007a4964 larl %r2,00000000fb56ccb4 00000000fa6239f2: c0e5005ef2ff brasl %r14,00000000fb201ff0 #00000000fa6239f8: af000000 mc 0,0 >00000000fa6239fc: ecb60057007c cgij %r11,0,6,00000000fa623aaa 00000000fa623a02: c01000866149 larl %r1,00000000fb6efc94 00000000fa623a08: e31010000012 lt %r1,0(%r1) 00000000fa623a0e: a774ff73 brc 7,00000000fa6238f4 00000000fa623a12: c010008a9227 larl %r1,00000000fb775e60 [ 864.202949] Call Trace: [ 864.202959] [<00000000fa6239fc>] add_dma_entry+0x20c/0x2c8 [ 864.202971] ([<00000000fa6239f8>] add_dma_entry+0x208/0x2c8) [ 864.202981] [<00000000fa624988>] debug_dma_map_sg+0x140/0x160 [ 864.202992] [<00000000fa61eadc>] __dma_map_sg_attrs+0x9c/0xd8 [ 864.203002] [<00000000fa61eb3a>] dma_map_sg_attrs+0x22/0x40 [ 864.203012] [<000003ff80483bde>] smc_ib_buf_map_sg+0x5e/0x90 [smc] [ 864.203036] [<000003ff80486b44>] smcr_buf_map_link.part.0+0x12c/0x1e8 [smc] [ 864.203053] [<000003ff80486cb6>] _smcr_buf_map_lgr+0xb6/0xf8 [smc] [ 864.203071] [<000003ff8048b91c>] smcr_buf_map_lgr+0x4c/0x90 [smc] [ 864.211496] [<000003ff80490ac2>] smc_llc_cli_add_link+0x152/0x420 [smc] [ 864.211522] [<000003ff8047acbc>] smcr_clnt_conf_first_link+0x124/0x1e0 [smc] [ 864.211537] [<000003ff8047bfb2>] smc_connect_rdma+0x25a/0x2e8 [smc] [ 864.211551] [<000003ff8047da4a>] __smc_connect+0x38a/0x650 [smc] [ 864.211566] [<000003ff8047de70>] smc_connect+0x160/0x190 [smc] [ 864.211580] [<00000000faf10c70>] __sys_connect+0x98/0xd0 [ 864.211592] [<00000000faf12e9a>] __do_sys_socketcall+0x16a/0x350 [ 864.211603] [<00000000fb216752>] __do_syscall+0x1c2/0x1f0 [ 864.211616] [<00000000fb229148>] system_call+0x78/0xa0 -- Karsten ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-09-30 13:37 ` Karsten Graul @ 2021-10-01 12:52 ` Gerald Schaefer 2021-10-06 13:10 ` Gerald Schaefer 0 siblings, 1 reply; 15+ messages in thread From: Gerald Schaefer @ 2021-10-01 12:52 UTC (permalink / raw) To: Karsten Graul Cc: Ioana Ciornei, Jeremy Linton, Hamza Mahfooz, linux-kernel@vger.kernel.org, Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, Dan Williams, netdev@vger.kernel.org, linux-s390, Gerald Schaefer On Thu, 30 Sep 2021 15:37:33 +0200 Karsten Graul <kgraul@linux.ibm.com> wrote: > On 14/09/2021 17:45, Ioana Ciornei wrote: > > On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > >> +DPAA2, netdev maintainers > >> Hi, > >> > >> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > >>> Since, overlapping mappings are not supported by the DMA API we should > >>> report an error if active_cacheline_insert returns -EEXIST. > >> > >> It seems this patch found a victim. I was trying to run iperf3 on a > >> honeycomb (5.14.0, fedora 35) and the console is blasting this error message > >> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which > >> is attached below. > >> > > > > These frags are allocated by the stack, transformed into a scatterlist > > by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the > > dpaa2-eth's decision to use two fragments from the same page (that will > > also end un in the same cacheline) in two different in-flight skbs. > > > > Is this behavior normal? > > > > We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. > The CI has panic_on_warn enabled so we see the panic every day now. Adding a WARN for a case that be detected false-positive seems not acceptable, exactly for this reason (kernel panic on unaffected systems). So I guess it boils down to the question if the behavior that Ioana described is legit behavior, on a system that is dma coherent. We are apparently hitting the same scenario, although it could not yet be reproduced with debug printks for some reason. If the answer is yes, than please remove at lease the WARN, so that it will not make systems crash that behave valid, and have panic_on_warn set. Even a normal printk feels wrong to me in that case, it really sounds rather like you want to fix / better refine the overlap check, if you want to report anything here. BTW, there is already a WARN in the add_dma_entry() path, related to cachlline overlap and -EEXIST: add_dma_entry() -> active_cacheline_insert() -> -EEXIST -> active_cacheline_inc_overlap() That will only trigger when "overlap > ACTIVE_CACHELINE_MAX_OVERLAP". Not familiar with that code, but it seems that there are now two warnings for more or less the same, and the new warning is much more prone to false-positives. How do these 2 warnings relate, are they both really necessary? I think the new warning was only introduced because of some old TODO comment in add_dma_entry(), see commit 2b4bbc6231d78 ("dma-debug: report -EEXIST errors in add_dma_entry"). That comment was initially added by Dan long time ago, and he added several fix-ups for overlap detection after that, including the "overlap > ACTIVE_CACHELINE_MAX_OVERLAP" stuff in active_cacheline_inc_overlap(). So could it be that the TODO comment was simply not valid any more, and better be removed instead of adding new / double warnings, that also generate false-positives and kernel crashes? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-01 12:52 ` Gerald Schaefer @ 2021-10-06 13:10 ` Gerald Schaefer 2021-10-06 13:21 ` Gerald Schaefer 2021-10-06 14:23 ` Robin Murphy 0 siblings, 2 replies; 15+ messages in thread From: Gerald Schaefer @ 2021-10-06 13:10 UTC (permalink / raw) To: Hamza Mahfooz, Christoph Hellwig, Dan Williams Cc: Karsten Graul, Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390 On Fri, 1 Oct 2021 14:52:56 +0200 Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: > On Thu, 30 Sep 2021 15:37:33 +0200 > Karsten Graul <kgraul@linux.ibm.com> wrote: > > > On 14/09/2021 17:45, Ioana Ciornei wrote: > > > On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > > >> +DPAA2, netdev maintainers > > >> Hi, > > >> > > >> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > > >>> Since, overlapping mappings are not supported by the DMA API we should > > >>> report an error if active_cacheline_insert returns -EEXIST. > > >> > > >> It seems this patch found a victim. I was trying to run iperf3 on a > > >> honeycomb (5.14.0, fedora 35) and the console is blasting this error message > > >> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which > > >> is attached below. > > >> > > > > > > These frags are allocated by the stack, transformed into a scatterlist > > > by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the > > > dpaa2-eth's decision to use two fragments from the same page (that will > > > also end un in the same cacheline) in two different in-flight skbs. > > > > > > Is this behavior normal? > > > > > > > We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. > > The CI has panic_on_warn enabled so we see the panic every day now. > > Adding a WARN for a case that be detected false-positive seems not > acceptable, exactly for this reason (kernel panic on unaffected > systems). > > So I guess it boils down to the question if the behavior that Ioana > described is legit behavior, on a system that is dma coherent. We > are apparently hitting the same scenario, although it could not yet be > reproduced with debug printks for some reason. > > If the answer is yes, than please remove at lease the WARN, so that > it will not make systems crash that behave valid, and have > panic_on_warn set. Even a normal printk feels wrong to me in that > case, it really sounds rather like you want to fix / better refine > the overlap check, if you want to report anything here. Dan, Christoph, any opinion? So far it all looks a lot like a false positive, so could you please see that those patches get reverted? I do wonder a bit why this is not an issue for others, we surely cannot be the only ones running CI with panic_on_warn. We would need to disable DEBUG_DMA if this WARN stays in, which would be a shame. Of course, in theory, this might also indicate some real bug, but there really is no sign of that so far. Having multiple sg elements in the same page (or cacheline) is valid, correct? And this is also not a decision of the driver IIUC, so if it was bug, it should be addressed in common code, correct? > > BTW, there is already a WARN in the add_dma_entry() path, related > to cachlline overlap and -EEXIST: > > add_dma_entry() -> active_cacheline_insert() -> -EEXIST -> > active_cacheline_inc_overlap() > > That will only trigger when "overlap > ACTIVE_CACHELINE_MAX_OVERLAP". > Not familiar with that code, but it seems that there are now two > warnings for more or less the same, and the new warning is much more > prone to false-positives. > > How do these 2 warnings relate, are they both really necessary? > I think the new warning was only introduced because of some old > TODO comment in add_dma_entry(), see commit 2b4bbc6231d78 > ("dma-debug: report -EEXIST errors in add_dma_entry"). > > That comment was initially added by Dan long time ago, and he > added several fix-ups for overlap detection after that, including > the "overlap > ACTIVE_CACHELINE_MAX_OVERLAP" stuff in > active_cacheline_inc_overlap(). So could it be that the TODO > comment was simply not valid any more, and better be removed > instead of adding new / double warnings, that also generate > false-positives and kernel crashes? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-06 13:10 ` Gerald Schaefer @ 2021-10-06 13:21 ` Gerald Schaefer 2021-10-06 14:23 ` Robin Murphy 1 sibling, 0 replies; 15+ messages in thread From: Gerald Schaefer @ 2021-10-06 13:21 UTC (permalink / raw) To: Hamza Mahfooz, Christoph Hellwig, Dan Williams Cc: Karsten Graul, Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390 On Wed, 6 Oct 2021 15:10:43 +0200 Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: > On Fri, 1 Oct 2021 14:52:56 +0200 > Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: > > > On Thu, 30 Sep 2021 15:37:33 +0200 > > Karsten Graul <kgraul@linux.ibm.com> wrote: > > > > > On 14/09/2021 17:45, Ioana Ciornei wrote: > > > > On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > > > >> +DPAA2, netdev maintainers > > > >> Hi, > > > >> > > > >> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > > > >>> Since, overlapping mappings are not supported by the DMA API we should > > > >>> report an error if active_cacheline_insert returns -EEXIST. > > > >> > > > >> It seems this patch found a victim. I was trying to run iperf3 on a > > > >> honeycomb (5.14.0, fedora 35) and the console is blasting this error message > > > >> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which > > > >> is attached below. > > > >> > > > > > > > > These frags are allocated by the stack, transformed into a scatterlist > > > > by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the > > > > dpaa2-eth's decision to use two fragments from the same page (that will > > > > also end un in the same cacheline) in two different in-flight skbs. > > > > > > > > Is this behavior normal? > > > > > > > > > > We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. > > > The CI has panic_on_warn enabled so we see the panic every day now. > > > > Adding a WARN for a case that be detected false-positive seems not > > acceptable, exactly for this reason (kernel panic on unaffected > > systems). > > > > So I guess it boils down to the question if the behavior that Ioana > > described is legit behavior, on a system that is dma coherent. We > > are apparently hitting the same scenario, although it could not yet be > > reproduced with debug printks for some reason. > > > > If the answer is yes, than please remove at lease the WARN, so that > > it will not make systems crash that behave valid, and have > > panic_on_warn set. Even a normal printk feels wrong to me in that > > case, it really sounds rather like you want to fix / better refine > > the overlap check, if you want to report anything here. > > Dan, Christoph, any opinion? > > So far it all looks a lot like a false positive, so could you please > see that those patches get reverted? I do wonder a bit why this is > not an issue for others, we surely cannot be the only ones running > CI with panic_on_warn. For reference, we are talking about these commits: 2b4bbc6231d7 ("dma-debug: report -EEXIST errors in add_dma_entry") 510e1a724ab1 ("dma-debug: prevent an error message from causing runtime problems") The latter introduced the WARN (through err_printk usage), and should be reverted if it can be false-positive, but both seem wrong in that case. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-06 13:10 ` Gerald Schaefer 2021-10-06 13:21 ` Gerald Schaefer @ 2021-10-06 14:23 ` Robin Murphy 2021-10-06 15:06 ` Gerald Schaefer 2021-10-07 10:59 ` Karsten Graul 1 sibling, 2 replies; 15+ messages in thread From: Robin Murphy @ 2021-10-06 14:23 UTC (permalink / raw) To: Gerald Schaefer, Hamza Mahfooz, Christoph Hellwig, Dan Williams Cc: Karsten Graul, Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390 On 2021-10-06 14:10, Gerald Schaefer wrote: > On Fri, 1 Oct 2021 14:52:56 +0200 > Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: > >> On Thu, 30 Sep 2021 15:37:33 +0200 >> Karsten Graul <kgraul@linux.ibm.com> wrote: >> >>> On 14/09/2021 17:45, Ioana Ciornei wrote: >>>> On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: >>>>> +DPAA2, netdev maintainers >>>>> Hi, >>>>> >>>>> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: >>>>>> Since, overlapping mappings are not supported by the DMA API we should >>>>>> report an error if active_cacheline_insert returns -EEXIST. >>>>> >>>>> It seems this patch found a victim. I was trying to run iperf3 on a >>>>> honeycomb (5.14.0, fedora 35) and the console is blasting this error message >>>>> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which >>>>> is attached below. >>>>> >>>> >>>> These frags are allocated by the stack, transformed into a scatterlist >>>> by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the >>>> dpaa2-eth's decision to use two fragments from the same page (that will >>>> also end un in the same cacheline) in two different in-flight skbs. >>>> >>>> Is this behavior normal? >>>> >>> >>> We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. >>> The CI has panic_on_warn enabled so we see the panic every day now. >> >> Adding a WARN for a case that be detected false-positive seems not >> acceptable, exactly for this reason (kernel panic on unaffected >> systems). >> >> So I guess it boils down to the question if the behavior that Ioana >> described is legit behavior, on a system that is dma coherent. We >> are apparently hitting the same scenario, although it could not yet be >> reproduced with debug printks for some reason. >> >> If the answer is yes, than please remove at lease the WARN, so that >> it will not make systems crash that behave valid, and have >> panic_on_warn set. Even a normal printk feels wrong to me in that >> case, it really sounds rather like you want to fix / better refine >> the overlap check, if you want to report anything here. > > Dan, Christoph, any opinion? > > So far it all looks a lot like a false positive, so could you please > see that those patches get reverted? I do wonder a bit why this is > not an issue for others, we surely cannot be the only ones running > CI with panic_on_warn. What convinces you it's a false-positive? I'm hardly familiar with most of that callstack, but it appears to be related to mlx5, and I know that exists on expansion cards which could be plugged into a system with non-coherent PCIe where partial cacheline overlap *would* be a real issue. Of course it's dubious that there are many real use-cases for plugging a NIC with a 4-figure price tag into a little i.MX8 or whatever, but the point is that it *should* still work correctly. > We would need to disable DEBUG_DMA if this WARN stays in, which > would be a shame. Of course, in theory, this might also indicate > some real bug, but there really is no sign of that so far. The whole point of DMA debug is to flag up things that you *do* get away with on the vast majority of systems, precisely because most testing happens on those systems rather than more esoteric embedded setups. Say your system only uses dma-direct and a driver starts triggering the warning for not calling dma_mapping_error(), would you argue for removing that warning as well since dma_map_single() can't fail on your machine so it's "not a bug"? > Having multiple sg elements in the same page (or cacheline) is > valid, correct? And this is also not a decision of the driver > IIUC, so if it was bug, it should be addressed in common code, > correct? According to the streaming DMA API documentation, it is *not* valid: ".. warning:: Memory coherency operates at a granularity called the cache line width. In order for memory mapped by this API to operate correctly, the mapped region must begin exactly on a cache line boundary and end exactly on one (to prevent two separately mapped regions from sharing a single cache line). Since the cache line size may not be known at compile time, the API will not enforce this requirement. Therefore, it is recommended that driver writers who don't take special care to determine the cache line size at run time only map virtual regions that begin and end on page boundaries (which are guaranteed also to be cache line boundaries)." >> BTW, there is already a WARN in the add_dma_entry() path, related >> to cachlline overlap and -EEXIST: >> >> add_dma_entry() -> active_cacheline_insert() -> -EEXIST -> >> active_cacheline_inc_overlap() >> >> That will only trigger when "overlap > ACTIVE_CACHELINE_MAX_OVERLAP". >> Not familiar with that code, but it seems that there are now two >> warnings for more or less the same, and the new warning is much more >> prone to false-positives. >> >> How do these 2 warnings relate, are they both really necessary? >> I think the new warning was only introduced because of some old >> TODO comment in add_dma_entry(), see commit 2b4bbc6231d78 >> ("dma-debug: report -EEXIST errors in add_dma_entry"). AFAICS they are different things. I believe the new warning is supposed to be for the fundementally incorrect API usage (as above) of mapping different regions overlapping within the same cacheline. The existing one is about dma-debug losing internal consistency when tracking the *same* region being mapped multiple times, which is a legal thing to do - e.g. buffer sharing between devices - but if anyone's doing it to excess that's almost certainly a bug (i.e. they probably intended to unmap it in between but missed that out). Robin. >> That comment was initially added by Dan long time ago, and he >> added several fix-ups for overlap detection after that, including >> the "overlap > ACTIVE_CACHELINE_MAX_OVERLAP" stuff in >> active_cacheline_inc_overlap(). So could it be that the TODO >> comment was simply not valid any more, and better be removed >> instead of adding new / double warnings, that also generate >> false-positives and kernel crashes? > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-06 14:23 ` Robin Murphy @ 2021-10-06 15:06 ` Gerald Schaefer 2021-10-07 10:59 ` Karsten Graul 1 sibling, 0 replies; 15+ messages in thread From: Gerald Schaefer @ 2021-10-06 15:06 UTC (permalink / raw) To: Robin Murphy Cc: Hamza Mahfooz, Christoph Hellwig, Dan Williams, Karsten Graul, Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390, linux-rdma On Wed, 6 Oct 2021 15:23:36 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > On 2021-10-06 14:10, Gerald Schaefer wrote: > > On Fri, 1 Oct 2021 14:52:56 +0200 > > Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: > > > >> On Thu, 30 Sep 2021 15:37:33 +0200 > >> Karsten Graul <kgraul@linux.ibm.com> wrote: > >> > >>> On 14/09/2021 17:45, Ioana Ciornei wrote: > >>>> On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: > >>>>> +DPAA2, netdev maintainers > >>>>> Hi, > >>>>> > >>>>> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: > >>>>>> Since, overlapping mappings are not supported by the DMA API we should > >>>>>> report an error if active_cacheline_insert returns -EEXIST. > >>>>> > >>>>> It seems this patch found a victim. I was trying to run iperf3 on a > >>>>> honeycomb (5.14.0, fedora 35) and the console is blasting this error message > >>>>> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which > >>>>> is attached below. > >>>>> > >>>> > >>>> These frags are allocated by the stack, transformed into a scatterlist > >>>> by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the > >>>> dpaa2-eth's decision to use two fragments from the same page (that will > >>>> also end un in the same cacheline) in two different in-flight skbs. > >>>> > >>>> Is this behavior normal? > >>>> > >>> > >>> We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. > >>> The CI has panic_on_warn enabled so we see the panic every day now. > >> > >> Adding a WARN for a case that be detected false-positive seems not > >> acceptable, exactly for this reason (kernel panic on unaffected > >> systems). > >> > >> So I guess it boils down to the question if the behavior that Ioana > >> described is legit behavior, on a system that is dma coherent. We > >> are apparently hitting the same scenario, although it could not yet be > >> reproduced with debug printks for some reason. > >> > >> If the answer is yes, than please remove at lease the WARN, so that > >> it will not make systems crash that behave valid, and have > >> panic_on_warn set. Even a normal printk feels wrong to me in that > >> case, it really sounds rather like you want to fix / better refine > >> the overlap check, if you want to report anything here. > > > > Dan, Christoph, any opinion? > > > > So far it all looks a lot like a false positive, so could you please > > see that those patches get reverted? I do wonder a bit why this is > > not an issue for others, we surely cannot be the only ones running > > CI with panic_on_warn. > > What convinces you it's a false-positive? I'm hardly familiar with most > of that callstack, but it appears to be related to mlx5, and I know that > exists on expansion cards which could be plugged into a system with > non-coherent PCIe where partial cacheline overlap *would* be a real > issue. Of course it's dubious that there are many real use-cases for > plugging a NIC with a 4-figure price tag into a little i.MX8 or > whatever, but the point is that it *should* still work correctly. I would assume that a *proper* warning would check if we see the "non-coherent" case, e.g. by using dev_is_dma_coherent() and only report with potentially fatal WARN on systems where it is appropriate. However, I am certainly even less familiar with all that, and might just have gotten the wrong impression here. Also not sure about mlx5 relation here, it does not really show in the call trace, only in the err_printk() output, probably from dev_driver_string(dev) or dev_name(dev). But I do not see where mlx5 code would be involved here. [...] > According to the streaming DMA API documentation, it is *not* valid: > > ".. warning:: > > Memory coherency operates at a granularity called the cache > line width. In order for memory mapped by this API to operate > correctly, the mapped region must begin exactly on a cache line > boundary and end exactly on one (to prevent two separately mapped > regions from sharing a single cache line). Since the cache line size > may not be known at compile time, the API will not enforce this > requirement. Therefore, it is recommended that driver writers who > don't take special care to determine the cache line size at run time > only map virtual regions that begin and end on page boundaries (which > are guaranteed also to be cache line boundaries)." Thanks, but I cannot really make a lot of sense out if this. Which driver exactly would be the one that needs to take care of the cache line alignment for sg elements? If this WARN is really reporting a bug, could you please help pointing to where it would need to be addressed? And does this really say that it is illegal to have multiple sg elements within the same cache line, regardless of cache coherence? Adding linux-rdma@vger.kernel.org, sorry for the noise, but maybe somebody on that list can make more sense of this. For reference, the link to the start of this thread: https://lkml.kernel.org/r/fd67fbac-64bf-f0ea-01e1-5938ccfab9d0@arm.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-06 14:23 ` Robin Murphy 2021-10-06 15:06 ` Gerald Schaefer @ 2021-10-07 10:59 ` Karsten Graul 2021-10-07 16:40 ` Gerald Schaefer 2021-10-11 11:47 ` Christoph Hellwig 1 sibling, 2 replies; 15+ messages in thread From: Karsten Graul @ 2021-10-07 10:59 UTC (permalink / raw) To: Robin Murphy, Gerald Schaefer, Hamza Mahfooz, Christoph Hellwig, Dan Williams Cc: Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390 On 06/10/2021 16:23, Robin Murphy wrote: > On 2021-10-06 14:10, Gerald Schaefer wrote: >> On Fri, 1 Oct 2021 14:52:56 +0200 >> Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: >> >>> On Thu, 30 Sep 2021 15:37:33 +0200 >>> Karsten Graul <kgraul@linux.ibm.com> wrote: >>> >>>> On 14/09/2021 17:45, Ioana Ciornei wrote: >>>>> On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote: >>>>>> +DPAA2, netdev maintainers >>>>>> Hi, >>>>>> >>>>>> On 5/18/21 7:54 AM, Hamza Mahfooz wrote: >>>>>>> Since, overlapping mappings are not supported by the DMA API we should >>>>>>> report an error if active_cacheline_insert returns -EEXIST. >>>>>> >>>>>> It seems this patch found a victim. I was trying to run iperf3 on a >>>>>> honeycomb (5.14.0, fedora 35) and the console is blasting this error message >>>>>> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace, which >>>>>> is attached below. >>>>>> >>>>> >>>>> These frags are allocated by the stack, transformed into a scatterlist >>>>> by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the >>>>> dpaa2-eth's decision to use two fragments from the same page (that will >>>>> also end un in the same cacheline) in two different in-flight skbs. >>>>> >>>>> Is this behavior normal? >>>>> >>>> >>>> We see the same problem here and it started with 5.15-rc2 in our nightly CI runs. >>>> The CI has panic_on_warn enabled so we see the panic every day now. >>> >>> Adding a WARN for a case that be detected false-positive seems not >>> acceptable, exactly for this reason (kernel panic on unaffected >>> systems). >>> >>> So I guess it boils down to the question if the behavior that Ioana >>> described is legit behavior, on a system that is dma coherent. We >>> are apparently hitting the same scenario, although it could not yet be >>> reproduced with debug printks for some reason. >>> >>> If the answer is yes, than please remove at lease the WARN, so that >>> it will not make systems crash that behave valid, and have >>> panic_on_warn set. Even a normal printk feels wrong to me in that >>> case, it really sounds rather like you want to fix / better refine >>> the overlap check, if you want to report anything here. >> >> Dan, Christoph, any opinion? >> >> So far it all looks a lot like a false positive, so could you please >> see that those patches get reverted? I do wonder a bit why this is >> not an issue for others, we surely cannot be the only ones running >> CI with panic_on_warn. > > What convinces you it's a false-positive? I'm hardly familiar with most of that callstack, but it appears to be related to mlx5, and I know that exists on expansion cards which could be plugged into a system with non-coherent PCIe where partial cacheline overlap *would* be a real issue. Of course it's dubious that there are many real use-cases for plugging a NIC with a 4-figure price tag into a little i.MX8 or whatever, but the point is that it *should* still work correctly. > >> We would need to disable DEBUG_DMA if this WARN stays in, which >> would be a shame. Of course, in theory, this might also indicate >> some real bug, but there really is no sign of that so far. > > The whole point of DMA debug is to flag up things that you *do* get away with on the vast majority of systems, precisely because most testing happens on those systems rather than more esoteric embedded setups. Say your system only uses dma-direct and a driver starts triggering the warning for not calling dma_mapping_error(), would you argue for removing that warning as well since dma_map_single() can't fail on your machine so it's "not a bug"? > >> Having multiple sg elements in the same page (or cacheline) is >> valid, correct? And this is also not a decision of the driver >> IIUC, so if it was bug, it should be addressed in common code, >> correct? > > According to the streaming DMA API documentation, it is *not* valid: > > ".. warning:: > > Memory coherency operates at a granularity called the cache > line width. In order for memory mapped by this API to operate > correctly, the mapped region must begin exactly on a cache line > boundary and end exactly on one (to prevent two separately mapped > regions from sharing a single cache line). Since the cache line size > may not be known at compile time, the API will not enforce this > requirement. Therefore, it is recommended that driver writers who > don't take special care to determine the cache line size at run time > only map virtual regions that begin and end on page boundaries (which > are guaranteed also to be cache line boundaries)." > >>> BTW, there is already a WARN in the add_dma_entry() path, related >>> to cachlline overlap and -EEXIST: >>> >>> add_dma_entry() -> active_cacheline_insert() -> -EEXIST -> >>> active_cacheline_inc_overlap() >>> >>> That will only trigger when "overlap > ACTIVE_CACHELINE_MAX_OVERLAP". >>> Not familiar with that code, but it seems that there are now two >>> warnings for more or less the same, and the new warning is much more >>> prone to false-positives. >>> >>> How do these 2 warnings relate, are they both really necessary? >>> I think the new warning was only introduced because of some old >>> TODO comment in add_dma_entry(), see commit 2b4bbc6231d78 >>> ("dma-debug: report -EEXIST errors in add_dma_entry"). > > AFAICS they are different things. I believe the new warning is supposed to be for the fundementally incorrect API usage (as above) of mapping different regions overlapping within the same cacheline. The existing one is about dma-debug losing internal consistency when tracking the *same* region being mapped multiple times, which is a legal thing to do - e.g. buffer sharing between devices - but if anyone's doing it to excess that's almost certainly a bug (i.e. they probably intended to unmap it in between but missed that out). Thanks for the explanation Robin. In our case its really that a buffer is mapped twice for 2 different devices which we use in SMC to provide failover capabilities. We see that -EEXIST is returned when a buffer is mapped for the second device. Since there is a maximum of 2 parallel mappings we never see the warning shown by active_cacheline_inc_overlap() because we don't exceed ACTIVE_CACHELINE_MAX_OVERLAP. So how to deal with this kind of "legal thing", looks like there is no way to suppress the newly introduced EEXIST warning for that case? Karsten ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-07 10:59 ` Karsten Graul @ 2021-10-07 16:40 ` Gerald Schaefer 2021-10-11 11:47 ` Christoph Hellwig 1 sibling, 0 replies; 15+ messages in thread From: Gerald Schaefer @ 2021-10-07 16:40 UTC (permalink / raw) To: Karsten Graul Cc: Robin Murphy, Hamza Mahfooz, Christoph Hellwig, Dan Williams, Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390 On Thu, 7 Oct 2021 12:59:32 +0200 Karsten Graul <kgraul@linux.ibm.com> wrote: [...] > > > >>> BTW, there is already a WARN in the add_dma_entry() path, related > >>> to cachlline overlap and -EEXIST: > >>> > >>> add_dma_entry() -> active_cacheline_insert() -> -EEXIST -> > >>> active_cacheline_inc_overlap() > >>> > >>> That will only trigger when "overlap > ACTIVE_CACHELINE_MAX_OVERLAP". > >>> Not familiar with that code, but it seems that there are now two > >>> warnings for more or less the same, and the new warning is much more > >>> prone to false-positives. > >>> > >>> How do these 2 warnings relate, are they both really necessary? > >>> I think the new warning was only introduced because of some old > >>> TODO comment in add_dma_entry(), see commit 2b4bbc6231d78 > >>> ("dma-debug: report -EEXIST errors in add_dma_entry"). > > > > AFAICS they are different things. I believe the new warning is supposed to be for the fundementally incorrect API usage (as above) of mapping different regions overlapping within the same cacheline. The existing one is about dma-debug losing internal consistency when tracking the *same* region being mapped multiple times, which is a legal thing to do - e.g. buffer sharing between devices - but if anyone's doing it to excess that's almost certainly a bug (i.e. they probably intended to unmap it in between but missed that out). > > Thanks for the explanation Robin. > > In our case its really that a buffer is mapped twice for 2 different devices which we use in SMC to provide failover capabilities. We see that -EEXIST is returned when a buffer is mapped for the second device. Since there is a maximum of 2 parallel mappings we never see the warning shown by active_cacheline_inc_overlap() because we don't exceed ACTIVE_CACHELINE_MAX_OVERLAP. > > So how to deal with this kind of "legal thing", looks like there is no way to suppress the newly introduced EEXIST warning for that case? Thanks Karsten, very interesting. We assumed so far that we hit the same case as Ioana, i.e. having multiple sg elements in one cacheline. With debug output it now seems that we hit a completely different case, not at all related to any cacheline or coherency issues. So it really seems that the new warning is basically the same as the already present one, with the difference that it already triggers on the first occurrence. Looking at the code again, it also seems rather obvious now... IIUC, from what Robin described, this means that the "legal thing to do - e.g. buffer sharing between devices" will now immediately trigger the new warning? Not sure if I missed something (again), because then I would expect much more reports on this, and of course it would then obviously be false-positive. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-07 10:59 ` Karsten Graul 2021-10-07 16:40 ` Gerald Schaefer @ 2021-10-11 11:47 ` Christoph Hellwig 1 sibling, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2021-10-11 11:47 UTC (permalink / raw) To: Karsten Graul Cc: Robin Murphy, Gerald Schaefer, Hamza Mahfooz, Christoph Hellwig, Dan Williams, Ioana Ciornei, Jeremy Linton, linux-kernel@vger.kernel.org, Marek Szyprowski, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390 On Thu, Oct 07, 2021 at 12:59:32PM +0200, Karsten Graul wrote: > In our case its really that a buffer is mapped twice for 2 different devices which we use in SMC to provide failover capabilities. We see that -EEXIST is returned when a buffer is mapped for the second device. Since there is a maximum of 2 parallel mappings we never see the warning shown by active_cacheline_inc_overlap() because we don't exceed ACTIVE_CACHELINE_MAX_OVERLAP. Mapping something twice is possible, but needs special care. Basically one device always needs to do the first mapping and the other one needs to use DMA_ATTR_SKIP_CPU_SYNC to opt out of the coherency protocol. So we have two TODO items here: 1) the driver needs to use the above scheme and 2) this dma-debug check needs to understand DMA_ATTR_SKIP_CPU_SYNC. Can I trick you into doing both? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-09-14 15:45 ` Ioana Ciornei 2021-09-30 13:37 ` Karsten Graul @ 2021-10-01 4:19 ` Christoph Hellwig 2021-10-01 9:21 ` Ioana Ciornei 1 sibling, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2021-10-01 4:19 UTC (permalink / raw) To: Ioana Ciornei Cc: Jeremy Linton, Hamza Mahfooz, linux-kernel@vger.kernel.org, Christoph Hellwig, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, Dan Williams, netdev@vger.kernel.org On Tue, Sep 14, 2021 at 03:45:06PM +0000, Ioana Ciornei wrote: > [ 245.927020] fsl_dpaa2_eth dpni.3: scather-gather idx 0 P=20a7320000 N=20a7320 D=20a7320000 L=30 DMA_BIDIRECTIONAL dma map error check not applicable· > [ 245.927048] fsl_dpaa2_eth dpni.3: scather-gather idx 1 P=20a7320030 N=20a7320 D=20a7320030 L=5a8 DMA_BIDIRECTIONAL dma map error check not applicable > [ 245.927062] DMA-API: cacheline tracking EEXIST, overlapping mappings aren't supported > > The first line is the dump of the dma_debug_entry which is already present > in the radix tree and the second one is the entry which just triggered > the EEXIST. > > As we can see, they are not actually overlapping, at least from my > understanding. The first one starts at 0x20a7320000 with a size 0x30 > and the second one at 0x20a7320030. They overlap the cache lines. Which means if you use this driver on a system that is not dma coherent you will corrupt data. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry 2021-10-01 4:19 ` Christoph Hellwig @ 2021-10-01 9:21 ` Ioana Ciornei 0 siblings, 0 replies; 15+ messages in thread From: Ioana Ciornei @ 2021-10-01 9:21 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeremy Linton, Hamza Mahfooz, linux-kernel@vger.kernel.org, Marek Szyprowski, Robin Murphy, iommu@lists.linux-foundation.org, Dan Williams, netdev@vger.kernel.org On Fri, Oct 01, 2021 at 06:19:59AM +0200, Christoph Hellwig wrote: > On Tue, Sep 14, 2021 at 03:45:06PM +0000, Ioana Ciornei wrote: > > [ 245.927020] fsl_dpaa2_eth dpni.3: scather-gather idx 0 P=20a7320000 N=20a7320 D=20a7320000 L=30 DMA_BIDIRECTIONAL dma map error check not applicable· > > [ 245.927048] fsl_dpaa2_eth dpni.3: scather-gather idx 1 P=20a7320030 N=20a7320 D=20a7320030 L=5a8 DMA_BIDIRECTIONAL dma map error check not applicable > > [ 245.927062] DMA-API: cacheline tracking EEXIST, overlapping mappings aren't supported > > > > The first line is the dump of the dma_debug_entry which is already present > > in the radix tree and the second one is the entry which just triggered > > the EEXIST. > > > > As we can see, they are not actually overlapping, at least from my > > understanding. The first one starts at 0x20a7320000 with a size 0x30 > > and the second one at 0x20a7320030. > > They overlap the cache lines. Which means if you use this driver > on a system that is not dma coherent you will corrupt data. This is a driver of an integrated ethernet controller which is DMA coherent. I added a print just to make sure of this: --- a/kernel/dma/debug.c +++ b/kernel/dma/debug.c @@ -567,6 +567,7 @@ static void add_dma_entry(struct dma_debug_entry *entry) pr_err("cacheline tracking ENOMEM, dma-debug disabled\n"); global_disable = true; } else if (rc == -EEXIST) { + pr_err("dev_is_dma_coherent(%s) = %d\n", dev_name(entry->dev), dev_is_dma_coherent(entry->dev)); err_printk(entry->dev, entry, "cacheline tracking EEXIST, overlapping mappings aren't supported\n"); } [ 85.852218] DMA-API: dev_is_dma_coherent(dpni.3) = 1 [ 85.858891] ------------[ cut here ]------------ [ 85.858893] DMA-API: fsl_dpaa2_eth dpni.3: cacheline tracking EEXIST, overlapping mappings aren't supported [ 85.858901] WARNING: CPU: 13 PID: 1046 at kernel/dma/debug.c:571 add_dma_entry+0x330/0x390 [ 85.858911] Modules linked in: [ 85.858915] CPU: 13 PID: 1046 Comm: iperf3 Not tainted 5.15.0-rc2-00478-g34286ba6a164-dirty #1275 [ 85.858919] Hardware name: NXP Layerscape LX2160ARDB (DT) Shouldn't this case not generate this kind of warning? Ioana ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-10-11 11:47 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210518125443.34148-1-someguy@effective-light.com>
2021-09-09 3:33 ` DPAA2 triggers, [PATCH] dma debug: report -EEXIST errors in add_dma_entry Jeremy Linton
2021-09-09 21:16 ` Ioana Ciornei
2021-09-10 10:23 ` Christoph Hellwig
2021-09-14 15:45 ` Ioana Ciornei
2021-09-30 13:37 ` Karsten Graul
2021-10-01 12:52 ` Gerald Schaefer
2021-10-06 13:10 ` Gerald Schaefer
2021-10-06 13:21 ` Gerald Schaefer
2021-10-06 14:23 ` Robin Murphy
2021-10-06 15:06 ` Gerald Schaefer
2021-10-07 10:59 ` Karsten Graul
2021-10-07 16:40 ` Gerald Schaefer
2021-10-11 11:47 ` Christoph Hellwig
2021-10-01 4:19 ` Christoph Hellwig
2021-10-01 9:21 ` Ioana Ciornei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).