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
