Re: [PATCH net-next] ipvs: annotate the lockless read of sysctl_sync_threshold[0]

[Julian Anastasov]

	Hello,

On Thu, 29 Oct 2026, Linkui Xiao wrote:

> Hi Julian, Simon,
> 
> Thanks for the reviews.
> 
> On Wed, Oct 28, 2026 at 07:15:34PM +0300, Julian Anastasov wrote:
> > Yes, ACCESS_ONCE was used to solve possible division-by-zero problem,
> > not to solve the data race. Even if memcpy is converted to 2 int copies
> > on some archs (eg. x86) may be it is time to use 2 WRITE_ONCE instead of
> > the last memcpy. Can you include this change in v2. Post it when net-next
> > opens again by using nf-next tag.
> 
> Agreed. v2 replaces the last memcpy() in proc_do_sync_threshold() with
> two WRITE_ONCE() stores, one per element. The if/else branches gained
> braces to keep checkpatch --strict happy about the now multi-statement
> else.
> 
> net-next is currently closed and is scheduled to reopen on November 9th
> (https://netdev.bots.linux.dev/net-next.html). I will post it as
> [PATCH nf-next v2] then.
> 
> On Tue, Oct 28, 2026 at 12:49:35PM +0000, Simon Horman wrote:
> > I wonder if it would also be appropriate to call
> > sysctl_sync_threshold() only once (per-packet) in
> > ip_vs_sync_conn_needed(). Stashing the value in a local variable if
> > appropriate.
> 
> Agreed, and thank you. v2 reads the value once into a local variable,
> sync_threshold, before the branch and uses it in either branch.
> 
> One precision while re-checking the code: the two call sites are on
> mutually exclusive branches of the same if/else, so a single packet
> never reads the value twice at runtime. My v1 commit message overstated
> this ("the two comparisons ... can observe different values for one and
> the same packet"); v2 rewords that paragraph and notes the rewording in
> Changes in v2. Stashing a single snapshot is still the cleaner shape, so
> the change is worthwhile regardless.

	IMHO, ip_vs_sync_conn_needed() should not be changed, 
sysctl_sync_threshold() is not called twice in same flow. OTOH,
sysctl_sync_period() is properly stored in sync_period because
it is used twice. 

	But there is still little gap where proc_do_sync_threshold()
changes the two values one after another while ip_vs_sync_conn_needed()
can not take snapshot without using some common lock, eg. some
seqcount should be more appropriate for this. Writers are already
synchronized with the mutex, so ip_vs_sync_conn_needed() can read the two
values into temp vars under read_seqcount_begin()+read_seqcount_retry().
And later to use the temp vars. proc_do_sync_threshold() can use 
write_seqcount_begin() and write_seqcount_end():

	local_bh_disable();
	preempt_disable_nested();
	write_seqcount_begin();
	2 WRITE_ONCE
	write_seqcount_end();
	preempt_enable_nested();
	local_bh_enable();

Regards

--
Julian Anastasov <ja@xxxxxx>