Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [v2] net_sched: act_police: add 2 new attributes to support
 police 64bit rate and peakrate
From:   "David Z. Dai" <zdai@linux.vnet.ibm.com>
To:     David Miller <davem@davemloft.net>
Cc:     xiyou.wangcong@gmail.com, jhs@mojatatu.com, jiri@resnulli.us,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        zdai@us.ibm.com
In-Reply-To: <20190830.133335.323827182628557013.davem@davemloft.net>
References: <1567191974-11578-1-git-send-email-zdai@linux.vnet.ibm.com>
         <CAM_iQpVMYQUdQN5L+ntXZTffZkW4q659bvXoZ8+Ar+zeud7Y4Q@mail.gmail.com>
         <1567195432.20025.18.camel@oc5348122405>
         <20190830.133335.323827182628557013.davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Date:   Fri, 30 Aug 2019 19:30:47 -0500
Message-ID: <1567211447.25082.3.camel@oc5348122405>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 2019-08-30 at 13:33 -0700, David Miller wrote:
> From: "David Z. Dai" <zdai@linux.vnet.ibm.com>
> Date: Fri, 30 Aug 2019 15:03:52 -0500
> 
> > I have the impression that last parameter num value should be larger
> > than the attribute num value in 2nd parameter (TC_POLICE_RATE64 in this
> > case).
> 
> The argument in question is explicitly the "padding" value.
> 
> Please explain in detail where you got the impression that the
> argument has to be larger?
In include/uapi/linux/pkt_sched.h header:
For HTB:
enum {
        TCA_HTB_UNSPEC,
        TCA_HTB_PARMS,
        TCA_HTB_INIT,
        TCA_HTB_CTAB,
        TCA_HTB_RTAB,
        TCA_HTB_DIRECT_QLEN,
        TCA_HTB_RATE64,    /* <--- */
        TCA_HTB_CEIL64,    /* <--- */
        TCA_HTB_PAD,       /* <--- */
        __TCA_HTB_MAX,
};
/* TCA_HTB_RATE64,TCA_HTB_CEIL64 are declared *before* TCA_HTB_PAD */

For TBF:
enum {
        TCA_TBF_UNSPEC,
        TCA_TBF_PARMS,
        TCA_TBF_RTAB,
        TCA_TBF_PTAB,
        TCA_TBF_RATE64,   /* <--- */
        TCA_TBF_PRATE64,  /* <--- */
        TCA_TBF_BURST,
        TCA_TBF_PBURST,
        TCA_TBF_PAD,      /* <--- */
        __TCA_TBF_MAX,
};
/* TCA_TBF_RATE64, TCA_TBF_PRATE64 are declared *before* TCA_TBF_PAD */

For HTB, in net/sched/sch_htb.c file, htb_dump_class() routine:
        if ((cl->rate.rate_bytes_ps >= (1ULL << 32)) &&
            nla_put_u64_64bit(skb, TCA_HTB_RATE64,
cl->rate.rate_bytes_ps,
                              TCA_HTB_PAD))
                goto nla_put_failure;
        if ((cl->ceil.rate_bytes_ps >= (1ULL << 32)) &&
            nla_put_u64_64bit(skb, TCA_HTB_CEIL64,
cl->ceil.rate_bytes_ps,
                              TCA_HTB_PAD))
                goto nla_put_failure;

For TBF, in net/sched/sch_tbf.c file, tbf_dump() routine:
       if (q->rate.rate_bytes_ps >= (1ULL << 32) &&
            nla_put_u64_64bit(skb, TCA_TBF_RATE64,
q->rate.rate_bytes_ps,
                              TCA_TBF_PAD))
                goto nla_put_failure;
        if (tbf_peak_present(q) &&
            q->peak.rate_bytes_ps >= (1ULL << 32) &&
            nla_put_u64_64bit(skb, TCA_TBF_PRATE64,
q->peak.rate_bytes_ps,
                              TCA_TBF_PAD))
                goto nla_put_failure;

The last parameter used TCA_TBF_PAD, TCA_TBF_PAD are all declared
*after* those attributes.

I am trying to keep it consistent in police part. That's where my
impression is coming from.

Now for suggestion/comment, do you think is it better to add a new PAD
attribute in include/uapi/pkt_cls.h like this:
enum {
        TCA_POLICE_UNSPEC,
        TCA_POLICE_TBF,
        TCA_POLICE_RATE,
        TCA_POLICE_PEAKRATE,
        TCA_POLICE_AVRATE,
        TCA_POLICE_RESULT,
        TCA_POLICE_TM,
        TCA_POLICE_PAD,
        TCA_POLICE_RATE64,      /* <--- */
        TCA_POLICE_PEAKRATE64,  /* <--- */
        TCA_POLICE_PAD2,        /* <--- new PAD */
        __TCA_POLICE_MAX
#define TCA_POLICE_RESULT TCA_POLICE_RESULT
#};
Then use this TCA_POLICE_PAD2 as the last parameter in
nla_put_u64_64bit()? 

Thanks!