Would you like to submit a fix?
On Tue, Apr 12, 2016 at 01:34:28PM +0200, Miguel Angel Ajo Pelayo wrote: > After looking at iproute2/tc sources and ovs sources, I believe the > error is in ovs, > In tc, the burst is calculated with > > tc_calc_xmittime(size, rate) = tick_in_us * TIME_UNITS_PER_SEC * > (size/rate) > > tick_in_us seems to be the number of ticks in a microsecond, > and TIME_UNITS_PER_SEC is 1000000 > > In ovs the burst is calculated with tc_bytes_to_ticks(rate, size) = > ticks_per_s * (size/rate) > and we pass "size" as bits, while it should be bytes. > > https://github.com/openvswitch/ovs/blob/master/lib/netdev-linux.c#L4736 > > it should be (kbits_burst * 1024) / 8 instead of kbits_burst*1024 > > If somebody could doublecheck, that'd be great. > > > > > On Tue, Apr 12, 2016 at 11:05 AM, Miguel Angel Ajo Pelayo > <majop...@redhat.com> wrote: > > Hi Ben, > > > > I think slawe is not worried about the 1000 vs 1024 difference. > > > > But on the fact that when setting 1000kbit burst on an policy, > > when you check via tc cmdline, you see 1000kbyte. > > > > Is TC command line reporting it wrong?, is it TC API?, or is it a > > bug in OVS?. > > > > I'm reading the TC API and cmdline tool trying to identify that, > > but I probably don't have the expertise... we will see.. > > > > Best regards, > > > > On Sun, Apr 10, 2016 at 10:25 PM, Ben Pfaff <b...@ovn.org> wrote: > >> No, I'm talking about the rest of the comment, please read it again. > >> The difference between 1000 vs 1024 bits is a trivial 2.4% difference. > >> > >> On Sun, Apr 10, 2016 at 09:46:28PM +0200, Sławek Kapłoński wrote: > >>> Hello, > >>> > >>> Thx for anwear and checking that. AFAIU You are talking about issue with > >>> SI > >>> and IEC units and problem with it. > >>> I'm not talking about such issue or not issue. What I pointed here is that > >>> (from comment in ovs code) ovs is doing something like: > >>> "sbin/tc filter add dev <devname> parent ffff: protocol all prio 49 basic > >>> police rate <kbits_rate>kbit burst <kbits_burst>k > >>> " > >>> please note this <kbit_burst>k > >>> but problem is that when You give "k" to tc it is not kilobits but > >>> kiloBYTES. > >>> So ovs should do something like ".... burst <kbits_burst>kbit" in this > >>> command. Or maybe it is intentional to use "k" there so IMHO message in > >>> comment should be something like "... burst <kilobyte_burst>k" > >>> > >>> -- > >>> Pozdrawiam / Best regards > >>> Sławek Kapłoński > >>> sla...@kaplonski.pl > >>> > >>> Dnia niedziela, 10 kwietnia 2016 12:36:46 CEST Ben Pfaff pisze: > >>> > On Thu, Apr 07, 2016 at 09:28:50PM +0200, Sławek Kapłoński wrote: > >>> > > Hello, > >>> > > > >>> > > I'm playing littlebit with ingress policing in openvswitch and I found > >>> > > some > >>> > > kind of inconsistency. > >>> > > In docs and in source code > >>> > > (https://github.com/openvswitch/ovs/blob/master/ > >>> > > lib/netdev-linux.c#L4698) there is info that burst is set in > >>> > > kilobits, but > >>> > > I found that in fact it is set in kilobytes in tc. > >>> > > So long story short: > >>> > > > >>> > > root@ubuntu-1604-test:/home/ubuntu# ovs-vsctl show > >>> > > f0d1f90d-174f-47bf-89bc-bf37f2da0271 > >>> > > > >>> > > Bridge "br1" > >>> > > > >>> > > Port vethA > >>> > > > >>> > > Interface vethA > >>> > > > >>> > > Port "br1" > >>> > > > >>> > > Interface "br1" > >>> > > > >>> > > type: internal > >>> > > > >>> > > ovs_version: "2.5.0" > >>> > > > >>> > > root@ubuntu-1604-test:/home/ubuntu# ovs-vsctl set Interface vethA > >>> > > ingress_policing_rate=1000 -- set Interface vethA > >>> > > ingress_policing_burst=100 > >>> > > > >>> > > root@ubuntu-1604-test:/home/ubuntu# ovs-vsctl list interface vethA | > >>> > > grep > >>> > > ingress > >>> > > ingress_policing_burst: 100 > >>> > > ingress_policing_rate: 1000 > >>> > > > >>> > > > >>> > > results in: > >>> > > root@ubuntu-1604-test:/home/ubuntu# tc filter show dev vethA parent > >>> > > ffff: > >>> > > filter protocol all pref 49 basic > >>> > > filter protocol all pref 49 basic handle 0x1 > >>> > > > >>> > > police 0x1 rate 1Mbit burst 100Kb mtu 64Kb action drop overhead 0b > >>> > > > >>> > > ref 1 bind 1 > >>> > > > >>> > > > >>> > > As You can see above, burst is given in "Kb" units what, according to > >>> > > tc > >>> > > man (http://lartc.org/manpages/tc.txt) means kilobytes. > >>> > > > >>> > > So question: is it intentional inconsistency or bug? If bug then > >>> > > where: in > >>> > > code or in docs? > >>> > > >>> > We applied a fix to the policing code last year, in the following > >>> > commit. If you read the long comment that it adds, and then compare > >>> > that with what you're saying, it sounds like tc once had a bug where it > >>> > confused bits and bytes, and we modified OVS so that it had the same > >>> > bug. Presumably, now tc's bug has been fixed, and so we should fix OVS > >>> > the same way. > >>> > > >>> > --8<--------------------------cut here-------------------------->8-- > >>> > > >>> > From: Ben Pfaff <b...@nicira.com> > >>> > Date: Fri, 13 Mar 2015 11:30:18 -0700 > >>> > Subject: [PATCH] netdev-linux: Be more careful about integer overflow in > >>> > policing. > >>> > > >>> > Otherwise the policing limits could make no sense if large rates were > >>> > specified. > >>> > > >>> > Reported-by: Zhangguanghui <zhang.guang...@h3c.com> > >>> > Signed-off-by: Ben Pfaff <b...@nicira.com> > >>> > Acked-by: Ansis Atteka <aatt...@nicira.com> > >>> > --- > >>> > AUTHORS | 1 + > >>> > lib/netdev-linux.c | 29 ++++++++++++++++++++++------- > >>> > vswitchd/bridge.c | 4 ++-- > >>> > 3 files changed, 25 insertions(+), 9 deletions(-) > >>> > > >>> > diff --git a/AUTHORS b/AUTHORS > >>> > index fe79acd..d7925db 100644 > >>> > --- a/AUTHORS > >>> > +++ b/AUTHORS > >>> > @@ -345,6 +345,7 @@ Voravit T. vora...@kth.se > >>> > Yeming Zhao zhaoyem...@gmail.com > >>> > Ying Chen yingc...@vmware.com > >>> > Yongqiang Liu liuyq7...@gmail.com > >>> > +Zhangguanghui zhang.guang...@h3c.com > >>> > Ziyou Wang ziy...@vmware.com > >>> > Zoltán Balogh zoltan.bal...@ericsson.com > >>> > ankur dwivedi ankurengg2...@gmail.com > >>> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > >>> > index 662ccc9..6e574cd 100644 > >>> > --- a/lib/netdev-linux.c > >>> > +++ b/lib/netdev-linux.c > >>> > @@ -1,5 +1,5 @@ > >>> > /* > >>> > - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. > >>> > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. > >>> > * > >>> > * Licensed under the Apache License, Version 2.0 (the "License"); > >>> > * you may not use this file except in compliance with the License. > >>> > @@ -399,8 +399,8 @@ static struct tcmsg *tc_make_request(const struct > >>> > netdev > >>> > *, int type, unsigned int flags, struct ofpbuf *); static int > >>> > tc_transact(struct ofpbuf *request, struct ofpbuf **replyp); static int > >>> > tc_add_del_ingress_qdisc(struct netdev *netdev, bool add); -static int > >>> > tc_add_policer(struct netdev *netdev, int kbits_rate, > >>> > - int kbits_burst); > >>> > +static int tc_add_policer(struct netdev *, > >>> > + uint32_t kbits_rate, uint32_t kbits_burst); > >>> > > >>> > static int tc_parse_qdisc(const struct ofpbuf *, const char **kind, > >>> > struct nlattr **options); > >>> > @@ -4028,12 +4028,13 @@ tc_add_del_ingress_qdisc(struct netdev *netdev, > >>> > bool > >>> > add) * mtu 65535 drop > >>> > * > >>> > * The configuration and stats may be seen with the following command: > >>> > - * /sbin/tc -s filter show <devname> eth0 parent ffff: > >>> > + * /sbin/tc -s filter show dev <devname> parent ffff: > >>> > * > >>> > * Returns 0 if successful, otherwise a positive errno value. > >>> > */ > >>> > static int > >>> > -tc_add_policer(struct netdev *netdev, int kbits_rate, int kbits_burst) > >>> > +tc_add_policer(struct netdev *netdev, > >>> > + uint32_t kbits_rate, uint32_t kbits_burst) > >>> > { > >>> > struct tc_police tc_police; > >>> > struct ofpbuf request; > >>> > @@ -4047,8 +4048,22 @@ tc_add_policer(struct netdev *netdev, int > >>> > kbits_rate, > >>> > int kbits_burst) tc_police.action = TC_POLICE_SHOT; > >>> > tc_police.mtu = mtu; > >>> > tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, > >>> > mtu); > >>> > - tc_police.burst = tc_bytes_to_ticks(tc_police.rate.rate, > >>> > - kbits_burst * 1024); > >>> > + > >>> > + /* The following appears wrong in two ways: > >>> > + * > >>> > + * - tc_bytes_to_ticks() should take "bytes" as quantity for both > >>> > of > >>> > its + * arguments (or at least consistently "bytes" as both or > >>> > "bits" > >>> > as + * both), but this supplies bytes for the first argument and > >>> > bits > >>> > for the + * second. > >>> > + * > >>> > + * - In networking a kilobit is usually 1000 bits but this uses > >>> > 1024 > >>> > bits. + * > >>> > + * However if you "fix" those problems then "tc filter show ..." > >>> > shows > >>> > + * "125000b", meaning 125,000 bits, when OVS configures it for 1000 > >>> > kbit == + * 1,000,000 bits, whereas this actually ends up doing the > >>> > right thing from + * tc's point of view. Whatever. */ > >>> > + tc_police.burst = tc_bytes_to_ticks( > >>> > + tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * > >>> > 1024); > >>> > > >>> > tcmsg = tc_make_request(netdev, RTM_NEWTFILTER, > >>> > NLM_F_EXCL | NLM_F_CREATE, &request); > >>> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > >>> > index 85bbfa3..68648b9 100644 > >>> > --- a/vswitchd/bridge.c > >>> > +++ b/vswitchd/bridge.c > >>> > @@ -4445,8 +4445,8 @@ iface_configure_qos(struct iface *iface, const > >>> > struct > >>> > ovsrec_qos *qos) } > >>> > > >>> > netdev_set_policing(iface->netdev, > >>> > - iface->cfg->ingress_policing_rate, > >>> > - iface->cfg->ingress_policing_burst); > >>> > + MIN(UINT32_MAX, > >>> > iface->cfg->ingress_policing_rate), > >>> > + MIN(UINT32_MAX, > >>> > iface->cfg->ingress_policing_burst)); > >>> > > >>> > ofpbuf_uninit(&queues_buf); > >>> > } > >> > >> _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev