-----Original Message----- From: Jakub Kicinski kuba@kernel.org Sent: Monday, June 30, 2025 6:23 PM 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 15:51:11 +0000 Chia-Yu Chang (Nokia) wrote:
if (tb[TCA_DUALPI2_STEP_THRESH]) {u32 step_th = nla_get_u32(tb[TCA_DUALPI2_STEP_THRESH]);bool step_pkt =- nla_get_flag(tb[TCA_DUALPI2_STEP_PACKETS]);
WRITE_ONCE(q->step_in_packets, step_pkt);WRITE_ONCE(q->step_thresh,step_pkt ? step_th : convert_us_to_nsec(step_th));}I don't get the reason for all these WRITE_ONCE()s. You lock the qdisc to make modifications, right? 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..
Thanks again for other comments, and I will take actions in the next version.
As there is only one step marking in L-queue, so we still need two WRITE_ONCE even two attributes (one for threshold in packets and one in time) are used.
When applying the step marking, we need to know either the computation is based on the sojourn time or queue length.
Let me ask again - why do you use WRITE_ONCE() at all if the modification takes the qdisc lock? Which reader are you afraid of racing with?
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?