On 3/9/22 15:19, Wan Junjie wrote:
> When several pmd threads race for the meter lock, the time 'now' each pmd
> runs will
> have no guarantee in the order they really happen.
>
> For example, pmd A runs meter at time x, and pmd B runs meter at time x+1.
> However
> if pmd B get the lock and run meter first, meter->used will be assgined to
> x+1, and
> then pmd A would run with delta_t equal to 0, the calculation for buckets is
> good now.
> But pmd A will set meter->used to x, move the the time back a little.
>
> If pmd B or other pmd runs it again, it will get a delta_t greater than it
> should be
> and buckets is bigger than we expect. With really high thoughput and more
> than four
> pmd threads, the deviation here will grow up very high.
>
> This fix makes sure the time will only be added up.
>
> Signed-off-by: Wan Junjie <[email protected]>
> ---
> lib/dpif-netdev.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 3acf5512a..26b584579 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7160,8 +7160,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
> dp_packet_batch *packets_,
> struct dp_meter *meter;
> struct dp_meter_band *band;
> struct dp_packet *packet;
> - long long int long_delta_t; /* msec */
> - uint32_t delta_t; /* msec */
> + long long int long_delta_t, delta_t;
> const size_t cnt = dp_packet_batch_size(packets_);
> uint32_t bytes, volume;
> int exceeded_band[NETDEV_MAX_BURST];
> @@ -7184,7 +7183,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
> dp_packet_batch *packets_,
>
> ovs_mutex_lock(&meter->lock);
> /* All packets will hit the meter at the same time. */
> - long_delta_t = now / 1000 - meter->used / 1000; /* msec */
> + long_delta_t = now - meter->used;
>
> if (long_delta_t < 0) {
> /* This condition means that we have several threads fighting for a
> @@ -7196,11 +7195,11 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
> dp_packet_batch *packets_,
>
> /* Make sure delta_t will not be too large, so that bucket will not
> * wrap around below. */
> - delta_t = (long_delta_t > (long long int)meter->max_delta_t)
> - ? meter->max_delta_t : (uint32_t)long_delta_t;
> + delta_t = (long_delta_t > (long long int)meter->max_delta_t * 1000)
> + ? (meter->max_delta_t * 1000) : long_delta_t;
>
> /* Update meter stats. */
> - meter->used = now;
> + meter->used += long_delta_t;
Maybe it's easier to just move the assignment to the 'else'
branch of the (long_delta_t < 0) condition? Seems cleaner
to me this way.
> meter->packet_count += cnt;
> bytes = 0;
> DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
> @@ -7228,7 +7227,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
> dp_packet_batch *packets_,
> band = &meter->bands[m];
> max_bucket_size = band->burst_size * 1000ULL;
> /* Update band's bucket. */
> - band->bucket += (uint64_t) delta_t * band->rate;
> + band->bucket += (delta_t * band->rate / 1000);
> if (band->bucket > max_bucket_size) {
> band->bucket = max_bucket_size;
> }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
