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

From: David Z. Dai
Date: Fri Aug 30 2019 - 20:31:00 EST


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!