Download raw body.
igc(4) tso
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<UP,BROADCAST,RUNNING,ALLMULTI,SIMPLEX,MULTICAST> mtu 1500
> > > hwfeatures=31b7<CSUM_IPv4,CSUM_TCPv4,CSUM_UDPv4,VLAN_MTU,VLAN_HWTAGGING,CSUM_TCPv6,CSUM_UDPv6,TSOv4,TSOv6> 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 */
igc(4) tso