[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> Thank you for the patches and the reviews.
> Is it then OK if I apply this series as it is?

I think so.

[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[Paolo Abeni]

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

On Wed, 2019-11-06 at 16:58 +0100, Matthieu Baerts wrote:
> Thank you for the patches and the reviews.
> Is it then OK if I apply this series as it is?

yes, I agree with that.

Cheers,

Paolo

[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[Matthieu Baerts]

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

Hi Florian, Mat, Paolo,

On 06/11/2019 16:09, Florian Westphal wrote:
> Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
>> sk_wait_event is a macro that evaluates its third arg twice (but we've
>> discussed this before: https://lists.01.org/hyperkitty/list/mptcp(a)lists.01.org/thread/7HAWM43BLQVRBNIPV5II7C2WGTARAMDR/#7S54V7TAZUXZEUXVANQZ3OETQMPQGK6J),
>> but in this case the second check of the flag is likely to be an inexpensive
>> READ_ONCE() on a hot cache line. So, the extra code was there a reason but
>> I'm ok with merging this for maintainability purposes.
> 
> We can also drop it.  Its likely this needs to be rewritten again in
> the future.
> 
> I'd like to get rid of __tcp_poll() and make mptcp_poll standalone.
> 
> For that to work reliably its needed that the mptcp sk has a reliable
> indicator that there is data available.
> 
> Right now this isn't the case because the bit is cleared immediately
> on mptcp_recvmsg entry.
> 
> Removing the clear_bit could get us in a state where the bit is still
> set, but the last recvmsg did drain the last byte.
> 
> Checking the ssk read queue and clearing the bit on revmsg exit doesn't
> work either because we could have data available in another subflow,
> but walking the subflow list isn't exactly ideal.

Thank you for the patches and the reviews.
Is it then OK if I apply this series as it is?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> sk_wait_event is a macro that evaluates its third arg twice (but we've
> discussed this before: https://lists.01.org/hyperkitty/list/mptcp(a)lists.01.org/thread/7HAWM43BLQVRBNIPV5II7C2WGTARAMDR/#7S54V7TAZUXZEUXVANQZ3OETQMPQGK6J),
> but in this case the second check of the flag is likely to be an inexpensive
> READ_ONCE() on a hot cache line. So, the extra code was there a reason but
> I'm ok with merging this for maintainability purposes.

We can also drop it.  Its likely this needs to be rewritten again in
the future.

I'd like to get rid of __tcp_poll() and make mptcp_poll standalone.

For that to work reliably its needed that the mptcp sk has a reliable
indicator that there is data available.

Right now this isn't the case because the bit is cleared immediately
on mptcp_recvmsg entry.

Removing the clear_bit could get us in a state where the bit is still
set, but the last recvmsg did drain the last byte.

Checking the ssk read queue and clearing the bit on revmsg exit doesn't
work either because we could have data available in another subflow,
but walking the subflow list isn't exactly ideal.

[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[Mat Martineau]

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

On Tue, 5 Nov 2019, Paolo Abeni wrote:

> On Tue, 2019-11-05 at 18:05 +0100, Florian Westphal wrote:
>> Avoids a bit of copy&paste code.
>> squashto:  mptcp: recvmsg() can drain data from multiple subflows
>>
>> Signed-off-by: Florian Westphal <fw(a)strlen.de>
>> ---
>>  net/mptcp/protocol.c | 14 ++------------
>>  1 file changed, 2 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 3af5f121af9e..8cc9cc5675c2 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -443,22 +443,12 @@ static void mptcp_wait_data(struct sock *sk, long *timeo)
>>  {
>>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>  	struct mptcp_sock *msk = mptcp_sk(sk);
>> -	int data_ready;
>>
>>  	add_wait_queue(sk_sleep(sk), &wait);
>>  	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>>
>> -	release_sock(sk);
>> -
>> -	smp_mb__before_atomic();
>> -	data_ready = test_and_clear_bit(MPTCP_DATA_READY, &msk->flags);
>> -	smp_mb__after_atomic();
>> -
>> -	if (!data_ready)
>> -		*timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
>> -
>> -	sched_annotate_sleep();
>> -	lock_sock(sk);
>> +	sk_wait_event(sk, timeo,
>> +		      test_and_clear_bit(MPTCP_DATA_READY, &msk->flags), &wait);
>>
>>  	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>>  	remove_wait_queue(sk_sleep(sk), &wait);
>
> IIRC, sk_wait_event() was intentionally avoided to allow placing the
> smp_mb__{before,after}_atomic() pair around test_and_clear_bit(), which
> I thought was required.
>
> Reading again the relevant documentation:
>
> https://elixir.bootlin.com/linux/latest/source/Documentation/core-api/atomic_ops.rst#L485
>
> it looks like test_and_clear_bit() provides already the required
> barriers (also grepping inside the kernel sources suggests that).
>
> So the above should be correct, I'm sorry for being cyclothymic ;)

I agree that the barriers can be removed.

sk_wait_event is a macro that evaluates its third arg twice (but we've 
discussed this before: 
https://lists.01.org/hyperkitty/list/mptcp(a)lists.01.org/thread/7HAWM43BLQVRBNIPV5II7C2WGTARAMDR/#7S54V7TAZUXZEUXVANQZ3OETQMPQGK6J), 
but in this case the second check of the flag is likely to be an 
inexpensive READ_ONCE() on a hot cache line. So, the extra code was there 
a reason but I'm ok with merging this for maintainability purposes.


--
Mat Martineau
Intel

[MPTCP] Re: [PATCH mptcp 3/4] protocol: use sk_wait_event helper

[Paolo Abeni]

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

On Tue, 2019-11-05 at 18:05 +0100, Florian Westphal wrote:
> Avoids a bit of copy&paste code.
> squashto:  mptcp: recvmsg() can drain data from multiple subflows
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3af5f121af9e..8cc9cc5675c2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -443,22 +443,12 @@ static void mptcp_wait_data(struct sock *sk, long *timeo)
>  {
>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  	struct mptcp_sock *msk = mptcp_sk(sk);
> -	int data_ready;
>  
>  	add_wait_queue(sk_sleep(sk), &wait);
>  	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>  
> -	release_sock(sk);
> -
> -	smp_mb__before_atomic();
> -	data_ready = test_and_clear_bit(MPTCP_DATA_READY, &msk->flags);
> -	smp_mb__after_atomic();
> -
> -	if (!data_ready)
> -		*timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
> -
> -	sched_annotate_sleep();
> -	lock_sock(sk);
> +	sk_wait_event(sk, timeo,
> +		      test_and_clear_bit(MPTCP_DATA_READY, &msk->flags), &wait);
>  
>  	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>  	remove_wait_queue(sk_sleep(sk), &wait);

IIRC, sk_wait_event() was intentionally avoided to allow placing the
smp_mb__{before,after}_atomic() pair around test_and_clear_bit(), which
I thought was required.

Reading again the relevant documentation:

https://elixir.bootlin.com/linux/latest/source/Documentation/core-api/atomic_ops.rst#L485

it looks like test_and_clear_bit() provides already the required
barriers (also grepping inside the kernel sources suggests that).

So the above should be correct, I'm sorry for being cyclothymic ;)

/P