From: Yewon Choi <woni9911@gmail.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
rds-devel@oss.oracle.com, linux-kernel@vger.kernel.org,
"Dae R. Jeong" <threeearcat@gmail.com>
Subject: Re: net/rds: Improper memory ordering semantic in release_in_xmit()
Date: Thu, 14 Mar 2024 20:39:38 +0900 [thread overview]
Message-ID: <ZfLh+va60YU2U86q@libra05> (raw)
In-Reply-To: <86d88699e8f22ebe0d45ffb5229fb73d78c5aae9.camel@oracle.com>
On Thu, Mar 07, 2024 at 08:13:50PM +0000, Allison Henderson wrote:
> On Wed, 2024-03-06 at 22:04 +0900, Yewon Choi wrote:
> > Hello,
> >
> > It seems to be that clear_bit() in release_in_xmit() doesn't have
> > release semantic while it works as a bit lock in rds_send_xmit().
> > Since acquire/release_in_xmit() are used in rds_send_xmit() for the
> > serialization between callers of rds_send_xmit(), they should imply
> > acquire/release semantics like other locks.
> >
> > Although smp_mb__after_atomic() is placed after clear_bit(), it
> > cannot
> > prevent that instructions before clear_bit() (in critical section)
> > are
> > reordered after clear_bit().
> > As a result, mutual exclusion may not be guaranteed in specific
> > HW architectures like Arm.
> >
> > We tested that this locking implementation doesn't guarantee the
> > atomicity of
> > critical section in Arm server. Testing was done with Arm Neoverse N1
> > cores,
> > and the testing code was generated by litmus testing tool (klitmus7).
> >
> > Initial condition:
> >
> > l = x = y = r0 = r1 = 0
> >
> > Thread 0:
> >
> > if (test_and_set_bit(0, l) == 0) {
> > WRITE_ONCE(*x, 1);
> > WRITE_ONCE(*y, 1);
> > clear_bit(0, l);
> > smp_mb__after_atomic();
> > }
> >
> > Thread 1:
> >
> > if (test_and_set_bit(0, l) == 0) {
> > r0 = READ_ONCE(*x);
> > r1 = READ_ONCE(*y);
> > clear_bit(0, l);
> > smp_mb__after_atomic();
> > }
> >
> > If the implementation is correct, the value of r0 and r1 should show
> > all-or-nothing behavior (both 0 or 1). However, below test result
> > shows
> > that atomicity violation is very rare, but exists:
> >
> > Histogram (4 states)
> > 9673811 :>1:r0=0; 1:r1=0;
> > 5647 :>1:r0=1; 1:r1=0; // Violate atomicity
> > 9605 :>1:r0=0; 1:r1=1; // Violate atomicity
> > 6310937 :>1:r0=1; 1:r1=1;
> >
> > So, we suggest introducing release semantic using clear_bit_unlock()
> > instead of clear_bit():
> >
> > diff --git a/net/rds/send.c b/net/rds/send.c
> > index 5e57a1581dc6..65b1bb06ca71 100644
> > --- a/net/rds/send.c
> > +++ b/net/rds/send.c
> > @@ -108,7 +108,7 @@ static int acquire_in_xmit(struct rds_conn_path
> > *cp)
> >
> > static void release_in_xmit(struct rds_conn_path *cp)
> > {
> > - clear_bit(RDS_IN_XMIT, &cp->cp_flags);
> > + clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
> > smp_mb__after_atomic();
> > /*
> > * We don't use wait_on_bit()/wake_up_bit() because our
> > waking is in a
> >
> > Could you check this please? If needed, we will send a patch.
>
> Hi Yewon,
>
> Thank you for finding this. I had a look at the code you had
> mentioned, and while I don't see any use cases of release_in_xmit()
> that might result in an out of order read, I do think that the proposed
> change is a good clean up. If you choose to submit a patch, please
> remove the proceeding "smp_mb__after_atomic" line as well, as it would
> no longer be needed. Also, please update acquire_in_xmit() to use the
> corresponding test_and_set_bit_lock() call. Thank you!
>
Thank you for examining this and giving suggestions!
I sent a patch with changes including your suggestions. If it has
problems, I will correct them as soon as possible.
Sincerely,
Yewon Choi
> Allison
>
>
> >
> > Best Regards,
> > Yewon Choi
>
prev parent reply other threads:[~2024-03-14 11:39 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 13:04 net/rds: Improper memory ordering semantic in release_in_xmit() Yewon Choi
2024-03-07 20:13 ` Allison Henderson
2024-03-14 11:39 ` Yewon Choi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZfLh+va60YU2U86q@libra05 \
--to=woni9911@gmail.com \
--cc=allison.henderson@oracle.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=rds-devel@oss.oracle.com \
--cc=threeearcat@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).