| Message ID | 20200207182443.1501016-3-vinicius.gomes@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | igc: Add initial TSN qdiscs offloading | expand |
Hi Vinicius, Quoting Vinicius Costa Gomes (2020-02-07 10:24:43) ... > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c ... > @@ -4497,6 +4516,32 @@ static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) > } > } > > +static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue, > + bool enable) > +{ > + struct igc_ring *ring; > + int i; > + > + if (queue < 0 || queue > adapter->num_tx_queues) > + return -EINVAL; I believe we have an off-by-one bug here. Shouldn't it be queue >= adapter->num_tx_queues instead? > @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, > case TC_SETUP_QDISC_TAPRIO: > return igc_tsn_enable_qbv_scheduling(adapter, type_data); > > + case TC_SETUP_QDISC_ETF: > + return igc_tsn_enable_launchtime(adapter, type_data); Consider the scenario where both TAPRIO and ETF offloads are disabled and we want to enable ETF offload. ETF depends on adapter->base_time is set to work properly, but I couldn't find where in this patch it is set. Could you please clarify that? Regards, Andre
Andre Guedes <andre.guedes@linux.intel.com> writes: > Hi Vinicius, > > Quoting Vinicius Costa Gomes (2020-02-07 10:24:43) > ... >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > ... >> @@ -4497,6 +4516,32 @@ static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) >> } >> } >> >> +static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue, >> + bool enable) >> +{ >> + struct igc_ring *ring; >> + int i; >> + >> + if (queue < 0 || queue > adapter->num_tx_queues) >> + return -EINVAL; > > I believe we have an off-by-one bug here. Shouldn't it be queue >= > adapter->num_tx_queues instead? I will test this. Thanks for noticing. > >> @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, >> case TC_SETUP_QDISC_TAPRIO: >> return igc_tsn_enable_qbv_scheduling(adapter, type_data); >> >> + case TC_SETUP_QDISC_ETF: >> + return igc_tsn_enable_launchtime(adapter, type_data); > > Consider the scenario where both TAPRIO and ETF offloads are disabled and we > want to enable ETF offload. ETF depends on adapter->base_time is set to work > properly, but I couldn't find where in this patch it is set. Could you please > clarify that? '->base_time' doesn't need to be set, i.e. if it's zero (which is the case that only ETF offloading is enabled), it should be fine. H ow we calculate the launchtime in igc_tx_launchtime() is able to handle 'base_time_ being zero just fine, and the BASET_{L,H} registers will have sane values when it's zero as well. Cheers, -- Vinicius
Hi Vinicius, Quoting Vinicius Costa Gomes (2020-02-10 17:13:13) > >> @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, > >> case TC_SETUP_QDISC_TAPRIO: > >> return igc_tsn_enable_qbv_scheduling(adapter, type_data); > >> > >> + case TC_SETUP_QDISC_ETF: > >> + return igc_tsn_enable_launchtime(adapter, type_data); > > > > Consider the scenario where both TAPRIO and ETF offloads are disabled and we > > want to enable ETF offload. ETF depends on adapter->base_time is set to work > > properly, but I couldn't find where in this patch it is set. Could you please > > clarify that? > > '->base_time' doesn't need to be set, i.e. if it's zero (which is the > case that only ETF offloading is enabled), it should be fine. H > > ow we calculate the launchtime in igc_tx_launchtime() is able to handle > 'base_time_ being zero just fine, and the BASET_{L,H} registers will > have sane values when it's zero as well. If '->base_time' is never set, I suspect ETF disabling has an issue. See this piece of code: int igc_tsn_offload_apply(struct igc_adapter *adapter) { bool is_any_enabled = adapter->base_time || is_any_launchtime(adapter); if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED) && !is_any_enabled) return 0; ... } By the time igc_tsn_offload_apply() is called, we had already set 'ring->launchtime' to false. Since IGC_FLAG_TSN_QBV_ENABLED isn't set and 'is_any_enabled' is false, we return without calling igc_tsn_disable_offload(). What am I missing? Thanks, Andre
Andre Guedes <andre.guedes@linux.intel.com> writes: > Hi Vinicius, > > Quoting Vinicius Costa Gomes (2020-02-10 17:13:13) >> >> @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, >> >> case TC_SETUP_QDISC_TAPRIO: >> >> return igc_tsn_enable_qbv_scheduling(adapter, type_data); >> >> >> >> + case TC_SETUP_QDISC_ETF: >> >> + return igc_tsn_enable_launchtime(adapter, type_data); >> > >> > Consider the scenario where both TAPRIO and ETF offloads are disabled and we >> > want to enable ETF offload. ETF depends on adapter->base_time is set to work >> > properly, but I couldn't find where in this patch it is set. Could you please >> > clarify that? >> >> '->base_time' doesn't need to be set, i.e. if it's zero (which is the >> case that only ETF offloading is enabled), it should be fine. H >> >> ow we calculate the launchtime in igc_tx_launchtime() is able to handle >> 'base_time_ being zero just fine, and the BASET_{L,H} registers will >> have sane values when it's zero as well. > > If '->base_time' is never set, I suspect ETF disabling has an issue. See this > piece of code: > > int igc_tsn_offload_apply(struct igc_adapter *adapter) > { > bool is_any_enabled = adapter->base_time || is_any_launchtime(adapter); > > if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED) && !is_any_enabled) > return 0; > > ... > } > > By the time igc_tsn_offload_apply() is called, we had already set > 'ring->launchtime' to false. Since IGC_FLAG_TSN_QBV_ENABLED isn't set and > 'is_any_enabled' is false, we return without calling igc_tsn_disable_offload(). > I am still not seeing the problem. If ETF offloading was disabled, and then enabled, igc_save_launchtime_params() got called and some of the queues have launchtime_enable bit set. igc_save_launchtime_params() calls igc_tsn_offload_apply() and IGC_FLAG_TSN_QBV_ENABLED is set. Later, when the user wants to disable ETF offloading (the only way is to remove the qdisc, remember), the same thing happens: save_launchtime() and then offload_apply(), but now the IGC_FLAG_TSN_QBV_ENABLED is enabled, but there's no user, so igc_tsn_disable_offload() is called and the controller is reset if necessary.
Andre Guedes <andre.guedes@linux.intel.com> writes: > Hi Vinicius, > > Quoting Vinicius Costa Gomes (2020-02-07 10:24:43) > ... >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > ... >> @@ -4497,6 +4516,32 @@ static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) >> } >> } >> >> +static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue, >> + bool enable) >> +{ >> + struct igc_ring *ring; >> + int i; >> + >> + if (queue < 0 || queue > adapter->num_tx_queues) >> + return -EINVAL; > > I believe we have an off-by-one bug here. Shouldn't it be queue >= > adapter->num_tx_queues instead? Will fix this. (just a note that this off-by-one cannot be exploited by luck almost, mqprio/taprio do the check correctly on their side) > >> @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, >> case TC_SETUP_QDISC_TAPRIO: >> return igc_tsn_enable_qbv_scheduling(adapter, type_data); >> >> + case TC_SETUP_QDISC_ETF: >> + return igc_tsn_enable_launchtime(adapter, type_data); > > Consider the scenario where both TAPRIO and ETF offloads are disabled and we > want to enable ETF offload. ETF depends on adapter->base_time is set to work > properly, but I couldn't find where in this patch it is set. Could you please > clarify that? > > Regards, > > Andre
Hi Vinicius, Quoting Vinicius Costa Gomes (2020-02-12 16:13:14) > >> >> @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, > >> >> case TC_SETUP_QDISC_TAPRIO: > >> >> return igc_tsn_enable_qbv_scheduling(adapter, type_data); > >> >> > >> >> + case TC_SETUP_QDISC_ETF: > >> >> + return igc_tsn_enable_launchtime(adapter, type_data); > >> > > >> > Consider the scenario where both TAPRIO and ETF offloads are disabled and we > >> > want to enable ETF offload. ETF depends on adapter->base_time is set to work > >> > properly, but I couldn't find where in this patch it is set. Could you please > >> > clarify that? > >> > >> '->base_time' doesn't need to be set, i.e. if it's zero (which is the > >> case that only ETF offloading is enabled), it should be fine. H > >> > >> ow we calculate the launchtime in igc_tx_launchtime() is able to handle > >> 'base_time_ being zero just fine, and the BASET_{L,H} registers will > >> have sane values when it's zero as well. > > > > If '->base_time' is never set, I suspect ETF disabling has an issue. See this > > piece of code: > > > > int igc_tsn_offload_apply(struct igc_adapter *adapter) > > { > > bool is_any_enabled = adapter->base_time || is_any_launchtime(adapter); > > > > if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED) && !is_any_enabled) > > return 0; > > > > ... > > } > > > > By the time igc_tsn_offload_apply() is called, we had already set > > 'ring->launchtime' to false. Since IGC_FLAG_TSN_QBV_ENABLED isn't set and > > 'is_any_enabled' is false, we return without calling igc_tsn_disable_offload(). > > > > I am still not seeing the problem. > > If ETF offloading was disabled, and then enabled, > igc_save_launchtime_params() got called and some of the queues have > launchtime_enable bit set. igc_save_launchtime_params() calls > igc_tsn_offload_apply() and IGC_FLAG_TSN_QBV_ENABLED is set. Later, when > the user wants to disable ETF offloading (the only way is to remove the > qdisc, remember), the same thing happens: save_launchtime() and then > offload_apply(), but now the IGC_FLAG_TSN_QBV_ENABLED is enabled, but > there's no user, so igc_tsn_disable_offload() is called and the > controller is reset if necessary. Oh, I see it now, thanks. The source of my misunderstanding was the fact that I was not expecting IGC_FLAG_TSN_QBV_ENABLED flag to be set since we are not enabling Qbv, but launchtime. I understand these features share configuration registers, but they are different features after all. - Andre
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h index 976fa97b082a..6c8850b43951 100644 --- a/drivers/net/ethernet/intel/igc/igc_defines.h +++ b/drivers/net/ethernet/intel/igc/igc_defines.h @@ -440,6 +440,7 @@ #define IGC_TQAVCTRL_TRANSMIT_MODE_TSN 0x00000001 #define IGC_TQAVCTRL_ENHANCED_QAV 0x00000008 +#define IGC_TXQCTL_QUEUE_MODE_LAUNCHT 0x00000001 #define IGC_TXQCTL_STRICT_CYCLE 0x00000002 #define IGC_TXQCTL_STRICT_END 0x00000004 #define IGC_TXQCTL_PREEMPTIBLE 0x00000008 diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 0cb0c72160ab..96c838393631 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -869,6 +869,23 @@ static int igc_write_mc_addr_list(struct net_device *netdev) return netdev_mc_count(netdev); } +static __le32 igc_tx_launchtime(struct igc_adapter *adapter, ktime_t txtime) +{ + ktime_t cycle_time = adapter->cycle_time; + ktime_t base_time = adapter->base_time; + u32 launchtime; + + /* FIXME: when using ETF together with taprio, we may have a + * case where 'delta' is larger than the cycle_time, this may + * cause problems if we don't read the current value of + * IGC_BASET, as the value writen into the launchtime + * descriptor field may be misinterpreted. + */ + div_s64_rem(ktime_sub_ns(txtime, base_time), cycle_time, &launchtime); + + return cpu_to_le32(launchtime); +} + static void igc_tx_ctxtdesc(struct igc_ring *tx_ring, struct igc_tx_buffer *first, u32 vlan_macip_lens, u32 type_tucmd, @@ -876,7 +893,6 @@ static void igc_tx_ctxtdesc(struct igc_ring *tx_ring, { struct igc_adv_tx_context_desc *context_desc; u16 i = tx_ring->next_to_use; - struct timespec64 ts; context_desc = IGC_TX_CTXTDESC(tx_ring, i); @@ -898,9 +914,12 @@ static void igc_tx_ctxtdesc(struct igc_ring *tx_ring, * should have been handled by the upper layers. */ if (tx_ring->launchtime_enable) { - ts = ktime_to_timespec64(first->skb->tstamp); + struct igc_adapter *adapter = netdev_priv(tx_ring->netdev); + ktime_t txtime = first->skb->tstamp; + first->skb->tstamp = ktime_set(0, 0); - context_desc->launch_time = cpu_to_le32(ts.tv_nsec / 32); + context_desc->launch_time = igc_tx_launchtime(adapter, + txtime); } else { context_desc->launch_time = 0; } @@ -4497,6 +4516,32 @@ static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) } } +static int igc_save_launchtime_params(struct igc_adapter *adapter, int queue, + bool enable) +{ + struct igc_ring *ring; + int i; + + if (queue < 0 || queue > adapter->num_tx_queues) + return -EINVAL; + + ring = adapter->tx_ring[queue]; + ring->launchtime_enable = enable; + + if (adapter->base_time) + return 0; + + adapter->cycle_time = NSEC_PER_SEC; + + for (i = 0; i < adapter->num_tx_queues; i++) { + ring = adapter->tx_ring[i]; + ring->start_time = 0; + ring->end_time = NSEC_PER_SEC; + } + + return 0; +} + static bool validate_schedule(const struct tc_taprio_qopt_offload *qopt) { int queue_uses[IGC_MAX_TX_QUEUES] = { }; @@ -4529,6 +4574,22 @@ static bool validate_schedule(const struct tc_taprio_qopt_offload *qopt) return true; } +static int igc_tsn_enable_launchtime(struct igc_adapter *adapter, + struct tc_etf_qopt_offload *qopt) +{ + struct igc_hw *hw = &adapter->hw; + int err; + + if (hw->mac.type != igc_i225) + return -EOPNOTSUPP; + + err = igc_save_launchtime_params(adapter, qopt->queue, qopt->enable); + if (err) + return err; + + return igc_tsn_offload_apply(adapter); +} + static int igc_save_qbv_schedule(struct igc_adapter *adapter, struct tc_taprio_qopt_offload *qopt) { @@ -4600,6 +4661,9 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, case TC_SETUP_QDISC_TAPRIO: return igc_tsn_enable_qbv_scheduling(adapter, type_data); + case TC_SETUP_QDISC_ETF: + return igc_tsn_enable_launchtime(adapter, type_data); + default: return -EOPNOTSUPP; } diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index ebd61b89d90e..05beb6b8ca00 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -3,6 +3,20 @@ #include "igc.h" +static bool is_any_launchtime(struct igc_adapter *adapter) +{ + int i; + + for (i = 0; i < adapter->num_tx_queues; i++) { + struct igc_ring *ring = adapter->tx_ring[i]; + + if (ring->launchtime_enable) + return true; + } + + return false; +} + /* Returns the TSN specific registers to their default values after * TSN offloading is disabled. */ @@ -87,6 +101,9 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) IGC_TXQCTL_STRICT_END; } + if (ring->launchtime_enable) + txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT; + wr32(IGC_TXQCTL(i), txqctl); } @@ -114,7 +131,7 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) int igc_tsn_offload_apply(struct igc_adapter *adapter) { - bool is_any_enabled = adapter->base_time; + bool is_any_enabled = adapter->base_time || is_any_launchtime(adapter); if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED) && !is_any_enabled) return 0;
This adds support for ETF offloading for the i225 controller. For i225, the LaunchTime feature is almost a subset of the Qbv feature. The main change from the i210 is that the launchtime of each packet is specified as an offset applied to the BASET register. BASET is automatically incremented each cycle. For i225, the approach chosen is to re-use most of the setup used for taprio offloading. With a few changes: - The more or less obvious one is that when ETF is enabled, we should set add the expected launchtime to the (advanced) transmit descriptor; - The less obvious, is that when taprio offloading is not enabled, we add a dummy schedule (all queues are open all the time, with a cycle time of 1 second). Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> --- drivers/net/ethernet/intel/igc/igc_defines.h | 1 + drivers/net/ethernet/intel/igc/igc_main.c | 70 +++++++++++++++++++- drivers/net/ethernet/intel/igc/igc_tsn.c | 19 +++++- 3 files changed, 86 insertions(+), 4 deletions(-)