| Message ID | 20200523103320.47497-4-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | expand the meter table and fix bug. | expand |
On 5/23/20 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. > > 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> > Cc: Pravin Shelar <pshelar@ovn.org> > Acked-by: William Tu <u9012063@gmail.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > include/openvswitch/ofp-meter.h | 2 +- > lib/dpif-netdev.c | 4 ++-- > lib/ofp-meter.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h > index 6776eae87e26..f55f89ac1a71 100644 > --- a/include/openvswitch/ofp-meter.h > +++ b/include/openvswitch/ofp-meter.h > @@ -37,7 +37,7 @@ struct ofputil_meter_band { > uint16_t type; > uint8_t prec_level; /* Non-zero if type == OFPMBT_DSCP_REMARK. */ > uint32_t rate; > - uint32_t burst_size; > + uint64_t burst_size; This structure is part of a public API. We can't change it that simple. IIUC, the issue is on a datapath level, so it should be solvable without modifying anything outside the datapath code. Could you prepare this kind of fix and send it separately from the series? Best regards, Ilya Maximets.
On Mon, Jan 11, 2021 at 7:43 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 5/23/20 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. > > > > 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> > > Cc: Pravin Shelar <pshelar@ovn.org> > > Acked-by: William Tu <u9012063@gmail.com> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > include/openvswitch/ofp-meter.h | 2 +- > > lib/dpif-netdev.c | 4 ++-- > > lib/ofp-meter.c | 4 ++-- > > 3 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h > > index 6776eae87e26..f55f89ac1a71 100644 > > --- a/include/openvswitch/ofp-meter.h > > +++ b/include/openvswitch/ofp-meter.h > > @@ -37,7 +37,7 @@ struct ofputil_meter_band { > > uint16_t type; > > uint8_t prec_level; /* Non-zero if type == OFPMBT_DSCP_REMARK. */ > > uint32_t rate; > > - uint32_t burst_size; > > + uint64_t burst_size; > > This structure is part of a public API. We can't change it that simple. > > IIUC, the issue is on a datapath level, so it should be solvable without > modifying anything outside the datapath code. Good idea. Now the meter uses the burst_size of ofputil_meter_band to store the buckets of meters, it will overflow when we set the rate to 4294968. In my next version patch, I remove the "up" and introduce the "uint64_t burst_size", that may make the code clear, and fix that bug. More details, please see my patch, thanks. > Could you prepare this kind of fix and send it separately from the series? Yes, I sent with another path, which related to that patch, but patch 1 was not included. http://patchwork.ozlabs.org/project/openvswitch/patch/20210112113308.48307-1-xiangxia.m.yue@gmail.com/ http://patchwork.ozlabs.org/project/openvswitch/patch/20210112113308.48307-2-xiangxia.m.yue@gmail.com/ > Best regards, Ilya Maximets.
diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h index 6776eae87e26..f55f89ac1a71 100644 --- a/include/openvswitch/ofp-meter.h +++ b/include/openvswitch/ofp-meter.h @@ -37,7 +37,7 @@ struct ofputil_meter_band { uint16_t type; uint8_t prec_level; /* Non-zero if type == OFPMBT_DSCP_REMARK. */ uint32_t rate; - uint32_t burst_size; + uint64_t burst_size; }; void ofputil_format_meter_band(struct ds *, enum ofp13_meter_flags, diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index b23bb8281a89..776a16dab6e8 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -278,7 +278,7 @@ static bool dpcls_lookup(struct dpcls *cls, 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) */ + uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */ uint64_t packet_count; uint64_t byte_count; }; @@ -5972,7 +5972,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, band = &meter->bands[m]; /* Update band's bucket. */ - band->bucket += delta_t * band->up.rate; + band->bucket += (uint64_t)delta_t * band->up.rate; band->bucket += delta_in_us * band->up.rate / 1000; if (band->bucket > band->up.burst_size) { band->bucket = band->up.burst_size; diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c index 9ea40a0bfb63..1ac993bb028b 100644 --- a/lib/ofp-meter.c +++ b/lib/ofp-meter.c @@ -72,7 +72,7 @@ ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags, ds_put_format(s, " rate=%"PRIu32, mb->rate); if (flags & OFPMF13_BURST) { - ds_put_format(s, " burst_size=%"PRIu32, mb->burst_size); + ds_put_format(s, " burst_size=%"PRIu64, mb->burst_size); } if (mb->type == OFPMBT13_DSCP_REMARK) { ds_put_format(s, " prec_level=%"PRIu8, mb->prec_level); @@ -703,7 +703,7 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, return error; } } else if (!strcmp(name, "burst_size")) { - char *error = str_to_u32(value, &band->burst_size); + char *error = str_to_u64(value, &band->burst_size); if (error) { return error; }