Re: [MPTCP] [PRE-RFC 6/6] mptcp: implement memory accounting for mptcp rtx queue

Re: [MPTCP] [PRE-RFC 6/6] mptcp: implement memory accounting for mptcp rtx queue

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Mon, 2019-08-19 at 13:47 +0200, Florian Westphal wrote:
> > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > Charge the data on the rtx queue to the master MPTCP socket, too.
> > > Such memory in uncharged when the data is acked/dequeued.
> > 
> > Could you briefly explain why this is needed?
> > The data is already accounted on the subflows.
> > 
> > Under what circumstances is the data un-charged from a subflow,
> > but still retained on the mptcp retrans list?
> 
> With the proposed patches and the current MPTCP code, that happens
> quite often, even with a single stream and no loss.
> 
> The MPTCP-level acks arrives later than TCP-level ones, as we send
> (MPTCP) ack when the user space really read the data. Most/every
> fragment stays in the MPTCP RTX queue for a while, after that the
> corresponding skb has been removed for the TCP transmission queue.
> 
> Please let me know if the above is somehow human-parsable ;)

Yes, make sense, thanks for explaining.

Re: [MPTCP] [PRE-RFC 6/6] mptcp: implement memory accounting for mptcp rtx queue

[Paolo Abeni]

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

On Mon, 2019-08-19 at 13:47 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > Charge the data on the rtx queue to the master MPTCP socket, too.
> > Such memory in uncharged when the data is acked/dequeued.
> 
> Could you briefly explain why this is needed?
> The data is already accounted on the subflows.
> 
> Under what circumstances is the data un-charged from a subflow,
> but still retained on the mptcp retrans list?

With the proposed patches and the current MPTCP code, that happens
quite often, even with a single stream and no loss.

The MPTCP-level acks arrives later than TCP-level ones, as we send
(MPTCP) ack when the user space really read the data. Most/every
fragment stays in the MPTCP RTX queue for a while, after that the
corresponding skb has been removed for the TCP transmission queue.

Please let me know if the above is somehow human-parsable ;)

Cheers,

Paolo

Re: [MPTCP] [PRE-RFC 6/6] mptcp: implement memory accounting for mptcp rtx queue

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> Charge the data on the rtx queue to the master MPTCP socket, too.
> Such memory in uncharged when the data is acked/dequeued.

Could you briefly explain why this is needed?
The data is already accounted on the subflows.

Under what circumstances is the data un-charged from a subflow,
but still retained on the mptcp retrans list?

[MPTCP] [PRE-RFC 6/6] mptcp: implement memory accounting for mptcp rtx queue

[Paolo Abeni]

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

Charge the data on the rtx queue to the master MPTCP socket, too.
Such memory in uncharged when the data is acked/dequeued.

Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
 net/mptcp/protocol.c | 18 +++++++++++++++---
 net/mptcp/protocol.h |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 5eba4e65ca93..2776a408f3e5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -121,8 +121,10 @@ static u64 mptcp_update_msk_una(struct sock *sk)
 			break;
 
 		list_del(&dfrag->list);
+		sk_mem_uncharge(sk, dfrag->data_len + dfrag->overhead);
 		put_page(dfrag->page);
 	}
+	sk_mem_reclaim_partial(sk);
 
 	if (msk->snd_una != snd_una) {
 		msk->snd_una = snd_una;
@@ -172,14 +174,15 @@ bool mptcp_page_frag_refill(struct sock *sk, struct page_frag *pfrag)
 
 static inline struct mptcp_data_frag *
 mptcp_carve_data_frag(const struct mptcp_sock *msk, struct page_frag *pfrag,
-		      int offset)
+		      int orig_offset)
 {
+	int offset = ALIGN(orig_offset, BITS_PER_LONG / 8);
 	struct mptcp_data_frag *dfrag;
 
-	offset = ALIGN(offset, BITS_PER_LONG / 8);
 	dfrag = (struct mptcp_data_frag *)(pfrag->page + offset);
 	dfrag->data_len = 0;
 	dfrag->data_seq = msk->write_seq;
+	dfrag->overhead = offset - orig_offset + sizeof(struct mptcp_data_frag);
 	dfrag->offset = offset + sizeof(struct mptcp_data_frag);
 	dfrag->page = pfrag->page;
 
@@ -190,8 +193,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 			      struct msghdr *msg, long *timeo, int *pmss_now,
 			      int *ps_goal)
 {
+	int mss_now, avail_size, size_goal, offset, ret, frag_truesize = 0;
 	bool dfrag_collapsed, collapsed, can_collapse = false;
-	int mss_now, avail_size, size_goal, offset, ret;
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct mptcp_ext *mpext = NULL;
 	struct mptcp_data_frag *dfrag;
@@ -243,6 +246,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	if (!dfrag_collapsed) {
 		dfrag = mptcp_carve_data_frag(msk, pfrag, offset);
 		offset = dfrag->offset;
+		frag_truesize = dfrag->overhead;
 	}
 	psize = min_t(size_t, pfrag->size - offset, avail_size);
 
@@ -274,6 +278,13 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 		list_add_tail(&dfrag->list, &msk->rtx_queue);
 	}
 
+	/* charge data on mptcp rtx queue to the master socket
+	 * Note: we charge such data both to sk and ssk
+	 */
+	frag_truesize += ret;
+	sk_wmem_schedule(sk, frag_truesize);
+	sk->sk_forward_alloc -= frag_truesize;
+
 	collapsed = skb == tcp_write_queue_tail(ssk);
 	BUG_ON(collapsed && !can_collapse);
 	if (collapsed) {
@@ -888,6 +899,7 @@ static void mptcp_destroy(struct sock *sk)
 
 	list_for_each_entry_safe(dfrag, dtmp, &msk->rtx_queue, list) {
 		list_del(&dfrag->list);
+		sk_mem_uncharge(sk, dfrag->data_len + dfrag->overhead);
 		put_page(dfrag->page);
 	}
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 0a6c7c39fc8b..3a0d266576f2 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -113,6 +113,7 @@ struct mptcp_data_frag {
 	u64 data_seq;
 	int data_len;
 	int offset;
+	int overhead;
 	struct page *page;
 };
 
-- 
2.20.1