* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing [not found] <20200629184712.12449-2-ioana.ciornei () nxp ! com> @ 2020-12-10 17:31 ` Daniel Thompson 2020-12-10 18:06 ` Ioana Ciornei 2020-12-11 9:30 ` David Laight 0 siblings, 2 replies; 11+ messages in thread From: Daniel Thompson @ 2020-12-10 17:31 UTC (permalink / raw) To: Ioana Ciornei; +Cc: linux-netdev, linux-kernel Hi Ioana On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > Instead of realloc-ing the skb on the Tx path when the provided headroom > is smaller than the HW requirements, create a Scatter/Gather frame > descriptor with only one entry. > > Remove the '[drv] tx realloc frames' counter exposed previously through > ethtool since it is no longer used. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- I've been chasing down a networking regression on my LX2160A board (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. It makes the board unreliable opening outbound connections meaning things like `apt update` or `git fetch` often can't open the connection. It does not happen all the time but is sufficient to make the boards built-in networking useless for workstation use. The problem is strongly linked to warnings in the logs so I used the warnings to bisect down to locate the cause of the regression and it pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 that reverting this patch (and fixing up the merge issues) fixes the regression and the warnings stop appearing. A typical example of the warning is below (io-pgtable-arm.c:281 is an error path that I guess would cause dma_map_page_attrs() to return an error): [ 714.464927] WARNING: CPU: 13 PID: 0 at drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) spi_nxp_fspi(E) i2c_imx(E) fixed(E) [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E 5.10.0-rc7-00001-gba98d13279ca #52 [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c [ 714.465211] lr : __arm_lpae_map+0x114/0x30c [ 714.465215] sp : ffff80001006b340 [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 [ 714.465343] Call trace: [ 714.465348] __arm_lpae_map+0x2d4/0x30c [ 714.465353] __arm_lpae_map+0x114/0x30c [ 714.465357] __arm_lpae_map+0x114/0x30c [ 714.465362] __arm_lpae_map+0x114/0x30c [ 714.465366] arm_lpae_map+0xf4/0x180 [ 714.465373] arm_smmu_map+0x4c/0xc0 [ 714.465379] __iommu_map+0x100/0x2bc [ 714.465385] iommu_map_atomic+0x20/0x30 [ 714.465391] __iommu_dma_map+0xb0/0x110 [ 714.465397] iommu_dma_map_page+0xb8/0x120 [ 714.465404] dma_map_page_attrs+0x1a8/0x210 [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 [ 714.465433] sch_direct_xmit+0x1a0/0x550 [ 714.465438] __qdisc_run+0x140/0x670 [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 [ 714.465449] dev_queue_xmit+0x20/0x2c [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] [ 714.465489] __br_forward+0x160/0x1c0 [bridge] [ 714.465502] br_forward+0x13c/0x160 [bridge] [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 [ 714.465531] dev_queue_xmit+0x20/0x2c [ 714.465538] arp_xmit+0xc0/0xd0 [ 714.465544] arp_send_dst+0x78/0xa0 [ 714.465550] arp_solicit+0xf4/0x260 [ 714.465554] neigh_probe+0x64/0xb0 [ 714.465560] neigh_timer_handler+0x2f4/0x400 [ 714.465566] call_timer_fn+0x3c/0x184 [ 714.465572] __run_timers.part.0+0x2bc/0x370 [ 714.465578] run_timer_softirq+0x48/0x80 [ 714.465583] __do_softirq+0x120/0x36c [ 714.465589] irq_exit+0xac/0x100 [ 714.465596] __handle_domain_irq+0x8c/0xf0 [ 714.465600] gic_handle_irq+0xcc/0x14c [ 714.465605] el1_irq+0xc4/0x180 [ 714.465610] arch_cpu_idle+0x18/0x30 [ 714.465617] default_idle_call+0x4c/0x180 [ 714.465623] do_idle+0x238/0x2b0 [ 714.465629] cpu_startup_entry+0x30/0xa0 [ 714.465636] secondary_start_kernel+0x134/0x180 [ 714.465640] ---[ end trace a84a7f61b559005f ]--- Given it is the iommu code that is provoking the warning I should probably mention that the board I have requires arm-smmu.disable_bypass=0 on the kernel command line in order to boot. Also if it matters I am running the latest firmware from Solidrun which is based on LSDK-20.04. Is there any reason for this code not to be working for LX2160A? Daniel. PS A few months have gone by so I decided not to trim the patch out of this reply so you don't have to go digging! > .../freescale/dpaa2/dpaa2-eth-debugfs.c | 7 +- > .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 177 +++++++++++++++--- > .../net/ethernet/freescale/dpaa2/dpaa2-eth.h | 9 +- > .../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 1 - > 4 files changed, 160 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > index 2880ca02d7e7..5cb357c74dec 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > @@ -19,14 +19,14 @@ static int dpaa2_dbg_cpu_show(struct seq_file *file, void *offset) > int i; > > seq_printf(file, "Per-CPU stats for %s\n", priv->net_dev->name); > - seq_printf(file, "%s%16s%16s%16s%16s%16s%16s%16s%16s%16s\n", > + seq_printf(file, "%s%16s%16s%16s%16s%16s%16s%16s%16s\n", > "CPU", "Rx", "Rx Err", "Rx SG", "Tx", "Tx Err", "Tx conf", > - "Tx SG", "Tx realloc", "Enq busy"); > + "Tx SG", "Enq busy"); > > for_each_online_cpu(i) { > stats = per_cpu_ptr(priv->percpu_stats, i); > extras = per_cpu_ptr(priv->percpu_extras, i); > - seq_printf(file, "%3d%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu\n", > + seq_printf(file, "%3d%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu\n", > i, > stats->rx_packets, > stats->rx_errors, > @@ -35,7 +35,6 @@ static int dpaa2_dbg_cpu_show(struct seq_file *file, void *offset) > stats->tx_errors, > extras->tx_conf_frames, > extras->tx_sg_frames, > - extras->tx_reallocs, > extras->tx_portal_busy); > } > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 712bbfdbe7d7..4a264b75c035 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -685,6 +685,86 @@ static int build_sg_fd(struct dpaa2_eth_priv *priv, > return err; > } > > +/* Create a SG frame descriptor based on a linear skb. > + * > + * This function is used on the Tx path when the skb headroom is not large > + * enough for the HW requirements, thus instead of realloc-ing the skb we > + * create a SG frame descriptor with only one entry. > + */ > +static int build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, > + struct sk_buff *skb, > + struct dpaa2_fd *fd) > +{ > + struct device *dev = priv->net_dev->dev.parent; > + struct dpaa2_eth_sgt_cache *sgt_cache; > + struct dpaa2_sg_entry *sgt; > + struct dpaa2_eth_swa *swa; > + dma_addr_t addr, sgt_addr; > + void *sgt_buf = NULL; > + int sgt_buf_size; > + int err; > + > + /* Prepare the HW SGT structure */ > + sgt_cache = this_cpu_ptr(priv->sgt_cache); > + sgt_buf_size = priv->tx_data_offset + sizeof(struct dpaa2_sg_entry); > + > + if (sgt_cache->count == 0) > + sgt_buf = kzalloc(sgt_buf_size + DPAA2_ETH_TX_BUF_ALIGN, > + GFP_ATOMIC); > + else > + sgt_buf = sgt_cache->buf[--sgt_cache->count]; > + if (unlikely(!sgt_buf)) > + return -ENOMEM; > + > + sgt_buf = PTR_ALIGN(sgt_buf, DPAA2_ETH_TX_BUF_ALIGN); > + sgt = (struct dpaa2_sg_entry *)(sgt_buf + priv->tx_data_offset); > + > + addr = dma_map_single(dev, skb->data, skb->len, DMA_BIDIRECTIONAL); > + if (unlikely(dma_mapping_error(dev, addr))) { > + err = -ENOMEM; > + goto data_map_failed; > + } > + > + /* Fill in the HW SGT structure */ > + dpaa2_sg_set_addr(sgt, addr); > + dpaa2_sg_set_len(sgt, skb->len); > + dpaa2_sg_set_final(sgt, true); > + > + /* Store the skb backpointer in the SGT buffer */ > + swa = (struct dpaa2_eth_swa *)sgt_buf; > + swa->type = DPAA2_ETH_SWA_SINGLE; > + swa->single.skb = skb; > + swa->sg.sgt_size = sgt_buf_size; > + > + /* Separately map the SGT buffer */ > + sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); > + if (unlikely(dma_mapping_error(dev, sgt_addr))) { > + err = -ENOMEM; > + goto sgt_map_failed; > + } > + > + dpaa2_fd_set_offset(fd, priv->tx_data_offset); > + dpaa2_fd_set_format(fd, dpaa2_fd_sg); > + dpaa2_fd_set_addr(fd, sgt_addr); > + dpaa2_fd_set_len(fd, skb->len); > + dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA); > + > + if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) > + enable_tx_tstamp(fd, sgt_buf); > + > + return 0; > + > +sgt_map_failed: > + dma_unmap_single(dev, addr, skb->len, DMA_BIDIRECTIONAL); > +data_map_failed: > + if (sgt_cache->count >= DPAA2_ETH_SGT_CACHE_SIZE) > + kfree(sgt_buf); > + else > + sgt_cache->buf[sgt_cache->count++] = sgt_buf; > + > + return err; > +} > + > /* Create a frame descriptor based on a linear skb */ > static int build_single_fd(struct dpaa2_eth_priv *priv, > struct sk_buff *skb, > @@ -743,13 +823,16 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > const struct dpaa2_fd *fd, bool in_napi) > { > struct device *dev = priv->net_dev->dev.parent; > - dma_addr_t fd_addr; > + dma_addr_t fd_addr, sg_addr; > struct sk_buff *skb = NULL; > unsigned char *buffer_start; > struct dpaa2_eth_swa *swa; > u8 fd_format = dpaa2_fd_get_format(fd); > u32 fd_len = dpaa2_fd_get_len(fd); > > + struct dpaa2_eth_sgt_cache *sgt_cache; > + struct dpaa2_sg_entry *sgt; > + > fd_addr = dpaa2_fd_get_addr(fd); > buffer_start = dpaa2_iova_to_virt(priv->iommu_domain, fd_addr); > swa = (struct dpaa2_eth_swa *)buffer_start; > @@ -769,16 +852,29 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > DMA_BIDIRECTIONAL); > } > } else if (fd_format == dpaa2_fd_sg) { > - skb = swa->sg.skb; > + if (swa->type == DPAA2_ETH_SWA_SG) { > + skb = swa->sg.skb; > + > + /* Unmap the scatterlist */ > + dma_unmap_sg(dev, swa->sg.scl, swa->sg.num_sg, > + DMA_BIDIRECTIONAL); > + kfree(swa->sg.scl); > > - /* Unmap the scatterlist */ > - dma_unmap_sg(dev, swa->sg.scl, swa->sg.num_sg, > - DMA_BIDIRECTIONAL); > - kfree(swa->sg.scl); > + /* Unmap the SGT buffer */ > + dma_unmap_single(dev, fd_addr, swa->sg.sgt_size, > + DMA_BIDIRECTIONAL); > + } else { > + skb = swa->single.skb; > > - /* Unmap the SGT buffer */ > - dma_unmap_single(dev, fd_addr, swa->sg.sgt_size, > - DMA_BIDIRECTIONAL); > + /* Unmap the SGT Buffer */ > + dma_unmap_single(dev, fd_addr, swa->single.sgt_size, > + DMA_BIDIRECTIONAL); > + > + sgt = (struct dpaa2_sg_entry *)(buffer_start + > + priv->tx_data_offset); > + sg_addr = dpaa2_sg_get_addr(sgt); > + dma_unmap_single(dev, sg_addr, skb->len, DMA_BIDIRECTIONAL); > + } > } else { > netdev_dbg(priv->net_dev, "Invalid FD format\n"); > return; > @@ -808,8 +904,17 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > } > > /* Free SGT buffer allocated on tx */ > - if (fd_format != dpaa2_fd_single) > - skb_free_frag(buffer_start); > + if (fd_format != dpaa2_fd_single) { > + sgt_cache = this_cpu_ptr(priv->sgt_cache); > + if (swa->type == DPAA2_ETH_SWA_SG) { > + skb_free_frag(buffer_start); > + } else { > + if (sgt_cache->count >= DPAA2_ETH_SGT_CACHE_SIZE) > + kfree(buffer_start); > + else > + sgt_cache->buf[sgt_cache->count++] = buffer_start; > + } > + } > > /* Move on with skb release */ > napi_consume_skb(skb, in_napi); > @@ -833,22 +938,6 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > percpu_extras = this_cpu_ptr(priv->percpu_extras); > > needed_headroom = dpaa2_eth_needed_headroom(priv, skb); > - if (skb_headroom(skb) < needed_headroom) { > - struct sk_buff *ns; > - > - ns = skb_realloc_headroom(skb, needed_headroom); > - if (unlikely(!ns)) { > - percpu_stats->tx_dropped++; > - goto err_alloc_headroom; > - } > - percpu_extras->tx_reallocs++; > - > - if (skb->sk) > - skb_set_owner_w(ns, skb->sk); > - > - dev_kfree_skb(skb); > - skb = ns; > - } > > /* We'll be holding a back-reference to the skb until Tx Confirmation; > * we don't want that overwritten by a concurrent Tx with a cloned skb. > @@ -867,6 +956,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > err = build_sg_fd(priv, skb, &fd); > percpu_extras->tx_sg_frames++; > percpu_extras->tx_sg_bytes += skb->len; > + } else if (skb_headroom(skb) < needed_headroom) { > + err = build_sg_fd_single_buf(priv, skb, &fd); > + percpu_extras->tx_sg_frames++; > + percpu_extras->tx_sg_bytes += skb->len; > } else { > err = build_single_fd(priv, skb, &fd); > } > @@ -924,7 +1017,6 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > return NETDEV_TX_OK; > > err_build_fd: > -err_alloc_headroom: > dev_kfree_skb(skb); > > return NETDEV_TX_OK; > @@ -1161,6 +1253,22 @@ static int refill_pool(struct dpaa2_eth_priv *priv, > return 0; > } > > +static void dpaa2_eth_sgt_cache_drain(struct dpaa2_eth_priv *priv) > +{ > + struct dpaa2_eth_sgt_cache *sgt_cache; > + u16 count; > + int k, i; > + > + for_each_online_cpu(k) { > + sgt_cache = per_cpu_ptr(priv->sgt_cache, k); > + count = sgt_cache->count; > + > + for (i = 0; i < count; i++) > + kfree(sgt_cache->buf[i]); > + sgt_cache->count = 0; > + } > +} > + > static int pull_channel(struct dpaa2_eth_channel *ch) > { > int err; > @@ -1562,6 +1670,9 @@ static int dpaa2_eth_stop(struct net_device *net_dev) > /* Empty the buffer pool */ > drain_pool(priv); > > + /* Empty the Scatter-Gather Buffer cache */ > + dpaa2_eth_sgt_cache_drain(priv); > + > return 0; > } > > @@ -3846,6 +3957,13 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) > goto err_alloc_percpu_extras; > } > > + priv->sgt_cache = alloc_percpu(*priv->sgt_cache); > + if (!priv->sgt_cache) { > + dev_err(dev, "alloc_percpu(sgt_cache) failed\n"); > + err = -ENOMEM; > + goto err_alloc_sgt_cache; > + } > + > err = netdev_init(net_dev); > if (err) > goto err_netdev_init; > @@ -3914,6 +4032,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) > err_alloc_rings: > err_csum: > err_netdev_init: > + free_percpu(priv->sgt_cache); > +err_alloc_sgt_cache: > free_percpu(priv->percpu_extras); > err_alloc_percpu_extras: > free_percpu(priv->percpu_stats); > @@ -3959,6 +4079,7 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev) > fsl_mc_free_irqs(ls_dev); > > free_rings(priv); > + free_percpu(priv->sgt_cache); > free_percpu(priv->percpu_stats); > free_percpu(priv->percpu_extras); > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > index 2d7ada0f0dbd..9e4ceb92f240 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > @@ -125,6 +125,7 @@ struct dpaa2_eth_swa { > union { > struct { > struct sk_buff *skb; > + int sgt_size; > } single; > struct { > struct sk_buff *skb; > @@ -282,7 +283,6 @@ struct dpaa2_eth_drv_stats { > __u64 tx_conf_bytes; > __u64 tx_sg_frames; > __u64 tx_sg_bytes; > - __u64 tx_reallocs; > __u64 rx_sg_frames; > __u64 rx_sg_bytes; > /* Enqueues retried due to portal busy */ > @@ -395,6 +395,12 @@ struct dpaa2_eth_cls_rule { > u8 in_use; > }; > > +#define DPAA2_ETH_SGT_CACHE_SIZE 256 > +struct dpaa2_eth_sgt_cache { > + void *buf[DPAA2_ETH_SGT_CACHE_SIZE]; > + u16 count; > +}; > + > /* Driver private data */ > struct dpaa2_eth_priv { > struct net_device *net_dev; > @@ -409,6 +415,7 @@ struct dpaa2_eth_priv { > > u8 num_channels; > struct dpaa2_eth_channel *channel[DPAA2_ETH_MAX_DPCONS]; > + struct dpaa2_eth_sgt_cache __percpu *sgt_cache; > > struct dpni_attr dpni_attrs; > u16 dpni_ver_major; > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > index e88269fe3de7..c4cbbcaa9a3f 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > @@ -43,7 +43,6 @@ static char dpaa2_ethtool_extras[][ETH_GSTRING_LEN] = { > "[drv] tx conf bytes", > "[drv] tx sg frames", > "[drv] tx sg bytes", > - "[drv] tx realloc frames", > "[drv] rx sg frames", > "[drv] rx sg bytes", > "[drv] enqueue portal busy", > -- > 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-10 17:31 ` [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing Daniel Thompson @ 2020-12-10 18:06 ` Ioana Ciornei 2020-12-11 7:21 ` Jon Nettleton 2020-12-11 14:01 ` Ioana Ciornei 2020-12-11 9:30 ` David Laight 1 sibling, 2 replies; 11+ messages in thread From: Ioana Ciornei @ 2020-12-10 18:06 UTC (permalink / raw) To: Daniel Thompson; +Cc: linux-netdev, linux-kernel, netdev [Added also the netdev mailing list, I haven't heard of linux-netdev before but kept it] On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > Hi Ioana Hi Daniel, > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > is smaller than the HW requirements, create a Scatter/Gather frame > > descriptor with only one entry. > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > ethtool since it is no longer used. > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > --- > > I've been chasing down a networking regression on my LX2160A board > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > It makes the board unreliable opening outbound connections meaning > things like `apt update` or `git fetch` often can't open the connection. > It does not happen all the time but is sufficient to make the boards > built-in networking useless for workstation use. > > The problem is strongly linked to warnings in the logs so I used the > warnings to bisect down to locate the cause of the regression and it > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > that reverting this patch (and fixing up the merge issues) fixes the > regression and the warnings stop appearing. > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > error path that I guess would cause dma_map_page_attrs() to return > an error): > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > 5.10.0-rc7-00001-gba98d13279ca #52 > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > [ 714.465215] sp : ffff80001006b340 > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > [ 714.465343] Call trace: > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > [ 714.465353] __arm_lpae_map+0x114/0x30c > [ 714.465357] __arm_lpae_map+0x114/0x30c > [ 714.465362] __arm_lpae_map+0x114/0x30c > [ 714.465366] arm_lpae_map+0xf4/0x180 > [ 714.465373] arm_smmu_map+0x4c/0xc0 > [ 714.465379] __iommu_map+0x100/0x2bc > [ 714.465385] iommu_map_atomic+0x20/0x30 > [ 714.465391] __iommu_dma_map+0xb0/0x110 > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > [ 714.465438] __qdisc_run+0x140/0x670 > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > [ 714.465449] dev_queue_xmit+0x20/0x2c > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > [ 714.465502] br_forward+0x13c/0x160 [bridge] > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > [ 714.465531] dev_queue_xmit+0x20/0x2c > [ 714.465538] arp_xmit+0xc0/0xd0 > [ 714.465544] arp_send_dst+0x78/0xa0 > [ 714.465550] arp_solicit+0xf4/0x260 > [ 714.465554] neigh_probe+0x64/0xb0 > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > [ 714.465566] call_timer_fn+0x3c/0x184 > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > [ 714.465578] run_timer_softirq+0x48/0x80 > [ 714.465583] __do_softirq+0x120/0x36c > [ 714.465589] irq_exit+0xac/0x100 > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > [ 714.465600] gic_handle_irq+0xcc/0x14c > [ 714.465605] el1_irq+0xc4/0x180 > [ 714.465610] arch_cpu_idle+0x18/0x30 > [ 714.465617] default_idle_call+0x4c/0x180 > [ 714.465623] do_idle+0x238/0x2b0 > [ 714.465629] cpu_startup_entry+0x30/0xa0 > [ 714.465636] secondary_start_kernel+0x134/0x180 > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > Given it is the iommu code that is provoking the warning I should > probably mention that the board I have requires > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > Also if it matters I am running the latest firmware from Solidrun > which is based on LSDK-20.04. > Hmmm, from what I remember I think I tested this with the smmu bypassed so that is why I didn't catch it. > Is there any reason for this code not to be working for LX2160A? I wouldn't expect this to be LX2160A specific but rather a bug in the implementation.. sorry. Let me reproduce it and see if I can get to the bottom of it and I will get back with some more info. Ioana > > Daniel. > > > PS A few months have gone by so I decided not to trim the patch out > of this reply so you don't have to go digging! > > > > > .../freescale/dpaa2/dpaa2-eth-debugfs.c | 7 +- > > .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 177 +++++++++++++++--- > > .../net/ethernet/freescale/dpaa2/dpaa2-eth.h | 9 +- > > .../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 1 - > > 4 files changed, 160 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > > index 2880ca02d7e7..5cb357c74dec 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > > @@ -19,14 +19,14 @@ static int dpaa2_dbg_cpu_show(struct seq_file *file, void *offset) > > int i; > > > > seq_printf(file, "Per-CPU stats for %s\n", priv->net_dev->name); > > - seq_printf(file, "%s%16s%16s%16s%16s%16s%16s%16s%16s%16s\n", > > + seq_printf(file, "%s%16s%16s%16s%16s%16s%16s%16s%16s\n", > > "CPU", "Rx", "Rx Err", "Rx SG", "Tx", "Tx Err", "Tx conf", > > - "Tx SG", "Tx realloc", "Enq busy"); > > + "Tx SG", "Enq busy"); > > > > for_each_online_cpu(i) { > > stats = per_cpu_ptr(priv->percpu_stats, i); > > extras = per_cpu_ptr(priv->percpu_extras, i); > > - seq_printf(file, "%3d%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu\n", > > + seq_printf(file, "%3d%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu\n", > > i, > > stats->rx_packets, > > stats->rx_errors, > > @@ -35,7 +35,6 @@ static int dpaa2_dbg_cpu_show(struct seq_file *file, void *offset) > > stats->tx_errors, > > extras->tx_conf_frames, > > extras->tx_sg_frames, > > - extras->tx_reallocs, > > extras->tx_portal_busy); > > } > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > index 712bbfdbe7d7..4a264b75c035 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -685,6 +685,86 @@ static int build_sg_fd(struct dpaa2_eth_priv *priv, > > return err; > > } > > > > +/* Create a SG frame descriptor based on a linear skb. > > + * > > + * This function is used on the Tx path when the skb headroom is not large > > + * enough for the HW requirements, thus instead of realloc-ing the skb we > > + * create a SG frame descriptor with only one entry. > > + */ > > +static int build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, > > + struct sk_buff *skb, > > + struct dpaa2_fd *fd) > > +{ > > + struct device *dev = priv->net_dev->dev.parent; > > + struct dpaa2_eth_sgt_cache *sgt_cache; > > + struct dpaa2_sg_entry *sgt; > > + struct dpaa2_eth_swa *swa; > > + dma_addr_t addr, sgt_addr; > > + void *sgt_buf = NULL; > > + int sgt_buf_size; > > + int err; > > + > > + /* Prepare the HW SGT structure */ > > + sgt_cache = this_cpu_ptr(priv->sgt_cache); > > + sgt_buf_size = priv->tx_data_offset + sizeof(struct dpaa2_sg_entry); > > + > > + if (sgt_cache->count == 0) > > + sgt_buf = kzalloc(sgt_buf_size + DPAA2_ETH_TX_BUF_ALIGN, > > + GFP_ATOMIC); > > + else > > + sgt_buf = sgt_cache->buf[--sgt_cache->count]; > > + if (unlikely(!sgt_buf)) > > + return -ENOMEM; > > + > > + sgt_buf = PTR_ALIGN(sgt_buf, DPAA2_ETH_TX_BUF_ALIGN); > > + sgt = (struct dpaa2_sg_entry *)(sgt_buf + priv->tx_data_offset); > > + > > + addr = dma_map_single(dev, skb->data, skb->len, DMA_BIDIRECTIONAL); > > + if (unlikely(dma_mapping_error(dev, addr))) { > > + err = -ENOMEM; > > + goto data_map_failed; > > + } > > + > > + /* Fill in the HW SGT structure */ > > + dpaa2_sg_set_addr(sgt, addr); > > + dpaa2_sg_set_len(sgt, skb->len); > > + dpaa2_sg_set_final(sgt, true); > > + > > + /* Store the skb backpointer in the SGT buffer */ > > + swa = (struct dpaa2_eth_swa *)sgt_buf; > > + swa->type = DPAA2_ETH_SWA_SINGLE; > > + swa->single.skb = skb; > > + swa->sg.sgt_size = sgt_buf_size; > > + > > + /* Separately map the SGT buffer */ > > + sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); > > + if (unlikely(dma_mapping_error(dev, sgt_addr))) { > > + err = -ENOMEM; > > + goto sgt_map_failed; > > + } > > + > > + dpaa2_fd_set_offset(fd, priv->tx_data_offset); > > + dpaa2_fd_set_format(fd, dpaa2_fd_sg); > > + dpaa2_fd_set_addr(fd, sgt_addr); > > + dpaa2_fd_set_len(fd, skb->len); > > + dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA); > > + > > + if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) > > + enable_tx_tstamp(fd, sgt_buf); > > + > > + return 0; > > + > > +sgt_map_failed: > > + dma_unmap_single(dev, addr, skb->len, DMA_BIDIRECTIONAL); > > +data_map_failed: > > + if (sgt_cache->count >= DPAA2_ETH_SGT_CACHE_SIZE) > > + kfree(sgt_buf); > > + else > > + sgt_cache->buf[sgt_cache->count++] = sgt_buf; > > + > > + return err; > > +} > > + > > /* Create a frame descriptor based on a linear skb */ > > static int build_single_fd(struct dpaa2_eth_priv *priv, > > struct sk_buff *skb, > > @@ -743,13 +823,16 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > > const struct dpaa2_fd *fd, bool in_napi) > > { > > struct device *dev = priv->net_dev->dev.parent; > > - dma_addr_t fd_addr; > > + dma_addr_t fd_addr, sg_addr; > > struct sk_buff *skb = NULL; > > unsigned char *buffer_start; > > struct dpaa2_eth_swa *swa; > > u8 fd_format = dpaa2_fd_get_format(fd); > > u32 fd_len = dpaa2_fd_get_len(fd); > > > > + struct dpaa2_eth_sgt_cache *sgt_cache; > > + struct dpaa2_sg_entry *sgt; > > + > > fd_addr = dpaa2_fd_get_addr(fd); > > buffer_start = dpaa2_iova_to_virt(priv->iommu_domain, fd_addr); > > swa = (struct dpaa2_eth_swa *)buffer_start; > > @@ -769,16 +852,29 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > > DMA_BIDIRECTIONAL); > > } > > } else if (fd_format == dpaa2_fd_sg) { > > - skb = swa->sg.skb; > > + if (swa->type == DPAA2_ETH_SWA_SG) { > > + skb = swa->sg.skb; > > + > > + /* Unmap the scatterlist */ > > + dma_unmap_sg(dev, swa->sg.scl, swa->sg.num_sg, > > + DMA_BIDIRECTIONAL); > > + kfree(swa->sg.scl); > > > > - /* Unmap the scatterlist */ > > - dma_unmap_sg(dev, swa->sg.scl, swa->sg.num_sg, > > - DMA_BIDIRECTIONAL); > > - kfree(swa->sg.scl); > > + /* Unmap the SGT buffer */ > > + dma_unmap_single(dev, fd_addr, swa->sg.sgt_size, > > + DMA_BIDIRECTIONAL); > > + } else { > > + skb = swa->single.skb; > > > > - /* Unmap the SGT buffer */ > > - dma_unmap_single(dev, fd_addr, swa->sg.sgt_size, > > - DMA_BIDIRECTIONAL); > > + /* Unmap the SGT Buffer */ > > + dma_unmap_single(dev, fd_addr, swa->single.sgt_size, > > + DMA_BIDIRECTIONAL); > > + > > + sgt = (struct dpaa2_sg_entry *)(buffer_start + > > + priv->tx_data_offset); > > + sg_addr = dpaa2_sg_get_addr(sgt); > > + dma_unmap_single(dev, sg_addr, skb->len, DMA_BIDIRECTIONAL); > > + } > > } else { > > netdev_dbg(priv->net_dev, "Invalid FD format\n"); > > return; > > @@ -808,8 +904,17 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > > } > > > > /* Free SGT buffer allocated on tx */ > > - if (fd_format != dpaa2_fd_single) > > - skb_free_frag(buffer_start); > > + if (fd_format != dpaa2_fd_single) { > > + sgt_cache = this_cpu_ptr(priv->sgt_cache); > > + if (swa->type == DPAA2_ETH_SWA_SG) { > > + skb_free_frag(buffer_start); > > + } else { > > + if (sgt_cache->count >= DPAA2_ETH_SGT_CACHE_SIZE) > > + kfree(buffer_start); > > + else > > + sgt_cache->buf[sgt_cache->count++] = buffer_start; > > + } > > + } > > > > /* Move on with skb release */ > > napi_consume_skb(skb, in_napi); > > @@ -833,22 +938,6 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > > percpu_extras = this_cpu_ptr(priv->percpu_extras); > > > > needed_headroom = dpaa2_eth_needed_headroom(priv, skb); > > - if (skb_headroom(skb) < needed_headroom) { > > - struct sk_buff *ns; > > - > > - ns = skb_realloc_headroom(skb, needed_headroom); > > - if (unlikely(!ns)) { > > - percpu_stats->tx_dropped++; > > - goto err_alloc_headroom; > > - } > > - percpu_extras->tx_reallocs++; > > - > > - if (skb->sk) > > - skb_set_owner_w(ns, skb->sk); > > - > > - dev_kfree_skb(skb); > > - skb = ns; > > - } > > > > /* We'll be holding a back-reference to the skb until Tx Confirmation; > > * we don't want that overwritten by a concurrent Tx with a cloned skb. > > @@ -867,6 +956,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > > err = build_sg_fd(priv, skb, &fd); > > percpu_extras->tx_sg_frames++; > > percpu_extras->tx_sg_bytes += skb->len; > > + } else if (skb_headroom(skb) < needed_headroom) { > > + err = build_sg_fd_single_buf(priv, skb, &fd); > > + percpu_extras->tx_sg_frames++; > > + percpu_extras->tx_sg_bytes += skb->len; > > } else { > > err = build_single_fd(priv, skb, &fd); > > } > > @@ -924,7 +1017,6 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > > return NETDEV_TX_OK; > > > > err_build_fd: > > -err_alloc_headroom: > > dev_kfree_skb(skb); > > > > return NETDEV_TX_OK; > > @@ -1161,6 +1253,22 @@ static int refill_pool(struct dpaa2_eth_priv *priv, > > return 0; > > } > > > > +static void dpaa2_eth_sgt_cache_drain(struct dpaa2_eth_priv *priv) > > +{ > > + struct dpaa2_eth_sgt_cache *sgt_cache; > > + u16 count; > > + int k, i; > > + > > + for_each_online_cpu(k) { > > + sgt_cache = per_cpu_ptr(priv->sgt_cache, k); > > + count = sgt_cache->count; > > + > > + for (i = 0; i < count; i++) > > + kfree(sgt_cache->buf[i]); > > + sgt_cache->count = 0; > > + } > > +} > > + > > static int pull_channel(struct dpaa2_eth_channel *ch) > > { > > int err; > > @@ -1562,6 +1670,9 @@ static int dpaa2_eth_stop(struct net_device *net_dev) > > /* Empty the buffer pool */ > > drain_pool(priv); > > > > + /* Empty the Scatter-Gather Buffer cache */ > > + dpaa2_eth_sgt_cache_drain(priv); > > + > > return 0; > > } > > > > @@ -3846,6 +3957,13 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) > > goto err_alloc_percpu_extras; > > } > > > > + priv->sgt_cache = alloc_percpu(*priv->sgt_cache); > > + if (!priv->sgt_cache) { > > + dev_err(dev, "alloc_percpu(sgt_cache) failed\n"); > > + err = -ENOMEM; > > + goto err_alloc_sgt_cache; > > + } > > + > > err = netdev_init(net_dev); > > if (err) > > goto err_netdev_init; > > @@ -3914,6 +4032,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) > > err_alloc_rings: > > err_csum: > > err_netdev_init: > > + free_percpu(priv->sgt_cache); > > +err_alloc_sgt_cache: > > free_percpu(priv->percpu_extras); > > err_alloc_percpu_extras: > > free_percpu(priv->percpu_stats); > > @@ -3959,6 +4079,7 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev) > > fsl_mc_free_irqs(ls_dev); > > > > free_rings(priv); > > + free_percpu(priv->sgt_cache); > > free_percpu(priv->percpu_stats); > > free_percpu(priv->percpu_extras); > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > index 2d7ada0f0dbd..9e4ceb92f240 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > @@ -125,6 +125,7 @@ struct dpaa2_eth_swa { > > union { > > struct { > > struct sk_buff *skb; > > + int sgt_size; > > } single; > > struct { > > struct sk_buff *skb; > > @@ -282,7 +283,6 @@ struct dpaa2_eth_drv_stats { > > __u64 tx_conf_bytes; > > __u64 tx_sg_frames; > > __u64 tx_sg_bytes; > > - __u64 tx_reallocs; > > __u64 rx_sg_frames; > > __u64 rx_sg_bytes; > > /* Enqueues retried due to portal busy */ > > @@ -395,6 +395,12 @@ struct dpaa2_eth_cls_rule { > > u8 in_use; > > }; > > > > +#define DPAA2_ETH_SGT_CACHE_SIZE 256 > > +struct dpaa2_eth_sgt_cache { > > + void *buf[DPAA2_ETH_SGT_CACHE_SIZE]; > > + u16 count; > > +}; > > + > > /* Driver private data */ > > struct dpaa2_eth_priv { > > struct net_device *net_dev; > > @@ -409,6 +415,7 @@ struct dpaa2_eth_priv { > > > > u8 num_channels; > > struct dpaa2_eth_channel *channel[DPAA2_ETH_MAX_DPCONS]; > > + struct dpaa2_eth_sgt_cache __percpu *sgt_cache; > > > > struct dpni_attr dpni_attrs; > > u16 dpni_ver_major; > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > > index e88269fe3de7..c4cbbcaa9a3f 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > > @@ -43,7 +43,6 @@ static char dpaa2_ethtool_extras[][ETH_GSTRING_LEN] = { > > "[drv] tx conf bytes", > > "[drv] tx sg frames", > > "[drv] tx sg bytes", > > - "[drv] tx realloc frames", > > "[drv] rx sg frames", > > "[drv] rx sg bytes", > > "[drv] enqueue portal busy", > > -- > > 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-10 18:06 ` Ioana Ciornei @ 2020-12-11 7:21 ` Jon Nettleton 2020-12-11 14:01 ` Ioana Ciornei 1 sibling, 0 replies; 11+ messages in thread From: Jon Nettleton @ 2020-12-11 7:21 UTC (permalink / raw) To: Ioana Ciornei; +Cc: Daniel Thompson, linux-netdev, linux-kernel, netdev On Thu, Dec 10, 2020 at 7:08 PM Ioana Ciornei <ioana.ciornei@nxp.com> wrote: > > [Added also the netdev mailing list, I haven't heard of linux-netdev > before but kept it] > > On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > > Hi Ioana > > Hi Daniel, > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > descriptor with only one entry. > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > ethtool since it is no longer used. > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > --- > > > > I've been chasing down a networking regression on my LX2160A board > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > It makes the board unreliable opening outbound connections meaning > > things like `apt update` or `git fetch` often can't open the connection. > > It does not happen all the time but is sufficient to make the boards > > built-in networking useless for workstation use. > > > > The problem is strongly linked to warnings in the logs so I used the > > warnings to bisect down to locate the cause of the regression and it > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > that reverting this patch (and fixing up the merge issues) fixes the > > regression and the warnings stop appearing. > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > error path that I guess would cause dma_map_page_attrs() to return > > an error): > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > > 5.10.0-rc7-00001-gba98d13279ca #52 > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > [ 714.465215] sp : ffff80001006b340 > > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > > [ 714.465343] Call trace: > > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > > [ 714.465353] __arm_lpae_map+0x114/0x30c > > [ 714.465357] __arm_lpae_map+0x114/0x30c > > [ 714.465362] __arm_lpae_map+0x114/0x30c > > [ 714.465366] arm_lpae_map+0xf4/0x180 > > [ 714.465373] arm_smmu_map+0x4c/0xc0 > > [ 714.465379] __iommu_map+0x100/0x2bc > > [ 714.465385] iommu_map_atomic+0x20/0x30 > > [ 714.465391] __iommu_dma_map+0xb0/0x110 > > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > > [ 714.465438] __qdisc_run+0x140/0x670 > > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > > [ 714.465449] dev_queue_xmit+0x20/0x2c > > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > > [ 714.465502] br_forward+0x13c/0x160 [bridge] > > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > > [ 714.465531] dev_queue_xmit+0x20/0x2c > > [ 714.465538] arp_xmit+0xc0/0xd0 > > [ 714.465544] arp_send_dst+0x78/0xa0 > > [ 714.465550] arp_solicit+0xf4/0x260 > > [ 714.465554] neigh_probe+0x64/0xb0 > > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > > [ 714.465566] call_timer_fn+0x3c/0x184 > > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > > [ 714.465578] run_timer_softirq+0x48/0x80 > > [ 714.465583] __do_softirq+0x120/0x36c > > [ 714.465589] irq_exit+0xac/0x100 > > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > > [ 714.465600] gic_handle_irq+0xcc/0x14c > > [ 714.465605] el1_irq+0xc4/0x180 > > [ 714.465610] arch_cpu_idle+0x18/0x30 > > [ 714.465617] default_idle_call+0x4c/0x180 > > [ 714.465623] do_idle+0x238/0x2b0 > > [ 714.465629] cpu_startup_entry+0x30/0xa0 > > [ 714.465636] secondary_start_kernel+0x134/0x180 > > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > > > > Given it is the iommu code that is provoking the warning I should > > probably mention that the board I have requires > > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > > Also if it matters I am running the latest firmware from Solidrun > > which is based on LSDK-20.04. > > > > Hmmm, from what I remember I think I tested this with the smmu bypassed > so that is why I didn't catch it. > > > Is there any reason for this code not to be working for LX2160A? > > I wouldn't expect this to be LX2160A specific but rather a bug in the > implementation.. sorry. > > Let me reproduce it and see if I can get to the bottom of it and I will > get back with some more info. > > Ioana Ioana, I reported this issue to Calvin last week. I can verify that reverting that change also fixes the issue for me. -Jon > > > > > Daniel. > > > > > > PS A few months have gone by so I decided not to trim the patch out > > of this reply so you don't have to go digging! > > > > > > > > > .../freescale/dpaa2/dpaa2-eth-debugfs.c | 7 +- > > > .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 177 +++++++++++++++--- > > > .../net/ethernet/freescale/dpaa2/dpaa2-eth.h | 9 +- > > > .../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 1 - > > > 4 files changed, 160 insertions(+), 34 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > > > index 2880ca02d7e7..5cb357c74dec 100644 > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-debugfs.c > > > @@ -19,14 +19,14 @@ static int dpaa2_dbg_cpu_show(struct seq_file *file, void *offset) > > > int i; > > > > > > seq_printf(file, "Per-CPU stats for %s\n", priv->net_dev->name); > > > - seq_printf(file, "%s%16s%16s%16s%16s%16s%16s%16s%16s%16s\n", > > > + seq_printf(file, "%s%16s%16s%16s%16s%16s%16s%16s%16s\n", > > > "CPU", "Rx", "Rx Err", "Rx SG", "Tx", "Tx Err", "Tx conf", > > > - "Tx SG", "Tx realloc", "Enq busy"); > > > + "Tx SG", "Enq busy"); > > > > > > for_each_online_cpu(i) { > > > stats = per_cpu_ptr(priv->percpu_stats, i); > > > extras = per_cpu_ptr(priv->percpu_extras, i); > > > - seq_printf(file, "%3d%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu\n", > > > + seq_printf(file, "%3d%16llu%16llu%16llu%16llu%16llu%16llu%16llu%16llu\n", > > > i, > > > stats->rx_packets, > > > stats->rx_errors, > > > @@ -35,7 +35,6 @@ static int dpaa2_dbg_cpu_show(struct seq_file *file, void *offset) > > > stats->tx_errors, > > > extras->tx_conf_frames, > > > extras->tx_sg_frames, > > > - extras->tx_reallocs, > > > extras->tx_portal_busy); > > > } > > > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > index 712bbfdbe7d7..4a264b75c035 100644 > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > @@ -685,6 +685,86 @@ static int build_sg_fd(struct dpaa2_eth_priv *priv, > > > return err; > > > } > > > > > > +/* Create a SG frame descriptor based on a linear skb. > > > + * > > > + * This function is used on the Tx path when the skb headroom is not large > > > + * enough for the HW requirements, thus instead of realloc-ing the skb we > > > + * create a SG frame descriptor with only one entry. > > > + */ > > > +static int build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, > > > + struct sk_buff *skb, > > > + struct dpaa2_fd *fd) > > > +{ > > > + struct device *dev = priv->net_dev->dev.parent; > > > + struct dpaa2_eth_sgt_cache *sgt_cache; > > > + struct dpaa2_sg_entry *sgt; > > > + struct dpaa2_eth_swa *swa; > > > + dma_addr_t addr, sgt_addr; > > > + void *sgt_buf = NULL; > > > + int sgt_buf_size; > > > + int err; > > > + > > > + /* Prepare the HW SGT structure */ > > > + sgt_cache = this_cpu_ptr(priv->sgt_cache); > > > + sgt_buf_size = priv->tx_data_offset + sizeof(struct dpaa2_sg_entry); > > > + > > > + if (sgt_cache->count == 0) > > > + sgt_buf = kzalloc(sgt_buf_size + DPAA2_ETH_TX_BUF_ALIGN, > > > + GFP_ATOMIC); > > > + else > > > + sgt_buf = sgt_cache->buf[--sgt_cache->count]; > > > + if (unlikely(!sgt_buf)) > > > + return -ENOMEM; > > > + > > > + sgt_buf = PTR_ALIGN(sgt_buf, DPAA2_ETH_TX_BUF_ALIGN); > > > + sgt = (struct dpaa2_sg_entry *)(sgt_buf + priv->tx_data_offset); > > > + > > > + addr = dma_map_single(dev, skb->data, skb->len, DMA_BIDIRECTIONAL); > > > + if (unlikely(dma_mapping_error(dev, addr))) { > > > + err = -ENOMEM; > > > + goto data_map_failed; > > > + } > > > + > > > + /* Fill in the HW SGT structure */ > > > + dpaa2_sg_set_addr(sgt, addr); > > > + dpaa2_sg_set_len(sgt, skb->len); > > > + dpaa2_sg_set_final(sgt, true); > > > + > > > + /* Store the skb backpointer in the SGT buffer */ > > > + swa = (struct dpaa2_eth_swa *)sgt_buf; > > > + swa->type = DPAA2_ETH_SWA_SINGLE; > > > + swa->single.skb = skb; > > > + swa->sg.sgt_size = sgt_buf_size; > > > + > > > + /* Separately map the SGT buffer */ > > > + sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); > > > + if (unlikely(dma_mapping_error(dev, sgt_addr))) { > > > + err = -ENOMEM; > > > + goto sgt_map_failed; > > > + } > > > + > > > + dpaa2_fd_set_offset(fd, priv->tx_data_offset); > > > + dpaa2_fd_set_format(fd, dpaa2_fd_sg); > > > + dpaa2_fd_set_addr(fd, sgt_addr); > > > + dpaa2_fd_set_len(fd, skb->len); > > > + dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA); > > > + > > > + if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) > > > + enable_tx_tstamp(fd, sgt_buf); > > > + > > > + return 0; > > > + > > > +sgt_map_failed: > > > + dma_unmap_single(dev, addr, skb->len, DMA_BIDIRECTIONAL); > > > +data_map_failed: > > > + if (sgt_cache->count >= DPAA2_ETH_SGT_CACHE_SIZE) > > > + kfree(sgt_buf); > > > + else > > > + sgt_cache->buf[sgt_cache->count++] = sgt_buf; > > > + > > > + return err; > > > +} > > > + > > > /* Create a frame descriptor based on a linear skb */ > > > static int build_single_fd(struct dpaa2_eth_priv *priv, > > > struct sk_buff *skb, > > > @@ -743,13 +823,16 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > > > const struct dpaa2_fd *fd, bool in_napi) > > > { > > > struct device *dev = priv->net_dev->dev.parent; > > > - dma_addr_t fd_addr; > > > + dma_addr_t fd_addr, sg_addr; > > > struct sk_buff *skb = NULL; > > > unsigned char *buffer_start; > > > struct dpaa2_eth_swa *swa; > > > u8 fd_format = dpaa2_fd_get_format(fd); > > > u32 fd_len = dpaa2_fd_get_len(fd); > > > > > > + struct dpaa2_eth_sgt_cache *sgt_cache; > > > + struct dpaa2_sg_entry *sgt; > > > + > > > fd_addr = dpaa2_fd_get_addr(fd); > > > buffer_start = dpaa2_iova_to_virt(priv->iommu_domain, fd_addr); > > > swa = (struct dpaa2_eth_swa *)buffer_start; > > > @@ -769,16 +852,29 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > > > DMA_BIDIRECTIONAL); > > > } > > > } else if (fd_format == dpaa2_fd_sg) { > > > - skb = swa->sg.skb; > > > + if (swa->type == DPAA2_ETH_SWA_SG) { > > > + skb = swa->sg.skb; > > > + > > > + /* Unmap the scatterlist */ > > > + dma_unmap_sg(dev, swa->sg.scl, swa->sg.num_sg, > > > + DMA_BIDIRECTIONAL); > > > + kfree(swa->sg.scl); > > > > > > - /* Unmap the scatterlist */ > > > - dma_unmap_sg(dev, swa->sg.scl, swa->sg.num_sg, > > > - DMA_BIDIRECTIONAL); > > > - kfree(swa->sg.scl); > > > + /* Unmap the SGT buffer */ > > > + dma_unmap_single(dev, fd_addr, swa->sg.sgt_size, > > > + DMA_BIDIRECTIONAL); > > > + } else { > > > + skb = swa->single.skb; > > > > > > - /* Unmap the SGT buffer */ > > > - dma_unmap_single(dev, fd_addr, swa->sg.sgt_size, > > > - DMA_BIDIRECTIONAL); > > > + /* Unmap the SGT Buffer */ > > > + dma_unmap_single(dev, fd_addr, swa->single.sgt_size, > > > + DMA_BIDIRECTIONAL); > > > + > > > + sgt = (struct dpaa2_sg_entry *)(buffer_start + > > > + priv->tx_data_offset); > > > + sg_addr = dpaa2_sg_get_addr(sgt); > > > + dma_unmap_single(dev, sg_addr, skb->len, DMA_BIDIRECTIONAL); > > > + } > > > } else { > > > netdev_dbg(priv->net_dev, "Invalid FD format\n"); > > > return; > > > @@ -808,8 +904,17 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv, > > > } > > > > > > /* Free SGT buffer allocated on tx */ > > > - if (fd_format != dpaa2_fd_single) > > > - skb_free_frag(buffer_start); > > > + if (fd_format != dpaa2_fd_single) { > > > + sgt_cache = this_cpu_ptr(priv->sgt_cache); > > > + if (swa->type == DPAA2_ETH_SWA_SG) { > > > + skb_free_frag(buffer_start); > > > + } else { > > > + if (sgt_cache->count >= DPAA2_ETH_SGT_CACHE_SIZE) > > > + kfree(buffer_start); > > > + else > > > + sgt_cache->buf[sgt_cache->count++] = buffer_start; > > > + } > > > + } > > > > > > /* Move on with skb release */ > > > napi_consume_skb(skb, in_napi); > > > @@ -833,22 +938,6 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > > > percpu_extras = this_cpu_ptr(priv->percpu_extras); > > > > > > needed_headroom = dpaa2_eth_needed_headroom(priv, skb); > > > - if (skb_headroom(skb) < needed_headroom) { > > > - struct sk_buff *ns; > > > - > > > - ns = skb_realloc_headroom(skb, needed_headroom); > > > - if (unlikely(!ns)) { > > > - percpu_stats->tx_dropped++; > > > - goto err_alloc_headroom; > > > - } > > > - percpu_extras->tx_reallocs++; > > > - > > > - if (skb->sk) > > > - skb_set_owner_w(ns, skb->sk); > > > - > > > - dev_kfree_skb(skb); > > > - skb = ns; > > > - } > > > > > > /* We'll be holding a back-reference to the skb until Tx Confirmation; > > > * we don't want that overwritten by a concurrent Tx with a cloned skb. > > > @@ -867,6 +956,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > > > err = build_sg_fd(priv, skb, &fd); > > > percpu_extras->tx_sg_frames++; > > > percpu_extras->tx_sg_bytes += skb->len; > > > + } else if (skb_headroom(skb) < needed_headroom) { > > > + err = build_sg_fd_single_buf(priv, skb, &fd); > > > + percpu_extras->tx_sg_frames++; > > > + percpu_extras->tx_sg_bytes += skb->len; > > > } else { > > > err = build_single_fd(priv, skb, &fd); > > > } > > > @@ -924,7 +1017,6 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev) > > > return NETDEV_TX_OK; > > > > > > err_build_fd: > > > -err_alloc_headroom: > > > dev_kfree_skb(skb); > > > > > > return NETDEV_TX_OK; > > > @@ -1161,6 +1253,22 @@ static int refill_pool(struct dpaa2_eth_priv *priv, > > > return 0; > > > } > > > > > > +static void dpaa2_eth_sgt_cache_drain(struct dpaa2_eth_priv *priv) > > > +{ > > > + struct dpaa2_eth_sgt_cache *sgt_cache; > > > + u16 count; > > > + int k, i; > > > + > > > + for_each_online_cpu(k) { > > > + sgt_cache = per_cpu_ptr(priv->sgt_cache, k); > > > + count = sgt_cache->count; > > > + > > > + for (i = 0; i < count; i++) > > > + kfree(sgt_cache->buf[i]); > > > + sgt_cache->count = 0; > > > + } > > > +} > > > + > > > static int pull_channel(struct dpaa2_eth_channel *ch) > > > { > > > int err; > > > @@ -1562,6 +1670,9 @@ static int dpaa2_eth_stop(struct net_device *net_dev) > > > /* Empty the buffer pool */ > > > drain_pool(priv); > > > > > > + /* Empty the Scatter-Gather Buffer cache */ > > > + dpaa2_eth_sgt_cache_drain(priv); > > > + > > > return 0; > > > } > > > > > > @@ -3846,6 +3957,13 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) > > > goto err_alloc_percpu_extras; > > > } > > > > > > + priv->sgt_cache = alloc_percpu(*priv->sgt_cache); > > > + if (!priv->sgt_cache) { > > > + dev_err(dev, "alloc_percpu(sgt_cache) failed\n"); > > > + err = -ENOMEM; > > > + goto err_alloc_sgt_cache; > > > + } > > > + > > > err = netdev_init(net_dev); > > > if (err) > > > goto err_netdev_init; > > > @@ -3914,6 +4032,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) > > > err_alloc_rings: > > > err_csum: > > > err_netdev_init: > > > + free_percpu(priv->sgt_cache); > > > +err_alloc_sgt_cache: > > > free_percpu(priv->percpu_extras); > > > err_alloc_percpu_extras: > > > free_percpu(priv->percpu_stats); > > > @@ -3959,6 +4079,7 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev) > > > fsl_mc_free_irqs(ls_dev); > > > > > > free_rings(priv); > > > + free_percpu(priv->sgt_cache); > > > free_percpu(priv->percpu_stats); > > > free_percpu(priv->percpu_extras); > > > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > > index 2d7ada0f0dbd..9e4ceb92f240 100644 > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > > @@ -125,6 +125,7 @@ struct dpaa2_eth_swa { > > > union { > > > struct { > > > struct sk_buff *skb; > > > + int sgt_size; > > > } single; > > > struct { > > > struct sk_buff *skb; > > > @@ -282,7 +283,6 @@ struct dpaa2_eth_drv_stats { > > > __u64 tx_conf_bytes; > > > __u64 tx_sg_frames; > > > __u64 tx_sg_bytes; > > > - __u64 tx_reallocs; > > > __u64 rx_sg_frames; > > > __u64 rx_sg_bytes; > > > /* Enqueues retried due to portal busy */ > > > @@ -395,6 +395,12 @@ struct dpaa2_eth_cls_rule { > > > u8 in_use; > > > }; > > > > > > +#define DPAA2_ETH_SGT_CACHE_SIZE 256 > > > +struct dpaa2_eth_sgt_cache { > > > + void *buf[DPAA2_ETH_SGT_CACHE_SIZE]; > > > + u16 count; > > > +}; > > > + > > > /* Driver private data */ > > > struct dpaa2_eth_priv { > > > struct net_device *net_dev; > > > @@ -409,6 +415,7 @@ struct dpaa2_eth_priv { > > > > > > u8 num_channels; > > > struct dpaa2_eth_channel *channel[DPAA2_ETH_MAX_DPCONS]; > > > + struct dpaa2_eth_sgt_cache __percpu *sgt_cache; > > > > > > struct dpni_attr dpni_attrs; > > > u16 dpni_ver_major; > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > > > index e88269fe3de7..c4cbbcaa9a3f 100644 > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c > > > @@ -43,7 +43,6 @@ static char dpaa2_ethtool_extras[][ETH_GSTRING_LEN] = { > > > "[drv] tx conf bytes", > > > "[drv] tx sg frames", > > > "[drv] tx sg bytes", > > > - "[drv] tx realloc frames", > > > "[drv] rx sg frames", > > > "[drv] rx sg bytes", > > > "[drv] enqueue portal busy", > > > -- > > > 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-10 18:06 ` Ioana Ciornei 2020-12-11 7:21 ` Jon Nettleton @ 2020-12-11 14:01 ` Ioana Ciornei 2020-12-11 16:29 ` Daniel Thompson 1 sibling, 1 reply; 11+ messages in thread From: Ioana Ciornei @ 2020-12-11 14:01 UTC (permalink / raw) To: Daniel Thompson; +Cc: linux-kernel, netdev On Thu, Dec 10, 2020 at 08:06:36PM +0200, Ioana Ciornei wrote: > [Added also the netdev mailing list, I haven't heard of linux-netdev > before but kept it] > > On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > > Hi Ioana > > Hi Daniel, > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > descriptor with only one entry. > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > ethtool since it is no longer used. > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > --- > > > > I've been chasing down a networking regression on my LX2160A board > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > It makes the board unreliable opening outbound connections meaning > > things like `apt update` or `git fetch` often can't open the connection. > > It does not happen all the time but is sufficient to make the boards > > built-in networking useless for workstation use. > > > > The problem is strongly linked to warnings in the logs so I used the > > warnings to bisect down to locate the cause of the regression and it > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > that reverting this patch (and fixing up the merge issues) fixes the > > regression and the warnings stop appearing. > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > error path that I guess would cause dma_map_page_attrs() to return > > an error): > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > > 5.10.0-rc7-00001-gba98d13279ca #52 > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > [ 714.465215] sp : ffff80001006b340 > > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > > [ 714.465343] Call trace: > > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > > [ 714.465353] __arm_lpae_map+0x114/0x30c > > [ 714.465357] __arm_lpae_map+0x114/0x30c > > [ 714.465362] __arm_lpae_map+0x114/0x30c > > [ 714.465366] arm_lpae_map+0xf4/0x180 > > [ 714.465373] arm_smmu_map+0x4c/0xc0 > > [ 714.465379] __iommu_map+0x100/0x2bc > > [ 714.465385] iommu_map_atomic+0x20/0x30 > > [ 714.465391] __iommu_dma_map+0xb0/0x110 > > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > > [ 714.465438] __qdisc_run+0x140/0x670 > > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > > [ 714.465449] dev_queue_xmit+0x20/0x2c > > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > > [ 714.465502] br_forward+0x13c/0x160 [bridge] > > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > > [ 714.465531] dev_queue_xmit+0x20/0x2c > > [ 714.465538] arp_xmit+0xc0/0xd0 > > [ 714.465544] arp_send_dst+0x78/0xa0 > > [ 714.465550] arp_solicit+0xf4/0x260 > > [ 714.465554] neigh_probe+0x64/0xb0 > > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > > [ 714.465566] call_timer_fn+0x3c/0x184 > > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > > [ 714.465578] run_timer_softirq+0x48/0x80 > > [ 714.465583] __do_softirq+0x120/0x36c > > [ 714.465589] irq_exit+0xac/0x100 > > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > > [ 714.465600] gic_handle_irq+0xcc/0x14c > > [ 714.465605] el1_irq+0xc4/0x180 > > [ 714.465610] arch_cpu_idle+0x18/0x30 > > [ 714.465617] default_idle_call+0x4c/0x180 > > [ 714.465623] do_idle+0x238/0x2b0 > > [ 714.465629] cpu_startup_entry+0x30/0xa0 > > [ 714.465636] secondary_start_kernel+0x134/0x180 > > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > > > > Given it is the iommu code that is provoking the warning I should > > probably mention that the board I have requires > > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > > Also if it matters I am running the latest firmware from Solidrun > > which is based on LSDK-20.04. > > > > Hmmm, from what I remember I think I tested this with the smmu bypassed > so that is why I didn't catch it. > > > Is there any reason for this code not to be working for LX2160A? > > I wouldn't expect this to be LX2160A specific but rather a bug in the > implementation.. sorry. > > Let me reproduce it and see if I can get to the bottom of it and I will > get back with some more info. > Hi Daniel, It seems that the dma-unmapping on the SGT buffer was incorrectly done with a zero size since on the Tx path I initialized the improper field. Could you test the following diff and let me know if you can generate the WARNINGs anymore? --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -878,7 +878,7 @@ static int dpaa2_eth_build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, swa = (struct dpaa2_eth_swa *)sgt_buf; swa->type = DPAA2_ETH_SWA_SINGLE; swa->single.skb = skb; - swa->sg.sgt_size = sgt_buf_size; + swa->single.sgt_size = sgt_buf_size; /* Separately map the SGT buffer */ sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); Ioana ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-11 14:01 ` Ioana Ciornei @ 2020-12-11 16:29 ` Daniel Thompson 2020-12-11 16:54 ` Ioana Ciornei 0 siblings, 1 reply; 11+ messages in thread From: Daniel Thompson @ 2020-12-11 16:29 UTC (permalink / raw) To: Ioana Ciornei; +Cc: linux-kernel, netdev On Fri, Dec 11, 2020 at 02:01:28PM +0000, Ioana Ciornei wrote: > On Thu, Dec 10, 2020 at 08:06:36PM +0200, Ioana Ciornei wrote: > > [Added also the netdev mailing list, I haven't heard of linux-netdev > > before but kept it] > > > > On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > > > Hi Ioana > > > > Hi Daniel, > > > > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > > descriptor with only one entry. > > > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > > ethtool since it is no longer used. > > > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > > --- > > > > > > I've been chasing down a networking regression on my LX2160A board > > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > > > It makes the board unreliable opening outbound connections meaning > > > things like `apt update` or `git fetch` often can't open the connection. > > > It does not happen all the time but is sufficient to make the boards > > > built-in networking useless for workstation use. > > > > > > The problem is strongly linked to warnings in the logs so I used the > > > warnings to bisect down to locate the cause of the regression and it > > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > > that reverting this patch (and fixing up the merge issues) fixes the > > > regression and the warnings stop appearing. > > > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > > error path that I guess would cause dma_map_page_attrs() to return > > > an error): > > > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > > > 5.10.0-rc7-00001-gba98d13279ca #52 > > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > > [ 714.465215] sp : ffff80001006b340 > > > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > > > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > > > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > > > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > > > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > > > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > > > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > > > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > > > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > > > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > > > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > > > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > > > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > > > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > > > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > > > [ 714.465343] Call trace: > > > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > > > [ 714.465353] __arm_lpae_map+0x114/0x30c > > > [ 714.465357] __arm_lpae_map+0x114/0x30c > > > [ 714.465362] __arm_lpae_map+0x114/0x30c > > > [ 714.465366] arm_lpae_map+0xf4/0x180 > > > [ 714.465373] arm_smmu_map+0x4c/0xc0 > > > [ 714.465379] __iommu_map+0x100/0x2bc > > > [ 714.465385] iommu_map_atomic+0x20/0x30 > > > [ 714.465391] __iommu_dma_map+0xb0/0x110 > > > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > > > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > > > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > > > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > > > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > > > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > > > [ 714.465438] __qdisc_run+0x140/0x670 > > > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > > > [ 714.465449] dev_queue_xmit+0x20/0x2c > > > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > > > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > > > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > > > [ 714.465502] br_forward+0x13c/0x160 [bridge] > > > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > > > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > > > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > > > [ 714.465531] dev_queue_xmit+0x20/0x2c > > > [ 714.465538] arp_xmit+0xc0/0xd0 > > > [ 714.465544] arp_send_dst+0x78/0xa0 > > > [ 714.465550] arp_solicit+0xf4/0x260 > > > [ 714.465554] neigh_probe+0x64/0xb0 > > > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > > > [ 714.465566] call_timer_fn+0x3c/0x184 > > > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > > > [ 714.465578] run_timer_softirq+0x48/0x80 > > > [ 714.465583] __do_softirq+0x120/0x36c > > > [ 714.465589] irq_exit+0xac/0x100 > > > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > > > [ 714.465600] gic_handle_irq+0xcc/0x14c > > > [ 714.465605] el1_irq+0xc4/0x180 > > > [ 714.465610] arch_cpu_idle+0x18/0x30 > > > [ 714.465617] default_idle_call+0x4c/0x180 > > > [ 714.465623] do_idle+0x238/0x2b0 > > > [ 714.465629] cpu_startup_entry+0x30/0xa0 > > > [ 714.465636] secondary_start_kernel+0x134/0x180 > > > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > > > > > > > Given it is the iommu code that is provoking the warning I should > > > probably mention that the board I have requires > > > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > > > Also if it matters I am running the latest firmware from Solidrun > > > which is based on LSDK-20.04. > > > > > > > Hmmm, from what I remember I think I tested this with the smmu bypassed > > so that is why I didn't catch it. > > > > > Is there any reason for this code not to be working for LX2160A? > > > > I wouldn't expect this to be LX2160A specific but rather a bug in the > > implementation.. sorry. > > > > Let me reproduce it and see if I can get to the bottom of it and I will > > get back with some more info. > > > > Hi Daniel, > > It seems that the dma-unmapping on the SGT buffer was incorrectly done > with a zero size since on the Tx path I initialized the improper field. > > Could you test the following diff and let me know if you can generate > the WARNINGs anymore? I fired this up and, with your change, I've not been able to trigger the warning with the tests that I used the drive my bisect. Thanks for the quick response. Daniel. > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -878,7 +878,7 @@ static int dpaa2_eth_build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, > swa = (struct dpaa2_eth_swa *)sgt_buf; > swa->type = DPAA2_ETH_SWA_SINGLE; > swa->single.skb = skb; > - swa->sg.sgt_size = sgt_buf_size; > + swa->single.sgt_size = sgt_buf_size; > > /* Separately map the SGT buffer */ > sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); > > > Ioana ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-11 16:29 ` Daniel Thompson @ 2020-12-11 16:54 ` Ioana Ciornei 2020-12-12 15:58 ` Jon Nettleton 0 siblings, 1 reply; 11+ messages in thread From: Ioana Ciornei @ 2020-12-11 16:54 UTC (permalink / raw) To: Daniel Thompson; +Cc: linux-kernel, netdev On Fri, Dec 11, 2020 at 04:29:14PM +0000, Daniel Thompson wrote: > On Fri, Dec 11, 2020 at 02:01:28PM +0000, Ioana Ciornei wrote: > > On Thu, Dec 10, 2020 at 08:06:36PM +0200, Ioana Ciornei wrote: > > > [Added also the netdev mailing list, I haven't heard of linux-netdev > > > before but kept it] > > > > > > On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > > > > Hi Ioana > > > > > > Hi Daniel, > > > > > > > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > > > descriptor with only one entry. > > > > > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > > > ethtool since it is no longer used. > > > > > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > > > --- > > > > > > > > I've been chasing down a networking regression on my LX2160A board > > > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > > > > > It makes the board unreliable opening outbound connections meaning > > > > things like `apt update` or `git fetch` often can't open the connection. > > > > It does not happen all the time but is sufficient to make the boards > > > > built-in networking useless for workstation use. > > > > > > > > The problem is strongly linked to warnings in the logs so I used the > > > > warnings to bisect down to locate the cause of the regression and it > > > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > > > that reverting this patch (and fixing up the merge issues) fixes the > > > > regression and the warnings stop appearing. > > > > > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > > > error path that I guess would cause dma_map_page_attrs() to return > > > > an error): > > > > > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > > > > 5.10.0-rc7-00001-gba98d13279ca #52 > > > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > > > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > > > [ 714.465215] sp : ffff80001006b340 > > > > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > > > > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > > > > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > > > > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > > > > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > > > > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > > > > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > > > > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > > > > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > > > > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > > > > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > > > > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > > > > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > > > > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > > > > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > > > > [ 714.465343] Call trace: > > > > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > > > > [ 714.465353] __arm_lpae_map+0x114/0x30c > > > > [ 714.465357] __arm_lpae_map+0x114/0x30c > > > > [ 714.465362] __arm_lpae_map+0x114/0x30c > > > > [ 714.465366] arm_lpae_map+0xf4/0x180 > > > > [ 714.465373] arm_smmu_map+0x4c/0xc0 > > > > [ 714.465379] __iommu_map+0x100/0x2bc > > > > [ 714.465385] iommu_map_atomic+0x20/0x30 > > > > [ 714.465391] __iommu_dma_map+0xb0/0x110 > > > > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > > > > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > > > > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > > > > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > > > > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > > > > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > > > > [ 714.465438] __qdisc_run+0x140/0x670 > > > > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > > > > [ 714.465449] dev_queue_xmit+0x20/0x2c > > > > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > > > > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > > > > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > > > > [ 714.465502] br_forward+0x13c/0x160 [bridge] > > > > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > > > > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > > > > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > > > > [ 714.465531] dev_queue_xmit+0x20/0x2c > > > > [ 714.465538] arp_xmit+0xc0/0xd0 > > > > [ 714.465544] arp_send_dst+0x78/0xa0 > > > > [ 714.465550] arp_solicit+0xf4/0x260 > > > > [ 714.465554] neigh_probe+0x64/0xb0 > > > > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > > > > [ 714.465566] call_timer_fn+0x3c/0x184 > > > > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > > > > [ 714.465578] run_timer_softirq+0x48/0x80 > > > > [ 714.465583] __do_softirq+0x120/0x36c > > > > [ 714.465589] irq_exit+0xac/0x100 > > > > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > > > > [ 714.465600] gic_handle_irq+0xcc/0x14c > > > > [ 714.465605] el1_irq+0xc4/0x180 > > > > [ 714.465610] arch_cpu_idle+0x18/0x30 > > > > [ 714.465617] default_idle_call+0x4c/0x180 > > > > [ 714.465623] do_idle+0x238/0x2b0 > > > > [ 714.465629] cpu_startup_entry+0x30/0xa0 > > > > [ 714.465636] secondary_start_kernel+0x134/0x180 > > > > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > > > > > > > > > > Given it is the iommu code that is provoking the warning I should > > > > probably mention that the board I have requires > > > > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > > > > Also if it matters I am running the latest firmware from Solidrun > > > > which is based on LSDK-20.04. > > > > > > > > > > Hmmm, from what I remember I think I tested this with the smmu bypassed > > > so that is why I didn't catch it. > > > > > > > Is there any reason for this code not to be working for LX2160A? > > > > > > I wouldn't expect this to be LX2160A specific but rather a bug in the > > > implementation.. sorry. > > > > > > Let me reproduce it and see if I can get to the bottom of it and I will > > > get back with some more info. > > > > > > > Hi Daniel, > > > > It seems that the dma-unmapping on the SGT buffer was incorrectly done > > with a zero size since on the Tx path I initialized the improper field. > > > > Could you test the following diff and let me know if you can generate > > the WARNINGs anymore? > > I fired this up and, with your change, I've not been able to trigger > the warning with the tests that I used the drive my bisect. > Great, thanks for testing this. I will take care of sending the fix to -net. Ioana > Thanks for the quick response. > > > Daniel. > > > > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -878,7 +878,7 @@ static int dpaa2_eth_build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, > > swa = (struct dpaa2_eth_swa *)sgt_buf; > > swa->type = DPAA2_ETH_SWA_SINGLE; > > swa->single.skb = skb; > > - swa->sg.sgt_size = sgt_buf_size; > > + swa->single.sgt_size = sgt_buf_size; > > > > /* Separately map the SGT buffer */ > > sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); > > > > > > Ioana ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-11 16:54 ` Ioana Ciornei @ 2020-12-12 15:58 ` Jon Nettleton 2020-12-12 19:31 ` Ioana Ciornei 0 siblings, 1 reply; 11+ messages in thread From: Jon Nettleton @ 2020-12-12 15:58 UTC (permalink / raw) To: Ioana Ciornei; +Cc: Daniel Thompson, linux-kernel, netdev On Fri, Dec 11, 2020 at 5:56 PM Ioana Ciornei <ioana.ciornei@nxp.com> wrote: > > On Fri, Dec 11, 2020 at 04:29:14PM +0000, Daniel Thompson wrote: > > On Fri, Dec 11, 2020 at 02:01:28PM +0000, Ioana Ciornei wrote: > > > On Thu, Dec 10, 2020 at 08:06:36PM +0200, Ioana Ciornei wrote: > > > > [Added also the netdev mailing list, I haven't heard of linux-netdev > > > > before but kept it] > > > > > > > > On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > > > > > Hi Ioana > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > > > > descriptor with only one entry. > > > > > > > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > > > > ethtool since it is no longer used. > > > > > > > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > > > > --- > > > > > > > > > > I've been chasing down a networking regression on my LX2160A board > > > > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > > > > > > > It makes the board unreliable opening outbound connections meaning > > > > > things like `apt update` or `git fetch` often can't open the connection. > > > > > It does not happen all the time but is sufficient to make the boards > > > > > built-in networking useless for workstation use. > > > > > > > > > > The problem is strongly linked to warnings in the logs so I used the > > > > > warnings to bisect down to locate the cause of the regression and it > > > > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > > > > that reverting this patch (and fixing up the merge issues) fixes the > > > > > regression and the warnings stop appearing. > > > > > > > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > > > > error path that I guess would cause dma_map_page_attrs() to return > > > > > an error): > > > > > > > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > > > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > > > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > > > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > > > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > > > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > > > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > > > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > > > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > > > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > > > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > > > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > > > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > > > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > > > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > > > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > > > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > > > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > > > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > > > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > > > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > > > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > > > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > > > > > 5.10.0-rc7-00001-gba98d13279ca #52 > > > > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > > > > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > > > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > > > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > > > > [ 714.465215] sp : ffff80001006b340 > > > > > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > > > > > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > > > > > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > > > > > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > > > > > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > > > > > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > > > > > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > > > > > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > > > > > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > > > > > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > > > > > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > > > > > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > > > > > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > > > > > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > > > > > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > > > > > [ 714.465343] Call trace: > > > > > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > > > > > [ 714.465353] __arm_lpae_map+0x114/0x30c > > > > > [ 714.465357] __arm_lpae_map+0x114/0x30c > > > > > [ 714.465362] __arm_lpae_map+0x114/0x30c > > > > > [ 714.465366] arm_lpae_map+0xf4/0x180 > > > > > [ 714.465373] arm_smmu_map+0x4c/0xc0 > > > > > [ 714.465379] __iommu_map+0x100/0x2bc > > > > > [ 714.465385] iommu_map_atomic+0x20/0x30 > > > > > [ 714.465391] __iommu_dma_map+0xb0/0x110 > > > > > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > > > > > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > > > > > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > > > > > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > > > > > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > > > > > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > > > > > [ 714.465438] __qdisc_run+0x140/0x670 > > > > > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > > > > > [ 714.465449] dev_queue_xmit+0x20/0x2c > > > > > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > > > > > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > > > > > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > > > > > [ 714.465502] br_forward+0x13c/0x160 [bridge] > > > > > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > > > > > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > > > > > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > > > > > [ 714.465531] dev_queue_xmit+0x20/0x2c > > > > > [ 714.465538] arp_xmit+0xc0/0xd0 > > > > > [ 714.465544] arp_send_dst+0x78/0xa0 > > > > > [ 714.465550] arp_solicit+0xf4/0x260 > > > > > [ 714.465554] neigh_probe+0x64/0xb0 > > > > > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > > > > > [ 714.465566] call_timer_fn+0x3c/0x184 > > > > > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > > > > > [ 714.465578] run_timer_softirq+0x48/0x80 > > > > > [ 714.465583] __do_softirq+0x120/0x36c > > > > > [ 714.465589] irq_exit+0xac/0x100 > > > > > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > > > > > [ 714.465600] gic_handle_irq+0xcc/0x14c > > > > > [ 714.465605] el1_irq+0xc4/0x180 > > > > > [ 714.465610] arch_cpu_idle+0x18/0x30 > > > > > [ 714.465617] default_idle_call+0x4c/0x180 > > > > > [ 714.465623] do_idle+0x238/0x2b0 > > > > > [ 714.465629] cpu_startup_entry+0x30/0xa0 > > > > > [ 714.465636] secondary_start_kernel+0x134/0x180 > > > > > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > > > > > > > > > > > > > Given it is the iommu code that is provoking the warning I should > > > > > probably mention that the board I have requires > > > > > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > > > > > Also if it matters I am running the latest firmware from Solidrun > > > > > which is based on LSDK-20.04. > > > > > > > > > > > > > Hmmm, from what I remember I think I tested this with the smmu bypassed > > > > so that is why I didn't catch it. > > > > > > > > > Is there any reason for this code not to be working for LX2160A? > > > > > > > > I wouldn't expect this to be LX2160A specific but rather a bug in the > > > > implementation.. sorry. > > > > > > > > Let me reproduce it and see if I can get to the bottom of it and I will > > > > get back with some more info. > > > > > > > > > > Hi Daniel, > > > > > > It seems that the dma-unmapping on the SGT buffer was incorrectly done > > > with a zero size since on the Tx path I initialized the improper field. > > > > > > Could you test the following diff and let me know if you can generate > > > the WARNINGs anymore? > > > > I fired this up and, with your change, I've not been able to trigger > > the warning with the tests that I used the drive my bisect. > > > > Great, thanks for testing this. > > I will take care of sending the fix to -net. > > Ioana Ioana, Please CC me when you send the patch to -net, I will put my Tested-by: on it. Thanks Jon > > > Thanks for the quick response. > > > > > > Daniel. > > > > > > > > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > @@ -878,7 +878,7 @@ static int dpaa2_eth_build_sg_fd_single_buf(struct dpaa2_eth_priv *priv, > > > swa = (struct dpaa2_eth_swa *)sgt_buf; > > > swa->type = DPAA2_ETH_SWA_SINGLE; > > > swa->single.skb = skb; > > > - swa->sg.sgt_size = sgt_buf_size; > > > + swa->single.sgt_size = sgt_buf_size; > > > > > > /* Separately map the SGT buffer */ > > > sgt_addr = dma_map_single(dev, sgt_buf, sgt_buf_size, DMA_BIDIRECTIONAL); > > > > > > > > > Ioana ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-12 15:58 ` Jon Nettleton @ 2020-12-12 19:31 ` Ioana Ciornei 0 siblings, 0 replies; 11+ messages in thread From: Ioana Ciornei @ 2020-12-12 19:31 UTC (permalink / raw) To: Jon Nettleton; +Cc: Daniel Thompson, linux-kernel, netdev On Sat, Dec 12, 2020 at 04:58:56PM +0100, Jon Nettleton wrote: > On Fri, Dec 11, 2020 at 5:56 PM Ioana Ciornei <ioana.ciornei@nxp.com> wrote: > > > > On Fri, Dec 11, 2020 at 04:29:14PM +0000, Daniel Thompson wrote: > > > On Fri, Dec 11, 2020 at 02:01:28PM +0000, Ioana Ciornei wrote: > > > > On Thu, Dec 10, 2020 at 08:06:36PM +0200, Ioana Ciornei wrote: > > > > > [Added also the netdev mailing list, I haven't heard of linux-netdev > > > > > before but kept it] > > > > > > > > > > On Thu, Dec 10, 2020 at 05:31:56PM +0000, Daniel Thompson wrote: > > > > > > Hi Ioana > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > > > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > > > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > > > > > descriptor with only one entry. > > > > > > > > > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > > > > > ethtool since it is no longer used. > > > > > > > > > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > > > > > --- > > > > > > > > > > > > I've been chasing down a networking regression on my LX2160A board > > > > > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > > > > > > > > > It makes the board unreliable opening outbound connections meaning > > > > > > things like `apt update` or `git fetch` often can't open the connection. > > > > > > It does not happen all the time but is sufficient to make the boards > > > > > > built-in networking useless for workstation use. > > > > > > > > > > > > The problem is strongly linked to warnings in the logs so I used the > > > > > > warnings to bisect down to locate the cause of the regression and it > > > > > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > > > > > that reverting this patch (and fixing up the merge issues) fixes the > > > > > > regression and the warnings stop appearing. > > > > > > > > > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > > > > > error path that I guess would cause dma_map_page_attrs() to return > > > > > > an error): > > > > > > > > > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > > > > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > > > > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > > > > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > > > > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > > > > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > > > > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > > > > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > > > > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > > > > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > > > > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > > > > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > > > > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > > > > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > > > > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > > > > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > > > > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > > > > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > > > > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > > > > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > > > > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > > > > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > > > > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: G W E > > > > > > 5.10.0-rc7-00001-gba98d13279ca #52 > > > > > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > > > > > [ 714.465202] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > > > > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > > > > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > > > > > [ 714.465215] sp : ffff80001006b340 > > > > > > [ 714.465219] x29: ffff80001006b340 x28: 0000002086538003 > > > > > > [ 714.465227] x27: 0000000000000a20 x26: 0000000000001000 > > > > > > [ 714.465236] x25: 0000000000000f44 x24: 00000020adf8d000 > > > > > > [ 714.465245] x23: 0000000000000001 x22: 0000fffffaeca000 > > > > > > [ 714.465253] x21: 0000000000000003 x20: ffff19b60d64d200 > > > > > > [ 714.465261] x19: 00000000000000ca x18: 0000000000000000 > > > > > > [ 714.465270] x17: 0000000000000000 x16: ffffcccb7cf3ca20 > > > > > > [ 714.465278] x15: 0000000000000000 x14: 0000000000000000 > > > > > > [ 714.465286] x13: 0000000000000003 x12: 0000000000000010 > > > > > > [ 714.465294] x11: 0000000000000000 x10: 0000000000000002 > > > > > > [ 714.465302] x9 : ffffcccb7d5b6e78 x8 : 00000000000001ff > > > > > > [ 714.465311] x7 : ffff19b606538650 x6 : ffff19b606538000 > > > > > > [ 714.465319] x5 : 0000000000000009 x4 : 0000000000000f44 > > > > > > [ 714.465327] x3 : 0000000000001000 x2 : 00000020adf8d000 > > > > > > [ 714.465335] x1 : 0000000000000002 x0 : 0000000000000003 > > > > > > [ 714.465343] Call trace: > > > > > > [ 714.465348] __arm_lpae_map+0x2d4/0x30c > > > > > > [ 714.465353] __arm_lpae_map+0x114/0x30c > > > > > > [ 714.465357] __arm_lpae_map+0x114/0x30c > > > > > > [ 714.465362] __arm_lpae_map+0x114/0x30c > > > > > > [ 714.465366] arm_lpae_map+0xf4/0x180 > > > > > > [ 714.465373] arm_smmu_map+0x4c/0xc0 > > > > > > [ 714.465379] __iommu_map+0x100/0x2bc > > > > > > [ 714.465385] iommu_map_atomic+0x20/0x30 > > > > > > [ 714.465391] __iommu_dma_map+0xb0/0x110 > > > > > > [ 714.465397] iommu_dma_map_page+0xb8/0x120 > > > > > > [ 714.465404] dma_map_page_attrs+0x1a8/0x210 > > > > > > [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] > > > > > > [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] > > > > > > [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 > > > > > > [ 714.465433] sch_direct_xmit+0x1a0/0x550 > > > > > > [ 714.465438] __qdisc_run+0x140/0x670 > > > > > > [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 > > > > > > [ 714.465449] dev_queue_xmit+0x20/0x2c > > > > > > [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] > > > > > > [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] > > > > > > [ 714.465489] __br_forward+0x160/0x1c0 [bridge] > > > > > > [ 714.465502] br_forward+0x13c/0x160 [bridge] > > > > > > [ 714.465514] br_dev_xmit+0x228/0x3b0 [bridge] > > > > > > [ 714.465520] dev_hard_start_xmit+0x10c/0x2b0 > > > > > > [ 714.465526] __dev_queue_xmit+0x8f0/0xa74 > > > > > > [ 714.465531] dev_queue_xmit+0x20/0x2c > > > > > > [ 714.465538] arp_xmit+0xc0/0xd0 > > > > > > [ 714.465544] arp_send_dst+0x78/0xa0 > > > > > > [ 714.465550] arp_solicit+0xf4/0x260 > > > > > > [ 714.465554] neigh_probe+0x64/0xb0 > > > > > > [ 714.465560] neigh_timer_handler+0x2f4/0x400 > > > > > > [ 714.465566] call_timer_fn+0x3c/0x184 > > > > > > [ 714.465572] __run_timers.part.0+0x2bc/0x370 > > > > > > [ 714.465578] run_timer_softirq+0x48/0x80 > > > > > > [ 714.465583] __do_softirq+0x120/0x36c > > > > > > [ 714.465589] irq_exit+0xac/0x100 > > > > > > [ 714.465596] __handle_domain_irq+0x8c/0xf0 > > > > > > [ 714.465600] gic_handle_irq+0xcc/0x14c > > > > > > [ 714.465605] el1_irq+0xc4/0x180 > > > > > > [ 714.465610] arch_cpu_idle+0x18/0x30 > > > > > > [ 714.465617] default_idle_call+0x4c/0x180 > > > > > > [ 714.465623] do_idle+0x238/0x2b0 > > > > > > [ 714.465629] cpu_startup_entry+0x30/0xa0 > > > > > > [ 714.465636] secondary_start_kernel+0x134/0x180 > > > > > > [ 714.465640] ---[ end trace a84a7f61b559005f ]--- > > > > > > > > > > > > > > > > > > Given it is the iommu code that is provoking the warning I should > > > > > > probably mention that the board I have requires > > > > > > arm-smmu.disable_bypass=0 on the kernel command line in order to boot. > > > > > > Also if it matters I am running the latest firmware from Solidrun > > > > > > which is based on LSDK-20.04. > > > > > > > > > > > > > > > > Hmmm, from what I remember I think I tested this with the smmu bypassed > > > > > so that is why I didn't catch it. > > > > > > > > > > > Is there any reason for this code not to be working for LX2160A? > > > > > > > > > > I wouldn't expect this to be LX2160A specific but rather a bug in the > > > > > implementation.. sorry. > > > > > > > > > > Let me reproduce it and see if I can get to the bottom of it and I will > > > > > get back with some more info. > > > > > > > > > > > > > Hi Daniel, > > > > > > > > It seems that the dma-unmapping on the SGT buffer was incorrectly done > > > > with a zero size since on the Tx path I initialized the improper field. > > > > > > > > Could you test the following diff and let me know if you can generate > > > > the WARNINGs anymore? > > > > > > I fired this up and, with your change, I've not been able to trigger > > > the warning with the tests that I used the drive my bisect. > > > > > > > Great, thanks for testing this. > > > > I will take care of sending the fix to -net. > > > > Ioana > > Ioana, > > Please CC me when you send the patch to -net, I will put my Tested-by: on it. > Hi Jon, I sent out the patch yesterday. You are also copied. https://lore.kernel.org/netdev/20201211171607.108034-1-ciorneiioana@gmail.com/ Ioana ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-10 17:31 ` [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing Daniel Thompson 2020-12-10 18:06 ` Ioana Ciornei @ 2020-12-11 9:30 ` David Laight 2020-12-11 9:38 ` Ioana Ciornei 1 sibling, 1 reply; 11+ messages in thread From: David Laight @ 2020-12-11 9:30 UTC (permalink / raw) To: 'Daniel Thompson', Ioana Ciornei; +Cc: linux-netdev, linux-kernel From: Daniel Thompson > Sent: 10 December 2020 17:32 > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > is smaller than the HW requirements, create a Scatter/Gather frame > > descriptor with only one entry. Is it worth simplifying the code by permanently allocating (and dma-mapping) the extra structure for every ring entry. It is (probably) only one page and 1 iommu entry. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-11 9:30 ` David Laight @ 2020-12-11 9:38 ` Ioana Ciornei 2020-12-11 10:03 ` David Laight 0 siblings, 1 reply; 11+ messages in thread From: Ioana Ciornei @ 2020-12-11 9:38 UTC (permalink / raw) To: David Laight; +Cc: 'Daniel Thompson', linux-netdev, linux-kernel On Fri, Dec 11, 2020 at 09:30:43AM +0000, David Laight wrote: > From: Daniel Thompson > > Sent: 10 December 2020 17:32 > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > descriptor with only one entry. > > Is it worth simplifying the code by permanently allocating (and dma-mapping) > the extra structure for every ring entry. > It is (probably) only one page and 1 iommu entry. That is exactly what I was thinking. At the moment the SGT structure is pre-allocated but not pre-mapped. I'll let you know how it goes. Ioana ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing 2020-12-11 9:38 ` Ioana Ciornei @ 2020-12-11 10:03 ` David Laight 0 siblings, 0 replies; 11+ messages in thread From: David Laight @ 2020-12-11 10:03 UTC (permalink / raw) To: 'Ioana Ciornei' Cc: 'Daniel Thompson', linux-netdev, linux-kernel From: Ioana Ciornei > Sent: 11 December 2020 09:39 > > On Fri, Dec 11, 2020 at 09:30:43AM +0000, David Laight wrote: > > From: Daniel Thompson > > > Sent: 10 December 2020 17:32 > > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +0000, Ioana Ciornei wrote: > > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > > descriptor with only one entry. > > > > Is it worth simplifying the code by permanently allocating (and dma-mapping) > > the extra structure for every ring entry. > > It is (probably) only one page and 1 iommu entry. > > > That is exactly what I was thinking. At the moment the SGT structure is > pre-allocated but not pre-mapped. > > I'll let you know how it goes. How much does the dma-map actually cost? For short fragments it is probably worth copying into a pre-allocated pre-mapped transmit buffer area. You'd want to do aligned full-word copies and use separate cache lines for each frame. It does make tx setup more error prone - since you need the space in the tx buffer area as well as in the tx ring. For one OS (not sun's) on a sparc mbus+sbus system one of my colleagues measured a cutoff point of about 1k. The copy to tx buffer path also helps with the pathological skb that are 1500 bytes in 1 byte fragments. (Maybe skb can't get that bad, but I've seen that on other OS.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-12 19:35 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200629184712.12449-2-ioana.ciornei () nxp ! com>
2020-12-10 17:31 ` [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing Daniel Thompson
2020-12-10 18:06 ` Ioana Ciornei
2020-12-11 7:21 ` Jon Nettleton
2020-12-11 14:01 ` Ioana Ciornei
2020-12-11 16:29 ` Daniel Thompson
2020-12-11 16:54 ` Ioana Ciornei
2020-12-12 15:58 ` Jon Nettleton
2020-12-12 19:31 ` Ioana Ciornei
2020-12-11 9:30 ` David Laight
2020-12-11 9:38 ` Ioana Ciornei
2020-12-11 10:03 ` David Laight
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).