[ovs-dev] [PATCH] netdev-linux: correct unit of burst parameter

[Tonghao Zhang]

On Thu, Feb 25, 2021 at 4:14 PM Simon Horman <simon.horman at netronome.com> wrote:
>
> On Thu, Feb 25, 2021 at 10:21:15AM +0800, Tonghao Zhang wrote:
> > On Wed, Feb 24, 2021 at 10:38 PM Simon Horman <simon.horman at netronome.com>
> > wrote:
> >
> > > From: Yong Xu <yong.xu at corigine.com>
> > >
> > > Correct calculation of burst parameter used when configuring TC policer
> > > action for ingress port-based policing in the case where TC offload is in
> > > use. This now matches the value calculated for the case where TC offload is
> > > not in use.
> > >
> > > The division by 8 is to convert from bits to bytes.
> > > Its unclear why 64 was previously used.
> > >
> > > Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for
> > > tc-offload")
> > > Signed-off-by: Yong Xu <yong.xu at corigine.com>
> > > [simon: reworked changelog]
> > > Signed-off-by: Simon Horman <simon.horman at netronome.com>
> > > Signed-off-by: Louis Peens <louis.peens at netronome.com>
> > > ---
> > >  lib/netdev-linux.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > > index 6be23dbee..eb28777f9 100644
> > > --- a/lib/netdev-linux.c
> > > +++ b/lib/netdev-linux.c
> > > @@ -2572,7 +2572,7 @@ exit:
> > >  static struct tc_police
> > >  tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
> > >  {
> > > -    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
> > > +    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
> >
> > There is any test case for this in ovs?
>
> Thanks, you raise a good point.
>
> What this does is change the value that is configured by OVS in TC.
> I took a look at the test-suite and I do not see any tests for that.
> Which I would say partially explains how this bug made it to upstream
> in the first place.
>
> We'll see about creating some tests to exercise the configuration of TC by
> OVS for rate limiting. But as these will be entirely new tests I suspect
> they will be, even if restricted to a small number, rather voluminous
> compared to the fix above. So I'm not sure if they should be a
> pre-requisite for accepting the fix or not.
Thanks for your patience to explain. we can apply this to upstream,
and send a testcase patch later.



-- 
Best regards, Tonghao