[ovs-dev] [PATCH OVS 2/4] dpif-netdev: Add burst size to buckets

[William Tu]

On Sun, May 10, 2020 at 7:12 PM Tonghao Zhang <xiangxia.m.yue at gmail.com> wrote:
>
> On Sat, May 9, 2020 at 11:26 PM William Tu <u9012063 at gmail.com> wrote:
> >
> > On Sat, May 09, 2020 at 09:54:10AM +0800, Tonghao Zhang wrote:
> > > On Sat, May 9, 2020 at 7:23 AM William Tu <u9012063 at gmail.com> wrote:
> > > >
> > > > On Thu, Apr 30, 2020 at 07:00:37PM +0800, xiangxia.m.yue at gmail.com wrote:
> > > > > From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> > > > >
> > > > > For now, the meter of the userspace datapath, don't include
> > > > > the bucket burst size to buckets. This patch includes it now.
> > > > >
> > > > > Cc: Ilya Maximets <i.maximets at ovn.org>
> > > > > Cc: William Tu <u9012063 at gmail.com>
> > > > > Cc: Jarno Rajahalme <jarno at ovn.org>
> > > > > Cc: Ben Pfaff <blp at ovn.org>
> > > > > Cc: Andy Zhou <azhou at ovn.org>
> > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> > > > > ---
> > > > >  lib/dpif-netdev.c | 7 +------
> > > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > > > index 17c0241aa2e2..59546db6a2a2 100644
> > > > > --- a/lib/dpif-netdev.c
> > > > > +++ b/lib/dpif-netdev.c
> > > > > @@ -6092,15 +6092,10 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id meter_id,
> > > > >      for (i = 0; i < config->n_bands; ++i) {
> > > > >          uint32_t band_max_delta_t;
> > > > >
> > > > > -        /* Set burst size to a workable value if none specified. */
> > > > > -        if (config->bands[i].burst_size == 0) {
> > > > > -            config->bands[i].burst_size = config->bands[i].rate;
> > > > > -        }
> > > > > -
> > > > >          meter->bands[i].up = config->bands[i];
> > > > >          /* Convert burst size to the bucket units: */
> > > > >          /* pkts => 1/1000 packets, kilobits => bits. */
> > > > > -        meter->bands[i].up.burst_size *= 1000;
> > > > > +        meter->bands[i].up.burst_size += config->bands[i].rate * 1000ULL;
> > > >
> > > > I don't quite understand.
> > > > Isn't this remove the setting of burst_size and always use
> > > > 'config->bands[i].rate * 1000ULL;'?
> > > Hi William, thanks for you reviews,
> > > meter->bands[i].up.burst_size += config->bands[i].rate * 1000ULL;
> > > burst_size  will plus the config->bands[i].rate * 1000ULL  and then
> > > assigned to burst_size again.
> > > so if user don't set the burst_size, burst_size is 0, and only plus
> > > the config->bands[i].rate * 1000ULL.
> > > Before the patch, if user don't set the burst_sze, burst_size = 0, and
> > > will the rate *1000.
> > > Here, burst_size is different from kernel datapath. burst_size in
> > > netdev will be used as bucket. so buckets shoud be "burst_size" + rate
> > >
> > > > Ex: When user set
> > > > ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=123
> > > > does 123 get set?
> > > burst_size(used for bucket size )should be (burst_size + rate) *1000
> > > my patch should be: because burst_size uint kilobits
> > > -        /* Set burst size to a workable value if none specified. */
> > > -        if (config->bands[i].burst_size == 0) {
> > > -            config->bands[i].burst_size = config->bands[i].rate;
> > > -        }
> > > -
> > >          meter->bands[i].up = config->bands[i];
> > >          /* Convert burst size to the bucket units: */
> > >          /* pkts => 1/1000 packets, kilobits => bits. */
> > > -        meter->bands[i].up.burst_size *= 1000;
> > > +        meter->bands[i].up.burst_size += config->bands[i].rate;
> > > +        meter->bands[i].up.burst_size *= 1000ULL;
> >
> >
> > OK, thanks.
> > btw, why should we include bucket to burst_size?
> In netdev datapath, up.burst_size will be used as buckets, the
> "burst_size" in the command:
> ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats
> bands=type=drop rate=1 burst_size=1024'
>
> should be included to buckets, ("up.burst_size"). Think about tbf in kernel:
> $ tc qdisc add dev enp130s0f0 handle 10: root tbf rate 10mbit burst
> 2mb latency 70ms
> the command above, allow 2mb burst, and ovs kernel datapath also do that:
> in dp_meter_create function:
> band->bucket = (band->burst_size + band->rate) * 1000ULL;

Thanks, now I understand.
William