[MPTCP] Re: [PATCH v2 1/6] mptcp: separate accouting for wmem forward alloc mem

[MPTCP] Re: [PATCH v2 1/6] mptcp: separate accouting for wmem forward alloc mem

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> As mentioned before, one possible follow-up to this series is open-code 
> a MPTCP-specific variant of lock_sock()/release_sock() which will allow
> performing some additional/MPTCP specific actions under the msk level
> spinlock.
> 
> Specifically:
> 
> - in recevmsg(), in lock_sock(), splice sk->sk_receive_queue in msk-
> >receive_queue
> - still in recvmsg(), in release_sock(), update the bulk freed rmem 
> 
> Overall this 2 will allow acquiring no additional spinlock at all in
> the average case for recvmsg()
> 
> - in sendmsg(), in lock_sock(), move as much memory as needed from
> sk_forward_memory to wforward_memory, eventually allocating it.
> - in sendmsg(), in release_sock(), move again the unused
> wforward_memory to sk_forward_memory.
> 
> Overall this 2 will allow acquiring no additional spinlock at all in
> the average case for sendmsg() and will avoid artifacts due to the
> sk_forward_memory/wforward_memory separation.
> 
> But they additionally will allow replacing the wforward_memory field
> with a local variable. 
> 
> So I'm wondering if I should already code the above, to avoid
> introducing this additional new msk field just to remove it in a few
> patches !?!

Tricky.  Do you have hunch of how much additional code churn that
means?

If its not too much 'rewrite', I would prefer to do it later on
even if that means axing the newly added field.

If it means to rewrite most of this again then its probably
better to do it right away.

[MPTCP] Re: [PATCH v2 1/6] mptcp: separate accouting for wmem forward alloc mem

[Paolo Abeni]

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

On Tue, 2020-11-17 at 13:07 -0800, Mat Martineau wrote:
> On Tue, 17 Nov 2020, Paolo Abeni wrote:
> 
> > On Tue, 2020-11-17 at 11:47 +0100, Florian Westphal wrote:
> > > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > > As mentioned before, one possible follow-up to this series is open-code
> > > > a MPTCP-specific variant of lock_sock()/release_sock() which will allow
> > > > performing some additional/MPTCP specific actions under the msk level
> > > > spinlock.
> > > > 
> > > > Specifically:
> > > > 
> > > > - in recevmsg(), in lock_sock(), splice sk->sk_receive_queue in msk-
> > > > > receive_queue
> > > > - still in recvmsg(), in release_sock(), update the bulk freed rmem
> > > > 
> > > > Overall this 2 will allow acquiring no additional spinlock at all in
> > > > the average case for recvmsg()
> > > > 
> > > > - in sendmsg(), in lock_sock(), move as much memory as needed from
> > > > sk_forward_memory to wforward_memory, eventually allocating it.
> > > > - in sendmsg(), in release_sock(), move again the unused
> > > > wforward_memory to sk_forward_memory.
> > > > 
> > > > Overall this 2 will allow acquiring no additional spinlock at all in
> > > > the average case for sendmsg() and will avoid artifacts due to the
> > > > sk_forward_memory/wforward_memory separation.
> > > > 
> > > > But they additionally will allow replacing the wforward_memory field
> > > > with a local variable.
> > > > 
> > > > So I'm wondering if I should already code the above, to avoid
> > > > introducing this additional new msk field just to remove it in a few
> > > > patches !?!
> > > 
> > > Tricky.
> > 
> > The trickest part is probably getting a corret estimate for the wmem we
> > are going to use. But we don't really need to be very accurate.
> > 
> > > Do you have hunch of how much additional code churn that
> > > means?
> > > 
> > > If its not too much 'rewrite', I would prefer to do it later on
> > > even if that means axing the newly added field.
> > > 
> > > If it means to rewrite most of this again then its probably
> > > better to do it right away.
> > 
> > eh... the only way I had for a reasonable answer to the above was...
> > trying to code it, do I did it ;)
> > 
> > In the end I had to retain the newly added field - with a slightly
> > different semantic and name:
> > 
> > https://github.com/pabeni/mptcp/tree/worker_must_die_6
> > 
> > I think overall it cleans things more than a bit...
> > 
> 
> Yes, I find the version of this patch in
> https://github.com/pabeni/mptcp/tree/worker_must_die_6.1 to be cleaner 
> than the version posted here without mptcp_lock_sock().

That was my impression, too ;)

> Did you intend to use __sk in the body of the mptcp_lock_sock() macro?

That is typo on my side! Thanks for finding it. The macro actually
works only because a 'sk' variable is always available in the scopes
where the macro is used/invoked. I'll fix that.

Since looks like _6 is a viable option I'll post a v3 with that, ok?

Thanks!

Paolo

[MPTCP] Re: [PATCH v2 1/6] mptcp: separate accouting for wmem forward alloc mem

[Mat Martineau]

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

On Tue, 17 Nov 2020, Paolo Abeni wrote:

> On Tue, 2020-11-17 at 11:47 +0100, Florian Westphal wrote:
>> Paolo Abeni <pabeni(a)redhat.com> wrote:
>>> As mentioned before, one possible follow-up to this series is open-code
>>> a MPTCP-specific variant of lock_sock()/release_sock() which will allow
>>> performing some additional/MPTCP specific actions under the msk level
>>> spinlock.
>>>
>>> Specifically:
>>>
>>> - in recevmsg(), in lock_sock(), splice sk->sk_receive_queue in msk-
>>>> receive_queue
>>> - still in recvmsg(), in release_sock(), update the bulk freed rmem
>>>
>>> Overall this 2 will allow acquiring no additional spinlock at all in
>>> the average case for recvmsg()
>>>
>>> - in sendmsg(), in lock_sock(), move as much memory as needed from
>>> sk_forward_memory to wforward_memory, eventually allocating it.
>>> - in sendmsg(), in release_sock(), move again the unused
>>> wforward_memory to sk_forward_memory.
>>>
>>> Overall this 2 will allow acquiring no additional spinlock at all in
>>> the average case for sendmsg() and will avoid artifacts due to the
>>> sk_forward_memory/wforward_memory separation.
>>>
>>> But they additionally will allow replacing the wforward_memory field
>>> with a local variable.
>>>
>>> So I'm wondering if I should already code the above, to avoid
>>> introducing this additional new msk field just to remove it in a few
>>> patches !?!
>>
>> Tricky.
>
> The trickest part is probably getting a corret estimate for the wmem we
> are going to use. But we don't really need to be very accurate.
>
>> Do you have hunch of how much additional code churn that
>> means?
>>
>> If its not too much 'rewrite', I would prefer to do it later on
>> even if that means axing the newly added field.
>>
>> If it means to rewrite most of this again then its probably
>> better to do it right away.
>
> eh... the only way I had for a reasonable answer to the above was...
> trying to code it, do I did it ;)
>
> In the end I had to retain the newly added field - with a slightly
> different semantic and name:
>
> https://github.com/pabeni/mptcp/tree/worker_must_die_6
>
> I think overall it cleans things more than a bit...
>

Yes, I find the version of this patch in
https://github.com/pabeni/mptcp/tree/worker_must_die_6.1 to be cleaner 
than the version posted here without mptcp_lock_sock().

Did you intend to use __sk in the body of the mptcp_lock_sock() macro?


Thanks,

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH v2 1/6] mptcp: separate accouting for wmem forward alloc mem

[Paolo Abeni]

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

On Tue, 2020-11-17 at 11:47 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > As mentioned before, one possible follow-up to this series is open-code 
> > a MPTCP-specific variant of lock_sock()/release_sock() which will allow
> > performing some additional/MPTCP specific actions under the msk level
> > spinlock.
> > 
> > Specifically:
> > 
> > - in recevmsg(), in lock_sock(), splice sk->sk_receive_queue in msk-
> > > receive_queue
> > - still in recvmsg(), in release_sock(), update the bulk freed rmem 
> > 
> > Overall this 2 will allow acquiring no additional spinlock at all in
> > the average case for recvmsg()
> > 
> > - in sendmsg(), in lock_sock(), move as much memory as needed from
> > sk_forward_memory to wforward_memory, eventually allocating it.
> > - in sendmsg(), in release_sock(), move again the unused
> > wforward_memory to sk_forward_memory.
> > 
> > Overall this 2 will allow acquiring no additional spinlock at all in
> > the average case for sendmsg() and will avoid artifacts due to the
> > sk_forward_memory/wforward_memory separation.
> > 
> > But they additionally will allow replacing the wforward_memory field
> > with a local variable. 
> > 
> > So I'm wondering if I should already code the above, to avoid
> > introducing this additional new msk field just to remove it in a few
> > patches !?!
> 
> Tricky.  

The trickest part is probably getting a corret estimate for the wmem we
are going to use. But we don't really need to be very accurate.

> Do you have hunch of how much additional code churn that
> means?
> 
> If its not too much 'rewrite', I would prefer to do it later on
> even if that means axing the newly added field.
> 
> If it means to rewrite most of this again then its probably
> better to do it right away.

eh... the only way I had for a reasonable answer to the above was...
trying to code it, do I did it ;)

In the end I had to retain the newly added field - with a slightly
different semantic and name:

https://github.com/pabeni/mptcp/tree/worker_must_die_6

I think overall it cleans things more than a bit...

/P

[MPTCP] Re: [PATCH v2 1/6] mptcp: separate accouting for wmem forward alloc mem

[Paolo Abeni]

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

On Fri, 2020-11-13 at 17:27 +0100, Paolo Abeni wrote:
> Account separatelly the memory forward allocated
> for write operation. When sendmsg() need more memory
> it looks at wforward_alloc first and if that is
> exaused, move more space from sk_forward_alloc.
> 
> This will simplify the next patch
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
>  net/mptcp/protocol.c | 52 +++++++++++++++++++++++++++++++++++++++-----
>  net/mptcp/protocol.h |  1 +
>  2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 6397083476e3..87d8659b9bcc 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -852,9 +852,48 @@ static bool mptcp_frag_can_collapse_to(const struct mptcp_sock *msk,
>  		df->data_seq + df->data_len == msk->write_seq;
>  }
>  
> +static bool mptcp_wmem_alloc(struct sock *sk, int size)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	int amount;
> +
> +	if (msk->wforward_alloc >= size)
> +		goto account;
> +
> +	if (!sk_wmem_schedule(sk, size))
> +		return false;
> +
> +	/* try to keep half fwd alloc memory for each direction */
> +	amount = max(size, sk->sk_forward_alloc >> 1);
> +	sk->sk_forward_alloc -= amount;
> +	msk->wforward_alloc += amount;
> +
> +account:
> +	msk->wforward_alloc -= size;
> +	return true;
> +}
> +
> +static void mptcp_wmem_uncharge(struct sock *sk, int size)
> +{
> +	mptcp_sk(sk)->wforward_alloc += size;
> +}
> +
> +static void mptcp_mem_reclaim_partial(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +
> +	sk->sk_forward_alloc += msk->wforward_alloc;
> +	msk->wforward_alloc = 0;
> +	sk_mem_reclaim_partial(sk);
> +
> +	/* split the remaining fwd allocated memory between rx and tx */
> +	msk->wforward_alloc = sk->sk_forward_alloc >> 1;
> +	sk->sk_forward_alloc -= msk->wforward_alloc;
> +}
> +
>  static void dfrag_uncharge(struct sock *sk, int len)
>  {
> -	sk_mem_uncharge(sk, len);
> +	mptcp_wmem_uncharge(sk, len);
>  	sk_wmem_queued_add(sk, -len);
>  }
>  
> @@ -909,8 +948,8 @@ static void mptcp_clean_una(struct sock *sk)
>  	}
>  
>  out:
> -	if (cleaned)
> -		sk_mem_reclaim_partial(sk);
> +	if (cleaned && tcp_under_memory_pressure(sk))
> +		mptcp_mem_reclaim_partial(sk);
>  }
>  
>  static void mptcp_clean_una_wakeup(struct sock *sk)
> @@ -1334,11 +1373,12 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		offset = dfrag->offset + dfrag->data_len;
>  		psize = pfrag->size - offset;
>  		psize = min_t(size_t, psize, msg_data_left(msg));
> -		if (!sk_wmem_schedule(sk, psize + frag_truesize))
> +		if (!mptcp_wmem_alloc(sk, psize + frag_truesize))
>  			goto wait_for_memory;

As mentioned before, one possible follow-up to this series is open-code 
a MPTCP-specific variant of lock_sock()/release_sock() which will allow
performing some additional/MPTCP specific actions under the msk level
spinlock.

Specifically:

- in recevmsg(), in lock_sock(), splice sk->sk_receive_queue in msk-
>receive_queue
- still in recvmsg(), in release_sock(), update the bulk freed rmem 

Overall this 2 will allow acquiring no additional spinlock at all in
the average case for recvmsg()

- in sendmsg(), in lock_sock(), move as much memory as needed from
sk_forward_memory to wforward_memory, eventually allocating it.
- in sendmsg(), in release_sock(), move again the unused
wforward_memory to sk_forward_memory.

Overall this 2 will allow acquiring no additional spinlock at all in
the average case for sendmsg() and will avoid artifacts due to the
sk_forward_memory/wforward_memory separation.

But they additionally will allow replacing the wforward_memory field
with a local variable. 

So I'm wondering if I should already code the above, to avoid
introducing this additional new msk field just to remove it in a few
patches !?!

Any opinion more than welcome!

Paolo