[RFC,4/6] options: ack pending sequence

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@strlen.de>
---
 net/mptcp/options.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

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@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@lists.01.org
> To unsubscribe send an email to mptcp-leave@lists.01.org
>

--
Mat Martineau
Intel

Mat Martineau <mathew.j.martineau@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.