From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jamal Hadi Salim <jhs@mojatatu.com>
Subject: Re: [RFC v2 net-next 06/10] net/sched: Introduce the TBS Qdisc
Date: Thu, 18 Jan 2018 08:35:18 -0500
Message-ID: <30e104db-3c88-b2f7-4494-98b2bece8ca9@mojatatu.com>
References: <20180117230621.26074-1-jesus.sanchez-palencia@intel.com>
 <20180117230621.26074-7-jesus.sanchez-palencia@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us,
        vinicius.gomes@intel.com, richardcochran@gmail.com,
        intel-wired-lan@lists.osuosl.org, anna-maria@linutronix.de,
        henrik@austad.us, tglx@linutronix.de, john.stultz@linaro.org,
        andre.guedes@intel.com, ivan.briano@intel.com,
        levi.pearson@harman.com
To: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>,
        netdev@vger.kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-it0-f49.google.com ([209.85.214.49]:41897 "EHLO
        mail-it0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755840AbeARNfZ (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 18 Jan 2018 08:35:25 -0500
Received: by mail-it0-f49.google.com with SMTP id b77so13497910itd.0
        for <netdev@vger.kernel.org>; Thu, 18 Jan 2018 05:35:25 -0800 (PST)
In-Reply-To: <20180117230621.26074-7-jesus.sanchez-palencia@intel.com>
Content-Language: en-GB
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 18-01-17 06:06 PM, Jesus Sanchez-Palencia wrote:
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> 
> TBS (Time Based Scheduler) uses the information added earlier in this
> series (the socket option SO_TXTIME and the new role of
> sk_buff->tstamp) to schedule traffic transmission based on absolute
> time.
> 
> For some workloads, just bandwidth enforcement is not enough, and
> precise control of the transmission of packets is necessary.
> 
> Example:
> 
> $ tc qdisc replace dev enp2s0 parent root handle 100 mqprio num_tc 3 \

handle 100:0 ?

>             map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 0
> 
> $ tc qdisc add dev enp2s0 parent 100:1 tbs delta 60000 clockid 11 offload 1
>
>
> In this example, the Qdisc will try to enable offloading (offload 1)
> the control of the transmission time to the network adapter, the
> time stamp in socket are in reference to the clockid '11' (CLOCK_TAI)
> and packets leave the Qdisc "delta" (60000) nanoseconds before its
> transmission time.
>


> When offloading is disabled, the network adapter will ignore the
> sk_buff time stamp, and so, the transmission time will be only "best
> effort" from the Qdisc.
>

General comments:
1) iproute2: Avoid magic numbers like 1 or 11 please; "offload"
(without 1) and "TAI" will be more human friendly.

2) Experience shows that adding padding fields in the control structs
implies they will _never ever_ be used. That was not design intent
for netlink but over years shit like that has happened.
Maybe look at using a 32 bitmap? It is more "future proof".
You seem to only have 2-3 flags but it gives you opportunity
to add more changes later. If you are 100% sure youll never need
it - then maybe just move the tc_tbs_qopt::offload to the end of
of the struct.

3)It would be helpful for debugging to increment some stats other
than drop counters on enqueu/dequeue obsolete packet drop. Maybe use
overlimits for the dequeu drops (in addition)?

4) I may be misreading things - but did you need to reset the
watchdog on dequeue? It is already being kicked for every incoming packet.

cheers,
jamal