[PATCH net-next] igc: Lift TAPRIO schedule restriction

[PATCH net-next] igc: Lift TAPRIO schedule restriction

[Kurt Kanzenbach]

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)))
 				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;
-- 
2.30.2

Re: [PATCH net-next] igc: Lift TAPRIO schedule restriction

[Vinicius Costa Gomes]

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,
-- 
Vinicius

Re: [PATCH net-next] igc: Lift TAPRIO schedule restriction

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 3517 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] igc: Lift TAPRIO schedule restriction

[Vinicius Costa Gomes]

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

-- 
Vinicius

Re: [PATCH net-next] igc: Lift TAPRIO schedule restriction

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

>>> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] igc: Lift TAPRIO schedule restriction

[Vinicius Costa Gomes]

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,
-- 
Vinicius

Re: [Intel-wired-lan] [PATCH net-next] igc: Lift TAPRIO schedule restriction

[naamax.meir]

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>