From: jan@openbsd.org Subject: Re: igc(4) tso To: Bjorn Ketelaars Cc: Moritz Buhl , tech@openbsd.org Date: Wed, 20 Mar 2024 00:17:24 +0100 On Tue, Mar 19, 2024 at 08:01:10PM +0100, Bjorn Ketelaars wrote: > On Mon 18/03/2024 13:34, Jan Klemkow wrote: > > On Fri, Mar 01, 2024 at 07:42:59AM +0100, Bjorn Ketelaars wrote: > > > On Tue 09/01/2024 23:22, 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 am not too happy how olinfo_status is initialized now so I am > > > > open to suggestions. > > > > > > > > I tested the diff on I225-V and I225-LM in various configurations. > > > > More testing welcome. > > > > > > As privately discussed, I rebased your diff on top of recent changes to > > > igc(4). Initially the interface locked up after about an hour, which was > > > fixed by copying a recent change to ix(4), em(4), and others (from > > > bluhm@ [0]). With this I have been running your diff for the last couple > > > of days without any issues. > > > > > > $ dmesg | grep igc0 > > > igc0 at pci2 dev 0 function 0 "Intel I225-V" rev 0x03, msix, 4 queues, address 00:e2:69:5b:a6:aa > > > > > > $ ifconfig igc0 hwfeatures > > > igc0: flags=8a43 mtu 1500 > > > hwfeatures=31b7 hardmtu 9216 > > > lladdr 00:e2:69:5b:a6:aa > > > index 1 priority 0 llprio 3 > > > media: Ethernet autoselect (2500baseT full-duplex) > > > status: active > > > > > > I refactored your diff to make it look more similar to the approach in > > > ix(4), and split it up in 1.) a vlan hwtagging part and 2.) a tso part, > > > as has been suggested by jan@. > > > > > > Included with this mail is the hwtagging part, which has been tested > > > successfully on a system with multiple igc(4) interfaces. I will send > > > the tso part later. BTW, the issue with locking that has been resolved > > > by picking a change from [0] is in the tso part. > > > > > > [0] https://codeberg.org/OpenBSD/src/commit/e78a66e5775d3e9cf59b16f193be327ae6d56426 > > > > > > diff --git sys/dev/pci/if_igc.c sys/dev/pci/if_igc.c > > > index b88644ade21..159051d8124 100644 > > > --- sys/dev/pci/if_igc.c > > > +++ sys/dev/pci/if_igc.c > > > @@ -117,7 +117,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,10 +791,8 @@ 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; > > > @@ -1001,7 +1000,11 @@ igc_start(struct ifqueue *ifq) > > > 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)) { > > > + cmd_type_len = IGC_ADVTXD_DCMD_IFCS | IGC_ADVTXD_DTYP_DATA | > > > + IGC_ADVTXD_DCMD_DEXT; > > > + > > > + if (igc_tx_ctx_setup(txr, m, prod, &cmd_type_len, > > > + &olinfo_status)) { > > > /* Consume the first descriptor */ > > > prod++; > > > prod &= mask; > > > @@ -1011,14 +1014,11 @@ igc_start(struct ifqueue *ifq) > > > 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; > > > > Please, do not use the conditional operator to check for map->dm_nsegs. > > Keep the original if statement. Thus, code it is more readable. > > > > Rest of the diff looks ok to me. > > Updated VLAN_HWTAGGING diff, which addresses feedback of jan@ > > diff --git sys/dev/pci/if_igc.c sys/dev/pci/if_igc.c > index b88644ade21..98cba52cfe2 100644 > --- sys/dev/pci/if_igc.c > +++ sys/dev/pci/if_igc.c > @@ -117,7 +117,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,10 +791,8 @@ 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; > @@ -954,7 +953,7 @@ 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, cmd_type_len_t; > uint32_t olinfo_status; > int post = 0; > #if NBPFILTER > 0 > @@ -1001,7 +1000,11 @@ igc_start(struct ifqueue *ifq) > 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)) { > + cmd_type_len = IGC_ADVTXD_DCMD_IFCS | IGC_ADVTXD_DTYP_DATA | > + IGC_ADVTXD_DCMD_DEXT; > + > + if (igc_tx_ctx_setup(txr, m, prod, &cmd_type_len, > + &olinfo_status)) { > /* Consume the first descriptor */ > prod++; > prod &= mask; > @@ -1011,16 +1014,20 @@ igc_start(struct ifqueue *ifq) > 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; > + cmd_type_len_t = cmd_type_len; > + > + cmd_type_len |= 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); > + htolem64(&txdesc->read.buffer_addr, > + map->dm_segs[i].ds_addr); > htolem32(&txdesc->read.cmd_type_len, cmd_type_len); > htolem32(&txdesc->read.olinfo_status, olinfo_status); > > + cmd_type_len = cmd_type_len_t; > + > last = prod; > > prod++; Try to avoid a temporary variable. Just clear the length area like this diff. Thanks for you effort! bye, jan diff --git a/sys/dev/pci/if_igc.c b/sys/dev/pci/if_igc.c index 333517effd5..0f92ac71f48 100644 --- a/sys/dev/pci/if_igc.c +++ b/sys/dev/pci/if_igc.c @@ -953,7 +953,7 @@ igc_start(struct ifqueue *ifq) struct mbuf *m; unsigned int prod, free, last, i; unsigned int mask; - uint32_t cmd_type_len, cmd_type_len_t; + uint32_t cmd_type_len; uint32_t olinfo_status; int post = 0; #if NBPFILTER > 0 @@ -1014,8 +1014,7 @@ igc_start(struct ifqueue *ifq) for (i = 0; i < map->dm_nsegs; i++) { txdesc = &txr->tx_base[prod]; - cmd_type_len_t = cmd_type_len; - + CLR(cmd_type_len, IGC_ADVTXD_DTALEN_MASK); cmd_type_len |= map->dm_segs[i].ds_len; if (i == map->dm_nsegs - 1) cmd_type_len |= IGC_ADVTXD_DCMD_EOP | @@ -1026,8 +1025,6 @@ igc_start(struct ifqueue *ifq) htolem32(&txdesc->read.cmd_type_len, cmd_type_len); htolem32(&txdesc->read.olinfo_status, olinfo_status); - cmd_type_len = cmd_type_len_t; - last = prod; prod++; diff --git a/sys/dev/pci/igc_base.h b/sys/dev/pci/igc_base.h index aec33e0d40f..da6ded2caf2 100644 --- a/sys/dev/pci/igc_base.h +++ b/sys/dev/pci/igc_base.h @@ -45,6 +45,7 @@ struct igc_adv_tx_context_desc { }; /* Adv Transmit Descriptor Config Masks */ +#define IGC_ADVTXD_DTALEN_MASK 0x0000FFFF #define IGC_ADVTXD_DTYP_CTXT 0x00200000 /* Advanced Context Descriptor */ #define IGC_ADVTXD_DTYP_DATA 0x00300000 /* Advanced Data Descriptor */ #define IGC_ADVTXD_DCMD_EOP 0x01000000 /* End of Packet */