Re: [PATCH net-next 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> On Thu, Oct 8, 2026 at 6:29 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> > The qdisc is left with autorate disabled but with the estimate installed
>> > instead of X.  On 32-bit machines the unlocked 64-bit rate_bps store is
>> > also not atomic, so a reader can observe a torn rate.  iproute2 reaches
>> > this because parsing 'bandwidth X' sets autorate = 0 and emits both
>> > TCA_CAKE_BASE_RATE64 and TCA_CAKE_AUTORATE.
>> >
>> > Commit the parsed configuration and reconfigure while holding the same
>> > qdisc lock that serializes the datapath, so the netlink writer and the
>> > autorate writer can no longer interleave.  Make rate_bps an atomic64_t
>> > so a 64-bit rate update is atomic on 32-bit machines as well, and keep
>> > the READ_ONCE()/WRITE_ONCE() annotations for the remaining lockless
>> > readers (cake_config_dump(), which runs without the lock, and the
>> > cake_mq shared config, which readers observe under different child
>> > locks).
>>
>> So atomic_t.txt has this section:
>>
>> "SEMANTICS
>> ---------
>>
>> Non-RMW ops:
>>
>> The non-RMW ops are (typically) regular LOADs and STOREs and are canonically
>> implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
>> smp_store_release() respectively. Therefore, if you find yourself only using
>> the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all
>> and are doing it wrong."
>>
>>
>> So AFAICT, we don't need atomic_t, we just need READ/WRITE_ONCE()
>> annotations?
>>
>
> hrm. actually, the sch_tree_lock() already protects cake_change(): it
> commits the parsed config and reconfigures together with the datapath
> under the same qdisc lock, so the netlink writer and the autorate
> store can no longer interleave. So the atomic64_t was never needed for
> the lost update.
>
> on the atomic_t.txt point you pointed out: rate_bps takes only non-RMW
> ops, so per SEMANTICS it should be a plain u64 with
> READ_ONCE()/WRITE_ONCE(). I'll drop the atomic64_t.
>
> The only lockless readers left are cake_config_dump() (cosmetic) and
> the cake_mq shared config, which a child reads while holding a
> different child's lock than the parent writes it under - on 32-bit
> READ_ONCE() on a u64 can tear there. It's an advisory rate that
> self-corrects on the next sync interval, so best-effort is fine.
>
> I will resend a v2 with rate_bps as u64 + READ/WRITE_ONCE. Let's wait
> for Sashiko first - it will always find something to complain about.

SGTM.

-Toke