Message ID | 20230710163503.2821068-5-anthony.l.nguyen@intel.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 8b86f10ab64eca0287ea8f7c94e9ad8b2e101c01 |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | igc: Fix corner cases for TSN offload | expand |
On Mon, Jul 10, 2023 at 09:35:01AM -0700, Tony Nguyen wrote: > From: Florian Kauer <florian.kauer@linutronix.de> > > The flags IGC_TXQCTL_STRICT_CYCLE and IGC_TXQCTL_STRICT_END > prevent the packet transmission over slot and cycle boundaries. > This is important for taprio offload where the slots and > cycles correspond to the slots and cycles configured for the > network. > > However, the Qbv offload feature of the i225 is also used for > enabling TX launchtime / ETF offload. In that case, however, > the cycle has no meaning for the network and is only used > internally to adapt the base time register after a second has > passed. > > Enabling strict mode in this case would unnecessarily prevent > the transmission of certain packets (i.e. at the boundary of a > second) and thus interferes with the ETF qdisc that promises > transmission at a certain point in time. > > Similar to ETF, this also applies to CBS offload that also should > not be influenced by strict mode unless taprio offload would be > enabled at the same time. > > This fully reverts > commit d8f45be01dd9 ("igc: Use strict cycles for Qbv scheduling") > but its commit message only describes what was already implemented > before that commit. The difference to a plain revert of that commit > is that it now copes with the base_time = 0 case that was fixed with > commit e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv") > > In particular, enabling strict mode leads to TX hang situations > under high traffic if taprio is applied WITHOUT taprio offload > but WITH ETF offload, e.g. as in > > sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \ > num_tc 1 \ > map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \ > queues 1@0 \ > base-time 0 \ > sched-entry S 01 300000 \ > flags 0x1 \ > txtime-delay 500000 \ > clockid CLOCK_TAI > sudo tc qdisc replace dev enp1s0 parent 100:1 etf \ > clockid CLOCK_TAI \ > delta 500000 \ > offload \ > skip_sock_check > > and traffic generator > > sudo trafgen -i traffic.cfg -o enp1s0 --cpp -n0 -q -t1400ns > > with traffic.cfg > > #define ETH_P_IP 0x0800 > > { > /* Ethernet Header */ > 0x30, 0x1f, 0x9a, 0xd0, 0xf0, 0x0e, # MAC Dest - adapt as needed > 0x24, 0x5e, 0xbe, 0x57, 0x2e, 0x36, # MAC Src - adapt as needed > const16(ETH_P_IP), > > /* IPv4 Header */ > 0b01000101, 0, # IPv4 version, IHL, TOS > const16(1028), # IPv4 total length (UDP length + 20 bytes (IP header)) > const16(2), # IPv4 ident > 0b01000000, 0, # IPv4 flags, fragmentation off > 64, # IPv4 TTL > 17, # Protocol UDP > csumip(14, 33), # IPv4 checksum > > /* UDP Header */ > 10, 0, 48, 1, # IP Src - adapt as needed > 10, 0, 48, 10, # IP Dest - adapt as needed > const16(5555), # UDP Src Port > const16(6666), # UDP Dest Port > const16(1008), # UDP length (UDP header 8 bytes + payload length) > csumudp(14, 34), # UDP checksum > > /* Payload */ > fill('W', 1000), > } > > and the observed message with that is for example > > igc 0000:01:00.0 enp1s0: Detected Tx Unit Hang > Tx Queue <0> > TDH <d0> > TDT <f0> > next_to_use <f0> > next_to_clean <d0> > buffer_info[next_to_clean] > time_stamp <ffff661f> > next_to_watch <00000000245a4efb> > jiffies <ffff6e48> > desc.status <1048000> > > Fixes: d8f45be01dd9 ("igc: Use strict cycles for Qbv scheduling") > Signed-off-by: Florian Kauer <florian.kauer@linutronix.de> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > Tested-by: Naama Meir <naamax.meir@linux.intel.com> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_tsn.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index b76ebfc10b1d..a9c08321aca9 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -132,8 +132,28 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) wr32(IGC_STQT(i), ring->start_time); wr32(IGC_ENDQT(i), ring->end_time); - txqctl |= IGC_TXQCTL_STRICT_CYCLE | - IGC_TXQCTL_STRICT_END; + if (adapter->taprio_offload_enable) { + /* If taprio_offload_enable is set we are in "taprio" + * mode and we need to be strict about the + * cycles: only transmit a packet if it can be + * completed during that cycle. + * + * If taprio_offload_enable is NOT true when + * enabling TSN offload, the cycle should have + * no external effects, but is only used internally + * to adapt the base time register after a second + * has passed. + * + * Enabling strict mode in this case would + * unnecessarily prevent the transmission of + * certain packets (i.e. at the boundary of a + * second) and thus interfere with the launchtime + * feature that promises transmission at a + * certain point in time. + */ + txqctl |= IGC_TXQCTL_STRICT_CYCLE | + IGC_TXQCTL_STRICT_END; + } if (ring->launchtime_enable) txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;