Message ID | 20210312115917.6838-2-simon.horman@netronome.com |
---|---|
State | Changes Requested |
Headers | show |
Series | 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 Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman 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. Yeah.. I have the feeling that it might be related to kernel's: /* Avoid doing 64 bit divide */ #define PSCHED_SHIFT 6 #define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT) #define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT) but I can't confirm it. > > 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 15b25084b..f87a20075 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; I know that the patch is not changing this but, while at this, why for bsize the 'k' is 1024, while for bps it's 1000? AFAICT it backtracks to netdev_set_policing(iface->netdev, MIN(UINT32_MAX, iface->cfg->ingress_policing_rate), MIN(UINT32_MAX, iface->cfg->ingress_policing_burst)); in iface_configure_qos() and I don't see a reason for them being different. qos.rst states: ``ingress_policing_rate`` the maximum rate (in Kbps) that this VM should be allowed to send ``ingress_policing_burst`` a parameter to the policing algorithm to indicate the maximum amount of data (in Kb) that this interface can send beyond the policing rate. Both with capital K. So if the 64 was bothering, the 1024 is likely doing so as well. Nevertheless, as the patch is fixing the /64, patch LGTM. > 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, Mar 25, 2021 at 03:51:11PM -0300, Marcelo Ricardo Leitner wrote: > On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman 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. > > Yeah.. I have the feeling that it might be related to kernel's: > /* Avoid doing 64 bit divide */ > #define PSCHED_SHIFT 6 > #define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT) > #define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT) > but I can't confirm it. > > > > > 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 15b25084b..f87a20075 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; > > I know that the patch is not changing this but, while at this, why for > bsize the 'k' is 1024, while for bps it's 1000? > > AFAICT it backtracks to > netdev_set_policing(iface->netdev, > MIN(UINT32_MAX, iface->cfg->ingress_policing_rate), > MIN(UINT32_MAX, iface->cfg->ingress_policing_burst)); > in iface_configure_qos() and I don't see a reason for them being > different. > > qos.rst states: > ``ingress_policing_rate`` > the maximum rate (in Kbps) that this VM should be allowed to send > > ``ingress_policing_burst`` > a parameter to the policing algorithm to indicate the maximum amount of data > (in Kb) that this interface can send beyond the policing rate. > > Both with capital K. So if the 64 was bothering, the 1024 is likely > doing so as well. While vswitchd/vswitch.xml says: <column name="ingress_policing_rate"> <p> Maximum rate for data received on this interface, in kbps. Data <column name="ingress_policing_burst"> <p>Maximum burst size for data received on this interface, in kb. The default burst size if set to <code>0</code> is 8000 kbit. This value I'm not sure which one has preference here. > > Nevertheless, as the patch is fixing the /64, patch LGTM. > > > 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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Mar 25, 2021 at 03:51:11PM -0300, Marcelo Ricardo Leitner wrote: > On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman 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. > > Yeah.. I have the feeling that it might be related to kernel's: > /* Avoid doing 64 bit divide */ > #define PSCHED_SHIFT 6 > #define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT) > #define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT) > but I can't confirm it. That is a pretty good point, and I suspect you are correct. But I honestly don't know either. > > 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 15b25084b..f87a20075 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; > > I know that the patch is not changing this but, while at this, why for > bsize the 'k' is 1024, while for bps it's 1000? > > AFAICT it backtracks to > netdev_set_policing(iface->netdev, > MIN(UINT32_MAX, iface->cfg->ingress_policing_rate), > MIN(UINT32_MAX, iface->cfg->ingress_policing_burst)); > in iface_configure_qos() and I don't see a reason for them being > different. > > qos.rst states: > ``ingress_policing_rate`` > the maximum rate (in Kbps) that this VM should be allowed to send > > ``ingress_policing_burst`` > a parameter to the policing algorithm to indicate the maximum amount of data > (in Kb) that this interface can send beyond the policing rate. > > Both with capital K. So if the 64 was bothering, the 1024 is likely > doing so as well. Thanks, I seem to recall looking into this once. I will dig once again and try and answer the question. Possibly then we will then decide to change things. Let's see. > Nevertheless, as the patch is fixing the /64, patch LGTM. > > > 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 >
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 15b25084b..f87a20075 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;