-----Original Message----- From: Jakub Kicinski kuba@kernel.org Sent: Tuesday, July 1, 2025 1:43 AM To: Chia-Yu Chang (Nokia) chia-yu.chang@nokia-bell-labs.com Cc: alok.a.tiwari@oracle.com; pctammela@mojatatu.com; horms@kernel.org; donald.hunter@gmail.com; xandfury@gmail.com; netdev@vger.kernel.org; dave.taht@gmail.com; pabeni@redhat.com; jhs@mojatatu.com; stephen@networkplumber.org; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net; edumazet@google.com; andrew+netdev@lunn.ch; ast@fiberby.net; liuhangbin@gmail.com; shuah@kernel.org; linux-kselftest@vger.kernel.org; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) koen.de_schepper@nokia-bell-labs.com; g.white@cablelabs.com; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel@apple.com Subject: Re: [PATCH v20 net-next 1/6] sched: Struct definition and parsing of dualpi2 qdisc
CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
On Mon, 30 Jun 2025 17:04:56 +0000 Chia-Yu Chang (Nokia) wrote:
This still needs 2 WRITE_ONCE even "step_thresh" (NLA_U32) and "step_in_packtes" (NLA_FLAG) are replaced with "step_pkt_thresh" (NLA_U32) and "step_time_thresh" (NLA_U32) - which was proposed in my another email.
If you don't understand the question - ask for clarifications :/
You are right.
Could you elaborate on the orignal comment "And the block under which I'm responding is performing two dependent writes, one to ->step_in_packets and the other to ->step_thresh a change which is definitely not atomic.."?
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?