| Message ID | 20220606092747.16730-1-kurt@linutronix.de (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] igc: Lift TAPRIO schedule restriction | expand |
Hi Kurt, Kurt Kanzenbach <kurt@linutronix.de> writes: > Add support for Qbv schedules where one queue stays open > in consecutive entries. Currently that's not supported. > > Example schedule: > > |tc qdisc replace dev ${INTERFACE} handle 100 parent root taprio num_tc 3 \ > | map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ > | queues 1@0 1@1 2@2 \ > | base-time ${BASETIME} \ > | sched-entry S 0x01 300000 \ # Stream High/Low > | sched-entry S 0x06 500000 \ # Management and Best Effort > | sched-entry S 0x04 200000 \ # Best Effort > | flags 0x02 > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index ae17af44fe02..4758bdbe5df3 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -5813,9 +5813,10 @@ static bool validate_schedule(struct igc_adapter *adapter, > return false; > > for (n = 0; n < qopt->num_entries; n++) { > - const struct tc_taprio_sched_entry *e; > + const struct tc_taprio_sched_entry *e, *prev; > int i; > > + prev = n ? &qopt->entries[n - 1] : NULL; > e = &qopt->entries[n]; > > /* i225 only supports "global" frame preemption > @@ -5828,7 +5829,12 @@ static bool validate_schedule(struct igc_adapter *adapter, > if (e->gate_mask & BIT(i)) > queue_uses[i]++; > > - if (queue_uses[i] > 1) > + /* There are limitations: A single queue cannot be > + * opened and closed multiple times per cycle unless the > + * gate stays open. Check for it. > + */ > + if (queue_uses[i] > 1 && > + !(prev->gate_mask & BIT(i))) Perhaps I am missing something, I didn't try to run, but not checking if 'prev' can be NULL, could lead to crashes for some schedules, no? What I have in mind is a schedule that queue 0 is mentioned multiple times, for example: | sched-entry S 0x01 300000 \ # Stream High/Low | sched-entry S 0x03 500000 \ # Management and Best Effort | sched-entry S 0x05 200000 \ # Best Effort Anyway, looks much cleaner than what I had in mind when I wrote that fixme. Cheers,
Hi Vinicius, On Mon Jun 06 2022, Vinicius Costa Gomes wrote: > Hi Kurt, > > Kurt Kanzenbach <kurt@linutronix.de> writes: > >> Add support for Qbv schedules where one queue stays open >> in consecutive entries. Currently that's not supported. >> >> Example schedule: >> >> |tc qdisc replace dev ${INTERFACE} handle 100 parent root taprio num_tc 3 \ >> | map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ >> | queues 1@0 1@1 2@2 \ >> | base-time ${BASETIME} \ >> | sched-entry S 0x01 300000 \ # Stream High/Low >> | sched-entry S 0x06 500000 \ # Management and Best Effort >> | sched-entry S 0x04 200000 \ # Best Effort >> | flags 0x02 >> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> >> --- >> drivers/net/ethernet/intel/igc/igc_main.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index ae17af44fe02..4758bdbe5df3 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -5813,9 +5813,10 @@ static bool validate_schedule(struct igc_adapter *adapter, >> return false; >> >> for (n = 0; n < qopt->num_entries; n++) { >> - const struct tc_taprio_sched_entry *e; >> + const struct tc_taprio_sched_entry *e, *prev; >> int i; >> >> + prev = n ? &qopt->entries[n - 1] : NULL; >> e = &qopt->entries[n]; >> >> /* i225 only supports "global" frame preemption >> @@ -5828,7 +5829,12 @@ static bool validate_schedule(struct igc_adapter *adapter, >> if (e->gate_mask & BIT(i)) >> queue_uses[i]++; >> >> - if (queue_uses[i] > 1) >> + /* There are limitations: A single queue cannot be >> + * opened and closed multiple times per cycle unless the >> + * gate stays open. Check for it. >> + */ >> + if (queue_uses[i] > 1 && >> + !(prev->gate_mask & BIT(i))) > > Perhaps I am missing something, I didn't try to run, but not checking if > 'prev' can be NULL, could lead to crashes for some schedules, no? My thinking was that `prev` can never be NULL, as `queue_uses[i] > 1` is checked first. This condition can only be true if there are at least two entries. > > What I have in mind is a schedule that queue 0 is mentioned multiple > times, for example: > > | sched-entry S 0x01 300000 \ # Stream High/Low > | sched-entry S 0x03 500000 \ # Management and Best Effort > | sched-entry S 0x05 200000 \ # Best Effort > So, this schedule works with the proposed patch. Queue 0 is opened in all three entries. My debug code shows: |tc-6145 [010] ....... 616.190589: igc_setup_tc: Qbv configuration: |tc-6145 [010] ....... 616.190592: igc_setup_tc: Queue 0 -- start_time=0 [ns] |tc-6145 [010] ....... 616.190592: igc_setup_tc: Queue 0 -- end_time=1000000 [ns] |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 1 -- start_time=300000 [ns] |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 1 -- end_time=800000 [ns] |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 2 -- start_time=800000 [ns] |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 2 -- end_time=1000000 [ns] |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 3 -- start_time=800000 [ns] |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 3 -- end_time=1000000 [ns] Anyway, I'd appreciate some testing on your side too :). Thanks, Kurt
Hi Kurt, Kurt Kanzenbach <kurt@linutronix.de> writes: > Hi Vinicius, > > On Mon Jun 06 2022, Vinicius Costa Gomes wrote: >> Hi Kurt, >> >> Kurt Kanzenbach <kurt@linutronix.de> writes: >> >>> Add support for Qbv schedules where one queue stays open >>> in consecutive entries. Currently that's not supported. >>> >>> Example schedule: >>> >>> |tc qdisc replace dev ${INTERFACE} handle 100 parent root taprio num_tc 3 \ >>> | map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ >>> | queues 1@0 1@1 2@2 \ >>> | base-time ${BASETIME} \ >>> | sched-entry S 0x01 300000 \ # Stream High/Low >>> | sched-entry S 0x06 500000 \ # Management and Best Effort >>> | sched-entry S 0x04 200000 \ # Best Effort >>> | flags 0x02 >>> >>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> >>> --- >>> drivers/net/ethernet/intel/igc/igc_main.c | 23 +++++++++++++++++------ >>> 1 file changed, 17 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>> index ae17af44fe02..4758bdbe5df3 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>> @@ -5813,9 +5813,10 @@ static bool validate_schedule(struct igc_adapter *adapter, >>> return false; >>> >>> for (n = 0; n < qopt->num_entries; n++) { >>> - const struct tc_taprio_sched_entry *e; >>> + const struct tc_taprio_sched_entry *e, *prev; >>> int i; >>> >>> + prev = n ? &qopt->entries[n - 1] : NULL; >>> e = &qopt->entries[n]; >>> >>> /* i225 only supports "global" frame preemption >>> @@ -5828,7 +5829,12 @@ static bool validate_schedule(struct igc_adapter *adapter, >>> if (e->gate_mask & BIT(i)) >>> queue_uses[i]++; >>> >>> - if (queue_uses[i] > 1) >>> + /* There are limitations: A single queue cannot be >>> + * opened and closed multiple times per cycle unless the >>> + * gate stays open. Check for it. >>> + */ >>> + if (queue_uses[i] > 1 && >>> + !(prev->gate_mask & BIT(i))) >> >> Perhaps I am missing something, I didn't try to run, but not checking if >> 'prev' can be NULL, could lead to crashes for some schedules, no? > > My thinking was that `prev` can never be NULL, as `queue_uses[i] > 1` is > checked first. This condition can only be true if there are at least two > entries. > Oh, yeah! That's true. I have missed that. >> >> What I have in mind is a schedule that queue 0 is mentioned multiple >> times, for example: >> >> | sched-entry S 0x01 300000 \ # Stream High/Low >> | sched-entry S 0x03 500000 \ # Management and Best Effort >> | sched-entry S 0x05 200000 \ # Best Effort >> > > So, this schedule works with the proposed patch. Queue 0 is opened in > all three entries. My debug code shows: > > |tc-6145 [010] ....... 616.190589: igc_setup_tc: Qbv configuration: > |tc-6145 [010] ....... 616.190592: igc_setup_tc: Queue 0 -- start_time=0 [ns] > |tc-6145 [010] ....... 616.190592: igc_setup_tc: Queue 0 -- end_time=1000000 [ns] > |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 1 -- start_time=300000 [ns] > |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 1 -- end_time=800000 [ns] > |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 2 -- start_time=800000 [ns] > |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 2 -- end_time=1000000 [ns] > |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 3 -- start_time=800000 [ns] > |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 3 -- end_time=1000000 [ns] > > Anyway, I'd appreciate some testing on your side too :). Sure, I can give it a spin, but it'll have to be later in the week, kind of swamped right now. > > Thanks, > Kurt
>>> What I have in mind is a schedule that queue 0 is mentioned multiple >>> times, for example: >>> >>> | sched-entry S 0x01 300000 \ # Stream High/Low >>> | sched-entry S 0x03 500000 \ # Management and Best Effort >>> | sched-entry S 0x05 200000 \ # Best Effort >>> >> >> So, this schedule works with the proposed patch. Queue 0 is opened in >> all three entries. My debug code shows: >> >> |tc-6145 [010] ....... 616.190589: igc_setup_tc: Qbv configuration: >> |tc-6145 [010] ....... 616.190592: igc_setup_tc: Queue 0 -- start_time=0 [ns] >> |tc-6145 [010] ....... 616.190592: igc_setup_tc: Queue 0 -- end_time=1000000 [ns] >> |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 1 -- start_time=300000 [ns] >> |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 1 -- end_time=800000 [ns] >> |tc-6145 [010] ....... 616.190593: igc_setup_tc: Queue 2 -- start_time=800000 [ns] >> |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 2 -- end_time=1000000 [ns] >> |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 3 -- start_time=800000 [ns] >> |tc-6145 [010] ....... 616.190594: igc_setup_tc: Queue 3 -- end_time=1000000 [ns] >> >> Anyway, I'd appreciate some testing on your side too :). > > Sure, I can give it a spin, but it'll have to be later in the week, kind > of swamped right now. No problem. Actually i'm out of office for the next two weeks. I'll update the patch afterwards if required. Thanks, Kurt
Hi, Kurt Kanzenbach <kurt@linutronix.de> writes: > Add support for Qbv schedules where one queue stays open > in consecutive entries. Currently that's not supported. > > Example schedule: > > |tc qdisc replace dev ${INTERFACE} handle 100 parent root taprio num_tc 3 \ > | map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ > | queues 1@0 1@1 2@2 \ > | base-time ${BASETIME} \ > | sched-entry S 0x01 300000 \ # Stream High/Low > | sched-entry S 0x06 500000 \ # Management and Best Effort > | sched-entry S 0x04 200000 \ # Best Effort > | flags 0x02 > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- Finally did a few rounds of testing here, everything worked as expected: Reviewed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> Cheers,
On 6/6/2022 12:27, Kurt Kanzenbach wrote: > Add support for Qbv schedules where one queue stays open > in consecutive entries. Currently that's not supported. > > Example schedule: > > |tc qdisc replace dev ${INTERFACE} handle 100 parent root taprio num_tc 3 \ > | map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ > | queues 1@0 1@1 2@2 \ > | base-time ${BASETIME} \ > | sched-entry S 0x01 300000 \ # Stream High/Low > | sched-entry S 0x06 500000 \ # Management and Best Effort > | sched-entry S 0x04 200000 \ # Best Effort > | flags 0x02 > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) Tested-by: Naama Meir <naamax.meir@linux.intel.com>
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index ae17af44fe02..4758bdbe5df3 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -5813,9 +5813,10 @@ static bool validate_schedule(struct igc_adapter *adapter, return false; for (n = 0; n < qopt->num_entries; n++) { - const struct tc_taprio_sched_entry *e; + const struct tc_taprio_sched_entry *e, *prev; int i; + prev = n ? &qopt->entries[n - 1] : NULL; e = &qopt->entries[n]; /* i225 only supports "global" frame preemption @@ -5828,7 +5829,12 @@ static bool validate_schedule(struct igc_adapter *adapter, if (e->gate_mask & BIT(i)) queue_uses[i]++; - if (queue_uses[i] > 1) + /* There are limitations: A single queue cannot be + * opened and closed multiple times per cycle unless the + * gate stays open. Check for it. + */ + if (queue_uses[i] > 1 && + !(prev->gate_mask & BIT(i))) return false; } } @@ -5872,6 +5878,7 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) static int igc_save_qbv_schedule(struct igc_adapter *adapter, struct tc_taprio_qopt_offload *qopt) { + bool queue_configured[IGC_MAX_TX_QUEUES] = { }; u32 start_time = 0, end_time = 0; size_t n; @@ -5887,9 +5894,6 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, adapter->cycle_time = qopt->cycle_time; adapter->base_time = qopt->base_time; - /* FIXME: be a little smarter about cases when the gate for a - * queue stays open for more than one entry. - */ for (n = 0; n < qopt->num_entries; n++) { struct tc_taprio_sched_entry *e = &qopt->entries[n]; int i; @@ -5902,8 +5906,15 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, if (!(e->gate_mask & BIT(i))) continue; - ring->start_time = start_time; + /* Check whether a queue stays open for more than one + * entry. If so, keep the start and advance the end + * time. + */ + if (!queue_configured[i]) + ring->start_time = start_time; ring->end_time = end_time; + + queue_configured[i] = true; } start_time += e->interval;
Add support for Qbv schedules where one queue stays open in consecutive entries. Currently that's not supported. Example schedule: |tc qdisc replace dev ${INTERFACE} handle 100 parent root taprio num_tc 3 \ | map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ | queues 1@0 1@1 2@2 \ | base-time ${BASETIME} \ | sched-entry S 0x01 300000 \ # Stream High/Low | sched-entry S 0x06 500000 \ # Management and Best Effort | sched-entry S 0x04 200000 \ # Best Effort | flags 0x02 Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- drivers/net/ethernet/intel/igc/igc_main.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)