On 6/30/23 08:40, Eelco Chaudron wrote:
>
>
> On 29 Jun 2023, at 17:54, Ilya Maximets wrote:
>
>> On 6/29/23 17:43, Eelco Chaudron wrote:
>>>
>>>
>>> On 22 Jun 2023, at 0:32, Ilya Maximets wrote:
>>>
>>>> Current implementation of meters in the userspace datapath takes
>>>> the meter lock for every packet batch. If more than one thread
>>>> hits the flow with the same meter, they will lock each other.
>>>>
>>>> Replace the critical section with atomic operations to avoid
>>>> interlocking. Meters themselves are RCU-protected, so it's safe
>>>> to access them without holding a lock.
>>>>
>>>> Implementation does the following:
>>>>
>>>> 1. Tries to advance the 'used' timer of the meter with atomic
>>>> compare+exchange if it's smaller than 'now'.
>>>> 2. If the timer change succeeds, atomically update band buckets.
>>>> 3. Atomically update packet statistics for a meter.
>>>> 4. Go over buckets and try to atomically subtract the amount of
>>>> packets or bytes, recording the highest exceeded band.
>>>> 5. Atomically update band statistics and drop packets.
>>>>
>>>> Bucket manipulations are implemented with atomic compare+exchange
>>>> operations with extra checks, because bucket size should never
>>>> exceed the maximum and it should never go below zero.
>>>>
>>>> Packet statistics may be momentarily inconsistent, i.e., number
>>>> of packets and the number of bytes may reflect different sets
>>>> of packets. But it should be eventually consistent. And the
>>>> difference at any given time should be in just few packets.
>>>>
>>>> For the sake of reduced code complexity PKTPS meter tries to push
>>>> packets through the band one by one, even though they all have
>>>> the same weight. This is also more fair if more than one thread
>>>> is passing packets through the same band at the same time.
>>>> Trying to predict the number of packets that can pass may also
>>>> cause extra atomic operations reducing the performance.
>>>>
>>>> This implementation shows similar performance to the previous one,
>>>> but should scale better with more threads hiting the same meter.
>>>
>>> This works looks great!! Some small comments below. Did limited testing and
>>> seems to work fine.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>> ---
>>>>
>>>> @Lin Huang, if you can try this change on your setup, that
>>>> would be great.
>>>>
>>>> NEWS | 2 +
>>>> lib/dpif-netdev.c | 250 +++++++++++++++++++++++++---------------------
>>>> 2 files changed, 140 insertions(+), 112 deletions(-)
>>>>
...
>>>
>>>> +
>>>> size_t j;
>>>> DP_PACKET_BATCH_REFILL_FOR_EACH (j, cnt, packet, packets_) {
>>>> - if (exceeded_band[j] >= 0) {
>>>> + uint32_t m = exceeded_band[j];
>>>> +
>>>> + if (m != UINT32_MAX) {
>>>> /* Meter drop packet. */
>>>> - band = &meter->bands[exceeded_band[j]];
>>>> - band->packet_count += 1;
>>>> - band->byte_count += dp_packet_size(packet);
>>>> - COVERAGE_INC(datapath_drop_meter);
>>>> + band = &meter->bands[m];
>>>> + band_packets[m]++;
>>>> + band_bytes[m] += dp_packet_size(packet);
>>>
>>>
>>> This code now looks like this (the diff is a mess to comment on):
>>>
>>> if (m != UINT32_MAX) {
>>> /* Meter drop packet. */
>>> band = &meter->bands[m];
>>>
>>> ! ^^^ This line can be removed as band is not used.
>>
>> True. Can remove.
>
> Thanks, now I see that even shows up with clang-analyze.
>
> I guess this is the only real change needed. So if you roll it in during
> commit time:
>
> Acked-by: Eelco Chaudron <[email protected]>
Thanks Simon and Eelco for review! And thanks Lin and Zhang for testing!
I removed the unused assignment and applied the change.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
