[MPTCP] Re: [PATCH v2 2/4] subflow: wake parent mptcp socket on subflow state change

[MPTCP] Re: [PATCH v2 2/4] subflow: wake parent mptcp socket on subflow state change

[Mat Martineau]

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


Hi Florian,

On Wed, 23 Oct 2019, Florian Westphal wrote:

> We need to wakeup readers blocking for POLLIN, as no
> more data will arrive they will never be awoken otherwise.
>
> This is needed at least until proper MPTCP-Level fin/reset
> signalling gets added.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> net/mptcp/subflow.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1f27a96d0439..7bae7c35ea6b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -722,6 +722,28 @@ static struct mptcp_subflow_context *subflow_create_ctx(struct sock *sk,
> 	return ctx;
> }
>
> +static void __subflow_state_change(struct sock *sk)
> +{
> +	struct socket_wq *wq;
> +
> +	rcu_read_lock();
> +	wq = rcu_dereference(sk->sk_wq);
> +	if (skwq_has_sleeper(wq))
> +		wake_up_interruptible_all(&wq->wait);
> +	rcu_read_unlock();
> +}
> +
> +static void subflow_state_change(struct sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct sock *parent = subflow->conn;
> +
> +	__subflow_state_change(sk);
> +
> +	if (parent)
> +		__subflow_state_change(parent);

In the v1 patch I asked about the missing sk_wake_async() for the parent 
socket - you proposed calling parent->sk_data_ready(parent) here. Can you 
confirm which is intended?

https://lists.01.org/hyperkitty/list/mptcp(a)lists.01.org/message/EMGGJL4WII7NYOVGY7VM7DAVYDGXSVJO/


Thanks,
Mat


> +}
> +
> static int subflow_ulp_init(struct sock *sk)
> {
> 	struct inet_connection_sock *icsk = inet_csk(sk);
> @@ -750,6 +772,7 @@ static int subflow_ulp_init(struct sock *sk)
> 	ctx->tcp_sk_data_ready = sk->sk_data_ready;
> 	sk->sk_data_ready = subflow_data_ready;
> 	sk->sk_write_space = subflow_write_space;
> +	sk->sk_state_change = subflow_state_change;
> out:
> 	return err;
> }
> -- 
> 2.21.0
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH v2 2/4] subflow: wake parent mptcp socket on subflow state change

[Mat Martineau]

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

On Wed, 23 Oct 2019, Florian Westphal wrote:

> Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
>> On Wed, 23 Oct 2019, Florian Westphal wrote:
>>
>>> We need to wakeup readers blocking for POLLIN, as no
>>> more data will arrive they will never be awoken otherwise.
>>>
>>> This is needed at least until proper MPTCP-Level fin/reset
>>> signalling gets added.
>>>
>>> Signed-off-by: Florian Westphal <fw(a)strlen.de>
>>> ---
>>> net/mptcp/subflow.c | 23 +++++++++++++++++++++++
>>> 1 file changed, 23 insertions(+)
>>>
>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>> index 1f27a96d0439..7bae7c35ea6b 100644
>>> --- a/net/mptcp/subflow.c
>>> +++ b/net/mptcp/subflow.c
>>> @@ -722,6 +722,28 @@ static struct mptcp_subflow_context *subflow_create_ctx(struct sock *sk,
>>> 	return ctx;
>>> }
>>>
>>> +static void __subflow_state_change(struct sock *sk)
>>> +{
>>> +	struct socket_wq *wq;
>>> +
>>> +	rcu_read_lock();
>>> +	wq = rcu_dereference(sk->sk_wq);
>>> +	if (skwq_has_sleeper(wq))
>>> +		wake_up_interruptible_all(&wq->wait);
>>> +	rcu_read_unlock();
>>> +}
>>> +
>>> +static void subflow_state_change(struct sock *sk)
>>> +{
>>> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>> +	struct sock *parent = subflow->conn;
>>> +
>>> +	__subflow_state_change(sk);
>>> +
>>> +	if (parent)
>>> +		__subflow_state_change(parent);
>>
>> In the v1 patch I asked about the missing sk_wake_async() for the parent
>> socket - you proposed calling parent->sk_data_ready(parent) here. Can you
>> confirm which is intended?
>>
>> https://lists.01.org/hyperkitty/list/mptcp(a)lists.01.org/message/EMGGJL4WII7NYOVGY7VM7DAVYDGXSVJO/
>
> That comment refers to subflow_data_ready().  The patch here was not
> part of v1.  An alternative would be to make sock_def_wakeup()
> non-static and use it but since the function is very small I don't think
> thats needed. We will also likely want to do something different once
> MPTCP is able to process mptcp-level reset/fin.
>
> The proposed change of
>
> if (mptcp_should_wake_parent(subflow))
> 	parent->sk_data_ready(parent);
>
> inside subflow_data_ready isn't needed anymore because after Paolos
> refactoring the existing parent->sk_data_ready(parent);
>
> is already conditional on 'we have in-sequence data', so I dropped
> that part from the last patch.
>

Yeah, I misread the new patch. Thanks for explaining. I do agree that it 
makes more sense to create __subflow_state_change() than it does to export 
sock_def_wakeup().

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH v2 2/4] subflow: wake parent mptcp socket on subflow state change

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> On Wed, 23 Oct 2019, Florian Westphal wrote:
> 
> > We need to wakeup readers blocking for POLLIN, as no
> > more data will arrive they will never be awoken otherwise.
> > 
> > This is needed at least until proper MPTCP-Level fin/reset
> > signalling gets added.
> > 
> > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > ---
> > net/mptcp/subflow.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> > 
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 1f27a96d0439..7bae7c35ea6b 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -722,6 +722,28 @@ static struct mptcp_subflow_context *subflow_create_ctx(struct sock *sk,
> > 	return ctx;
> > }
> > 
> > +static void __subflow_state_change(struct sock *sk)
> > +{
> > +	struct socket_wq *wq;
> > +
> > +	rcu_read_lock();
> > +	wq = rcu_dereference(sk->sk_wq);
> > +	if (skwq_has_sleeper(wq))
> > +		wake_up_interruptible_all(&wq->wait);
> > +	rcu_read_unlock();
> > +}
> > +
> > +static void subflow_state_change(struct sock *sk)
> > +{
> > +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > +	struct sock *parent = subflow->conn;
> > +
> > +	__subflow_state_change(sk);
> > +
> > +	if (parent)
> > +		__subflow_state_change(parent);
> 
> In the v1 patch I asked about the missing sk_wake_async() for the parent
> socket - you proposed calling parent->sk_data_ready(parent) here. Can you
> confirm which is intended?
>
> https://lists.01.org/hyperkitty/list/mptcp(a)lists.01.org/message/EMGGJL4WII7NYOVGY7VM7DAVYDGXSVJO/

That comment refers to subflow_data_ready().  The patch here was not
part of v1.  An alternative would be to make sock_def_wakeup()
non-static and use it but since the function is very small I don't think
thats needed. We will also likely want to do something different once
MPTCP is able to process mptcp-level reset/fin.

The proposed change of

if (mptcp_should_wake_parent(subflow))
	parent->sk_data_ready(parent);

inside subflow_data_ready isn't needed anymore because after Paolos
refactoring the existing parent->sk_data_ready(parent);

is already conditional on 'we have in-sequence data', so I dropped
that part from the last patch.