Re: [PATCH v20 net-next 1/6] sched: Struct definition and parsing of dualpi2 qdisc

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 1 Jul 2025 05:41:53 +0000 Chia-Yu Chang (Nokia) wrote:
> > > I don't see we access the same atomic variable multiple times in a 
> > > single expression, the 2 WRITE_ONCE() are in different expressions.
> > >
> > > And, in the last WRITE_ONCE(), what we access are local variables:
> > > "step_pkt" "step_th", will it create problem?  
> > 
> > Not really a problem, but what I'm saying is that I don't understand why all the writes are sprinkled with WRITE_ONCE(). You take
> > 
> >         sch_tree_lock(sch);
> > 
> > to block data path and the control path is under rtnl_lock. So why the WRITE_ONCE()? WRITE_ONCE() is used to annotate writes which can be read concurrently without holding relevant locks.  
> 
> This follows series https://lore.kernel.org/netdev/20240415132054.3822230-1-edumazet@google.com/ to use READ_ONCE in dual2_dump() and WRITE_ONCE in dualpi2_change.
> 
> And because of one previous chnage request (https://lore.kernel.org/all/26f2f366-aa14-4879-978a-58d46f9d83a4@redhat.com/) to split single dualpi2.c into 3 patches, so you can find them in the 2nd patch of this series.
> 
> What I can do is either move these WRITE_ONCE also to the next patch or merge these patches.
> 
> I would prefer the first approach to make it still 3 patches, any other suggestion?

I see. With the change to use two separate fields for packets and time 
I think this should be fine. We will hopefully report one or the other,
but not one misinterpreted as the other.