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
