[ovs-dev] [PATCH v2 1/2] add port-based ingress policing based packet-per-second rate-limiting

Simon Horman simon.horman at netronome.com
Fri Jun 4 14:33:33 UTC 2021


Hi,

On Thu, May 20, 2021 at 08:48:53AM -0700, Marcelo Ricardo Leitner wrote:
> Hi,
> 
> On Mon, May 17, 2021 at 01:18:53PM +0200, Simon Horman wrote:

...

> > @@ -2593,24 +2601,50 @@ tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
> >  }
> >
> >  static void
> > -nl_msg_put_act_police(struct ofpbuf *request, struct tc_police police)
> > +nl_msg_put_act_police(struct ofpbuf *request, struct tc_police police,
> > +                      uint32_t kpkts_rate, uint32_t kpkts_burst)
> >  {
> > -    size_t offset;
> > +    size_t offset, act_offset;
> > +    uint32_t i = 0, prio = 0;
> >
> > -    nl_msg_put_string(request, TCA_ACT_KIND, "police");
> > -    offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> > -    nl_msg_put_unspec(request, TCA_POLICE_TBF, &police, sizeof police);
> > -    tc_put_rtab(request, TCA_POLICE_RATE, &police.rate);
> > -    nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_UNSPEC);
> > -    nl_msg_end_nested(request, offset);
> > +    for (i = 0; i < OVS_TC_QOS_TYPE_MAX; i++) {
> > +        if (i == OVS_TC_QOS_TYPE_BPS && !police.rate.rate) {
> > +            continue;
> > +        }
> > +        if (i == OVS_TC_QOS_TYPE_PPS && !kpkts_rate) {
> > +            continue;
> > +        }
> > +        act_offset = nl_msg_start_nested(request, ++prio);
> > +        nl_msg_put_string(request, TCA_ACT_KIND, "police");
> > +        offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> > +        if (i == OVS_TC_QOS_TYPE_BPS && police.rate.rate) {
> 
> Nit, the "&& ..." condition is not needed, due to the if()s above.
> 
> > +            tc_put_rtab(request, TCA_POLICE_RATE, &police.rate);
> > +        } else if (i == OVS_TC_QOS_TYPE_PPS && kpkts_rate) {
> > +            unsigned int pkt_burst_ticks, pps_rate;
> > +            /* for PPS, set rate as 0 to act as a single action */
> > +            police.rate.rate = 0;
> > +            police.burst = 0;
> > +            police.rate.cell_log = 0;
> 
> This is changing the parameter and implicitly relies on the fact that
> OVS_TC_QOS_TYPE_PPS > OVS_TC_QOS_TYPE_BPS. It works, but may cause
> problems in the future.
> 
> Maybe add a new variable that will be actually used by
> nl_msg_put_unspec() below, and initialize it for each case?

Thanks. We'll rework this section, probably removing the for loop,
to address this and the other problem you've raised above.

> > +            pps_rate = kpkts_rate * 1000;
> > +            pkt_burst_ticks = tc_bytes_to_ticks(pps_rate,
> > +                                                MIN(UINT32_MAX / 1024, kpkts_burst) * 1024);
> 
> I don't get why for pps_rate 'k' is 1000, and for bursts, it's 1024.
> Anyhow, this is also there for bps and it makes sense to follow that.
> But some mention to it in the docs below is welcomed.

I think that I've concluded that the best approach here is to consistently
use 1000. And that part of the confusion stems from tc_bytes_to_ticks()
being used to convert from packets to ticks in this case, so we'll add
a comment noting that.

> 
> > +            nl_msg_put_u64(request, TCA_POLICE_PKTRATE64, (uint64_t)pps_rate);
> > +            nl_msg_put_u64(request, TCA_POLICE_PKTBURST64, (uint64_t)pkt_burst_ticks);
> > +        }

...

> > @@ -2688,6 +2721,10 @@ netdev_linux_set_policing(struct netdev *netdev_,
> >                     : !kbits_burst ? 8000 /* Default to 8000 kbits if 0. */
> >                     : kbits_burst);       /* Stick with user-specified value. */
> >
> > +    kpkts_burst = (!kpkts_rate ? 0       /* Force to 0 if no rate specified. */
> > +                   : !kpkts_burst ? 16   /* Default to 16000 packets if 0. */
> 
> It should be 16 -> 16*1024, or just '16 kpps'.

We think '16 kpps' is the way to go here.

> > +                   : kpkts_burst);       /* Stick with user-specified value. */

...

> > @@ -3712,6 +3750,24 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
> >            closer to achieving the full rate.
> >          </p>
> >        </column>
> > +
> > +      <column name="ingress_policing_kpkts_burst">
> > +        <p>
> > +          Maximum burst size for data received on this interface, in
> > +          kilo-packets.  The default burst size if set to <code>0</code> is
> > +          16 kilo-packets. This value has no effect if <ref
> > +          column="ingress_policing_kpkts_rate"/> is <code>0</code>.</p>
> > +        <p>
> > +          Specifying a larger burst size lets the algorithm be more
> > +          forgiving, 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 80% of <ref
> > +          column="ingress_policing_kpkts_rate"/> helps TCP come closer to
> > +          achieving the full rate.
> 
> This paragraph is present for ingress_policing_burst, and that's all
> in bytes. Seems it was partially updated to handle packets.
> 
> I fail to understand how MTU size, even if just numerically, is
> related to a count of packets. 9000 MTU allows more data through with
> less overhead but if with the same bandwidth, it would even have a
> smaller pps than a 1500 MTU.
> 
> The guidance here needs updating.

Thanks. We'll clean this up, including the MTU portion.


More information about the dev mailing list