diff mbox

[ovs-dev] netdev-linux: Fix ingress policing burst rate configuration via tc

Message ID 20160414095142.GA9944@devstack.localdomain
State Accepted
Headers show

Commit Message

Miguel Angel Ajo April 14, 2016, 9:51 a.m. UTC 
The tc_police structure was filled with a value calculated in bits
instead of bytes while bytes were expected. This led the setting
of an x8 higher burst value.

Documentation and defaults have been corrected accordingly to minimize
nuisances on users sticking to the defaults.

The suggested burst value is now 80% of policing rate to make sure
TCP works correctly.

Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
Tested-by: Miguel Angel Ajo <majopela@redhat.com>
---
 FAQ.md               |  2 +-
 lib/netdev-linux.c   | 14 ++++----------
 vswitchd/vswitch.xml |  4 ++--
 3 files changed, 7 insertions(+), 13 deletions(-)

Comments

Ben Pfaff April 21, 2016, 9:51 p.m. UTC | #1 
On Thu, Apr 14, 2016 at 11:51:44AM +0200, Miguel Angel Ajo wrote:
> The tc_police structure was filled with a value calculated in bits
> instead of bytes while bytes were expected. This led the setting
> of an x8 higher burst value.
> 
> Documentation and defaults have been corrected accordingly to minimize
> nuisances on users sticking to the defaults.
> 
> The suggested burst value is now 80% of policing rate to make sure
> TCP works correctly.
> 
> Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
> Tested-by: Miguel Angel Ajo <majopela@redhat.com>

Thanks.  I applied this to master and branch-2.5.
diff mbox

Patch

diff --git a/FAQ.md b/FAQ.md
index 04ffc84..7dcdb4c 100644
--- a/FAQ.md
+++ b/FAQ.md
@@ -1124,7 +1124,7 @@  A: A policing policy can be configured on an interface to drop packets
    generate to 10Mbps:
 
        ovs-vsctl set interface vif1.0 ingress_policing_rate=10000
-       ovs-vsctl set interface vif1.0 ingress_policing_burst=1000
+       ovs-vsctl set interface vif1.0 ingress_policing_burst=8000
 
    Traffic policing can interact poorly with some network protocols and
    can have surprising results.  The "Ingress Policing" section of
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index a7d7ac7..a2b6b8a 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2045,7 +2045,7 @@  netdev_linux_set_policing(struct netdev *netdev_,
     int error;
 
     kbits_burst = (!kbits_rate ? 0       /* Force to 0 if no rate specified. */
-                   : !kbits_burst ? 1000 /* Default to 1000 kbits if 0. */
+                   : !kbits_burst ? 8000 /* Default to 8000 kbits if 0. */
                    : kbits_burst);       /* Stick with user-specified value. */
 
     ovs_mutex_lock(&netdev->mutex);
@@ -4720,21 +4720,15 @@  tc_add_policer(struct netdev *netdev,
     tc_police.mtu = mtu;
     tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, mtu);
 
-    /* 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.
+    /* The following appears wrong in one way: 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);
+        tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8);
 
     tcmsg = tc_make_request(netdev, RTM_NEWTFILTER,
                             NLM_F_EXCL | NLM_F_CREATE, &request);
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 7d6976f..fca238d 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -2434,7 +2434,7 @@ 
 
       <column name="ingress_policing_burst">
         <p>Maximum burst size for data received on this interface, in kb.  The
-        default burst size if set to <code>0</code> is 1000 kb.  This value
+        default burst size if set to <code>0</code> is 8000 kbit.  This value
         has no effect if <ref column="ingress_policing_rate"/>
         is <code>0</code>.</p>
         <p>
@@ -2442,7 +2442,7 @@ 
           which is important for protocols like TCP that react severely to
           dropped packets.  The burst size should be at least the size of the
           interface's MTU.  Specifying a value that is numerically at least as
-          large as 10% of <ref column="ingress_policing_rate"/> helps TCP come
+          large as 80% of <ref column="ingress_policing_rate"/> helps TCP come
           closer to achieving the full rate.
         </p>
       </column>