[ovs-dev] unit of burst in ingress_policing

Sławek Kapłoński slawek at kaplonski.pl
Sun Apr 10 19:46:28 UTC 2016


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