* [MPTCP] Re: [PATCH v2 5/6] mptcp: protocol: re-check dsn before reading from subflow
@ 2020-02-18 8:33 Paolo Abeni
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2020-02-18 8:33 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4496 bytes --]
On Mon, 2020-02-17 at 23:14 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Mon, 2020-02-17 at 16:01 +0100, Florian Westphal wrote:
> > > mptcp_subflow_data_available() is commonly called via
> > > ssk->sk_data_ready(), in this case the mptcp socket lock
> > > cannot be acquired.
> > >
> > > Therefore, while we can safely discard subflow data that
> > > was already received up to msk->ack_seq, we cannot be sure
> > > that 'subflow->data_avail' will still be valid at the time
> > > userspace wants to read the data -- a previous read on a
> > > different subflow might have carried this data already.
> > >
> > > In that (unlikely) event, msk->ack_seq will have been updated
> > > and will be ahead of the subflow dsn.
> >
> > Does the above mean that we can get wake-up events and found no actual
> > data in the msk socket? (read will block)
> >
> > Something alike:
> > - user space is blocked on mptcp_recvmsg()/mptcp_wait_data() ->
> > wait_data == true
> > - subflow 1 delivers new in order data, wakes the user space which
> > start reading
> > - before mptcp_recvmsg updates the msk->ack_seq, subflow 2 delivers the
> > same DSS as subflow 1, consider that to be in order and sets the
> > MPTCP_DATA_READY bit
> > - mptcp_recvmsg fills the user-space buffer and completes. Since
> > wait_data == true, the MPTCP_DATA_READY bit is left untouched.
> > - next mptcp_poll will return EPOLLIN even if no data is really
> > available.
> >
> > Should we move the 'wait_data = false' initialization inside some inner
> > loop?
>
> Hmm, what about this (delta, would squash this to "mptcp: update mptcp ack
> sequence from work queue".
>
> In your scenario above, we would now hit the 'msk->rx_queue is empty'
> branch, DATA_READY gets cleared:
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -787,10 +787,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> int nonblock, int flags, int *addr_len)
> {
> struct mptcp_sock *msk = mptcp_sk(sk);
> - bool more_data_avail = true;
> - bool wait_data = false;
> struct socket *ssock;
> - struct sock *ssk;
> int copied = 0;
> int target;
> long timeo;
> @@ -814,7 +811,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
> __mptcp_flush_join_list(msk);
>
> - while (more_data_avail) {
> + while (len > (size_t)copied) {
> int bytes_read;
>
> bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied);
> @@ -826,12 +823,9 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>
> copied += bytes_read;
>
> - if (skb_queue_empty(&sk->sk_receive_queue)) {
> - if (!__mptcp_move_skbs(msk))
> - more_data_avail = false;
> -
> + if (skb_queue_empty(&sk->sk_receive_queue) &&
> + __mptcp_move_skbs(msk))
> continue;
> - }
>
> /* only the master socket status is relevant here. The exit
> * conditions mirror closely tcp_recvmsg()
> @@ -872,26 +866,24 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> }
>
> pr_debug("block timeout %ld", timeo);
> - wait_data = true;
> mptcp_wait_data(sk, &timeo);
> if (unlikely(__mptcp_tcp_fallback(msk)))
> goto fallback;
> }
>
> - if (more_data_avail) {
> - if (!test_bit(MPTCP_DATA_READY, &msk->flags))
> - set_bit(MPTCP_DATA_READY, &msk->flags);
> - } else if (!wait_data) {
> + if (skb_queue_empty(&sk->sk_receive_queue)) {
> + /* entire backlog drained, clear DATA_READY. */
> clear_bit(MPTCP_DATA_READY, &msk->flags);
>
> - /* .. race-breaker: ssk might get new data after last
> - * data_available() returns false.
> + /* .. race-breaker: ssk might have gotten new data
> + * after last __mptcp_move_skbs() returned false.
> */
> - ssk = mptcp_subflow_recv_lookup(msk);
> - if (unlikely(ssk))
> + if (unlikely(__mptcp_move_skbs(msk)))
> set_bit(MPTCP_DATA_READY, &msk->flags);
> + } else if (unlikely(!test_bit(MPTCP_DATA_READY, &msk->flags))) {
> + /* data to read but mptcp_wait_data() cleared DATA_READY */
> + set_bit(MPTCP_DATA_READY, &msk->flags);
> }
> -
> out_err:
> release_sock(sk);
> return copied;
>
LGTM, thanks for takling the concerns of my twisted mind ;)
Cheers,
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread
* [MPTCP] Re: [PATCH v2 5/6] mptcp: protocol: re-check dsn before reading from subflow
@ 2020-02-17 22:14 Florian Westphal
0 siblings, 0 replies; 3+ messages in thread
From: Florian Westphal @ 2020-02-17 22:14 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4121 bytes --]
Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Mon, 2020-02-17 at 16:01 +0100, Florian Westphal wrote:
> > mptcp_subflow_data_available() is commonly called via
> > ssk->sk_data_ready(), in this case the mptcp socket lock
> > cannot be acquired.
> >
> > Therefore, while we can safely discard subflow data that
> > was already received up to msk->ack_seq, we cannot be sure
> > that 'subflow->data_avail' will still be valid at the time
> > userspace wants to read the data -- a previous read on a
> > different subflow might have carried this data already.
> >
> > In that (unlikely) event, msk->ack_seq will have been updated
> > and will be ahead of the subflow dsn.
>
> Does the above mean that we can get wake-up events and found no actual
> data in the msk socket? (read will block)
>
> Something alike:
> - user space is blocked on mptcp_recvmsg()/mptcp_wait_data() ->
> wait_data == true
> - subflow 1 delivers new in order data, wakes the user space which
> start reading
> - before mptcp_recvmsg updates the msk->ack_seq, subflow 2 delivers the
> same DSS as subflow 1, consider that to be in order and sets the
> MPTCP_DATA_READY bit
> - mptcp_recvmsg fills the user-space buffer and completes. Since
> wait_data == true, the MPTCP_DATA_READY bit is left untouched.
> - next mptcp_poll will return EPOLLIN even if no data is really
> available.
>
> Should we move the 'wait_data = false' initialization inside some inner
> loop?
Hmm, what about this (delta, would squash this to "mptcp: update mptcp ack
sequence from work queue".
In your scenario above, we would now hit the 'msk->rx_queue is empty'
branch, DATA_READY gets cleared:
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -787,10 +787,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
int nonblock, int flags, int *addr_len)
{
struct mptcp_sock *msk = mptcp_sk(sk);
- bool more_data_avail = true;
- bool wait_data = false;
struct socket *ssock;
- struct sock *ssk;
int copied = 0;
int target;
long timeo;
@@ -814,7 +811,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
__mptcp_flush_join_list(msk);
- while (more_data_avail) {
+ while (len > (size_t)copied) {
int bytes_read;
bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied);
@@ -826,12 +823,9 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
copied += bytes_read;
- if (skb_queue_empty(&sk->sk_receive_queue)) {
- if (!__mptcp_move_skbs(msk))
- more_data_avail = false;
-
+ if (skb_queue_empty(&sk->sk_receive_queue) &&
+ __mptcp_move_skbs(msk))
continue;
- }
/* only the master socket status is relevant here. The exit
* conditions mirror closely tcp_recvmsg()
@@ -872,26 +866,24 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
}
pr_debug("block timeout %ld", timeo);
- wait_data = true;
mptcp_wait_data(sk, &timeo);
if (unlikely(__mptcp_tcp_fallback(msk)))
goto fallback;
}
- if (more_data_avail) {
- if (!test_bit(MPTCP_DATA_READY, &msk->flags))
- set_bit(MPTCP_DATA_READY, &msk->flags);
- } else if (!wait_data) {
+ if (skb_queue_empty(&sk->sk_receive_queue)) {
+ /* entire backlog drained, clear DATA_READY. */
clear_bit(MPTCP_DATA_READY, &msk->flags);
- /* .. race-breaker: ssk might get new data after last
- * data_available() returns false.
+ /* .. race-breaker: ssk might have gotten new data
+ * after last __mptcp_move_skbs() returned false.
*/
- ssk = mptcp_subflow_recv_lookup(msk);
- if (unlikely(ssk))
+ if (unlikely(__mptcp_move_skbs(msk)))
set_bit(MPTCP_DATA_READY, &msk->flags);
+ } else if (unlikely(!test_bit(MPTCP_DATA_READY, &msk->flags))) {
+ /* data to read but mptcp_wait_data() cleared DATA_READY */
+ set_bit(MPTCP_DATA_READY, &msk->flags);
}
-
out_err:
release_sock(sk);
return copied;
^ permalink raw reply [flat|nested] 3+ messages in thread
* [MPTCP] Re: [PATCH v2 5/6] mptcp: protocol: re-check dsn before reading from subflow
@ 2020-02-17 18:56 Paolo Abeni
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2020-02-17 18:56 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]
On Mon, 2020-02-17 at 16:01 +0100, Florian Westphal wrote:
> mptcp_subflow_data_available() is commonly called via
> ssk->sk_data_ready(), in this case the mptcp socket lock
> cannot be acquired.
>
> Therefore, while we can safely discard subflow data that
> was already received up to msk->ack_seq, we cannot be sure
> that 'subflow->data_avail' will still be valid at the time
> userspace wants to read the data -- a previous read on a
> different subflow might have carried this data already.
>
> In that (unlikely) event, msk->ack_seq will have been updated
> and will be ahead of the subflow dsn.
Does the above mean that we can get wake-up events and found no actual
data in the msk socket? (read will block)
Something alike:
- user space is blocked on mptcp_recvmsg()/mptcp_wait_data() ->
wait_data == true
- subflow 1 delivers new in order data, wakes the user space which
start reading
- before mptcp_recvmsg updates the msk->ack_seq, subflow 2 delivers the
same DSS as subflow 1, consider that to be in order and sets the
MPTCP_DATA_READY bit
- mptcp_recvmsg fills the user-space buffer and completes. Since
wait_data == true, the MPTCP_DATA_READY bit is left untouched.
- next mptcp_poll will return EPOLLIN even if no data is really
available.
Should we move the 'wait_data = false' initialization inside some inner
loop?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-18 8:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 8:33 [MPTCP] Re: [PATCH v2 5/6] mptcp: protocol: re-check dsn before reading from subflow Paolo Abeni
-- strict thread matches above, loose matches on Subject: below --
2020-02-17 22:14 Florian Westphal
2020-02-17 18:56 Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.