[PATCH v4 mptcp-next 1/4] mptcp: never fetch fwd memory from the subflow

[PATCH v4 mptcp-next 1/4] mptcp: never fetch fwd memory from the subflow

[Paolo Abeni]

The memory accounting is broken in such exceptional code
path, and after commit 4890b686f408 ("net: keep sk->sk_forward_alloc
as small as possible") we can't find much help there.

Drop the broken code.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This is possibly for net, but makes sense only on top of recent
net-next patches, so whatever ;)
---
 net/mptcp/protocol.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6d2aa41390e7..0d4b2c010da0 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -328,15 +328,10 @@ static bool mptcp_rmem_schedule(struct sock *sk, struct sock *ssk, int size)
 
 	amt = sk_mem_pages(size);
 	amount = amt << PAGE_SHIFT;
-	msk->rmem_fwd_alloc += amount;
-	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV)) {
-		if (ssk->sk_forward_alloc < amount) {
-			msk->rmem_fwd_alloc -= amount;
-			return false;
-		}
+	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV))
+		return false;
 
-		ssk->sk_forward_alloc -= amount;
-	}
+	msk->rmem_fwd_alloc += amount;
 	return true;
 }
 
-- 
2.35.3

[PATCH v4 mptcp-next 2/4] mptcp: drop SK_RECLAIM_* macros

[Paolo Abeni]

After commit 4890b686f408 ("net: keep sk->sk_forward_alloc as small as
possible"), the MPTCP protocol is the last SK_RECLAIM_CHUNK and
SK_RECLAIM_THRESHOLD users.

Update the MPTCP reclaim schema to match the core/TCP one and drop the
mentioned macros. This additionally clean the MPTCP code a bit.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 35 ++---------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0d4b2c010da0..b31bac33f87a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -181,8 +181,8 @@ static void mptcp_rmem_uncharge(struct sock *sk, int size)
 	reclaimable = msk->rmem_fwd_alloc - sk_unused_reserved_mem(sk);
 
 	/* see sk_mem_uncharge() for the rationale behind the following schema */
-	if (unlikely(reclaimable >= SK_RECLAIM_THRESHOLD))
-		__mptcp_rmem_reclaim(sk, SK_RECLAIM_CHUNK);
+	if (unlikely(reclaimable >= PAGE_SIZE))
+		__mptcp_rmem_reclaim(sk, reclaimable);
 }
 
 static void mptcp_rfree(struct sk_buff *skb)
@@ -961,25 +961,6 @@ static bool mptcp_frag_can_collapse_to(const struct mptcp_sock *msk,
 		df->data_seq + df->data_len == msk->write_seq;
 }
 
-static void __mptcp_mem_reclaim_partial(struct sock *sk)
-{
-	int reclaimable = mptcp_sk(sk)->rmem_fwd_alloc - sk_unused_reserved_mem(sk);
-
-	lockdep_assert_held_once(&sk->sk_lock.slock);
-
-	if (reclaimable > (int)PAGE_SIZE)
-		__mptcp_rmem_reclaim(sk, reclaimable - 1);
-
-	sk_mem_reclaim(sk);
-}
-
-static void mptcp_mem_reclaim_partial(struct sock *sk)
-{
-	mptcp_data_lock(sk);
-	__mptcp_mem_reclaim_partial(sk);
-	mptcp_data_unlock(sk);
-}
-
 static void dfrag_uncharge(struct sock *sk, int len)
 {
 	sk_mem_uncharge(sk, len);
@@ -999,7 +980,6 @@ static void __mptcp_clean_una(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	struct mptcp_data_frag *dtmp, *dfrag;
-	bool cleaned = false;
 	u64 snd_una;
 
 	/* on fallback we just need to ignore snd_una, as this is really
@@ -1022,7 +1002,6 @@ static void __mptcp_clean_una(struct sock *sk)
 		}
 
 		dfrag_clear(sk, dfrag);
-		cleaned = true;
 	}
 
 	dfrag = mptcp_rtx_head(sk);
@@ -1044,7 +1023,6 @@ static void __mptcp_clean_una(struct sock *sk)
 		dfrag->already_sent -= delta;
 
 		dfrag_uncharge(sk, delta);
-		cleaned = true;
 	}
 
 	/* all retransmitted data acked, recovery completed */
@@ -1052,9 +1030,6 @@ static void __mptcp_clean_una(struct sock *sk)
 		msk->recovery = false;
 
 out:
-	if (cleaned && tcp_under_memory_pressure(sk))
-		__mptcp_mem_reclaim_partial(sk);
-
 	if (snd_una == READ_ONCE(msk->snd_nxt) &&
 	    snd_una == READ_ONCE(msk->write_seq)) {
 		if (mptcp_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
@@ -1206,12 +1181,6 @@ static struct sk_buff *mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, boo
 {
 	gfp_t gfp = data_lock_held ? GFP_ATOMIC : sk->sk_allocation;
 
-	if (unlikely(tcp_under_memory_pressure(sk))) {
-		if (data_lock_held)
-			__mptcp_mem_reclaim_partial(sk);
-		else
-			mptcp_mem_reclaim_partial(sk);
-	}
 	return __mptcp_alloc_tx_skb(sk, ssk, gfp);
 }
 
-- 
2.35.3

[PATCH v4 mptcp-next 3/4] mptcp: refine memory scheduling

[Paolo Abeni]

Similar to commit 7c80b038d23e ("net: fix sk_wmem_schedule() and
sk_rmem_schedule() errors"), let the MPTCP receive path schedule
exactly the required amount of memory.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v3 -> v4:
 - do not allocate 0 bytes (Mat)
---
 net/mptcp/protocol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b31bac33f87a..b1fae2f747c9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -323,9 +323,10 @@ static bool mptcp_rmem_schedule(struct sock *sk, struct sock *ssk, int size)
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	int amt, amount;
 
-	if (size < msk->rmem_fwd_alloc)
+	if (size <= msk->rmem_fwd_alloc)
 		return true;
 
+	size -= msk->rmem_fwd_alloc;
 	amt = sk_mem_pages(size);
 	amount = amt << PAGE_SHIFT;
 	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV))
-- 
2.35.3

[PATCH v4 mptcp-next 4/4] net: remove SK_RECLAIM_THRESHOLD and SK_RECLAIM_CHUNK

[Paolo Abeni]

There are no more users for the mentioned macros, just
drop them.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sock.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 5bed1ea7a722..c3ac36b7b7b8 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1619,11 +1619,6 @@ static inline void sk_mem_charge(struct sock *sk, int size)
 	sk->sk_forward_alloc -= size;
 }
 
-/* the following macros control memory reclaiming in mptcp_rmem_uncharge()
- */
-#define SK_RECLAIM_THRESHOLD	(1 << 21)
-#define SK_RECLAIM_CHUNK	(1 << 20)
-
 static inline void sk_mem_uncharge(struct sock *sk, int size)
 {
 	if (!sk_has_account(sk))
-- 
2.35.3

Re: [PATCH v4 mptcp-next 1/4] mptcp: never fetch fwd memory from the subflow

[Mat Martineau]

On Tue, 21 Jun 2022, Paolo Abeni wrote:

> The memory accounting is broken in such exceptional code
> path, and after commit 4890b686f408 ("net: keep sk->sk_forward_alloc
> as small as possible") we can't find much help there.
>
> Drop the broken code.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---

Thanks Paolo, v4 looks good.

For patches 1-3:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

See patch 4 for different tag.


> This is possibly for net, but makes sense only on top of recent
> net-next patches, so whatever ;)

I agree it makes sense for net-next.

- Mat

> ---
> net/mptcp/protocol.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 6d2aa41390e7..0d4b2c010da0 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -328,15 +328,10 @@ static bool mptcp_rmem_schedule(struct sock *sk, struct sock *ssk, int size)
>
> 	amt = sk_mem_pages(size);
> 	amount = amt << PAGE_SHIFT;
> -	msk->rmem_fwd_alloc += amount;
> -	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV)) {
> -		if (ssk->sk_forward_alloc < amount) {
> -			msk->rmem_fwd_alloc -= amount;
> -			return false;
> -		}
> +	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV))
> +		return false;
>
> -		ssk->sk_forward_alloc -= amount;
> -	}
> +	msk->rmem_fwd_alloc += amount;
> 	return true;
> }
>
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH v4 mptcp-next 4/4] net: remove SK_RECLAIM_THRESHOLD and SK_RECLAIM_CHUNK

[Mat Martineau]

On Tue, 21 Jun 2022, Paolo Abeni wrote:

> There are no more users for the mentioned macros, just
> drop them.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Looks good:

Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>


(even though I'll end up replacing that with a Signed-off-by when 
upstreaming anyway...)


- Mat

> ---
> include/net/sock.h | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 5bed1ea7a722..c3ac36b7b7b8 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1619,11 +1619,6 @@ static inline void sk_mem_charge(struct sock *sk, int size)
> 	sk->sk_forward_alloc -= size;
> }
>
> -/* the following macros control memory reclaiming in mptcp_rmem_uncharge()
> - */
> -#define SK_RECLAIM_THRESHOLD	(1 << 21)
> -#define SK_RECLAIM_CHUNK	(1 << 20)
> -
> static inline void sk_mem_uncharge(struct sock *sk, int size)
> {
> 	if (!sk_has_account(sk))
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH v4 mptcp-next 1/4] mptcp: never fetch fwd memory from the subflow

[Mat Martineau]

On Tue, 21 Jun 2022, Mat Martineau wrote:

> On Tue, 21 Jun 2022, Paolo Abeni wrote:
>
>> The memory accounting is broken in such exceptional code
>> path, and after commit 4890b686f408 ("net: keep sk->sk_forward_alloc
>> as small as possible") we can't find much help there.
>> 
>> Drop the broken code.
>> 
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>
> Thanks Paolo, v4 looks good.
>

One more question Paolo: given the SCTP regression that was bisected to 
the fwd alloc change:

https://lore.kernel.org/netdev/20220619150456.GB34471@xsang-OptiPlex-9020/

is there any need to wait before upstreaming this series for net-next? 
(Such upstreaming will probably be after our Thursday meeting anyway)

- Mat


> For patches 1-3:
>
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
> See patch 4 for different tag.
>
>
>> This is possibly for net, but makes sense only on top of recent
>> net-next patches, so whatever ;)
>
> I agree it makes sense for net-next.
>
> - Mat
>
>> ---
>> net/mptcp/protocol.c | 11 +++--------
>> 1 file changed, 3 insertions(+), 8 deletions(-)
>> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 6d2aa41390e7..0d4b2c010da0 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -328,15 +328,10 @@ static bool mptcp_rmem_schedule(struct sock *sk, 
>> struct sock *ssk, int size)
>>
>> 	amt = sk_mem_pages(size);
>> 	amount = amt << PAGE_SHIFT;
>> -	msk->rmem_fwd_alloc += amount;
>> -	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV)) {
>> -		if (ssk->sk_forward_alloc < amount) {
>> -			msk->rmem_fwd_alloc -= amount;
>> -			return false;
>> -		}
>> +	if (!__sk_mem_raise_allocated(sk, size, amt, SK_MEM_RECV))
>> +		return false;
>> 
>> -		ssk->sk_forward_alloc -= amount;
>> -	}
>> +	msk->rmem_fwd_alloc += amount;
>> 	return true;
>> }
>> 
>> -- 
>> 2.35.3
>> 
>> 
>> 
>
> --
> Mat Martineau
> Intel
>
>

--
Mat Martineau
Intel

Re: [PATCH v4 mptcp-next 1/4] mptcp: never fetch fwd memory from the subflow

[Paolo Abeni]

On Tue, 2022-06-21 at 15:46 -0700, Mat Martineau wrote:
> On Tue, 21 Jun 2022, Mat Martineau wrote:
> 
> > On Tue, 21 Jun 2022, Paolo Abeni wrote:
> > 
> > > The memory accounting is broken in such exceptional code
> > > path, and after commit 4890b686f408 ("net: keep sk->sk_forward_alloc
> > > as small as possible") we can't find much help there.
> > > 
> > > Drop the broken code.
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > 
> > Thanks Paolo, v4 looks good.
> > 
> 
> One more question Paolo: given the SCTP regression that was bisected to 
> the fwd alloc change:
> 
> https://lore.kernel.org/netdev/20220619150456.GB34471@xsang-OptiPlex-9020/
> 
> is there any need to wait before upstreaming this series for net-next? 

I'm guild-guessing 4890b686f408 could cause regressions (for TCP and as
as consequence for everything else using it) when cgroups are in use,
because after that patch the cgroup memory accounting hooks are called
~twice per packet (one to increase, one to decrease the cgroup
accounted memory), instead that almost never before that patch.

In any case we can't do much to counter such regression. 

I did some benchmarking on top of this patches, and I could not measure
relevant changes (but no cgroups enabled).

I'm not 110% sure that the regressions reported by LTP are relevant.
Once upon time I hit a quite large one, and it was actually due to test
instability.

I agree staging this changes for a bit will not hurt/will be for the
better.

Cheers,

Paolo

Re: [PATCH v4 mptcp-next 4/4] net: remove SK_RECLAIM_THRESHOLD and SK_RECLAIM_CHUNK

[Matthieu Baerts]

Hi Paolo, Mat,

On 22/06/2022 00:38, Mat Martineau wrote:
> On Tue, 21 Jun 2022, Paolo Abeni wrote:
> 
>> There are no more users for the mentioned macros, just
>> drop them.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Looks good:
> 
> Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for the patches and the reviews!

Now in our tree (feat. for net-next) with Mat's RvB/ACK tags:

- 37c90ab38feb: mptcp: never fetch fwd memory from the subflow
- b6e446432fd3: mptcp: drop SK_RECLAIM_* macros
- 31d82b289435: mptcp: refine memory scheduling
- fcc809e3a1d4: net: remove SK_RECLAIM_THRESHOLD and SK_RECLAIM_CHUNK
- Results: d99676bfce76..8346c4604d29 (export)

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220622T210613
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export


Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net