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: <20210128120233.GA8059@netronome.com>
Date:   Thu, 28 Jan 2021 13:02:34 +0100
From:   Simon Horman <simon.horman@...ronome.com>
To:     Jakub Kicinski <kuba@...nel.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 Wed, Jan 27, 2021 at 12:59:05PM -0800, Jakub Kicinski wrote:
> On Wed, 27 Jan 2021 12:02:23 +0100 Simon Horman wrote:
> > > > +void psched_ppscfg_precompute(struct psched_pktrate *r,
> > > > +			      u64 pktrate64)
> > > > +{
> > > > +	memset(r, 0, sizeof(*r));
> > > > +	r->rate_pkts_ps = pktrate64;
> > > > +	r->mult = 1;
> > > > +	/* The deal here is to replace a divide by a reciprocal one
> > > > +	 * in fast path (a reciprocal divide is a multiply and a shift)
> > > > +	 *
> > > > +	 * Normal formula would be :
> > > > +	 *  time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
> > > > +	 *
> > > > +	 * We compute mult/shift to use instead :
> > > > +	 *  time_in_ns = (len * mult) >> shift;
> > > > +	 *
> > > > +	 * We try to get the highest possible mult value for accuracy,
> > > > +	 * but have to make sure no overflows will ever happen.
> > > > +	 */
> > > > +	if (r->rate_pkts_ps > 0) {
> > > > +		u64 factor = NSEC_PER_SEC;
> > > > +
> > > > +		for (;;) {
> > > > +			r->mult = div64_u64(factor, r->rate_pkts_ps);
> > > > +			if (r->mult & (1U << 31) || factor & (1ULL << 63))
> > > > +				break;
> > > > +			factor <<= 1;
> > > > +			r->shift++;  
> > > 
> > > Aren't there helpers somewhere for the reciprocal divide
> > > pre-calculation?  
> > 
> > Now that you mention it, yes.
> > 
> > Looking over reciprocal_divide() I don't think it a good fit here as it
> > operates on 32bit values, whereas the packet rate is 64 bit.
> > 
> > Packet rate could be changed to a 32 bit entity if we convince ourselves we
> > don't want more than 2^32 - 1 packets per second (a plausible position
> > IMHO) - but that leads us to a secondary issue.
> > 
> > The code above is very similar to an existing (long existing)
> > byte rate variant of this helper - psched_ratecfg_precompute().
> > And I do think we want to:
> > a) Support 64-bit byte rates. Indeed such support seems to have
> >    been added to support 25G use-cases
> > b) Calculate byte and packet rates the same way
> > 
> > So I feel less and less that reciprocal_divide() is a good fit.
> > But perhaps I am mistaken.
> > 
> > In the meantime I will take a look to see if a helper common function can
> > be made to do (64 bit) reciprocal divides for the packet and byte rate
> > use-cases.  I.e. the common code in psched_ppscfg_precompute() and
> > psched_ratecfg_precompute().
> 
> No strong feelings, I'll just ask to document the reasoning in the
> commit message or the comment above.

Thanks. I'll include some text explaining this when reposting.