Re: [MPTCP] [PATCH 05/12] mptcp: Render mptcp_sub_inherit_sockopts lockless

* Re: [MPTCP] [PATCH 05/12] mptcp: Render mptcp_sub_inherit_sockopts lockless
@ 2018-04-09  4:08 Christoph Paasch
  0 siblings, 0 replies; 4+ messages in thread
From: Christoph Paasch @ 2018-04-09  4:08 UTC (permalink / raw)
  To: mptcp

[-- Attachment #1: Type: text/plain, Size: 2921 bytes --]

On 04/04/18 - 21:29:29, Matthieu Baerts wrote:
> Hi Christoph,
> 
> On 26/03/2018 09:32, Christoph Paasch wrote:
> > We are accessing the meta-socket's TOS field twice. It could change
> > in-between as we do this in a lockless manner.
> 
> I hope this will not add more complexities in the code :)
> 
> Should we not clearly identify/mark all functions that are called with or
> without holding locks? Functions could be renamed maybe? I know, quite a big
> work :-)

Yes, and also that would mean quite some function-name-length bloat :)

For now, I just simply added a comment to each of these functions.
I think that should be fine, as since 4.9 all TCP connection establishment
is supposed to be lockless.

Christoph

> 
> Best regards,
> Matt
> 
> > Thus, let's make sure we read it only once.
> > 
> > Signed-off-by: Christoph Paasch <cpaasch(a)apple.com>
> > ---
> >   net/mptcp/mptcp_ctrl.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/mptcp_ctrl.c b/net/mptcp/mptcp_ctrl.c
> > index 29fe6485d69a..fb37053dd127 100644
> > --- a/net/mptcp/mptcp_ctrl.c
> > +++ b/net/mptcp/mptcp_ctrl.c
> > @@ -945,11 +945,15 @@ static void mptcp_mpcb_inherit_sockopts(struct sock *meta_sk, struct sock *maste
> >   	inet_sk(master_sk)->recverr = 0;
> >   }
> > +/* Called without holding lock on meta_sk */
> >   static void mptcp_sub_inherit_sockopts(const struct sock *meta_sk, struct sock *sub_sk)
> >   {
> > +	__u8 meta_tos;
> > +
> >   	/* IP_TOS also goes to the subflow. */
> > -	if (inet_sk(sub_sk)->tos != inet_sk(meta_sk)->tos) {
> > -		inet_sk(sub_sk)->tos = inet_sk(meta_sk)->tos;
> > +	meta_tos = READ_ONCE(inet_sk(meta_sk)->tos);
> > +	if (inet_sk(sub_sk)->tos != meta_tos) {
> > +		inet_sk(sub_sk)->tos = meta_tos;
> >   		sub_sk->sk_priority = meta_sk->sk_priority;
> >   		sk_dst_reset(sub_sk);
> >   	}
> > 
> 
> -- 
> Matthieu Baerts | R&D Engineer
> matthieu.baerts(a)tessares.net
> Tessares SA | Hybrid Access Solutions
> www.tessares.net
> 1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
> 
> -- 
> 
> 
> DISCLAIMER.
> This email and any files transmitted with it are confidential and intended
> solely for the use of the individual or entity to whom they are addressed.
> If you have received this email in error please notify the system manager.
> This message contains confidential information and is intended only for the
> individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and delete
> this e-mail from your system. If you are not the intended recipient you are
> notified that disclosing, copying, distributing or taking any action in
> reliance on the contents of this information is strictly prohibited.

^ permalink raw reply	[flat|nested] 4+ messages in thread