[ovs-dev] [PATCH] netdev-linux: Fix ingress policing burst rate configuration via tc
Ben Pfaff
blp at ovn.org
Wed Apr 13 23:08:58 UTC 2016
Thanks for working on this. The original patch, as opposed to the
followup quoted below) doesn't seem to have made it to the mailing list
archive or to patchwork. Can you resend it?
On Wed, Apr 13, 2016 at 12:49:28PM +0200, Miguel Angel Ajo Pelayo wrote:
> I verified the 10% / 80% changes with netperf, it's unable to sustain 10Mbps
> without at least a 8Mbit burst. I've seen recommendations from cisco to use
> policing bursts of around 150 to 200%, but I guess that depends on the path
> latency or even the implementation.
>
> My netperf tests were in-host.
>
>
>
> On Wed, Apr 13, 2016 at 12:44 PM, Miguel Angel Ajo <majopela at redhat.com> 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 acordingly 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 at redhat.com>
> > Tested-by: Miguel Angel Ajo <majopela at redhat.com>
> > ---
> > FAQ.md | 2 +-
> > lib/netdev-linux.c | 14 ++++----------
> > vswitchd/vswitch.xml | 4 ++--
> > 3 files changed, 7 insertions(+), 13 deletions(-)
> >
> > 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>
> > --
> > 1.8.3.1
> >
> > This patch fixes the burst setting of the ingress policing on netdev-linux, and
> > corrects the documentation recommendations accordingly.
> >
> > Before the patch we had:
> >
> >
> > # ovs-vsctl -- --may-exist add-port br-int test-port -- set Interface test-port type=internal
> > # ovs-vsctl set interface test-port ingress_policing_rate=10000
> > # ovs-vsctl set interface test-port ingress_policing_burst=1000
> > # tc filter show dev test-port parent ffff:
> > filter protocol all pref 49 basic
> > filter protocol all pref 49 basic handle 0x1
> > police 0x1 rate 10000Kbit burst 1000Kb mtu 64Kb action drop overhead 0b
> > ref 1 bind 1
> >
> > After the patch we have:
> >
> > # ovs-vsctl set interface test-port ingress_policing_rate=10000
> > # ovs-vsctl set interface test-port ingress_policing_burst=1000
> > # tc filter show dev test-port parent ffff:
> > filter protocol all pref 49 basic
> > filter protocol all pref 49 basic handle 0x1
> > police 0x2 rate 10000Kbit burst 125Kb mtu 64Kb action drop overhead 0b
> > ref 1 bind 1
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list