[MPTCP] Re: [PATCH 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[MPTCP] Re: [PATCH 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Paolo Abeni]

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

On Sat, 2019-11-30 at 23:49 +0100, Florian Westphal wrote:
> @@ -642,7 +647,7 @@ bool mptcp_subflow_data_available(struct sock *sk)
>  		/* set EoF only there is no data available - we already spooled
>  		 * all the pending skbs
>  		 */
> -		if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
> +		if (subflow_is_done(sk))
>  			subflow->rx_eof = 1;
>  		return false;
>  	}
> @@ -666,13 +671,15 @@ static void subflow_data_ready(struct sock *sk)
>  		return;
>  	}
>  
> -	/* always propagate the EoF */
> -	if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
> +	if (mptcp_subflow_data_available(sk)) {
>  		smp_mb__before_atomic();
>  		set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
>  		smp_mb__after_atomic();
>  
>  		parent->sk_data_ready(parent);
> +	} else if (subflow->rx_eof) {
> +		/* tell parent to (re-)check eof on all subflows */
> +		mptcp_subflow_eof(parent);
>  	}
>  }
> 

With subflow_state_change() in place, do we still need the above
chunks?!? e.g. why is not subflow_state_change() enough to catch the
subflow eof condition?

Thanks!

Paolo

[MPTCP] Re: [PATCH 9/9] don't flag parent socket as RCV_SHUTDOWN if one one subflow has closed

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Sat, 2019-11-30 at 23:49 +0100, Florian Westphal wrote:
> > @@ -642,7 +647,7 @@ bool mptcp_subflow_data_available(struct sock *sk)
> >  		/* set EoF only there is no data available - we already spooled
> >  		 * all the pending skbs
> >  		 */
> > -		if (sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE)
> > +		if (subflow_is_done(sk))
> >  			subflow->rx_eof = 1;
> >  		return false;
> >  	}
> > @@ -666,13 +671,15 @@ static void subflow_data_ready(struct sock *sk)
> >  		return;
> >  	}
> >  
> > -	/* always propagate the EoF */
> > -	if (mptcp_subflow_data_available(sk) || subflow->rx_eof) {
> > +	if (mptcp_subflow_data_available(sk)) {
> >  		smp_mb__before_atomic();
> >  		set_bit(MPTCP_DATA_READY, &mptcp_sk(parent)->flags);
> >  		smp_mb__after_atomic();
> >  
> >  		parent->sk_data_ready(parent);
> > +	} else if (subflow->rx_eof) {
> > +		/* tell parent to (re-)check eof on all subflows */
> > +		mptcp_subflow_eof(parent);
> >  	}
> >  }
> > 
> 
> With subflow_state_change() in place, do we still need the above
> chunks?!? e.g. why is not subflow_state_change() enough to catch the
> subflow eof condition?

Right, it might be possible to drop that indeed.