netdev - Re: [PATCH RFC net-next] net/sched: act_police: add support for packet-per-second policing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210201123824.GA26709@netronome.com>
Date:   Mon, 1 Feb 2021 13:38:25 +0100
From:   Simon Horman <simon.horman@...ronome.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...lanox.com>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com,
        Baowen Zheng <baowen.zheng@...igine.com>,
        Louis Peens <louis.peens@...ronome.com>
Subject: Re: [PATCH RFC net-next] net/sched: act_police: add support for
 packet-per-second policing

On Mon, Feb 01, 2021 at 01:31:17PM +0100, Simon Horman wrote:
> On Thu, Jan 28, 2021 at 06:19:33PM +0200, Ido Schimmel wrote:
> > On Mon, Jan 25, 2021 at 04:18:19PM +0100, Simon Horman wrote:
> > > From: Baowen Zheng <baowen.zheng@...igine.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.
> > 
> > Hi Simon,
> > 
> > Any reason to allow metering based on both packets and bytes at the same
> > action versus adding a mode (packets / bytes) parameter? You can then
> > chain two policers if you need to rate limit based on both. Something
> > like:
> > 
> > # tc filter add dev tap1 ingress pref 1 matchall \
> > 	action police rate 1000Mbit burst 128k conform-exceed drop/pipe \
> > 	action police pkts_rate 3000 pkts_burst 1000
> > 
> > I'm asking because the policers in the Spectrum ASIC are built that way
> > and I also don't remember seeing such a mixed mode online.
> 
> Hi Ido,
> 
> sorry for missing this email until you pointed it out to me in another
> thread.
> 
> We did consider this question during development and our conclusion was
> that it was useful as we do have use-cases which call for both to be used
> and it seems nice to allow lower layers to determine the order in which the
> actions are applied to satisfied the user's more general request for both -
> it should be no surprise that we plan to provide a hardware offload of this
> feature. It also seems to offer nice code re-use. We did also try to
> examine the performance impact of this change on existing use-cases and it
> appeared to be negligible/within noise of our measurements.
> 
> > > 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.zheng@...igine.com>
> > > Signed-off-by: Simon Horman <simon.horman@...ronome.com>
> > > Signed-off-by: Louis Peens <louis.peens@...ronome.com>
> > > ---
> > >  include/net/sch_generic.h      | 15 ++++++++++++++
> > >  include/net/tc_act/tc_police.h |  4 ++++
> > >  include/uapi/linux/pkt_cls.h   |  2 ++
> > >  net/sched/act_police.c         | 37 +++++++++++++++++++++++++++++++---
> > >  net/sched/sch_generic.c        | 32 +++++++++++++++++++++++++++++
> > >  5 files changed, 87 insertions(+), 3 deletions(-)
> > 
> > The intermediate representation in include/net/flow_offload.h needs to
> > carry the new configuration so that drivers will be able to veto
> > unsupported configuration.

Thanks for pointing that out. I do have a patch for that but was planning
to post it as a follow-up to keep this series simple. I do see your point
that the flow_offload.h change is a dependency of this patch. I'll include
it when reposting.