From: Mark Kettenis Subject: Re: ixl/ice(4): Avoid unnecessary defrag of TSO packets To: Alexander Bluhm Cc: jan@openbsd.org, tech@openbsd.org Date: Wed, 17 Sep 2025 11:06:06 +0200 > Date: Tue, 16 Sep 2025 21:20:43 +0200 > From: Alexander Bluhm > > On Sat, Sep 13, 2025 at 04:00:04PM +0200, Jan Klemkow wrote: > > On Fri, Sep 12, 2025 at 05:57:49PM +0200, Mark Kettenis wrote: > > > > Date: Fri, 12 Sep 2025 16:00:39 +0200 > > > > From: Jan Klemkow > > > > ixl(4) and ice(4) network cards can handle just up to 8 DMA segments for > > > > a regular packet, but 128 segments for a TSO packet. TSO packets reach > > > > the 8 segments limit very fast. Thus, we run into unnecessary > > > > m_defrag() calls, which cost throughput. > > > > > > > > This diff sets the limit to 128 segments for ice(4) and ixl(4) and adds an > > > > additional check for non-TSO packets. > > > > > > > > ok? > > > > > > I don't think so. This bypasses all the bus_dma(9) logic. There > > > isn't necessarily a relationship between the number of elements in an > > > mbuf chain and the number of bus_dma(9) segments. > > > > > > I think you should use separate bus_dma(9) maps for TSO. > > > > Like the following diff? > > I have tested it successfully. Comments inline. > > bluhm > > > Index: dev/pci/if_ice.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/pci/if_ice.c,v > > diff -u -p -r1.58 if_ice.c > > --- dev/pci/if_ice.c 19 Aug 2025 11:46:52 -0000 1.58 > > +++ dev/pci/if_ice.c 13 Sep 2025 13:29:55 -0000 > > @@ -13432,8 +13432,10 @@ ice_txq_clean(struct ice_softc *sc, stru > > > > if (txm->txm_m == NULL) > > continue; > > - > > - map = txm->txm_map; > > + if (ISSET(txm->txm_m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + map = txm->txm_map_tso; > > + else > > + map = txm->txm_map; > > bus_dmamap_sync(sc->sc_dmat, map, 0, map->dm_mapsize, > > BUS_DMASYNC_POSTWRITE); > > bus_dmamap_unload(sc->sc_dmat, map); > > @@ -13868,7 +13870,7 @@ ice_tso_detect_sparse(struct mbuf *m, st > > hlen -= seglen; > > } > > > > - maxsegs = ICE_MAX_TX_SEGS - hdrs; > > + maxsegs = ICE_MAX_TSO_SEGS - hdrs; > > > > /* We must count the headers, in order to verify that they take up > > * 3 or fewer descriptors. However, we don't need to check the data > > The comment above ice_tso_detect_sparse() talks about 8. That > should be 128 in some cases. Please adapt. > > In general, parts of ice_tso_detect_sparse() do what bus_dmamap_load_mbuf() > checks anyway. Other drivers don't have theses checks although I > guess they have similar hardware limitations. I think this function > has to be reconsidered. But not in this diff. Yes, that function is weird and I don't think it does what the comment says. And yes, that is another diff. > > @@ -14066,7 +14068,11 @@ ice_start(struct ifqueue *ifq) > > } > > > > txm = &txq->tx_map[prod]; > > - map = txm->txm_map; > > + > > + if (ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + map = txm->txm_map_tso; > > + else > > + map = txm->txm_map; > > > > if (ice_load_mbuf(sc->sc_dmat, map, m) != 0) { > > ifq->ifq_errors++; > > @@ -29478,7 +29484,10 @@ ice_txeof(struct ice_softc *sc, struct i > > if (dtype != htole64(ICE_TX_DESC_DTYPE_DESC_DONE)) > > break; > > > > - map = txm->txm_map; > > + if (ISSET(txm->txm_m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + map = txm->txm_map_tso; > > + else > > + map = txm->txm_map; > > > > bus_dmamap_sync(sc->sc_dmat, map, 0, map->dm_mapsize, > > BUS_DMASYNC_POSTWRITE); > > @@ -29601,6 +29610,11 @@ ice_free_tx_queues(struct ice_softc *sc) > > bus_dmamap_destroy(sc->sc_dmat, map->txm_map); > > map->txm_map = NULL; > > } > > + if (map->txm_map_tso != NULL) { > > + bus_dmamap_destroy(sc->sc_dmat, > > + map->txm_map_tso); > > + map->txm_map_tso = NULL; > > + } > > } > > free(txq->tx_map, M_DEVBUF, txq->desc_count * sizeof(*map)); > > txq->tx_map = NULL; > > @@ -29674,6 +29688,16 @@ ice_tx_queues_alloc(struct ice_softc *sc > > ICE_MAX_TX_SEGS, ICE_MAX_DMA_SEG_SIZE, 0, > > BUS_DMA_WAITOK | BUS_DMA_ALLOCNOW | BUS_DMA_64BIT, > > &map->txm_map) != 0) { > > + printf("%s: could not allocate Tx DMA map\n", > > + sc->sc_dev.dv_xname); > > + err = ENOMEM; > > + goto free_tx_queues; > > + } > > + > > + if (bus_dmamap_create(sc->sc_dmat, MAXMCLBYTES, > > + ICE_MAX_TSO_SEGS, ICE_MAX_DMA_SEG_SIZE, 0, > > + BUS_DMA_WAITOK | BUS_DMA_ALLOCNOW | BUS_DMA_64BIT, > > + &map->txm_map_tso) != 0) { > > printf("%s: could not allocate Tx DMA map\n", > > sc->sc_dev.dv_xname); > > err = ENOMEM; > > Plase use unique error messages. Rename the second one to > "%s: could not allocate TSO DMA map\n" > > > Index: dev/pci/if_icevar.h > > =================================================================== > > RCS file: /cvs/src/sys/dev/pci/if_icevar.h,v > > diff -u -p -r1.8 if_icevar.h > > --- dev/pci/if_icevar.h 19 Aug 2025 11:46:52 -0000 1.8 > > +++ dev/pci/if_icevar.h 13 Sep 2025 10:27:14 -0000 > > @@ -4537,6 +4537,7 @@ struct ice_pf_sw_stats { > > struct ice_tx_map { > > struct mbuf *txm_m; > > bus_dmamap_t txm_map; > > + bus_dmamap_t txm_map_tso; > > unsigned int txm_eop; > > }; > > > > Index: dev/pci/if_ixl.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/pci/if_ixl.c,v > > diff -u -p -r1.108 if_ixl.c > > --- dev/pci/if_ixl.c 24 Jun 2025 11:03:10 -0000 1.108 > > +++ dev/pci/if_ixl.c 13 Sep 2025 13:30:47 -0000 > > @@ -900,6 +900,7 @@ struct ixl_rx_wb_desc_32 { > > } __packed __aligned(16); > > > > #define IXL_TX_PKT_DESCS 8 > > +#define IXL_TX_TSO_PKT_DESCS 128 > > #define IXL_TX_QUEUE_ALIGN 128 > > #define IXL_RX_QUEUE_ALIGN 128 > > > > @@ -1142,6 +1143,7 @@ struct ixl_chip; > > struct ixl_tx_map { > > struct mbuf *txm_m; > > bus_dmamap_t txm_map; > > + bus_dmamap_t txm_map_tso; > > unsigned int txm_eop; > > }; > > > > @@ -2584,6 +2586,12 @@ ixl_txr_alloc(struct ixl_softc *sc, unsi > > &txm->txm_map) != 0) > > goto uncreate; > > > > + if (bus_dmamap_create(sc->sc_dmat, > > + MAXMCLBYTES, IXL_TX_TSO_PKT_DESCS, IXL_MAX_DMA_SEG_SIZE, 0, > > + BUS_DMA_WAITOK | BUS_DMA_ALLOCNOW | BUS_DMA_64BIT, > > + &txm->txm_map_tso) != 0) > > + goto uncreate; > > + > > txm->txm_eop = -1; > > txm->txm_m = NULL; > > } > > @@ -2604,6 +2612,11 @@ uncreate: > > continue; > > > > bus_dmamap_destroy(sc->sc_dmat, txm->txm_map); > > + > > + if (txm->txm_map_tso == NULL) > > + continue; > > + > > + bus_dmamap_destroy(sc->sc_dmat, txm->txm_map_tso); > > } > > > > ixl_dmamem_free(sc, &txr->txr_mem); > > This continue is wrong as the first one also affects the second map. > > if (txm->txm_map != NULL) > bus_dmamap_destroy(sc->sc_dmat, txm->txm_map); > if (txm->txm_map_tso != NULL) > bus_dmamap_destroy(sc->sc_dmat, txm->txm_map_tso); > > > @@ -2680,7 +2693,10 @@ ixl_txr_clean(struct ixl_softc *sc, stru > > if (txm->txm_m == NULL) > > continue; > > > > - map = txm->txm_map; > > + if (ISSET(txm->txm_m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + map = txm->txm_map_tso; > > + else > > + map = txm->txm_map; > > bus_dmamap_sync(sc->sc_dmat, map, 0, map->dm_mapsize, > > BUS_DMASYNC_POSTWRITE); > > bus_dmamap_unload(sc->sc_dmat, map); > > @@ -2739,6 +2755,7 @@ ixl_txr_free(struct ixl_softc *sc, struc > > txm = &maps[i]; > > > > bus_dmamap_destroy(sc->sc_dmat, txm->txm_map); > > + bus_dmamap_destroy(sc->sc_dmat, txm->txm_map_tso); > > } > > > > ixl_dmamem_free(sc, &txr->txr_mem); > > @@ -2885,7 +2902,7 @@ ixl_start(struct ifqueue *ifq) > > > > for (;;) { > > /* We need one extra descriptor for TSO packets. */ > > - if (free <= (IXL_TX_PKT_DESCS + 1)) { > > + if (free <= (IXL_TX_TSO_PKT_DESCS + 1)) { > > ifq_set_oactive(ifq); > > break; > > } > > @@ -2897,7 +2914,10 @@ ixl_start(struct ifqueue *ifq) > > offload = ixl_tx_setup_offload(m, txr, prod); > > > > txm = &txr->txr_maps[prod]; > > - map = txm->txm_map; > > + if (ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + map = txm->txm_map_tso; > > + else > > + map = txm->txm_map; > > > > if (ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) { > > prod++; > > @@ -2988,7 +3008,10 @@ ixl_txeof(struct ixl_softc *sc, struct i > > if (dtype != htole64(IXL_TX_DESC_DTYPE_DONE)) > > break; > > > > - map = txm->txm_map; > > + if (ISSET(txm->txm_m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + map = txm->txm_map_tso; > > + else > > + map = txm->txm_map; > > > > bus_dmamap_sync(sc->sc_dmat, map, 0, map->dm_mapsize, > > BUS_DMASYNC_POSTWRITE); >