From: Ilya Maximets <i.maxim...@ovn.org>
Date: 2021-05-13 18:21:31
To: Tao Liu <thomas....@ucloud.cn>,pshe...@ovn.org
Cc:
d...@openvswitch.org,net...@vger.kernel.org,linux-ker...@vger.kernel.org,i.maxim...@ovn.org,jean.tourril...@hpe.com,k...@kernel.org,da...@davemloft.net,Eelco
Chaudron <echau...@redhat.com>
Subject: Re: [ovs-dev] [PATCH net] openvswitch: meter: fix race when getting
now_ms.>On 5/13/21 12:03 PM, Tao Liu wrote:
>> We have observed meters working unexpected if traffic is 3+Gbit/s
>> with multiple connections.
>>
>> now_ms is not pretected by meter->lock, we may get a negative
>> long_delta_ms when another cpu updated meter->used, then:
>> delta_ms = (u32)long_delta_ms;
>> which will be a large value.
>>
>> band->bucket += delta_ms * band->rate;
>> then we get a wrong band->bucket.
>>
>> Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
>> Signed-off-by: Tao Liu <thomas....@ucloud.cn>
>> ---
>
>Hi. Thanks for the patch!
>We fixed the same issue in userspace datapath some time ago and
>we did that a bit differently by just setting negative long_delta_ms
>to zero in assumption that all threads received their packets at
>the same millisecond (which is most likely true if we have this
>kind of race). This should be also cheaper from form the performance
>point of view to not have an extra call and a division under the
>spinlock. What do you think?
Yes, I agree with you. The userspace implementation has same effection,
and looks a bit more efficient. I will send a v2.
>It's also a good thing to have more or less similar implementation
>for all datapaths.
>
>Here is a userspace patch:
>
>commit acc5df0e3cb036524d49891fdb9ba89b609dd26a
>Author: Ilya Maximets <i.maxim...@ovn.org>
>Date: Thu Oct 24 15:15:07 2019 +0200
>
> dpif-netdev: Fix time delta overflow in case of race for meter lock.
>
> There is a race window between getting the time and getting the meter
> lock. This could lead to situation where the thread with larger
> current time (this thread called time_{um}sec() later than others)
> will acquire meter lock first and update meter->used to the large
> value. Next threads will try to calculate time delta by subtracting
> the large meter->used from their lower time getting the negative value
> which will be converted to a big unsigned delta.
>
> Fix that by assuming that all these threads received packets in the
> same time in this case, i.e. dropping negative delta to 0.
>
> CC: Jarno Rajahalme <ja...@ovn.org>
> Fixes: 4b27db644a8c ("dpif-netdev: Simple DROP meter implementation.")
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-September/363126.html
> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
> Acked-by: William Tu <u9012...@gmail.com>
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index c09b8fd95..4720ba1ab 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -5646,6 +5646,14 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
>dp_packet_batch *packets_,
> /* All packets will hit the meter at the same time. */
> long_delta_t = now / 1000 - meter->used / 1000; /* msec */
>
>+ if (long_delta_t < 0) {
>+ /* This condition means that we have several threads fighting for a
>+ meter lock, and the one who received the packets a bit later wins.
>+ Assuming that all racing threads received packets at the same time
>+ to avoid overflow. */
>+ long_delta_t = 0;
>+ }
>+
> /* 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)
>---
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
