On Tue, May 17, 2022 at 2:56 AM Ilya Maximets <[email protected]> wrote:
>
> 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.
>
Makes no difference to me. Buckets will be calculated later anyway.
> > 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
