On Wed, Jan 27, 2021 at 12:02:23PM +0100, Simon Horman wrote:
> Hi Jakub,
>
> On Tue, Jan 26, 2021 at 06:38:12PM -0800, Jakub Kicinski wrote:
> > On Mon, 25 Jan 2021 16:18:19 +0100 Simon Horman wrote:
> > > From: Baowen Zheng <baowen.zh...@corigine.com>
> > >
> > > Allow a policer action to enforce a rate-limit based on
> > > packets-per-second,
> > > configurable using a packet-per-second rate and burst parameters. This may
> > > be used in conjunction with existing byte-per-second rate limiting in the
> > > same policer action.
> > >
> > > e.g.
> > > tc filter add dev tap1 parent ffff: u32 match \
> > > u32 0 0 police pkts_rate 3000 pkts_burst 1000
> > >
> > > Testing was unable to uncover a performance impact of this change on
> > > existing features.
> > >
> > > Signed-off-by: Baowen Zheng <baowen.zh...@corigine.com>
> > > Signed-off-by: Simon Horman <simon.hor...@netronome.com>
> > > Signed-off-by: Louis Peens <louis.pe...@netronome.com>
> >
> > > diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> > > index 8d8452b1cdd4..d700b2105535 100644
> > > --- a/net/sched/act_police.c
> > > +++ b/net/sched/act_police.c
> > > @@ -42,6 +42,8 @@ static const struct nla_policy
> > > police_policy[TCA_POLICE_MAX + 1] = {
> > > [TCA_POLICE_RESULT] = { .type = NLA_U32 },
> > > [TCA_POLICE_RATE64] = { .type = NLA_U64 },
> > > [TCA_POLICE_PEAKRATE64] = { .type = NLA_U64 },
> > > + [TCA_POLICE_PKTRATE64] = { .type = NLA_U64 },
> > > + [TCA_POLICE_PKTBURST64] = { .type = NLA_U64 },
> >
> > Should we set the policy so that .min = 1?
>
> Yes, I think so.
> Thanks for spotting that.
It seems that I was mistaken.
A value of 0 is used to clear packet-per-second rate limiting
while leaving bit-per-second rate configuration in place for a
policer action.
So I think the policy should be left as is...
> > > };
> > >
> > > static int tcf_police_init(struct net *net, struct nlattr *nla,
> > > @@ -61,6 +63,7 @@ static int tcf_police_init(struct net *net, struct
> > > nlattr *nla,
> > > bool exists = false;
> > > u32 index;
> > > u64 rate64, prate64;
> > > + u64 pps, ppsburst;
> > >
> > > if (nla == NULL)
> > > return -EINVAL;
> > > @@ -183,6 +186,16 @@ static int tcf_police_init(struct net *net, struct
> > > nlattr *nla,
> > > if (tb[TCA_POLICE_AVRATE])
> > > new->tcfp_ewma_rate = nla_get_u32(tb[TCA_POLICE_AVRATE]);
> > >
> > > + if (tb[TCA_POLICE_PKTRATE64] && tb[TCA_POLICE_PKTBURST64]) {
> >
> > Should we reject if only one is present?
>
> Again, yes I think so.
> I'll confirm this with the author too.
... but add this restriction so the code will require either:
1. Both PKTRATE64 and PKTBURST64 are non-zero: packet-per-second limit is set;
or
2. Both PKTRATE64 and PKTBURST64 are zero: packet-per-second limit is cleared
...
