| Message ID | 20220902215702.3895073-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for Felix DSA driver calculation of tc-taprio guard bands | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 82 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi, Am 2022-09-02 23:57, schrieb Vladimir Oltean: > The blamed commit broke tc-taprio schedules such as this one: > > tc qdisc replace dev $swp1 root taprio \ > num_tc 8 \ > map 0 1 2 3 4 5 6 7 \ > queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \ > base-time 0 \ > sched-entry S 0x7f 990000 \ > sched-entry S 0x80 10000 \ > flags 0x2 > > because the gate entry for TC 7 (S 0x80 10000 ns) now has a static > guard > band added earlier than its 'gate close' event, such that packet > overruns won't occur in the worst case of the largest packet possible. > > Since guard bands are statically determined based on the per-tc > QSYS_QMAXSDU_CFG_* with a fallback on the port-based QSYS_PORT_MAX_SDU, > we need to discuss depending on kernel version, since the driver, prior > to commit 55a515b1f5a9 ("net: dsa: felix: drop oversized frames with > tc-taprio instead of hanging the port"), did not touch > QSYS_QMAXSDU_CFG_*, and therefore relied on QSYS_PORT_MAX_SDU. > > 1 (before vsc9959_tas_guard_bands_update): QSYS_PORT_MAX_SDU defaults > to > 1518, and at gigabit this introduces a static guard band (independent > of packet sizes) of 12144 ns. But this is larger than the time window > itself, of 10000 ns. So, the queue system never considers a frame > with > TC 7 as eligible for transmission, since the gate practically never > opens, and these frames are forever stuck in the TX queues and hang > the port. IIRC we deliberately ignored that problem back then, because we couldn't set the maxsdu. > 2 (after vsc9959_tas_guard_bands_update): We make an effort to set > QSYS_QMAXSDU_CFG_7 to 1230 bytes, and this enables oversized frame > dropping for everything larger than that. But QSYS_QMAXSDU_CFG_7 > plays > 2 roles. One is oversized frame dropping, the other is the per-tc > static guard band. When we calculated QSYS_QMAXSDU_CFG_7 to be 1230, > we considered no guard band at all, and the entire time window > available for transmission, which is not the case. The larger > QSYS_QMAXSDU_CFG_7 is, the larger the static guard band for the tc > is, > too. > > In both cases, frames with any size (even 60 bytes sans FCS) are stuck > on egress rather than being considered for scheduling on TC 7, even if > they fit. This is because the static guard band is way too large. > Considering the current situation, with > vsc9959_tas_guard_bands_update(), > frames between 60 octets and 1230 octets in size are not eligible for > oversized dropping (because they are smaller than QSYS_QMAXSDU_CFG_7), > but won't be considered as eligible for scheduling either, because the > min_gate_len[7] (10000 ns) - the guard band determined by > QSYS_QMAXSDU_CFG_7 (1230 octets * 8 ns per octet == 9840 ns) is smaller > than their transmit time. > > A solution that is quite outrageous is to limit the minimum valid gate > interval acceptable through tc-taprio, such that intervals, when > transformed into L1 frame bit times, are never smaller than twice the > MTU of the interface. However, the tc-taprio UAPI operates in ns, and > the link speed can change at runtime (to 10 Mbps, where the > transmission > time of 1 octet is 800 ns). And since the max MTU is around 9000, we'd > have to limit the tc-taprio intervals to be no smaller than 14.4 ms on > the premise that it is possible for the link to renegotiate to 10 Mbps, > which is astonishingly limiting for real use cases, where the entire > *cycle* (here we're talking about a single interval) must be 100 us or > lower. > > The solution is to modify vsc9959_tas_guard_bands_update() to take into > account that the static per-tc guard bands consume time out of our time > window too, not just packet transmission. The unknown which needs to be > determined is the max admissible frame size. Both the useful bit time > and the guard band size will depend on this unknown variable, so > dividing the available 10000 ns into 2 halves sounds like the ideal > strategy. In this case, we will program QSYS_QMAXSDU_CFG_7 with a > maximum frame length (and guard band size) of 605 octets (this includes > FCS but not IPG and preamble/SFD). With this value, everything of L2 > size 601 (sans FCS) and higher is considered as oversized, and the > guard > band is low enough (605 + HSCH_MISC.FRM_ADJ, at 1Gbps => 5000 ns) in > order to not disturb the scheduling of any frame smaller than L2 size > 601. So one drawback with this is that you limit the maxsdu to match a frame half of the gate open time, right? The switch just schedule the *start* event of the frame. So even if the guard band takes 99% of the gate open time, it should be able to send a frame regardless of it's length during the first 1% of the period (and it doesn't limit the maxsdu by half). IIRC the guard band is exactly for that, that is that you don't know the frame length and you can still schedule the frame. I know of switches which don't use a guard band but know the exact length and the closing time of the queue and deduce by that if the frame can still be queued. Actually, I'd expect it to work after your vsc9959_tas_guard_bands_update. Hmm. To quote from you above: > min_gate_len[7] (10000 ns) - the guard band determined by > QSYS_QMAXSDU_CFG_7 (1230 octets * 8 ns per octet == 9840 ns) is smaller > than their transmit time. Are you sure this is the case? There should be 160ns time to schedule the start of the frame. Maybe the 160ns is just too small. -michael
On Mon, Sep 05, 2022 at 09:29:44AM +0200, Michael Walle wrote: > > 1 (before vsc9959_tas_guard_bands_update): QSYS_PORT_MAX_SDU defaults to > > 1518, and at gigabit this introduces a static guard band (independent > > of packet sizes) of 12144 ns. But this is larger than the time window > > itself, of 10000 ns. So, the queue system never considers a frame with > > TC 7 as eligible for transmission, since the gate practically never > > opens, and these frames are forever stuck in the TX queues and hang > > the port. > > IIRC we deliberately ignored that problem back then, because we couldn't > set the maxsdu. I don't remember exactly why that is. It seems stupid to ignore a condition that leads to the port hanging. I think part of the problem was that I didn't have a test setup at the time the guard band patches were proposed. > > The solution is to modify vsc9959_tas_guard_bands_update() to take into > > account that the static per-tc guard bands consume time out of our time > > window too, not just packet transmission. The unknown which needs to be > > determined is the max admissible frame size. Both the useful bit time > > and the guard band size will depend on this unknown variable, so > > dividing the available 10000 ns into 2 halves sounds like the ideal > > strategy. In this case, we will program QSYS_QMAXSDU_CFG_7 with a > > maximum frame length (and guard band size) of 605 octets (this includes > > FCS but not IPG and preamble/SFD). With this value, everything of L2 > > size 601 (sans FCS) and higher is considered as oversized, and the guard > > band is low enough (605 + HSCH_MISC.FRM_ADJ, at 1Gbps => 5000 ns) in > > order to not disturb the scheduling of any frame smaller than L2 size > > 601. > > So one drawback with this is that you limit the maxsdu to match a > frame half of the gate open time, right? Yes. > The switch just schedule the *start* event of the frame. So even if > the guard band takes 99% of the gate open time, it should be able > to send a frame regardless of it's length during the first 1% of > the period (and it doesn't limit the maxsdu by half). IIRC the guard > band is exactly for that, that is that you don't know the frame > length and you can still schedule the frame. I know of switches > which don't use a guard band but know the exact length and the > closing time of the queue and deduce by that if the frame can > still be queued. > > Actually, I'd expect it to work after your vsc9959_tas_guard_bands_update. > Hmm. > > To quote from you above: > > min_gate_len[7] (10000 ns) - the guard band determined by > > QSYS_QMAXSDU_CFG_7 (1230 octets * 8 ns per octet == 9840 ns) is smaller > > than their transmit time. > > Are you sure this is the case? There should be 160ns time to > schedule the start of the frame. Maybe the 160ns is just too > small. Yes, I'm absolutely sure that any packet gets dropped on egress with a 10 us window, and I can see from my explanation why that is not obvious. The reason is because the guard band for tc 7 is not only determined by QSYS_QMAXSDU_CFG_7, but also by adding the L1 overhead configured through HSCH_MISC.FRM_ADJ (default 20 decimal). So from the remaining 160 ns, we also lose 20 * 8 = 160 ns to the L1 overhead, and that's why the switch doesn't schedule anything. In fact now I finally understand the private message that Xiaoliang sent to me, where he said that he can make things work by making HSCH_MISC.FRM_ADJ smaller than the default of 20. I initially didn't understand why you'd want to do that. The problem with HSCH_MISC.FRM_ADJ is that it's global to the switch, and it's also used for some other shaper computations, so altering it is not such a great idea. But you (and Xiaoliang) do raise a valid point that the switch doesn't need a full window size of open gate to schedule a full window size worth of packet. So cutting the available window size in half is a bit drastic. I'll think a bit more whether there is any smarter adjustment I can do to ensure that any window, after trimming the extended static guard band, still has 32 ns (IIRC, that's the minimum required) of time. That should still ensure we don't have overruns. If you have any idea, shoot.
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 1cdce8a98d1d..6fa4e0161b34 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -1599,9 +1599,10 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port) u32 max_sdu; if (min_gate_len[tc] == U64_MAX /* Gate always open */ || - min_gate_len[tc] * PSEC_PER_NSEC > needed_bit_time_ps) { + min_gate_len[tc] * PSEC_PER_NSEC > 2 * needed_bit_time_ps) { /* Setting QMAXSDU_CFG to 0 disables oversized frame - * dropping. + * dropping and leaves just the port-based static + * guard band. */ max_sdu = 0; dev_dbg(ocelot->dev, @@ -1612,9 +1613,17 @@ static void vsc9959_tas_guard_bands_update(struct ocelot *ocelot, int port) /* If traffic class doesn't support a full MTU sized * frame, make sure to enable oversize frame dropping * for frames larger than the smallest that would fit. + * + * However, the exact same register, * QSYS_QMAXSDU_CFG_*, + * controls not only oversized frame dropping, but also + * per-tc static guard band lengths. Therefore, the max + * SDU supported by this tc is determined by splitting + * its time window into 2: one for the useful traffic + * and one for the guard band. Both halves have the + * length equal to one max sized packet. */ max_sdu = div_u64(min_gate_len[tc] * PSEC_PER_NSEC, - picos_per_byte); + 2 * picos_per_byte); /* A TC gate may be completely closed, which is a * special case where all packets are oversized. * Any limit smaller than 64 octets accomplishes this
The blamed commit broke tc-taprio schedules such as this one: tc qdisc replace dev $swp1 root taprio \ num_tc 8 \ map 0 1 2 3 4 5 6 7 \ queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \ base-time 0 \ sched-entry S 0x7f 990000 \ sched-entry S 0x80 10000 \ flags 0x2 because the gate entry for TC 7 (S 0x80 10000 ns) now has a static guard band added earlier than its 'gate close' event, such that packet overruns won't occur in the worst case of the largest packet possible. Since guard bands are statically determined based on the per-tc QSYS_QMAXSDU_CFG_* with a fallback on the port-based QSYS_PORT_MAX_SDU, we need to discuss depending on kernel version, since the driver, prior to commit 55a515b1f5a9 ("net: dsa: felix: drop oversized frames with tc-taprio instead of hanging the port"), did not touch QSYS_QMAXSDU_CFG_*, and therefore relied on QSYS_PORT_MAX_SDU. 1 (before vsc9959_tas_guard_bands_update): QSYS_PORT_MAX_SDU defaults to 1518, and at gigabit this introduces a static guard band (independent of packet sizes) of 12144 ns. But this is larger than the time window itself, of 10000 ns. So, the queue system never considers a frame with TC 7 as eligible for transmission, since the gate practically never opens, and these frames are forever stuck in the TX queues and hang the port. 2 (after vsc9959_tas_guard_bands_update): We make an effort to set QSYS_QMAXSDU_CFG_7 to 1230 bytes, and this enables oversized frame dropping for everything larger than that. But QSYS_QMAXSDU_CFG_7 plays 2 roles. One is oversized frame dropping, the other is the per-tc static guard band. When we calculated QSYS_QMAXSDU_CFG_7 to be 1230, we considered no guard band at all, and the entire time window available for transmission, which is not the case. The larger QSYS_QMAXSDU_CFG_7 is, the larger the static guard band for the tc is, too. In both cases, frames with any size (even 60 bytes sans FCS) are stuck on egress rather than being considered for scheduling on TC 7, even if they fit. This is because the static guard band is way too large. Considering the current situation, with vsc9959_tas_guard_bands_update(), frames between 60 octets and 1230 octets in size are not eligible for oversized dropping (because they are smaller than QSYS_QMAXSDU_CFG_7), but won't be considered as eligible for scheduling either, because the min_gate_len[7] (10000 ns) - the guard band determined by QSYS_QMAXSDU_CFG_7 (1230 octets * 8 ns per octet == 9840 ns) is smaller than their transmit time. A solution that is quite outrageous is to limit the minimum valid gate interval acceptable through tc-taprio, such that intervals, when transformed into L1 frame bit times, are never smaller than twice the MTU of the interface. However, the tc-taprio UAPI operates in ns, and the link speed can change at runtime (to 10 Mbps, where the transmission time of 1 octet is 800 ns). And since the max MTU is around 9000, we'd have to limit the tc-taprio intervals to be no smaller than 14.4 ms on the premise that it is possible for the link to renegotiate to 10 Mbps, which is astonishingly limiting for real use cases, where the entire *cycle* (here we're talking about a single interval) must be 100 us or lower. The solution is to modify vsc9959_tas_guard_bands_update() to take into account that the static per-tc guard bands consume time out of our time window too, not just packet transmission. The unknown which needs to be determined is the max admissible frame size. Both the useful bit time and the guard band size will depend on this unknown variable, so dividing the available 10000 ns into 2 halves sounds like the ideal strategy. In this case, we will program QSYS_QMAXSDU_CFG_7 with a maximum frame length (and guard band size) of 605 octets (this includes FCS but not IPG and preamble/SFD). With this value, everything of L2 size 601 (sans FCS) and higher is considered as oversized, and the guard band is low enough (605 + HSCH_MISC.FRM_ADJ, at 1Gbps => 5000 ns) in order to not disturb the scheduling of any frame smaller than L2 size 601. Fixes: 297c4de6f780 ("net: dsa: felix: re-enable TAS guard band mode") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/ocelot/felix_vsc9959.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)