Re: [PATCH] netfilter: Don't track counter updates of do_add_counters()

[takakura]

Hi Eric,

Thanks for taking a look at the patch! And sorry that I see that I was 
missing the point of the synchronization.

On 2024-08-22 6:03 Eric Dumazet wrote:
>On Thu, Aug 22, 2024 at 6:36 AM <takakura@xxxxxxxxxxxxx> wrote:
>>
>> From: Ryo Takakura <takakura@xxxxxxxxxxxxx>
>>
>> While adding counters in do_add_counters(), we call
>> xt_write_recseq_begin/end to indicate that counters are being updated.
>> Updates are being tracked so that the counters retrieved by get_counters()
>> will reflect concurrent updates.
>>
>> However, there is no need to track the updates done by do_add_counters() as
>> both do_add_counters() and get_counters() acquire per ipv4,ipv6,arp mutex
>> beforehand which prevents concurrent update and retrieval between the two.
>>
>> Moreover, as the xt_write_recseq_begin/end is shared among ipv4,ipv6,arp,
>> do_add_counters() called by one of ipv4,ipv6,arp can falsely delay the
>> synchronization of concurrent get_counters() or xt_replace_table() called
>> by any other than the one calling do_add_counters().
>>
>> So remove xt_write_recseq_begin/end from do_add_counters() for ipv4,ipv6,arp.
>
>Completely wrong patch.
>
>There is no way we can update pairs of 64bit counters without any
>synchronization.

Yes, I was completely wrong about why the synchronization is required...

>This is per cpu sequence, the 'shared among ipv4,ipv6,arp' part is moot.
>
>We could use cmpxchg128 on 64bit arches, but I suspect there will be
>no improvement.

I see. And if we were to use cmpxchg128, we would also need to come up with 
the way for xt_replace_table()'s synchronization which I guess the current 
per cpu sequence is more suited.

Sincerely,
Ryo Takakura