From: Jan Klemkow Subject: Re: igc(4) tso To: Moritz Buhl Cc: tech@openbsd.org Date: Wed, 10 Jan 2024 06:46:54 +0100 On Tue, Jan 09, 2024 at 11:22:37PM +0100, Moritz Buhl wrote: > Here is a diff that implements tso for igc(4). > It looks very similar to the other intel cards but it was missing > VLAN_HWTAGGING. I suggest to separate the vlan tagging part and commit it upfront of the tso part. Thus, your diff is shorter and we could better identify regressions in one of it. > I am not too happy how olinfo_status is initialized now so I am > open to suggestions. Some comments below. bye, Jan > I tested the diff on I225-V and I225-LM in various configurations. > More testing welcome. > > mbuhl > > Index: dev/pci//if_igc.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/if_igc.c,v > diff -u -p -r1.14 if_igc.c > --- dev/pci//if_igc.c 10 Nov 2023 15:51:20 -0000 1.14 > +++ dev/pci//if_igc.c 9 Jan 2024 18:46:50 -0000 > @@ -44,12 +44,16 @@ > > #include > #include > +#include > #include > > #include > #include > #include > #include > +#include > +#include > +#include > > #if NBPFILTER > 0 > #include > @@ -117,7 +121,8 @@ int igc_media_change(struct ifnet *); > void igc_iff(struct igc_softc *); > void igc_update_link_status(struct igc_softc *); > int igc_get_buf(struct rx_ring *, int); > -int igc_tx_ctx_setup(struct tx_ring *, struct mbuf *, int, uint32_t *); > +int igc_tx_ctx_setup(struct tx_ring *, struct mbuf *, int, uint32_t *, > + uint32_t *); > > void igc_configure_queues(struct igc_softc *); > void igc_set_queues(struct igc_softc *, uint32_t, uint32_t, int); > @@ -790,15 +795,14 @@ igc_setup_interface(struct igc_softc *sc > > ifp->if_capabilities = IFCAP_VLAN_MTU; > > -#ifdef notyet > #if NVLAN > 0 > ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING; > #endif > -#endif > > ifp->if_capabilities |= IFCAP_CSUM_IPv4; > ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4; > ifp->if_capabilities |= IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6; > + ifp->if_capabilities |= IFCAP_TSOv4 | IFCAP_TSOv6; > > /* Initialize ifmedia structures. */ > ifmedia_init(&sc->media, IFM_IMASK, igc_media_change, igc_media_status); > @@ -954,7 +958,8 @@ igc_start(struct ifqueue *ifq) > struct mbuf *m; > unsigned int prod, free, last, i; > unsigned int mask; > - uint32_t cmd_type_len; > + uint32_t cmd_type_len = IGC_ADVTXD_DCMD_IFCS | IGC_ADVTXD_DTYP_DATA | > + IGC_ADVTXD_DCMD_DEXT; > uint32_t olinfo_status; > int post = 0; > #if NBPFILTER > 0 > @@ -996,29 +1001,31 @@ igc_start(struct ifqueue *ifq) > continue; > } > > - olinfo_status = m->m_pkthdr.len << IGC_ADVTXD_PAYLEN_SHIFT; > - > bus_dmamap_sync(txr->txdma.dma_tag, map, 0, > map->dm_mapsize, BUS_DMASYNC_PREWRITE); > > - if (igc_tx_ctx_setup(txr, m, prod, &olinfo_status)) { > + if (ISSET(m->m_flags, M_VLANTAG)) > + cmd_type_len |= IGC_ADVTXD_DCMD_VLE; > + > + if (igc_tx_ctx_setup(txr, m, prod, &cmd_type_len, > + &olinfo_status)) { > /* Consume the first descriptor */ > prod++; > prod &= mask; > free--; > + } else { > + olinfo_status = > + m->m_pkthdr.len << IGC_ADVTXD_PAYLEN_SHIFT; > } > > for (i = 0; i < map->dm_nsegs; i++) { > txdesc = &txr->tx_base[prod]; > > - cmd_type_len = IGC_ADVTXD_DCMD_IFCS | IGC_ADVTXD_DTYP_DATA | > - IGC_ADVTXD_DCMD_DEXT | map->dm_segs[i].ds_len; > - if (i == map->dm_nsegs - 1) > - cmd_type_len |= IGC_ADVTXD_DCMD_EOP | > - IGC_ADVTXD_DCMD_RS; > - > - htolem64(&txdesc->read.buffer_addr, map->dm_segs[i].ds_addr); > - htolem32(&txdesc->read.cmd_type_len, cmd_type_len); > + htolem64(&txdesc->read.buffer_addr, > + map->dm_segs[i].ds_addr); > + htolem32(&txdesc->read.cmd_type_len, cmd_type_len | > + map->dm_segs[i].ds_len | ((i == map->dm_nsegs - 1)? > + IGC_ADVTXD_DCMD_EOP | IGC_ADVTXD_DCMD_RS : 0)); > htolem32(&txdesc->read.olinfo_status, olinfo_status); > > last = prod; > @@ -1999,23 +2006,26 @@ igc_free_transmit_buffers(struct tx_ring > > int > igc_tx_ctx_setup(struct tx_ring *txr, struct mbuf *mp, int prod, > - uint32_t *olinfo_status) > + uint32_t *cmd_type_len, uint32_t *olinfo_status) > { > struct ether_extracted ext; > struct igc_adv_tx_context_desc *txdesc; > + uint32_t mss_l4len_idx = 0; > uint32_t type_tucmd_mlhl = 0; > uint32_t vlan_macip_lens = 0; > - uint32_t iphlen; > + uint32_t ethlen, iphlen; > int off = 0; > > - vlan_macip_lens |= (sizeof(*ext.eh) << IGC_ADVTXD_MACLEN_SHIFT); Please call ether_extract_headers() before this line above. It looks super wired, if you use a struct member before initialization. Also, if its save for sizeof. Btw. we should avoid calling ether_extract_headers() and the rest of this function, when no offloading flags are used in this mbuf. > + if (mp->m_pkthdr.csum_flags & M_TCP_TSO) > + *olinfo_status = 0; > + else Remove this part and use CLR() below to override this value, like its done in ix(4). > + *olinfo_status = mp->m_pkthdr.len << IGC_ADVTXD_PAYLEN_SHIFT; > > /* > * In advanced descriptors the vlan tag must > * be placed into the context descriptor. Hence > * we need to make one even if not doing offloads. > */ > -#ifdef notyet > #if NVLAN > 0 > if (ISSET(mp->m_flags, M_VLANTAG)) { > uint32_t vtag = mp->m_pkthdr.ether_vtag; > @@ -2023,8 +2033,9 @@ igc_tx_ctx_setup(struct tx_ring *txr, st > off = 1; > } > #endif > -#endif > > + ethlen = sizeof(*ext.eh); > + vlan_macip_lens |= (ethlen << IGC_ADVTXD_MACLEN_SHIFT); > ether_extract_headers(mp, &ext); > > if (ext.ip4) { > @@ -2042,6 +2053,8 @@ igc_tx_ctx_setup(struct tx_ring *txr, st > type_tucmd_mlhl |= IGC_ADVTXD_TUCMD_IPV6; > #endif > } else { > + if (mp->m_pkthdr.csum_flags & M_TCP_TSO) > + tcpstat_inc(tcps_outbadtso); > return 0; > } > > @@ -2065,6 +2078,29 @@ igc_tx_ctx_setup(struct tx_ring *txr, st > if (off == 0) > return 0; > > + if (mp->m_pkthdr.csum_flags & M_TCP_TSO) { > + if (ext.tcp) { > + uint32_t hdrlen, thlen, paylen, outlen; > + > + thlen = ext.tcp->th_off << 2; > + > + outlen = mp->m_pkthdr.ph_mss; > + mss_l4len_idx |= outlen << IGC_ADVTXD_MSS_SHIFT; > + mss_l4len_idx |= thlen << IGC_ADVTXD_L4LEN_SHIFT; > + > + hdrlen = ethlen + iphlen + thlen; > + paylen = mp->m_pkthdr.len - hdrlen; > + *olinfo_status |= paylen << IGC_ADVTXD_PAYLEN_SHIFT; Do here some like this in ix(4): CLR(*olinfo_status, IXGBE_ADVTXD_PAYLEN_MASK << IXGBE_ADVTXD_PAYLEN_SHIFT); *olinfo_status |= paylen << IXGBE_ADVTXD_PAYLEN_SHIFT; > + > + *cmd_type_len |= IGC_ADVTXD_DCMD_TSE; > + off = 1; > + > + tcpstat_add(tcps_outpkttso, > + (paylen + outlen - 1) / outlen); > + } else > + tcpstat_inc(tcps_outbadtso); > + } > + > /* Now ready a context descriptor */ > txdesc = (struct igc_adv_tx_context_desc *)&txr->tx_base[prod]; > > @@ -2072,7 +2108,7 @@ igc_tx_ctx_setup(struct tx_ring *txr, st > htolem32(&txdesc->vlan_macip_lens, vlan_macip_lens); > htolem32(&txdesc->type_tucmd_mlhl, type_tucmd_mlhl); > htolem32(&txdesc->seqnum_seed, 0); > - htolem32(&txdesc->mss_l4len_idx, 0); > + htolem32(&txdesc->mss_l4len_idx, mss_l4len_idx); > > return 1; > } >