7:11 a.m.
The MPTCP-level socket doesn't use sk_receive_queue, so it was possible
for mptcp_recvmsg() to remain blocked when there was data ready for it
to read. When the MPTCP socket is waiting for additional data and it
releases the subflow socket lock, the subflow may have incoming packets
ready to process and it sometimes called subflow_data_ready() before the
MPTCP socket called sk_wait_data().
This change adds new functions for the MPTCP socket to use to wait and
to signal that data is ready. Atomic bitops are used to set, test, and
clear a MPTCP socket flag that indicates waiting subflow data. This flag
replaces the sk_receive_queue checks used by other socket types.
Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
---
Squashing this into "mptcp: Implement MPTCP receive path" has a few
conflicts with later commits that also fix up the receive code. It's
fine to add this to the end of the commit chain.
net/mptcp/protocol.c | 40 +++++++++++++++++++++++++++++++++++++++-
net/mptcp/protocol.h | 4 ++++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 774ed25d3b6d..5555ee1529bb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -357,6 +357,26 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
return ret;
}
+static void mptcp_wait_data(struct sock *sk, long *timeo)
+{
+ DEFINE_WAIT_FUNC(wait, woken_wake_function);
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
+
+ release_sock(sk);
+
+ if (!test_and_clear_bit(MPTCP_DATA_READY, &msk->flags))
+ *timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
+
+ sched_annotate_sleep();
+ lock_sock(sk);
+
+ sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
+ remove_wait_queue(sk_sleep(sk), &wait);
+}
+
static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
int nonblock, int flags, int *addr_len)
{
@@ -403,6 +423,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t
len,
u64 old_ack;
u32 ssn;
+ clear_bit(MPTCP_DATA_READY, &msk->flags);
status = mptcp_get_mapping(ssk);
if (status == MAPPING_ADDED) {
@@ -536,7 +557,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t
len,
pr_debug("block");
release_sock(ssk);
- sk_wait_data(sk, &timeo, NULL);
+ mptcp_wait_data(sk, &timeo);
lock_sock(ssk);
}
@@ -548,6 +569,22 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t
len,
return copied;
}
+static void mptcp_data_ready(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+ struct socket_wq *wq;
+
+ set_bit(MPTCP_DATA_READY, &msk->flags);
+
+ rcu_read_lock();
+ wq = rcu_dereference(sk->sk_wq);
+ if (skwq_has_sleeper(wq))
+ wake_up_interruptible_sync_poll(&wq->wait, EPOLLIN | EPOLLPRI |
+ EPOLLRDNORM | EPOLLRDBAND);
+ sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
+ rcu_read_unlock();
+}
+
static int mptcp_init_sock(struct sock *sk)
{
struct mptcp_sock *msk = mptcp_sk(sk);
@@ -555,6 +592,7 @@ static int mptcp_init_sock(struct sock *sk)
pr_debug("msk=%p", msk);
INIT_LIST_HEAD(&msk->conn_list);
+ sk->sk_data_ready = mptcp_data_ready;
return 0;
}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7f15f6aab93d..ca5e6d839575 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -71,6 +71,9 @@
#define MPTCP_ADDR_IPVERSION_4 4
#define MPTCP_ADDR_IPVERSION_6 6
+/* MPTCP socket flags */
+#define MPTCP_DATA_READY BIT(0)
+
static inline u32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
{
return htonl((TCPOPT_MPTCP << 24) | (len << 16) | (subopt << 12) |
@@ -97,6 +100,7 @@ struct mptcp_sock {
u64 write_seq;
u64 ack_seq;
u32 token;
+ unsigned long flags;
struct list_head conn_list;
struct socket *subflow; /* outgoing connect/listener/!mp_capable */
struct pm_data pm;
--
2.22.0
12:59 a.m.
Hi all,
[sorry for lagging behind so much, the relocation trapped me for a
while]
On Wed, 2019-06-26 at 16:11 -0700, Mat Martineau wrote:
The MPTCP-level socket doesn't use sk_receive_queue, so it was
possible
for mptcp_recvmsg() to remain blocked when there was data ready for it
to read. When the MPTCP socket is waiting for additional data and it
releases the subflow socket lock, the subflow may have incoming packets
ready to process and it sometimes called subflow_data_ready() before the
MPTCP socket called sk_wait_data().
This change adds new functions for the MPTCP socket to use to wait and
to signal that data is ready. Atomic bitops are used to set, test, and
clear a MPTCP socket flag that indicates waiting subflow data. This flag
replaces the sk_receive_queue checks used by other socket types.
Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
---
Squashing this into "mptcp: Implement MPTCP receive path" has a few
conflicts with later commits that also fix up the receive code. It's
fine to add this to the end of the commit chain.
net/mptcp/protocol.c | 40 +++++++++++++++++++++++++++++++++++++++-
net/mptcp/protocol.h | 4 ++++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 774ed25d3b6d..5555ee1529bb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -357,6 +357,26 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
return ret;
}
+static void mptcp_wait_data(struct sock *sk, long *timeo)
+{
+ DEFINE_WAIT_FUNC(wait, woken_wake_function);
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
+
+ release_sock(sk);
+
+ if (!test_and_clear_bit(MPTCP_DATA_READY, &msk->flags))
+ *timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
+
+ sched_annotate_sleep();
+ lock_sock(sk);
Can we reuse the sk_wait_event() macro here ? It would avoid a bit of
code duplication.
+
+ sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
+ remove_wait_queue(sk_sleep(sk), &wait);
+}
+
static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
int nonblock, int flags, int *addr_len)
{
@@ -403,6 +423,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t
len,
u64 old_ack;
u32 ssn;
+ clear_bit(MPTCP_DATA_READY, &msk->flags);
status = mptcp_get_mapping(ssk);
if (status == MAPPING_ADDED) {
@@ -536,7 +557,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t
len,
pr_debug("block");
release_sock(ssk);
- sk_wait_data(sk, &timeo, NULL);
+ mptcp_wait_data(sk, &timeo);
lock_sock(ssk);
}
@@ -548,6 +569,22 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t
len,
return copied;
}
+static void mptcp_data_ready(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+ struct socket_wq *wq;
+
+ set_bit(MPTCP_DATA_READY, &msk->flags);
+
+ rcu_read_lock();
+ wq = rcu_dereference(sk->sk_wq);
+ if (skwq_has_sleeper(wq))
+ wake_up_interruptible_sync_poll(&wq->wait, EPOLLIN | EPOLLPRI |
+ EPOLLRDNORM | EPOLLRDBAND);
+ sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
+ rcu_read_unlock();
It looks like the above chunk of code matches quite exactly
sock_def_readable().
To avoid code duplication, what about moving:
set_bit(MPTCP_DATA_READY, &msk->flags);
in subflow_data_ready(), just before calling:
parent->sk_data_ready(parent)
?
If so, we can also drop the following chunk.
BTW, were you able to reproduce the self-test bug with this patch
applied?
Cheers,
Paolo
2:10 a.m.
On Wed, 3 Jul 2019, Paolo Abeni wrote:
Hi all,
[sorry for lagging behind so much, the relocation trapped me for a
while]
On Wed, 2019-06-26 at 16:11 -0700, Mat Martineau wrote:
> The MPTCP-level socket doesn't use sk_receive_queue, so it was possible
> for mptcp_recvmsg() to remain blocked when there was data ready for it
> to read. When the MPTCP socket is waiting for additional data and it
> releases the subflow socket lock, the subflow may have incoming packets
> ready to process and it sometimes called subflow_data_ready() before the
> MPTCP socket called sk_wait_data().
>
> This change adds new functions for the MPTCP socket to use to wait and
> to signal that data is ready. Atomic bitops are used to set, test, and
> clear a MPTCP socket flag that indicates waiting subflow data. This flag
> replaces the sk_receive_queue checks used by other socket types.
>
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
>
> Squashing this into "mptcp: Implement MPTCP receive path" has a few
> conflicts with later commits that also fix up the receive code. It's
> fine to add this to the end of the commit chain.
>
> net/mptcp/protocol.c | 40 +++++++++++++++++++++++++++++++++++++++-
> net/mptcp/protocol.h | 4 ++++
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 774ed25d3b6d..5555ee1529bb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -357,6 +357,26 @@ static enum mapping_status mptcp_get_mapping(struct sock *ssk)
> return ret;
> }
>
> +static void mptcp_wait_data(struct sock *sk, long *timeo)
> +{
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + struct mptcp_sock *msk = mptcp_sk(sk);
> +
> + add_wait_queue(sk_sleep(sk), &wait);
> + sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> +
> + release_sock(sk);
> +
> + if (!test_and_clear_bit(MPTCP_DATA_READY, &msk->flags))
> + *timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
> +
> + sched_annotate_sleep();
> + lock_sock(sk);
Can we reuse the sk_wait_event() macro here ? It would avoid a bit of
code duplication.
My concern there was the second condition check in sk_wait_event() (after
the lock is re-acquired). It looks like normal sk_wait_event() conditions
do not modify anything, but test_and_clear_bit() does change the flag.
sk_wait_event() would call test_and_clear_bit() twice, which it doesn't
need to do and would modify the flag an extra time. While it doesn't break
anything (the flag gets cleared yet again running through the recv loop),
I thought I'd skip the extra atomic write operation.
I can see how that might be premature optimization, let me know if you
still think it's best to reuse sk_wait_event().
> +
> + sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> + remove_wait_queue(sk_sleep(sk), &wait);
> +}
> +
> static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> int nonblock, int flags, int *addr_len)
> {
> @@ -403,6 +423,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg,
size_t len,
> u64 old_ack;
> u32 ssn;
>
> + clear_bit(MPTCP_DATA_READY, &msk->flags);
> status = mptcp_get_mapping(ssk);
>
> if (status == MAPPING_ADDED) {
> @@ -536,7 +557,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg,
size_t len,
>
> pr_debug("block");
> release_sock(ssk);
> - sk_wait_data(sk, &timeo, NULL);
> + mptcp_wait_data(sk, &timeo);
> lock_sock(ssk);
> }
>
> @@ -548,6 +569,22 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg,
size_t len,
> return copied;
> }
>
> +static void mptcp_data_ready(struct sock *sk)
> +{
> + struct mptcp_sock *msk = mptcp_sk(sk);
> + struct socket_wq *wq;
> +
> + set_bit(MPTCP_DATA_READY, &msk->flags);
> +
> + rcu_read_lock();
> + wq = rcu_dereference(sk->sk_wq);
> + if (skwq_has_sleeper(wq))
> + wake_up_interruptible_sync_poll(&wq->wait, EPOLLIN | EPOLLPRI |
> + EPOLLRDNORM | EPOLLRDBAND);
> + sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
> + rcu_read_unlock();
It looks like the above chunk of code matches quite exactly
sock_def_readable().
To avoid code duplication, what about moving:
set_bit(MPTCP_DATA_READY, &msk->flags);
in subflow_data_ready(), just before calling:
parent->sk_data_ready(parent)
?
If so, we can also drop the following chunk.
I had wanted to make sure the set_bit() would happen for all callers of
the MPTCP socket's sk_data_ready(), and sock_def_readable is static within
net/core/sock.c so I couldn't call it without changing its scope.
Looking at other callers of sk_data_ready(), it does look like the only
one relevant for the MPTCP socket is subflow_data_ready() so I will take
your suggestion and place the set_bit() there instead.
BTW, were you able to reproduce the self-test bug with this patch
applied?
I haven't seen that bug lately with or without this patch. I'll try to
reproduce it.
--
Mat Martineau
Intel
9:10 p.m.
On Wed, 2019-07-03 at 11:10 -0700, Mat Martineau wrote:
On Wed, 3 Jul 2019, Paolo Abeni wrote:
> Hi all,
>
> [sorry for lagging behind so much, the relocation trapped me for a
> while]
>
> On Wed, 2019-06-26 at 16:11 -0700, Mat Martineau wrote:
> > The MPTCP-level socket doesn't use sk_receive_queue, so it was possible
> > for mptcp_recvmsg() to remain blocked when there was data ready for it
> > to read. When the MPTCP socket is waiting for additional data and it
> > releases the subflow socket lock, the subflow may have incoming packets
> > ready to process and it sometimes called subflow_data_ready() before the
> > MPTCP socket called sk_wait_data().
> >
> > This change adds new functions for the MPTCP socket to use to wait and
> > to signal that data is ready. Atomic bitops are used to set, test, and
> > clear a MPTCP socket flag that indicates waiting subflow data. This flag
> > replaces the sk_receive_queue checks used by other socket types.
> >
> > Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> > ---
> >
> > Squashing this into "mptcp: Implement MPTCP receive path" has a few
> > conflicts with later commits that also fix up the receive code. It's
> > fine to add this to the end of the commit chain.
> >
> > net/mptcp/protocol.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > net/mptcp/protocol.h | 4 ++++
> > 2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 774ed25d3b6d..5555ee1529bb 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -357,6 +357,26 @@ static enum mapping_status mptcp_get_mapping(struct sock
*ssk)
> > return ret;
> > }
> >
> > +static void mptcp_wait_data(struct sock *sk, long *timeo)
> > +{
> > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > + struct mptcp_sock *msk = mptcp_sk(sk);
> > +
> > + add_wait_queue(sk_sleep(sk), &wait);
> > + sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> > +
> > + release_sock(sk);
> > +
> > + if (!test_and_clear_bit(MPTCP_DATA_READY, &msk->flags))
> > + *timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
> > +
> > + sched_annotate_sleep();
> > + lock_sock(sk);
>
> Can we reuse the sk_wait_event() macro here ? It would avoid a bit of
> code duplication.
My concern there was the second condition check in sk_wait_event() (after
the lock is re-acquired). It looks like normal sk_wait_event() conditions
do not modify anything, but test_and_clear_bit() does change the flag.
sk_wait_event() would call test_and_clear_bit() twice, which it doesn't
need to do and would modify the flag an extra time. While it doesn't break
anything (the flag gets cleared yet again running through the recv loop),
I thought I'd skip the extra atomic write operation.
I think that the performance impact here would be minimal, as an atomic
op that does not update memory and operate on an already hot cacheline
is relatively cheap.
Anyhow I'm a bit concerned about possible races e.g. beween:
+ clear_bit(MPTCP_DATA_READY, &msk->flags);
in recvmsg and:
+ set_bit(MPTCP_DATA_READY, &msk->flags);
in mptcp_data_ready(). If I read the code correctly there arean't any
barriers between the pkt enqueue and set_bit, nor between clear_bit and
the pkt dequeue.
The the cpu is free to re-order e.g. the enqueue write op and the
set_bit op - same on the rx side - and we could end-up racing.
The wait_on_bit APIs use some non trivial barrier() in a similar
scenario.
Unfortunatelly I think we can't use the wait_on_bit() APIs here, and
adding the barriering on top of this code really make me wonder if we
could use some other synchronization APIs instead.
I'd love to ear opinion from others!
Thanks,
Paolo
7:54 a.m.
On Fri, 5 Jul 2019, Paolo Abeni wrote:
On Wed, 2019-07-03 at 11:10 -0700, Mat Martineau wrote:
> On Wed, 3 Jul 2019, Paolo Abeni wrote:
>
>> Hi all,
>>
>> [sorry for lagging behind so much, the relocation trapped me for a
>> while]
>>
>> On Wed, 2019-06-26 at 16:11 -0700, Mat Martineau wrote:
>>> The MPTCP-level socket doesn't use sk_receive_queue, so it was possible
>>> for mptcp_recvmsg() to remain blocked when there was data ready for it
>>> to read. When the MPTCP socket is waiting for additional data and it
>>> releases the subflow socket lock, the subflow may have incoming packets
>>> ready to process and it sometimes called subflow_data_ready() before the
>>> MPTCP socket called sk_wait_data().
>>>
>>> This change adds new functions for the MPTCP socket to use to wait and
>>> to signal that data is ready. Atomic bitops are used to set, test, and
>>> clear a MPTCP socket flag that indicates waiting subflow data. This flag
>>> replaces the sk_receive_queue checks used by other socket types.
>>>
>>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>>> ---
>>>
>>> Squashing this into "mptcp: Implement MPTCP receive path" has a
few
>>> conflicts with later commits that also fix up the receive code. It's
>>> fine to add this to the end of the commit chain.
>>>
>>> net/mptcp/protocol.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>> net/mptcp/protocol.h | 4 ++++
>>> 2 files changed, 43 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 774ed25d3b6d..5555ee1529bb 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -357,6 +357,26 @@ static enum mapping_status mptcp_get_mapping(struct sock
*ssk)
>>> return ret;
>>> }
>>>
>>> +static void mptcp_wait_data(struct sock *sk, long *timeo)
>>> +{
>>> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>> + struct mptcp_sock *msk = mptcp_sk(sk);
>>> +
>>> + add_wait_queue(sk_sleep(sk), &wait);
>>> + sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>>> +
>>> + release_sock(sk);
>>> +
>>> + if (!test_and_clear_bit(MPTCP_DATA_READY, &msk->flags))
>>> + *timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, *timeo);
>>> +
>>> + sched_annotate_sleep();
>>> + lock_sock(sk);
>>
>> Can we reuse the sk_wait_event() macro here ? It would avoid a bit of
>> code duplication.
>
> My concern there was the second condition check in sk_wait_event() (after
> the lock is re-acquired). It looks like normal sk_wait_event() conditions
> do not modify anything, but test_and_clear_bit() does change the flag.
> sk_wait_event() would call test_and_clear_bit() twice, which it doesn't
> need to do and would modify the flag an extra time. While it doesn't break
> anything (the flag gets cleared yet again running through the recv loop),
> I thought I'd skip the extra atomic write operation.
I think that the performance impact here would be minimal, as an atomic
op that does not update memory and operate on an already hot cacheline
is relatively cheap.
Anyhow I'm a bit concerned about possible races e.g. beween:
+ clear_bit(MPTCP_DATA_READY, &msk->flags);
in recvmsg and:
+ set_bit(MPTCP_DATA_READY, &msk->flags);
in mptcp_data_ready(). If I read the code correctly there arean't any
barriers between the pkt enqueue and set_bit, nor between clear_bit and
the pkt dequeue.
The the cpu is free to re-order e.g. the enqueue write op and the
set_bit op - same on the rx side - and we could end-up racing.
The wait_on_bit APIs use some non trivial barrier() in a similar
scenario.
Unfortunatelly I think we can't use the wait_on_bit() APIs here, and
adding the barriering on top of this code really make me wonder if we
could use some other synchronization APIs instead.
I'd love to ear opinion from others!
After looking over the regular socket wakeup code and the atomic bitops
(and associated barrier issues), I think the root problem is calling the
MPTCP socket's sk_data_ready() without holding the associated lock and
using the memory barriers that are part of locking & unlocking. The
subflow code that's in our tree right now is broken for both this reason
and for the issue I tried to handle in this patch (sk_receive_queue being
unused).
Can we get anywhere by using bh_lock_sock() to protect the flag? Might be
worthwhile to do a slow-but-correct fix and then figure out something more
efficient.
--
Mat Martineau
Intel
9:07 p.m.
Hi,
On Mon, 2019-07-08 at 16:54 -0700, Mat Martineau wrote:
Can we get anywhere by using bh_lock_sock() to protect the flag?
Might be
worthwhile to do a slow-but-correct fix and then figure out something more
efficient.
I'm sorry for lagging behind this. I think it should work. Also the
sk_stream_write_space()/sk_stream_wait_memory() functions are possibly
a good reference code (even if it acquires/relies on the full socket
lock).
Another (crazy???) alternative I'm wondering about is changing a bit
the overall locking schema using an intermediate work/timer/whatever
between subflow->data_ready() and the msk recvmsg(). This new function
can and will acquire the msk and subflow locks in the correct order
(msk lock -> subflow lock) and really enqueue the skb into msk receive
queue, so that finally we could use the plain sk_data_ready() function
to wake-up the reader.
I *think* the above implementation, even if more convoluted than the
present one and possibly with some latency problem, could help dealing
in the future with multiple subflows.
WDYT?
Cheers,
Paolo
8:47 a.m.
On Wed, 17 Jul 2019, Paolo Abeni wrote:
Hi,
On Mon, 2019-07-08 at 16:54 -0700, Mat Martineau wrote:
> Can we get anywhere by using bh_lock_sock() to protect the flag? Might be
> worthwhile to do a slow-but-correct fix and then figure out something more
> efficient.
I'm sorry for lagging behind this. I think it should work. Also the
sk_stream_write_space()/sk_stream_wait_memory() functions are possibly
a good reference code (even if it acquires/relies on the full socket
lock).
Ok, I'm trying that out.
Another (crazy???) alternative I'm wondering about is changing a bit
the overall locking schema using an intermediate work/timer/whatever
between subflow->data_ready() and the msk recvmsg(). This new function
can and will acquire the msk and subflow locks in the correct order
(msk lock -> subflow lock) and really enqueue the skb into msk receive
queue, so that finally we could use the plain sk_data_ready() function
to wake-up the reader.
I *think* the above implementation, even if more convoluted than the
present one and possibly with some latency problem, could help dealing
in the future with multiple subflows.
WDYT?
It's not only a matter of enqueuing an skb in the msk receive queue, it
would be necessary to enqueue the right data bytes for an entire mapped
segment as defined by the DSS. However, it may work to use an intermediate
step just to signal the wakeup - I need to think about that some more.
--
Mat Martineau
Intel
552
days inactive
581
days old
6 comments
2 participants
participants (2)
-
Mat Martineau -
Paolo Abeni