On Thu, 6 Jun 2019 10:50:56 -0700, Vedang Patel wrote:
> Currently, we are seeing non-critical packets being transmitted outside of
> their timeslice. We can confirm that the packets are being dequeued at the
> right time. So, the delay is induced in the hardware side. The most likely
> reason is the hardware queues are starving the lower priority queues.
>
> In order to improve the performance of taprio, we will be making use of the
> txtime feature provided by the ETF qdisc. For all the packets which do not
> have the SO_TXTIME option set, taprio will set the transmit timestamp (set
> in skb->tstamp) in this mode. TAPrio Qdisc will ensure that the transmit
> time for the packet is set to when the gate is open. If SO_TXTIME is set,
> the TAPrio qdisc will validate whether the timestamp (in skb->tstamp)
> occurs when the gate corresponding to skb's traffic class is open.
>
> Following two parameters added to support this mode:
> - flags: used to enable txtime-assist mode. Will also be used to enable
> other modes (like hardware offloading) later.
> - txtime-delay: This indicates the minimum time it will take for the packet
> to hit the wire after it reaches taprio_enqueue(). This is useful in
> determining whether we can transmit the packet in the remaining time if
> the gate corresponding to the packet is currently open.
>
> An example configuration for enabling txtime-assist:
>
> tc qdisc replace dev eth0 parent root handle 100 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@0 1@0 \\
> base-time 1558653424279842568 \\
> sched-entry S 01 300000 \\
> sched-entry S 02 300000 \\
> sched-entry S 04 400000 \\
> flags 0x1 \\
> txtime-delay 40000 \\
> clockid CLOCK_TAI
>
> tc qdisc replace dev $IFACE parent 100:1 etf skip_sock_check \\
> offload delta 200000 clockid CLOCK_TAI
>
> Note that all the traffic classes are mapped to the same queue. This is
> only possible in taprio when txtime-assist is enabled. Also, note that the
> ETF Qdisc is enabled with offload mode set.
>
> In this mode, if the packet's traffic class is open and the complete packet
> can be transmitted, taprio will try to transmit the packet immediately.
> This will be done by setting skb->tstamp to current_time + the time delta
> indicated in the txtime-delay parameter. This parameter indicates the time
> taken (in software) for packet to reach the network adapter.
>
> If the packet cannot be transmitted in the current interval or if the
> packet's traffic is not currently transmitting, the skb->tstamp is set to
> the next available timestamp value. This is tracked in the next_launchtime
> parameter in the struct sched_entry.
>
> The behaviour w.r.t admin and oper schedules is not changed from what is
> present in software mode.
>
> The transmit time is already known in advance. So, we do not need the HR
> timers to advance the schedule and wakeup the dequeue side of taprio. So,
> HR timer won't be run when this mode is enabled.
>
> Signed-off-by: Vedang Patel <vedang.pa...@intel.com>
> ---
> include/uapi/linux/pkt_sched.h | 4 +
> net/sched/sch_taprio.c | 344
> +++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 331 insertions(+), 17 deletions(-)
>
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 69fc52e4d6bd..c085860ff637 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -1159,6 +1159,8 @@ enum {
> * [TCA_TAPRIO_ATTR_SCHED_ENTRY_INTERVAL]
> */
>
> +#define TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST 0x1
> +
> enum {
> TCA_TAPRIO_ATTR_UNSPEC,
> TCA_TAPRIO_ATTR_PRIOMAP, /* struct tc_mqprio_qopt */
> @@ -1170,6 +1172,8 @@ enum {
> TCA_TAPRIO_ATTR_ADMIN_SCHED, /* The admin sched, only used in dump */
> TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME, /* s64 */
> TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION, /* s64 */
> + TCA_TAPRIO_ATTR_FLAGS, /* u32 */
> + TCA_TAPRIO_ATTR_TXTIME_DELAY, /* s32 */
> __TCA_TAPRIO_ATTR_MAX,
> };
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index a41d7d4434ee..a5676fb2b2dd 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -21,12 +21,17 @@
> #include <net/pkt_sched.h>
> #include <net/pkt_cls.h>
> #include <net/sch_generic.h>
> +#include <net/sock.h>
>
> static LIST_HEAD(taprio_list);
> static DEFINE_SPINLOCK(taprio_list_lock);
>
> #define TAPRIO_ALL_GATES_OPEN -1
>
> +#define FLAGS_VALID(flags) (!((flags) & ~TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST))
> +#define TXTIME_ASSIST_IS_ENABLED(flags) (FLAGS_VALID((flags)) && \
> + ((flags) & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST))
Thanks for the changes, since you now validate no unknown flags are
passed, perhaps there is no need to check if flags are == ~0?
IS_ENABLED() could just do: (flags) & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST
No?
> @@ -708,6 +978,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr
> *opt,
> struct taprio_sched *q = qdisc_priv(sch);
> struct net_device *dev = qdisc_dev(sch);
> struct tc_mqprio_qopt *mqprio = NULL;
> + u32 taprio_flags = U32_MAX;
Then this should default to 0, i.e. no flag set..
> int i, err, clockid;
> unsigned long flags;
> ktime_t start;
> @@ -720,7 +991,21 @@ static int taprio_change(struct Qdisc *sch, struct
> nlattr *opt,
> if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
> mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
>
> - err = taprio_parse_mqprio_opt(dev, mqprio, extack);
> + if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
> + taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
> +
> + if (q->flags != 0) {
> + NL_SET_ERR_MSG(extack, "Changing 'flags' of a running
> schedule is not supported");
So the parameter must not be passed at all? Perhaps it's fine if:
q->flags == taprio_flags
?
also: NL_SET_ERR_MSG_MOD() is better here
> + return -ENOTSUPP;
Probably EINVAL or EOPNOTSUPP, ENOTSUPP is a high error code which libc
doesn't understand, it's best avoided.
> + } else if (!FLAGS_VALID(taprio_flags)) {
> + NL_SET_ERR_MSG(extack, "Specified 'flags' are not
> valid.");
nit: you didn't have a period at the end of the previous extack
> + return -ENOTSUPP;
> + }
> +
> + q->flags = taprio_flags;
> + }
> +
> + err = taprio_parse_mqprio_opt(dev, mqprio, extack, taprio_flags);
> if (err < 0)
> return err;
>
> @@ -779,7 +1064,11 @@ static int taprio_change(struct Qdisc *sch, struct
> nlattr *opt,
> /* Protects against enqueue()/dequeue() */
> spin_lock_bh(qdisc_lock(sch));
>
> - if (!hrtimer_active(&q->advance_timer)) {
> + if (tb[TCA_TAPRIO_ATTR_TXTIME_DELAY])
> + q->txtime_delay = nla_get_s32(tb[TCA_TAPRIO_ATTR_TXTIME_DELAY]);
Perhaps this attribute should only be allowed if flags enabled
txtime-assist?
> + if (!TXTIME_ASSIST_IS_ENABLED(taprio_flags) &&
> + !hrtimer_active(&q->advance_timer)) {
> hrtimer_init(&q->advance_timer, q->clockid, HRTIMER_MODE_ABS);
> q->advance_timer.function = advance_sched;
> }
