| Message ID | 1588244439-58766-3-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | expand the meter table and fix bug | expand |
On Thu, Apr 30, 2020 at 07:00:37PM +0800, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@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@ovn.org> > Cc: William Tu <u9012063@gmail.com> > Cc: Jarno Rajahalme <jarno@ovn.org> > Cc: Ben Pfaff <blp@ovn.org> > Cc: Andy Zhou <azhou@ovn.org> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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;'? 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? William > /* Initialize bucket to empty. */ > meter->bands[i].bucket = 0; > > -- > 1.8.3.1 >
On Sat, May 9, 2020 at 7:23 AM William Tu <u9012063@gmail.com> wrote: > > On Thu, Apr 30, 2020 at 07:00:37PM +0800, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@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@ovn.org> > > Cc: William Tu <u9012063@gmail.com> > > Cc: Jarno Rajahalme <jarno@ovn.org> > > Cc: Ben Pfaff <blp@ovn.org> > > Cc: Andy Zhou <azhou@ovn.org> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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; > William > > /* Initialize bucket to empty. */ > > meter->bands[i].bucket = 0; > > > > -- > > 1.8.3.1 > >
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@gmail.com> wrote: > > > > On Thu, Apr 30, 2020 at 07:00:37PM +0800, xiangxia.m.yue@gmail.com wrote: > > > From: Tonghao Zhang <xiangxia.m.yue@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@ovn.org> > > > Cc: William Tu <u9012063@gmail.com> > > > Cc: Jarno Rajahalme <jarno@ovn.org> > > > Cc: Ben Pfaff <blp@ovn.org> > > > Cc: Andy Zhou <azhou@ovn.org> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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? William
On Sat, May 9, 2020 at 11:26 PM William Tu <u9012063@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@gmail.com> wrote: > > > > > > On Thu, Apr 30, 2020 at 07:00:37PM +0800, xiangxia.m.yue@gmail.com wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@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@ovn.org> > > > > Cc: William Tu <u9012063@gmail.com> > > > > Cc: Jarno Rajahalme <jarno@ovn.org> > > > > Cc: Ben Pfaff <blp@ovn.org> > > > > Cc: Andy Zhou <azhou@ovn.org> > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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 again. > William >
On Sun, May 10, 2020 at 7:12 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Sat, May 9, 2020 at 11:26 PM William Tu <u9012063@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@gmail.com> wrote: > > > > > > > > On Thu, Apr 30, 2020 at 07:00:37PM +0800, xiangxia.m.yue@gmail.com wrote: > > > > > From: Tonghao Zhang <xiangxia.m.yue@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@ovn.org> > > > > > Cc: William Tu <u9012063@gmail.com> > > > > > Cc: Jarno Rajahalme <jarno@ovn.org> > > > > > Cc: Ben Pfaff <blp@ovn.org> > > > > > Cc: Andy Zhou <azhou@ovn.org> > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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
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; /* Initialize bucket to empty. */ meter->bands[i].bucket = 0;