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

[Jamal Hadi Salim <jhs@mojatatu.com>]

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.

cheers,
jamal