[MPTCP] Re: [PATCH mptcp 1/3] mptcp: schedule worker when subflow is closed

[Florian Westphal <fw at strlen.de>]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index b8b670cb6116..57f6361eb07c 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1413,6 +1413,13 @@ static void subflow_state_change(struct sock *sk)
> >  	if (mptcp_subflow_data_available(sk))
> >  		mptcp_data_ready(parent, sk);
> >  
> > +	if (sk->sk_state == TCP_CLOSE &&
> > +	    !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(parent)->flags)) {
> > +		sock_hold(parent);
> > +		if (!schedule_work(&mptcp_sk(parent)->work))
> > +			sock_put(parent);
> > +	}
> > +
> 
> Looks like this is causing issue/154. Reverting this patch the issues
> goes away here. @Matttbe: could you please double check that?

No need, I am sure you are correct.

> In case of fallback, the subflow can be closed by the peer while there
> are data pending in the subflow receive queue. Calling
> mptcp_close_subflow() will discard such data and will corrupt the
> stream.

This.

> I think that scenario is possible even with full msk, if the peer
> closes a single subflow while the msk is still alive (before DATA_FIN).

You mean because there might be unprocessed data in subflow rx
queue?

> A possible fix could be additionally checking for empty ssk-
> >sk_recevive_queue, plush adding a similar chunk
> in __mptcp_move_skbs_from_subflow(). 

Yes, not nice, I don't see another solution atm though.

Will look into this tomorrow.

> That means that subflow_close even could be delayed by user-space, if
> the latter does not spool the pending ingress data.

We can decouple the event from actual close.

> Or the NL event generation cold be de-coupled from the actual subflow
> close() but I fear some side effect even there.

Hmm, why?

> Note that calling __mptcp_move_skbs_from_subflow()
> from mptcp_close_subflow() will not solve the issue, as the msk receive
> queue could be full - I think;)

Yes, it could be full.
thats possible.

> Side note: it would be nice using the deferred actions to do the
> netlink call, to avoid the workqueue usage.

Why?  How many events/s do you expect?

Events try to prefer GFP_KERNEL allocations where possible.