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
