| Message ID | 4603a4f68616ce41aca97bac2f55e5d51c865f53.1722421644.git.0x1207@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: FPE via ethtool + tc | expand |
On Wed, Jul 31, 2024 at 06:43:14PM +0800, Furong Xu wrote: > tc-taprio can select whether traffic classes are express or preemptible. > > After some traffic tests, MAC merge layer statistics are all good. > > Local device: > ethtool --include-statistics --json --show-mm eth1 > [ { > "ifname": "eth1", > "pmac-enabled": true, > "tx-enabled": true, > "tx-active": true, > "tx-min-frag-size": 60, > "rx-min-frag-size": 60, > "verify-enabled": true, > "verify-time": 100, > "max-verify-time": 128, > "verify-status": "SUCCEEDED", > "statistics": { > "MACMergeFrameAssErrorCount": 0, > "MACMergeFrameSmdErrorCount": 0, > "MACMergeFrameAssOkCount": 0, > "MACMergeFragCountRx": 0, > "MACMergeFragCountTx": 1398, > "MACMergeHoldCount": 15783 > } > } ] > > Remote device: > ethtool --include-statistics --json --show-mm eth1 > [ { > "ifname": "eth1", > "pmac-enabled": true, > "tx-enabled": true, > "tx-active": true, > "tx-min-frag-size": 60, > "rx-min-frag-size": 60, > "verify-enabled": true, > "verify-time": 100, > "max-verify-time": 128, > "verify-status": "SUCCEEDED", > "statistics": { > "MACMergeFrameAssErrorCount": 0, > "MACMergeFrameSmdErrorCount": 0, > "MACMergeFrameAssOkCount": 1388, > "MACMergeFragCountRx": 1398, > "MACMergeFragCountTx": 0, > "MACMergeHoldCount": 0 > } > } ] > > Tested on DWMAC CORE 5.10a > > Signed-off-by: Furong Xu <0x1207@gmail.com> > --- > .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 34 ++----------------- > 1 file changed, 3 insertions(+), 31 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > index 494fe2f68300..eeb5eb453b98 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > @@ -943,7 +943,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv, > u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep; > struct timespec64 time, current_time, qopt_time; > ktime_t current_time_ns; > - bool fpe = false; > int i, ret = 0; > u64 ctr; > > @@ -1028,16 +1027,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv, > > switch (qopt->entries[i].command) { > case TC_TAPRIO_CMD_SET_GATES: > - if (fpe) > - return -EINVAL; > - break; > - case TC_TAPRIO_CMD_SET_AND_HOLD: > - gates |= BIT(0); > - fpe = true; > - break; > - case TC_TAPRIO_CMD_SET_AND_RELEASE: > - gates &= ~BIT(0); > - fpe = true; I don't think these can go. In the DWMAC5 manual, I see: "To enable the support of hold and release operations, the format of the GCL is slightly changed while configuring and enabling the FPE. The first Queue (Q0) is always programmed to carry preemption traffic and therefore it is always Open. The bit corresponding to Q0 is renamed as Release/Hold MAC control. The TX Queues whose packets are preemptable are indicated by setting the PEC field of the MTL_FPE_CTRL_STS register. The GCL bit of the corresponding Queue are ignored and considered as always "Open". So, even if the software writes a "0" ("C"), it is ignored for such queues." It's relatively clear to me that this is what the "gates" variable is doing here - it's modulating when preemptible traffic begins to be "held", and when it is "released". Now, the "fpe" variable - that can definitely go. > break; > default: > return -EOPNOTSUPP; Also, this is more general advice. If TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE used to work as UAPI input into this driver, you don't want to break that now by letting those fall into the default -EOPNOTSUPP case. What worked must continue to work... somehow. > @@ -1068,16 +1057,11 @@ static int tc_taprio_configure(struct stmmac_priv *priv, > > tc_taprio_map_maxsdu_txq(priv, qopt); > > - if (fpe && !priv->dma_cap.fpesel) { > + if (qopt->mqprio.preemptible_tcs && !priv->dma_cap.fpesel) { > mutex_unlock(&priv->est_lock); > return -EOPNOTSUPP; > } > > - /* Actual FPE register configuration will be done after FPE handshake > - * is success. > - */ > - priv->plat->fpe_cfg->enable = fpe; > - > ret = stmmac_est_configure(priv, priv, priv->est, > priv->plat->clk_ptp_rate); > mutex_unlock(&priv->est_lock);
On Wed, Jul 31, 2024 at 06:43:14PM +0800, Furong Xu wrote: > tc-taprio can select whether traffic classes are express or preemptible. > > After some traffic tests, MAC merge layer statistics are all good. > > Local device: > ethtool --include-statistics --json --show-mm eth1 > [ { > "ifname": "eth1", > "pmac-enabled": true, > "tx-enabled": true, > "tx-active": true, > "tx-min-frag-size": 60, > "rx-min-frag-size": 60, > "verify-enabled": true, > "verify-time": 100, > "max-verify-time": 128, > "verify-status": "SUCCEEDED", > "statistics": { > "MACMergeFrameAssErrorCount": 0, > "MACMergeFrameSmdErrorCount": 0, > "MACMergeFrameAssOkCount": 0, > "MACMergeFragCountRx": 0, > "MACMergeFragCountTx": 1398, > "MACMergeHoldCount": 15783 In order for readers to really understand this output (including me), could you also post the associated tc-taprio command, please? You deleted the code that treated the Set-And-Hold-MAC GCL command - and according to 802.1Q, that is the only source of Hold requests. I _think_ that as a side effect of your reimplementation, every time the gate for TC 0 opens, the HoldCount bumps by one. Would that be a correct description? The more unfortunate part is that I haven't yet come across a NIC hardware design that would behave completely as you'd expect w.r.t. Hold requests. In the case of DWMAC, I would expect that with a taprio schedule that lacks any Set-And-Hold-MAC command, the HoldCount would stay at zero. I'm not sure, given the way they piggy back onto gate 0 for Hold/Release, that this is possible :( At least HoldCount stays constant with a tc-mqprio offload, right? > } > } ]
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c index 494fe2f68300..eeb5eb453b98 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c @@ -943,7 +943,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv, u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep; struct timespec64 time, current_time, qopt_time; ktime_t current_time_ns; - bool fpe = false; int i, ret = 0; u64 ctr; @@ -1028,16 +1027,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv, switch (qopt->entries[i].command) { case TC_TAPRIO_CMD_SET_GATES: - if (fpe) - return -EINVAL; - break; - case TC_TAPRIO_CMD_SET_AND_HOLD: - gates |= BIT(0); - fpe = true; - break; - case TC_TAPRIO_CMD_SET_AND_RELEASE: - gates &= ~BIT(0); - fpe = true; break; default: return -EOPNOTSUPP; @@ -1068,16 +1057,11 @@ static int tc_taprio_configure(struct stmmac_priv *priv, tc_taprio_map_maxsdu_txq(priv, qopt); - if (fpe && !priv->dma_cap.fpesel) { + if (qopt->mqprio.preemptible_tcs && !priv->dma_cap.fpesel) { mutex_unlock(&priv->est_lock); return -EOPNOTSUPP; } - /* Actual FPE register configuration will be done after FPE handshake - * is success. - */ - priv->plat->fpe_cfg->enable = fpe; - ret = stmmac_est_configure(priv, priv, priv->est, priv->plat->clk_ptp_rate); mutex_unlock(&priv->est_lock); @@ -1088,10 +1072,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv, netdev_info(priv->dev, "configured EST\n"); - if (fpe) { - stmmac_fpe_handshake(priv, true); - netdev_info(priv->dev, "start FPE handshake\n"); - } + stmmac_fpe_set_preemptible_tcs(priv, priv->ioaddr, qopt->mqprio.preemptible_tcs); return 0; @@ -1109,16 +1090,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv, mutex_unlock(&priv->est_lock); } - priv->plat->fpe_cfg->enable = false; - stmmac_fpe_configure(priv, priv->ioaddr, - priv->plat->fpe_cfg, - priv->plat->tx_queues_to_use, - priv->plat->rx_queues_to_use, - false); - netdev_info(priv->dev, "disabled FPE\n"); - - stmmac_fpe_handshake(priv, false); - netdev_info(priv->dev, "stop FPE handshake\n"); + stmmac_fpe_set_preemptible_tcs(priv, priv->ioaddr, 0); return ret; }
tc-taprio can select whether traffic classes are express or preemptible. After some traffic tests, MAC merge layer statistics are all good. Local device: ethtool --include-statistics --json --show-mm eth1 [ { "ifname": "eth1", "pmac-enabled": true, "tx-enabled": true, "tx-active": true, "tx-min-frag-size": 60, "rx-min-frag-size": 60, "verify-enabled": true, "verify-time": 100, "max-verify-time": 128, "verify-status": "SUCCEEDED", "statistics": { "MACMergeFrameAssErrorCount": 0, "MACMergeFrameSmdErrorCount": 0, "MACMergeFrameAssOkCount": 0, "MACMergeFragCountRx": 0, "MACMergeFragCountTx": 1398, "MACMergeHoldCount": 15783 } } ] Remote device: ethtool --include-statistics --json --show-mm eth1 [ { "ifname": "eth1", "pmac-enabled": true, "tx-enabled": true, "tx-active": true, "tx-min-frag-size": 60, "rx-min-frag-size": 60, "verify-enabled": true, "verify-time": 100, "max-verify-time": 128, "verify-status": "SUCCEEDED", "statistics": { "MACMergeFrameAssErrorCount": 0, "MACMergeFrameSmdErrorCount": 0, "MACMergeFrameAssOkCount": 1388, "MACMergeFragCountRx": 1398, "MACMergeFragCountTx": 0, "MACMergeHoldCount": 0 } } ] Tested on DWMAC CORE 5.10a Signed-off-by: Furong Xu <0x1207@gmail.com> --- .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 34 ++----------------- 1 file changed, 3 insertions(+), 31 deletions(-)