Re: [v2] net_sched: act_police: add 2 new attributes to support police 64bit rate and peakrate

[David Z. Dai]

On Fri, 2019-08-30 at 13:33 -0700, David Miller wrote:
> From: "David Z. Dai" <zdai@xxxxxxxxxxxxxxxxxx>
> 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!