| Message ID | 20210112113308.48307-1-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,ovs,1/2] dpif-netdev: Fix the meter buckets overflow. | expand |
Bleep bloop. Greetings Tonghao Zhang, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Inappropriate spacing around cast
#52 FILE: lib/dpif-netdev.c:6166:
band->bucket += (uint64_t)delta_t * band->rate;
Lines checked: 110, Warnings: 0, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 1/12/21 12:33 PM, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > When setting the meter rate to 4.3+Gbps, there is an overflow, the > meters don't work as expected. > > $ ovs-ofctl -O OpenFlow13 add-meter br-int "meter=1 kbps stats bands=type=drop rate=4294968" > > Before the patch, the buckets of meters was stored in its burst_size > of ofputil_meter_band. It was overflow when we set the rate to 4294968. > This patch don't change the public API and structure. This patch remove > the "up" from dp_meter_band, and introduce the type, rate to datapath's > meter bands. Then datapath don't depend upper layer. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- Hi. Unit tests are failing with these patches applied: 990: dpif-netdev - meters FAILED (dpif-netdev.at:319) Please, check them. Best regards, Ilya Maximets.
On Thu, Jan 14, 2021 at 7:30 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/12/21 12:33 PM, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > When setting the meter rate to 4.3+Gbps, there is an overflow, the > > meters don't work as expected. > > > > $ ovs-ofctl -O OpenFlow13 add-meter br-int "meter=1 kbps stats bands=type=drop rate=4294968" > > > > Before the patch, the buckets of meters was stored in its burst_size > > of ofputil_meter_band. It was overflow when we set the rate to 4294968. > > This patch don't change the public API and structure. This patch remove > > the "up" from dp_meter_band, and introduce the type, rate to datapath's > > meter bands. Then datapath don't depend upper layer. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > Hi. Unit tests are failing with these patches applied: > > 990: dpif-netdev - meters FAILED (dpif-netdev.at:319) Hi Ilya Before the patch, dpif-netdev meter uses the burst_size*1000 as the real buckets, if users don't specify the burst_size, buckets = rate *1000 as buckets. I think that is not the proper way. we should the max buckets: buckets = (burst_size + rate) * 1000. and when update buckets: band->bucket += (uint64_t)delta_t * band->rate; The test suite will be change: diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at index 2862a3c9b..9d7c092fc 100644 --- a/tests/dpif-netdev.at +++ b/tests/dpif-netdev.at @@ -282,7 +282,7 @@ OVS_VSWITCHD_START( AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1']) -AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=2 kbps burst stats bands=type=drop rate=1 burst_size=2']) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=2 kbps burst stats bands=type=drop rate=1 burst_size=1']) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7']) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1']) AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8']) @@ -295,7 +295,7 @@ meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1 meter=2 kbps burst stats bands= -type=drop rate=1 burst_size=2 +type=drop rate=1 burst_size=1 ]) ovs-appctl time/warp 5000 @@ -312,14 +312,14 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p8 'in_port(8),packet_type(ns=0,id=0), sleep 1 # wait for forwarders process packets # Meter 1 is measuring packets, allowing one packet per second with -# bursts of one packet, so 4 out of 5 packets should hit the drop +# bursts of one packet, so 3 out of 5 packets should hit the drop # band. -# Meter 2 is measuring kbps, with burst size 2 (== 2000 bits). 4 packets -# (240 bytes == 1920 bits) pass, but the last packet should hit the drop band. +# Meter 2 is measuring kbps, with burst size 1 (= 2000bits). 4 packets +# (120 bytes == 1920 bits) pass, but the last packet should hit the drop band. AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl OFPST_METER reply (OF1.3) (xid=0x2): meter:1 flow_count:1 packet_in_count:5 byte_in_count:300 duration:0.0s bands: -0: packet_count:4 byte_count:240 +0: packet_count:3 byte_count:180 meter:2 flow_count:1 packet_in_count:5 byte_in_count:300 duration:0.0s bands: 0: packet_count:1 byte_count:60 @@ -343,13 +343,13 @@ sleep 1 # wait for forwarders process packets # Meter 1 is measuring packets, allowing one packet per second with # bursts of one packet, so all 5 of the new packets should hit the drop # band. -# Meter 2 is measuring kbps, with burst size 2 (== 2000 bits). After 500ms +# Meter 2 is measuring kbps, with burst size 1 (== 2000 bits). After 500ms # there should be space for 80 + 500 bits, so one new 60 byte (480 bit) packet # should pass, remaining 4 should hit the drop band. AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl OFPST_METER reply (OF1.3) (xid=0x2): meter:1 flow_count:1 packet_in_count:10 byte_in_count:600 duration:0.0s bands: -0: packet_count:9 byte_count:540 +0: packet_count:8 byte_count:480 meter:2 flow_count:1 packet_in_count:10 byte_in_count:600 duration:0.0s bands: 0: packet_count:5 byte_count:300 > Please, check them. > > Best regards, Ilya Maximets.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 300861ca5..3ad737248 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -279,8 +279,10 @@ static bool dpcls_lookup(struct dpcls *cls, ( 1 << OFPMBT13_DROP ) struct dp_meter_band { - struct ofputil_meter_band up; /* type, prec_level, pad, rate, burst_size */ - uint32_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */ + uint16_t type; + uint32_t rate; + uint32_t burst_size; + uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */ uint64_t packet_count; uint64_t byte_count; }; @@ -6156,12 +6158,14 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, /* Update all bands and find the one hit with the highest rate for each * packet (if any). */ for (int m = 0; m < meter->n_bands; ++m) { - band = &meter->bands[m]; + uint64_t max_bucket_size; + band = &meter->bands[m]; /* Update band's bucket. */ - band->bucket += delta_t * band->up.rate; - if (band->bucket > band->up.burst_size) { - band->bucket = band->up.burst_size; + max_bucket_size = band->rate * 1000ULL; + band->bucket += (uint64_t)delta_t * band->rate; + if (band->bucket > max_bucket_size) { + band->bucket = max_bucket_size; } /* Drain the bucket for all the packets, if possible. */ @@ -6179,8 +6183,8 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, * (Only one band will be fired by a packet, and that * can be different for each packet.) */ for (int i = band_exceeded_pkt; i < cnt; i++) { - if (band->up.rate > exceeded_rate[i]) { - exceeded_rate[i] = band->up.rate; + if (band->rate > exceeded_rate[i]) { + exceeded_rate[i] = band->rate; exceeded_band[i] = m; } } @@ -6199,8 +6203,8 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, /* Update the exceeding band for the exceeding packet. * (Only one band will be fired by a packet, and that * can be different for each packet.) */ - if (band->up.rate > exceeded_rate[i]) { - exceeded_rate[i] = band->up.rate; + if (band->rate > exceeded_rate[i]) { + exceeded_rate[i] = band->rate; exceeded_band[i] = m; } } @@ -6277,21 +6281,15 @@ 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; - /* Initialize bucket to empty. */ - meter->bands[i].bucket = 0; + meter->bands[i].type = config->bands[i].type; + meter->bands[i].rate = config->bands[i].rate; + meter->bands[i].burst_size = config->bands[i].burst_size; + /* Start with a full bucket. */ + meter->bands[i].bucket = meter->bands[i].rate * 1000ULL; /* Figure out max delta_t that is enough to fill any bucket. */ band_max_delta_t - = meter->bands[i].up.burst_size / meter->bands[i].up.rate; + = meter->bands[i].bucket / meter->bands[i].rate; if (band_max_delta_t > meter->max_delta_t) { meter->max_delta_t = band_max_delta_t; }