|
Hi Julian,
> On Jul 8, 2026, at 03:18, Julian Anastasov <ja@xxxxxx> wrote:
>
>
> Hello,
>
> On Tue, 7 Jul 2026, Yizhou Zhao wrote:
>
>> is_unavailable() in the SH scheduler reads dest->flags from the packet
>> scheduling path while holding only the RCU read lock. The same word is
>> updated by read-modify-write operations from connection accounting and
>> destination update paths, for example ip_vs_bind_dest(),
>> ip_vs_unbind_dest(), and __ip_vs_update_dest().
>>
>> The RCU read lock only protects the destination lifetime; it does not
>> serialize accesses to dest->flags. A racing plain load or RMW update can
>> therefore observe stale state or lose an AVAILABLE/OVERLOAD bit update,
>> which can make the scheduler choose an overloaded destination or report no
>> available destination even though one should be usable.
>
> While the patch correctly serializes the concurrent
> modifications for the flags, we can not claim that the scheduler
> will not choose an overloaded or unavailable destination.
> The patch does not change the fact that we can work with
> stale data.
>
> We can compare 3 solutions, from fast to slow:
>
> 1. atomic_read or test_bit
> - no memory barriers for the readers
> - no memory ordering (=> stale data)
>
> PRO:
> - serializes RMW operations
>
> CON:
> - readers can use old values
> - writers may need to synchronize while changing
> the flags, eg. to check the thresholds and update the
> flags in atomic way. We do not do this.
>
> 2. Use refcount_inc_not_zero(&dest->available) from readers
>
> - and put the ref immediately or later:
>
> smp_mb__before_atomic();
> refcount_dec(&dest->available);
>
> - alternative: RMW such as atomic_fetch_add
>
> - writers can synchronize by using the IP_VS_DEST_F_AVAILABLE
> flag and then to inc/dec &dest->available when the
> flag changes
> - the same can be done for &dest->not_overloaded and
> IP_VS_DEST_F_OVERLOAD
> - PRO: readers are serialized perfectly with the
> changed value, new packets will detect the changes
> immediately
> - CON:
> - 2 full memory barriers for the readers
> - writers may need to synchronize while changing
> the flags
>
> 3. read_lock/write_lock
> - PRO: can modify more things under write lock
> - CON: full memory barriers
>
> With this patch you choose solution 1.
> The other solutions can be expensive for the fast path.
> Lets fix the commit message. Also, it is better to fix the
> scripts/checkpatch.pl warnings about the 'if' conditions,
> even if they are not introduced now.
>
> Regards
>
> --
> Julian Anastasov <ja@xxxxxx>
Thank you for your suggestions.
We have posted a v2 patch at:
https://lore.kernel.org/netfilter-devel/20260708060454.20534-1-zhaoyz24@xxxxxxxxxxxxxxxxxxxxx/
The v2 patch updates the commit message with more conservative
wording, and fixes the checkpatch logical-continuation warnings.
Regards,
Yizhou
|
