I will send it today. Best, Miguel Angel El 12/4/2016 20:33, "Ben Pfaff" <b...@ovn.org> escribió:
> 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
