* [MPTCP] Re: [PATCH mptcp-next 12/12] mptcp: Safely store sequence number when sending data
@ 2020-07-20 9:31 Paolo Abeni
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2020-07-20 9:31 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]
On Fri, 2020-07-17 at 12:16 -0700, Mat Martineau wrote:
> On Thu, 16 Jul 2020, Paolo Abeni wrote:
>
> > On Wed, 2020-07-15 at 17:20 -0700, Mat Martineau wrote:
> > > The MPTCP socket's write_seq member can be read without the msk lock
> > > held, so use WRITE_ONCE() to store it.
> > >
> > > Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> > > ---
> > > net/mptcp/protocol.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 30c5810771dd..ed3ca80d72f8 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -797,7 +797,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> > > out:
> > > if (!retransmission)
> > > pfrag->offset += frag_truesize;
> > > - *write_seq += ret;
> > > + WRITE_ONCE(*write_seq, *write_seq + ret);
> > > mptcp_subflow_ctx(ssk)->rel_write_seq += ret;
> >
> > I think the msk lock should be already held here ?!?
>
> Yes, but write_seq might be concurrently read by another CPU, and we want
> don't want the compiler to invent stores as described in
> memory-barriers.txt
Yep, you are right. I misread the commit message and wend ahead too
hastly. The patch LGTM.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread
* [MPTCP] Re: [PATCH mptcp-next 12/12] mptcp: Safely store sequence number when sending data
@ 2020-07-17 19:16 Mat Martineau
0 siblings, 0 replies; 3+ messages in thread
From: Mat Martineau @ 2020-07-17 19:16 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]
On Thu, 16 Jul 2020, Paolo Abeni wrote:
> On Wed, 2020-07-15 at 17:20 -0700, Mat Martineau wrote:
>> The MPTCP socket's write_seq member can be read without the msk lock
>> held, so use WRITE_ONCE() to store it.
>>
>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>> ---
>> net/mptcp/protocol.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 30c5810771dd..ed3ca80d72f8 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -797,7 +797,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>> out:
>> if (!retransmission)
>> pfrag->offset += frag_truesize;
>> - *write_seq += ret;
>> + WRITE_ONCE(*write_seq, *write_seq + ret);
>> mptcp_subflow_ctx(ssk)->rel_write_seq += ret;
>
> I think the msk lock should be already held here ?!?
Yes, but write_seq might be concurrently read by another CPU, and we want
don't want the compiler to invent stores as described in
memory-barriers.txt
My (possibly flawed?) understanding of READ_ONCE/WRITE_ONCE has led me to
these rules of thumb:
* Always use WRITE_ONCE to avoid storing weird intermediate values that
can be seen by concurrent readers
* For values that are only written with the lock held, but have
concurrent readers, READ_ONCE only required when not locked. The compiler
will correctly synchronize with any readers/writers on the same locked
thread.
* For values that have writes without the lock held (access is not
coordinated by locks at all), always use READ_ONCE. The compiler doesn't
know when any write will update the memory location.
I know the full story for READ_ONCE/WRITE_ONCE syncronization is more
complicated, but for the way we've been using them in MPTCP it seems like
the above rules work ok. If I'm mistaken I'd really like to fix that so
I write correct code :)
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 3+ messages in thread
* [MPTCP] Re: [PATCH mptcp-next 12/12] mptcp: Safely store sequence number when sending data
@ 2020-07-16 14:15 Paolo Abeni
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2020-07-16 14:15 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 847 bytes --]
On Wed, 2020-07-15 at 17:20 -0700, Mat Martineau wrote:
> The MPTCP socket's write_seq member can be read without the msk lock
> held, so use WRITE_ONCE() to store it.
>
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
> net/mptcp/protocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30c5810771dd..ed3ca80d72f8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -797,7 +797,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> out:
> if (!retransmission)
> pfrag->offset += frag_truesize;
> - *write_seq += ret;
> + WRITE_ONCE(*write_seq, *write_seq + ret);
> mptcp_subflow_ctx(ssk)->rel_write_seq += ret;
I think the msk lock should be already held here ?!?
/P
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-20 9:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 9:31 [MPTCP] Re: [PATCH mptcp-next 12/12] mptcp: Safely store sequence number when sending data Paolo Abeni
-- strict thread matches above, loose matches on Subject: below --
2020-07-17 19:16 Mat Martineau
2020-07-16 14:15 Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.