| Message ID | 20210224143815.11615-1-simon.horman@netronome.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netdev-linux: correct unit of burst parameter | expand |
Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@netronome.com>, Louis Peens <louis.peens@netronome.com> Lines checked: 38, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Wed, Feb 24, 2021 at 6:38 AM Simon Horman <simon.horman@netronome.com> wrote: > > From: Yong Xu <yong.xu@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. A minor nit. s/Its/It's > > Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload") > Signed-off-by: Yong Xu <yong.xu@corigine.com> > [simon: reworked changelog] > Signed-off-by: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Louis Peens <louis.peens@netronome.com> > --- This patch LGTM. Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>
On Wed, Feb 24, 2021 at 10:38 PM Simon Horman <simon.horman@netronome.com> wrote: > From: Yong Xu <yong.xu@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@corigine.com> > [simon: reworked changelog] > Signed-off-by: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Louis Peens <louis.peens@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? > > unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8; > struct tc_police police; > struct tc_ratespec rate; > -- > 2.20.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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@netronome.com> > wrote: > > > From: Yong Xu <yong.xu@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@corigine.com> > > [simon: reworked changelog] > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > Signed-off-by: Louis Peens <louis.peens@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.
On Thu, Feb 25, 2021 at 4:14 PM Simon Horman <simon.horman@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@netronome.com> > > wrote: > > > > > From: Yong Xu <yong.xu@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@corigine.com> > > > [simon: reworked changelog] > > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > > Signed-off-by: Louis Peens <louis.peens@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.
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; unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8; struct tc_police police; struct tc_ratespec rate;