[MPTCP] Re: [RFC PATCH 4/6] options: ack pending sequence

[MPTCP] Re: [RFC PATCH 4/6] options: ack pending sequence

[Mat Martineau]

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


On Fri, 8 Nov 2019, Florian Westphal wrote:

> This allows us to include some of the bytes already pending on the subflow
> in the DSS ack instead of having to wait for the next recv (which
> updates msk->ack_seq value).
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> net/mptcp/options.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 80dbe7662cea..b488bf07ada5 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -418,13 +418,20 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> 		ack_size += TCPOLEN_MPTCP_DSS_BASE;
>
> 	if (ack_size <= remaining) {
> +		const struct mptcp_subflow_context *subflow;
> 		struct mptcp_sock *msk;
>
> 		dss_size += ack_size;
>
> -		msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
> +		subflow = mptcp_subflow_ctx(sk);
> +		msk = mptcp_sk(subflow->conn);
> 		if (msk) {
> -			opts->ext_copy.data_ack = msk->ack_seq;
> +			u64 ack_seq = READ_ONCE(msk->ack_seq);
> +
> +			if (subflow->map_valid)
> +				ack_seq += subflow->data_avail;

I'm not sure this works well with multiple subflows. It could ack a later 
sequence number on this subflow, then send an earlier sequence number in a 
bare ack on another subflow. They should be close enough that the earlier 
seq would be ignored, but it would be better to send consistent acks.

Mat


> +
> +			opts->ext_copy.data_ack = ack_seq;
> 		} else {
> 			mptcp_crypto_key_sha1(mptcp_subflow_ctx(sk)->remote_key,
> 					      NULL, &opts->ext_copy.data_ack);
> -- 
> 2.23.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: [RFC PATCH 4/6] options: ack pending sequence

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 80dbe7662cea..b488bf07ada5 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -418,13 +418,20 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > 		ack_size += TCPOLEN_MPTCP_DSS_BASE;
> > 
> > 	if (ack_size <= remaining) {
> > +		const struct mptcp_subflow_context *subflow;
> > 		struct mptcp_sock *msk;
> > 
> > 		dss_size += ack_size;
> > 
> > -		msk = mptcp_sk(mptcp_subflow_ctx(sk)->conn);
> > +		subflow = mptcp_subflow_ctx(sk);
> > +		msk = mptcp_sk(subflow->conn);
> > 		if (msk) {
> > -			opts->ext_copy.data_ack = msk->ack_seq;
> > +			u64 ack_seq = READ_ONCE(msk->ack_seq);
> > +
> > +			if (subflow->map_valid)
> > +				ack_seq += subflow->data_avail;
> 
> I'm not sure this works well with multiple subflows. It could ack a later
> sequence number on this subflow, then send an earlier sequence number in a
> bare ack on another subflow. They should be close enough that the earlier
> seq would be ignored, but it would be better to send consistent acks.

That would require far more changes since this means we need to move
msk->ack_seq update away from recvmsg.

I think its best to drop this patch for now, its annoying to have this
behaviour of acking data that was consumed by userspace rather than what
was received but I don't see a simple solution nor is it related to wmem
accouting.