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);
}
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
