[PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

Although this is already in kernel, currently the tool does not support
them. We need these commands for full TSN features which are currently
supported in Synopsys IPs such as QoS and XGMAC3.

Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
---
Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tc/q_taprio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tc/q_taprio.c b/tc/q_taprio.c
index b9954436b0f9..62ff860e80ae 100644
--- a/tc/q_taprio.c
+++ b/tc/q_taprio.c
@@ -99,6 +99,10 @@ static const char *entry_cmd_to_str(__u8 cmd)
 	switch (cmd) {
 	case TC_TAPRIO_CMD_SET_GATES:
 		return "S";
+	case TC_TAPRIO_CMD_SET_AND_HOLD:
+		return "H";
+	case TC_TAPRIO_CMD_SET_AND_RELEASE:
+		return "R";
 	default:
 		return "Invalid";
 	}
@@ -108,6 +112,10 @@ static int str_to_entry_cmd(const char *str)
 {
 	if (strcmp(str, "S") == 0)
 		return TC_TAPRIO_CMD_SET_GATES;
+	if (strcmp(str, "H") == 0)
+		return TC_TAPRIO_CMD_SET_AND_HOLD;
+	if (strcmp(str, "R") == 0)
+		return TC_TAPRIO_CMD_SET_AND_RELEASE;
 
 	return -1;
 }
-- 
2.7.4

Re: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Andre Guedes]

Hi Jose,

Quoting Jose Abreu (2019-12-17 06:10:24)
> Although this is already in kernel, currently the tool does not support
> them. We need these commands for full TSN features which are currently
> supported in Synopsys IPs such as QoS and XGMAC3.
> 
> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
> ---
> Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
>  tc/q_taprio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tc/q_taprio.c b/tc/q_taprio.c
> index b9954436b0f9..62ff860e80ae 100644
> --- a/tc/q_taprio.c
> +++ b/tc/q_taprio.c
> @@ -99,6 +99,10 @@ static const char *entry_cmd_to_str(__u8 cmd)
>         switch (cmd) {
>         case TC_TAPRIO_CMD_SET_GATES:
>                 return "S";
> +       case TC_TAPRIO_CMD_SET_AND_HOLD:
> +               return "H";
> +       case TC_TAPRIO_CMD_SET_AND_RELEASE:
> +               return "R";
>         default:
>                 return "Invalid";
>         }
> @@ -108,6 +112,10 @@ static int str_to_entry_cmd(const char *str)
>  {
>         if (strcmp(str, "S") == 0)
>                 return TC_TAPRIO_CMD_SET_GATES;
> +       if (strcmp(str, "H") == 0)
> +               return TC_TAPRIO_CMD_SET_AND_HOLD;
> +       if (strcmp(str, "R") == 0)
> +               return TC_TAPRIO_CMD_SET_AND_RELEASE;
>  
>         return -1;
>  }

From the commit message, I had the impression that HOLD and RELEASE commands
are supported by QoS and XGMAC3. However, in '[PATCH net-next v2 7/7] net:
stmmac: Integrate EST with TAPRIO scheduler API' I see:

+               if (qopt->entries[i].command != TC_TAPRIO_CMD_SET_GATES)
+                       return -EOPNOTSUPP;

Am I missing something or these commands are indeed not supported by Synopsys
IPs?

Anyhow, this patch makes sense to me.

Regards,

Andre

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

From: Andre Guedes <andre.guedes@linux.intel.com>
Date: Dec/17/2019, 21:25:36 (UTC+00:00)

> From the commit message, I had the impression that HOLD and RELEASE commands
> are supported by QoS and XGMAC3. However, in '[PATCH net-next v2 7/7] net:
> stmmac: Integrate EST with TAPRIO scheduler API' I see:
> 
> +               if (qopt->entries[i].command != TC_TAPRIO_CMD_SET_GATES)
> +                       return -EOPNOTSUPP;
> 
> Am I missing something or these commands are indeed not supported by Synopsys
> IPs?

Its already supported internally but I just didn't add it in the series. 
I'll probably sent the EST and FP support in a different series today.

> Anyhow, this patch makes sense to me.

Thanks!

---
Thanks,
Jose Miguel Abreu

Re: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Vinicius Costa Gomes]

Hi Jose,

Jose Abreu <Jose.Abreu@synopsys.com> writes:

> Although this is already in kernel, currently the tool does not support
> them. We need these commands for full TSN features which are currently
> supported in Synopsys IPs such as QoS and XGMAC3.
>
> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>

This patch looks good in itself. 

However, I feel that this is incomplete. At least the way I understand
things, without specifying which traffic classes are going to be
preemptible (or it's dual concept, express), I don't see how this is
going to be used in practice. Or does the hardware have a default
configuration, that all traffic classes are preemptible, for example.

What am I missing here?


Cheers,
--
Vinicius

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Dec/18/2019, 23:05:13 (UTC+00:00)

> Hi Jose,
> 
> Jose Abreu <Jose.Abreu@synopsys.com> writes:
> 
> > Although this is already in kernel, currently the tool does not support
> > them. We need these commands for full TSN features which are currently
> > supported in Synopsys IPs such as QoS and XGMAC3.
> >
> > Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
> 
> This patch looks good in itself. 
> 
> However, I feel that this is incomplete. At least the way I understand
> things, without specifying which traffic classes are going to be
> preemptible (or it's dual concept, express), I don't see how this is
> going to be used in practice. Or does the hardware have a default
> configuration, that all traffic classes are preemptible, for example.
> 
> What am I missing here?

On our IPs Queue 0 is by preemptible and all remaining ones are express 
by default.

The way I tested it is quite easy: send traffic from queue 0 and at same 
time configure EST with SetAndHold for remaining queues. Which means 
queue 0 traffic will be blocked while remaining ones are sending.

---
Thanks,
Jose Miguel Abreu

Re: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[David Ahern]

On 12/18/19 4:05 PM, Vinicius Costa Gomes wrote:
> Hi Jose,
> 
> Jose Abreu <Jose.Abreu@synopsys.com> writes:
> 
>> Although this is already in kernel, currently the tool does not support
>> them. We need these commands for full TSN features which are currently
>> supported in Synopsys IPs such as QoS and XGMAC3.
>>
>> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
> 
> This patch looks good in itself. 
> 
> However, I feel that this is incomplete. At least the way I understand
> things, without specifying which traffic classes are going to be
> preemptible (or it's dual concept, express), I don't see how this is
> going to be used in practice. Or does the hardware have a default
> configuration, that all traffic classes are preemptible, for example.
> 
> What am I missing here?
> 

this patch has been lingering for a while. What's the status? good
enough for commit or are changes needed?

Re: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Murali Karicheri]

Hi Jose,

On 12/18/2019 06:08 PM, Jose Abreu wrote:
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Dec/18/2019, 23:05:13 (UTC+00:00)
> 
>> Hi Jose,
>>
>> Jose Abreu <Jose.Abreu@synopsys.com> writes:
>>
>>> Although this is already in kernel, currently the tool does not support
>>> them. We need these commands for full TSN features which are currently
>>> supported in Synopsys IPs such as QoS and XGMAC3.
>>>
>>> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
>>
>> This patch looks good in itself.
>>
>> However, I feel that this is incomplete. At least the way I understand
>> things, without specifying which traffic classes are going to be
>> preemptible (or it's dual concept, express), I don't see how this is
>> going to be used in practice. Or does the hardware have a default
>> configuration, that all traffic classes are preemptible, for example.
>>
>> What am I missing here?
> 
> On our IPs Queue 0 is by preemptible and all remaining ones are express
> by default.
> 
> The way I tested it is quite easy: send traffic from queue 0 and at same
> time configure EST with SetAndHold for remaining queues. Which means
> queue 0 traffic will be blocked while remaining ones are sending.
> 
So you have one sched entry that specify SetAndHold for all remaining
queues. So this means, queue 0 will never get sent. I guess you also
support SetAndRelease so that a mix of SetAndHold followed by 
SetAndRelease can be sent to enable sending from Queue 0. Is that
correct?

Something like
               sched-entry H 02 300000 \ <=== 300 usec tx from Q1
               sched-entry R 01 200000   <=== 300 usec tx from Q0

Just trying to understand how this is being used for real world
application.

Regards,
Murali
> ---
> Thanks,
> Jose Miguel Abreu
> 

-- 
Murali Karicheri
Texas Instruments

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Andre Guedes]

Hi Jose,

Quoting Jose Abreu (2019-12-18 15:08:45)
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Dec/18/2019, 23:05:13 (UTC+00:00)
> 
> > However, I feel that this is incomplete. At least the way I understand
> > things, without specifying which traffic classes are going to be
> > preemptible (or it's dual concept, express), I don't see how this is
> > going to be used in practice. Or does the hardware have a default
> > configuration, that all traffic classes are preemptible, for example.
> > 
> > What am I missing here?
> 
> On our IPs Queue 0 is by preemptible and all remaining ones are express 
> by default.

Is this configuration fixed in your IP or the user can control if a specific
queue is preemptible or express?

I'm trying to figure out how this discussion relates to the Qbu discussion
we're having in "[v1,net-next, 1/2] ethtool: add setting frame preemption of
traffic classes".

Regards,

Andre

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

From: David Ahern <dsahern@gmail.com>
Date: Dec/31/2019, 04:30:54 (UTC+00:00)

> this patch has been lingering for a while. What's the status? good
> enough for commit or are changes needed?

Thanks for asking David. There are some on-going discussions (I'll add 
you on cc), let's see how that goes.

---
Thanks,
Jose Miguel Abreu

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

From: Murali Karicheri <m-karicheri2@ti.com>
Date: Jan/03/2020, 22:24:14 
(UTC+00:00)

> So you have one sched entry that specify SetAndHold for all remaining
> queues. So this means, queue 0 will never get sent. I guess you also
> support SetAndRelease so that a mix of SetAndHold followed by 
> SetAndRelease can be sent to enable sending from Queue 0. Is that
> correct?
> 
> Something like
>                sched-entry H 02 300000 \ <=== 300 usec tx from Q1
>                sched-entry R 01 200000   <=== 300 usec tx from Q0
> 
> Just trying to understand how this is being used for real world
> application.

This is the command I use:

# tc qdisc add dev $intf handle 100: parent root taprio \
	num_tc 4 \
	map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
	base-time $base \
	cycle-time 1000000 \
	sched-entry R 00 100000 \
	sched-entry H 02 200000 \
	sched-entry H 04 300000 \
	sched-entry H 08 400000 \
	flags 0x2
# sleep 2
# iperf3 -c <ip> -u -b 0 -t 15 &
# sleep 5
# echo "Queue 3: Expected=40%, Queue 0 will now be preempted"
# tperf -i <ethX> -p 3

This will basically preempt Queue 0 and flood Queue 3 with express 
traffic.

You can find tperf utility here: https://github.com/joabreu/tperf

---
Thanks,
Jose Miguel Abreu

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

From: Andre Guedes <andre.guedes@linux.intel.com>
Date: Jan/07/2020, 00:19:17 (UTC+00:00)

> > On our IPs Queue 0 is by preemptible and all remaining ones are express 
> > by default.
> 
> Is this configuration fixed in your IP or the user can control if a specific
> queue is preemptible or express?

It's configurable for all Queues except 0 which is fixed as preemptible.

> I'm trying to figure out how this discussion relates to the Qbu discussion
> we're having in "[v1,net-next, 1/2] ethtool: add setting frame preemption of
> traffic classes".

Hmmm.

I think tc utility is the right way to do this, and not ethtool because 
EST and FP are highly tied ... Do you agree ?

---
Thanks,
Jose Miguel Abreu

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Andre Guedes]

Hi Jose,

> > > On our IPs Queue 0 is by preemptible and all remaining ones are express 
> > > by default.
> > 
> > Is this configuration fixed in your IP or the user can control if a specific
> > queue is preemptible or express?
> 
> It's configurable for all Queues except 0 which is fixed as preemptible.

Thanks for the clarification.

> > I'm trying to figure out how this discussion relates to the Qbu discussion
> > we're having in "[v1,net-next, 1/2] ethtool: add setting frame preemption of
> > traffic classes".
> 
> Hmmm.
> 
> I think tc utility is the right way to do this, and not ethtool because 
> EST and FP are highly tied ... Do you agree ?

I'm still wrapping my head around this :) See my last reply on that other
thread. (BTW, let's concentrate this 'Qbu enabling' discussion there)

Thanks,

Andre

Re: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Murali Karicheri]

Hi Jose,

On 01/07/2020 04:27 AM, Jose Abreu wrote:
> From: Murali Karicheri <m-karicheri2@ti.com>
> Date: Jan/03/2020, 22:24:14
> (UTC+00:00)
> 
>> So you have one sched entry that specify SetAndHold for all remaining
>> queues. So this means, queue 0 will never get sent. I guess you also
>> support SetAndRelease so that a mix of SetAndHold followed by
>> SetAndRelease can be sent to enable sending from Queue 0. Is that
>> correct?
>>
>> Something like
>>                 sched-entry H 02 300000 \ <=== 300 usec tx from Q1
>>                 sched-entry R 01 200000   <=== 300 usec tx from Q0
>>
>> Just trying to understand how this is being used for real world
>> application.
> 
> This is the command I use:
> 
> # tc qdisc add dev $intf handle 100: parent root taprio \
> 	num_tc 4 \
> 	map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> 	base-time $base \
> 	cycle-time 1000000 \
> 	sched-entry R 00 100000 \
> 	sched-entry H 02 200000 \
> 	sched-entry H 04 300000 \
> 	sched-entry H 08 400000 \
> 	flags 0x2
> # sleep 2
> # iperf3 -c <ip> -u -b 0 -t 15 &
> # sleep 5
> # echo "Queue 3: Expected=40%, Queue 0 will now be preempted"
> # tperf -i <ethX> -p 3
> 
> This will basically preempt Queue 0 and flood Queue 3 with express
> traffic.
I see you don't include Q0 in your schedule. Why is that the case?

Why is the entry with zero mask introduced (first entry)? Typo?
I thought it should be like below. No?

 > # tc qdisc add dev $intf handle 100: parent root taprio \
 > 	num_tc 4 \
 > 	map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
 > 	base-time $base \
 > 	cycle-time 1000000 \
 > 	sched-entry R 01 100000 \
 > 	sched-entry H 02 200000 \
 > 	sched-entry H 04 300000 \
 > 	sched-entry H 08 400000 \
 > 	flags 0x2

Based on my understanding, if holdAdvance is t1 and releaseAdvance is
t2, hold is asserted t1 nano second before Q1 slot (during first
entry) begins and release is asserted t2 nano second before the start of
Q0 slot (during fourth entry) so that pre-emptable frame start 
transmission during first entry.

I thought R/H entries are a replacement for zero mask entry that
are introduced in the schedule as a guard band before express queue
slot when frame pre-emption not supported. Is my understanding correct?

So the above make sense?

Regards,

Murali
> 
> You can find tperf utility here: https://github.com/joabreu/tperf
> 
> ---
> Thanks,
> Jose Miguel Abreu
> 

-- 
Murali Karicheri
Texas Instruments

RE: [PATCH iproute2-next] taprio: Add support for the SetAndHold and SetAndRelease commands

[Jose Abreu]

From: Murali Karicheri <m-karicheri2@ti.com>
Date: Jan/17/2020, 22:18:07 (UTC+00:00)

> Hi Jose,
> 
> On 01/07/2020 04:27 AM, Jose Abreu wrote:
> > From: Murali Karicheri <m-karicheri2@ti.com>
> > Date: Jan/03/2020, 22:24:14
> > (UTC+00:00)
> > 
> >> So you have one sched entry that specify SetAndHold for all remaining
> >> queues. So this means, queue 0 will never get sent. I guess you also
> >> support SetAndRelease so that a mix of SetAndHold followed by
> >> SetAndRelease can be sent to enable sending from Queue 0. Is that
> >> correct?
> >>
> >> Something like
> >>                 sched-entry H 02 300000 \ <=== 300 usec tx from Q1
> >>                 sched-entry R 01 200000   <=== 300 usec tx from Q0
> >>
> >> Just trying to understand how this is being used for real world
> >> application.
> > 
> > This is the command I use:
> > 
> > # tc qdisc add dev $intf handle 100: parent root taprio \
> > 	num_tc 4 \
> > 	map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> > 	base-time $base \
> > 	cycle-time 1000000 \
> > 	sched-entry R 00 100000 \
> > 	sched-entry H 02 200000 \
> > 	sched-entry H 04 300000 \
> > 	sched-entry H 08 400000 \
> > 	flags 0x2
> > # sleep 2
> > # iperf3 -c <ip> -u -b 0 -t 15 &
> > # sleep 5
> > # echo "Queue 3: Expected=40%, Queue 0 will now be preempted"
> > # tperf -i <ethX> -p 3
> > 
> > This will basically preempt Queue 0 and flood Queue 3 with express
> > traffic.
> I see you don't include Q0 in your schedule. Why is that the case?
> 
> Why is the entry with zero mask introduced (first entry)? Typo?
> I thought it should be like below. No?
> 
>  > # tc qdisc add dev $intf handle 100: parent root taprio \
>  > 	num_tc 4 \
>  > 	map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
>  > 	base-time $base \
>  > 	cycle-time 1000000 \
>  > 	sched-entry R 01 100000 \
>  > 	sched-entry H 02 200000 \
>  > 	sched-entry H 04 300000 \
>  > 	sched-entry H 08 400000 \
>  > 	flags 0x2
> 
> Based on my understanding, if holdAdvance is t1 and releaseAdvance is
> t2, hold is asserted t1 nano second before Q1 slot (during first
> entry) begins and release is asserted t2 nano second before the start of
> Q0 slot (during fourth entry) so that pre-emptable frame start 
> transmission during first entry.
> 
> I thought R/H entries are a replacement for zero mask entry that
> are introduced in the schedule as a guard band before express queue
> slot when frame pre-emption not supported. Is my understanding correct?
> 
> So the above make sense?

You are right, my example was an old one, sorry.

This is what I'm currently using:

[...]
cycle-time 1000000 \
sched-entry R 02 200000 \
sched-entry R 04 400000 \
sched-entry H 08 400000 \
[...]

Notice I don't need to set Queue 0 on sched-entry because its always 
preemptable queue.

Then I test it with:
# echo "Queue 0: Exp=60%"
# iperf3 -t 15 -c <ip> -Z -A <cpu> &
# sleep 5
# echo "Queue 3: Exp=40%"
# tperf -i <interface> -p 3 -c <cpu>

This will result in:
- iperf will have 60% of bandwidth because Queue 0-2 have 400 + 200 = 60% 
cycle time
- tperf will have 40% of bandwidth because Queue 3 has 40% cycle time and 
is express Queue

---
Thanks,
Jose Miguel Abreu