[ovs-dev] unit of burst in ingress_policing
Ben Pfaff
blp at ovn.org
Tue Apr 12 18:33:16 UTC 2016
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
> <majopela at 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 <blp at 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
> >>> slawek at 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 at 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 at ubuntu-1604-test:/home/ubuntu# ovs-vsctl set Interface vethA
> >>> > > ingress_policing_rate=1000 -- set Interface vethA
> >>> > > ingress_policing_burst=100
> >>> > >
> >>> > > root at ubuntu-1604-test:/home/ubuntu# ovs-vsctl list interface vethA | grep
> >>> > > ingress
> >>> > > ingress_policing_burst: 100
> >>> > > ingress_policing_rate: 1000
> >>> > >
> >>> > >
> >>> > > results in:
> >>> > > root at 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 <blp at 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.guanghui at h3c.com>
> >>> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> >>> > Acked-by: Ansis Atteka <aatteka at 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. voravit at kth.se
> >>> > Yeming Zhao zhaoyeming at gmail.com
> >>> > Ying Chen yingchen at vmware.com
> >>> > Yongqiang Liu liuyq7809 at gmail.com
> >>> > +Zhangguanghui zhang.guanghui at h3c.com
> >>> > Ziyou Wang ziyouw at vmware.com
> >>> > Zoltán Balogh zoltan.balogh at ericsson.com
> >>> > ankur dwivedi ankurengg2003 at 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);
> >>> > }
> >>
> >>
More information about the dev
mailing list