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
