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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
