2:03 p.m.
This is a simplified version of a sendmsg ssk selection strategy,
so sendmsg or retransmit could also occur on a backup subflow.
In the initial submission it was pointed out that we should not
send more data when a retransmission is happening.
As this isn't directly related to this I've decided to send those two
patches now and work on sendmsg moderation next.
The plan is to add wmem_queued accounting to the mptcp socket and
then make sendmsg/mptcp_poll consider mptcp socket wmem limits to
avoid sending more data if there are no mptcp-level acknowledgements.
This however might take some time because msk->ack_seq update occurs only
at recvmsg time and not during mptcp ack processing.
For this to work properly, mptcp-level ack update should not depend on
a recvmsg call.
The following changes since commit dcb69525165804b78e07ed7875d33463a3fde7c8:
subflow: wake parent mptcp socket on subflow state change (2019-10-30 02:24:34 +0000)
are available in the Git repository at:
git://git.breakpoint.cc/fw/mptcp-next.git sendmsg_refactor_19
for you to fetch changes up to a1cb9992e1f55156914dc0e6727f92463fb96938:
mptcp: sendmsg: transmit on backup if other subflows have been closed (2019-10-30
14:42:48 +0100)
----------------------------------------------------------------
Florian Westphal (2):
mptcp: add and use mptcp_subflow_get_retrans
mptcp: sendmsg: transmit on backup if other subflows have been closed
net/mptcp/protocol.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++-----
net/mptcp/protocol.h | 1 +
net/mptcp/subflow.c | 7 +++-
3 files changed, 100 insertions(+), 10 deletions(-)
2:03 p.m.
New subject: [PATCH v2 1/2] mptcp: add and use mptcp_subflow_get_retrans
Instead of having retransmit worker grab first subflow on the list,
make it always return NULL, unless either the first non-backup subflow
on the list is idle or all normal subflows have already been removed.
In the latter case, the first idle backup subflow is used.
Rationale is that it makes no sense to attempt to retransmit
at mptcp level if there is still unsent data in its write queue.
V2: always return NULL when first non-idle ssk is seen.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c2b29f8d0377..d15d64d16136 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -626,6 +626,37 @@ static void mptcp_retransmit_timer(struct timer_list *t)
sock_put(sk);
}
+/* Find an idle subflow. Return NULL if there is unacked data at tcp
+ * level.
+ *
+ * A backup subflow is returned only if thats the only kind available.
+ */
+static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct sock *backup = NULL;
+
+ sock_owned_by_me((const struct sock *)msk);
+
+ mptcp_for_each_subflow(msk, subflow) {
+ struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
+
+ /* still data outstanding at TCP level? Don't retransmit. */
+ if (!tcp_write_queue_empty(ssk))
+ return NULL;
+
+ if (subflow->backup) {
+ if (!backup)
+ backup = ssk;
+ continue;
+ }
+
+ return ssk;
+ }
+
+ return backup;
+}
+
static void mptcp_retransmit(struct work_struct *work)
{
int orig_len, orig_offset, ret, mss_now = 0, size_goal = 0;
@@ -646,7 +677,7 @@ static void mptcp_retransmit(struct work_struct *work)
if (!dfrag)
goto unlock;
- ssk = mptcp_subflow_get(msk);
+ ssk = mptcp_subflow_get_retrans(msk);
if (!ssk)
goto reset_unlock;
--
2.23.0
2:03 p.m.
New subject: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
Currently we always pick the first ssk on the list and then have
mptcp_sendmsg_frag wait until more space becomes available in case that
ssk has no write space available.
Instead check the first subflow on the list. If no more write space
is available, then we need to either return -EAGAIN to userspace (nonblock
case), or we need to wait until a subflow becomes available.
This is done by blocking the current thread via sk_stream_wait_memory()
and then make the subflow sk_write_space() unblock the parent mptcp socket.
We can't acquire the mptcp socket lock from the subflow callbacks, but
we can use the mptcp->flags to for signalling. MPTCP_SEND_SPACE flag is
added for this purpose. If it gets set, then at least one subflow has
become available for writing.
v2: dumb-down the selection: just pick the first ssk on the list and make
mptcp socket block if it has no wspace.
Backup is only used if no non-backup subflow exists.
Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
net/mptcp/protocol.c | 69 +++++++++++++++++++++++++++++++++++++++-----
net/mptcp/protocol.h | 1 +
net/mptcp/subflow.c | 7 ++++-
3 files changed, 68 insertions(+), 9 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d15d64d16136..2b847e079619 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -344,6 +344,38 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
return ret;
}
+static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct sock *backup = NULL;
+
+ sock_owned_by_me((const struct sock *)msk);
+
+ mptcp_for_each_subflow(msk, subflow) {
+ struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
+
+ if (!sk_stream_is_writeable(ssk)) {
+ struct socket *sock = ssk->sk_socket;
+
+ if (sock)
+ set_bit(SOCK_NOSPACE, &sock->flags);
+
+ return NULL;
+ }
+
+ if (subflow->backup) {
+ if (!backup)
+ backup = ssk;
+
+ continue;
+ }
+
+ return ssk;
+ }
+
+ return backup;
+}
+
static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
{
int mss_now = 0, size_goal = 0, ret = 0;
@@ -366,23 +398,36 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t
len)
return ret;
}
- ssk = mptcp_subflow_get(msk);
- if (!ssk) {
- release_sock(sk);
- return -ENOTCONN;
- }
-
- if (!msg_data_left(msg)) {
+ if (unlikely(!msg_data_left(msg))) {
+ ssk = mptcp_subflow_get(msk);
pr_debug("empty send");
ret = sock_sendmsg(ssk->sk_socket, msg);
goto out;
}
+ timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
+
+ smp_mb__before_atomic();
+ clear_bit(MPTCP_SEND_SPACE, &msk->flags);
+ smp_mb__after_atomic();
+
+ ssk = mptcp_subflow_get_send(msk);
+ while (!ssk) {
+ ret = sk_stream_wait_memory(sk, &timeo);
+ if (ret)
+ goto out;
+
+ ssk = mptcp_subflow_get_send(msk);
+ if (list_empty(&msk->conn_list)) {
+ ret = -ENOTCONN;
+ goto out;
+ }
+ }
+
pr_debug("conn_list->subflow=%p", ssk);
lock_sock(ssk);
mptcp_clean_una(sk);
- timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
while (msg_data_left(msg)) {
ret = mptcp_sendmsg_frag(sk, ssk, msg, NULL, &timeo, &mss_now,
&size_goal);
@@ -1077,6 +1122,13 @@ bool mptcp_sk_is_subflow(const struct sock *sk)
return subflow->mp_join == 1;
}
+static bool mptcp_memory_free(const struct sock *sk, int wake)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+
+ return test_bit(MPTCP_SEND_SPACE, &msk->flags);
+}
+
static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
@@ -1097,6 +1149,7 @@ static struct proto mptcp_prot = {
.sockets_allocated = &mptcp_sockets_allocated,
.memory_allocated = &tcp_memory_allocated,
.memory_pressure = &tcp_memory_pressure,
+ .stream_memory_free = mptcp_memory_free,
.sysctl_wmem_offset = offsetof(struct net, ipv4.sysctl_tcp_wmem),
.sysctl_mem = sysctl_tcp_mem,
.obj_size = sizeof(struct mptcp_sock),
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 791f2c19cfb8..4b7399efc8bb 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -76,6 +76,7 @@
/* MPTCP socket flags */
#define MPTCP_DATA_READY BIT(0)
+#define MPTCP_SEND_SPACE BIT(1)
static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
{
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 7bae7c35ea6b..150f0da8b0f0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -616,8 +616,13 @@ static void subflow_write_space(struct sock *sk)
struct sock *parent = subflow->conn;
sk_stream_write_space(sk);
- if (parent)
+ if (parent) {
+ smp_mb__before_atomic();
+ set_bit(MPTCP_SEND_SPACE, &mptcp_sk(parent)->flags);
+ smp_mb__after_atomic();
+
sk_stream_write_space(parent);
+ }
}
int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local,
--
2.23.0
3:14 p.m.
New subject: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
Hi,
On Wed, 2019-10-30 at 15:03 +0100, Florian Westphal wrote:
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d15d64d16136..2b847e079619 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -344,6 +344,38 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
return ret;
}
+static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct sock *backup = NULL;
+
+ sock_owned_by_me((const struct sock *)msk);
+
+ mptcp_for_each_subflow(msk, subflow) {
+ struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
+
+ if (!sk_stream_is_writeable(ssk)) {
+ struct socket *sock = ssk->sk_socket;
+
+ if (sock)
+ set_bit(SOCK_NOSPACE, &sock->flags);
Out of sheer ignorance, why is this required? I thought setting NOSPACE
was required only before waiting for memory.
[...]
@@ -366,23 +398,36 @@ static int mptcp_sendmsg(struct sock *sk,
struct msghdr *msg, size_t len)
return ret;
}
- ssk = mptcp_subflow_get(msk);
- if (!ssk) {
- release_sock(sk);
- return -ENOTCONN;
- }
-
- if (!msg_data_left(msg)) {
+ if (unlikely(!msg_data_left(msg))) {
+ ssk = mptcp_subflow_get(msk);
pr_debug("empty send");
ret = sock_sendmsg(ssk->sk_socket, msg);
goto out;
}
+ timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
+
+ smp_mb__before_atomic();
+ clear_bit(MPTCP_SEND_SPACE, &msk->flags);
+ smp_mb__after_atomic();
Perhaps the above lines could be moved just before the
sk_stream_wait_memory() call below?
Cheers,
Paolo
3:58 p.m.
New subject: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
Paolo Abeni <pabeni(a)redhat.com> wrote:
On Wed, 2019-10-30 at 15:03 +0100, Florian Westphal wrote:
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d15d64d16136..2b847e079619 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -344,6 +344,38 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock
*ssk,
> return ret;
> }
>
> +static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> +{
> + struct mptcp_subflow_context *subflow;
> + struct sock *backup = NULL;
> +
> + sock_owned_by_me((const struct sock *)msk);
> +
> + mptcp_for_each_subflow(msk, subflow) {
> + struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
> +
> + if (!sk_stream_is_writeable(ssk)) {
> + struct socket *sock = ssk->sk_socket;
> +
> + if (sock)
> + set_bit(SOCK_NOSPACE, &sock->flags);
Out of sheer ignorance, why is this required? I thought setting NOSPACE
was required only before waiting for memory.
Its needed for tcp to call sk->sk_write_space() once the substream
becomes writeable.
> - if (!msg_data_left(msg)) {
> + if (unlikely(!msg_data_left(msg))) {
> + ssk = mptcp_subflow_get(msk);
> pr_debug("empty send");
> ret = sock_sendmsg(ssk->sk_socket, msg);
> goto out;
> }
>
> + timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> +
> + smp_mb__before_atomic();
> + clear_bit(MPTCP_SEND_SPACE, &msk->flags);
> + smp_mb__after_atomic();
Perhaps the above lines could be moved just before the
sk_stream_wait_memory() call below?
you mean
while (!ssk) {
clear_bit(MPTCP_SEND_SPACE, &msk->flags);
ret = sk_stream_wait_memory(sk, &timeo);
...
?
5:21 p.m.
New subject: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
Florian Westphal <fw(a)strlen.de> wrote:
Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Wed, 2019-10-30 at 15:03 +0100, Florian Westphal wrote:
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index d15d64d16136..2b847e079619 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -344,6 +344,38 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock
*ssk,
> > return ret;
> > }
> >
> > +static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> > +{
> > + struct mptcp_subflow_context *subflow;
> > + struct sock *backup = NULL;
> > +
> > + sock_owned_by_me((const struct sock *)msk);
> > +
> > + mptcp_for_each_subflow(msk, subflow) {
> > + struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
> > +
> > + if (!sk_stream_is_writeable(ssk)) {
> > + struct socket *sock = ssk->sk_socket;
> > +
> > + if (sock)
> > + set_bit(SOCK_NOSPACE, &sock->flags);
>
> Out of sheer ignorance, why is this required? I thought setting NOSPACE
> was required only before waiting for memory.
Its needed for tcp to call sk->sk_write_space() once the substream
becomes writeable.
> > - if (!msg_data_left(msg)) {
> > + if (unlikely(!msg_data_left(msg))) {
> > + ssk = mptcp_subflow_get(msk);
> > pr_debug("empty send");
> > ret = sock_sendmsg(ssk->sk_socket, msg);
> > goto out;
> > }
> >
> > + timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> > +
> > + smp_mb__before_atomic();
> > + clear_bit(MPTCP_SEND_SPACE, &msk->flags);
> > + smp_mb__after_atomic();
>
> Perhaps the above lines could be moved just before the
> sk_stream_wait_memory() call below?
you mean
while (!ssk) {
clear_bit(MPTCP_SEND_SPACE, &msk->flags);
ret = sk_stream_wait_memory(sk, &timeo);
...
?
Paolo, what about this instead:
mptcp_for_each_subflow(msk, subflow) {
struct sock *ssk = mptcp_subflow_tcp_socket(subflow)->sk;
if (!sk_stream_is_writeable(ssk)) {
struct socket *sock = ssk->sk_socket;
clear_bit(MPTCP_SEND_SPACE, &msk->flags);
if (sock)
set_bit(SOCK_NOSPACE, &sock->flags);
i.e., clear it only when we the ssk isn't writeable anymore.
I agree that the unconditional clear_bit() is not needed.
11:17 p.m.
On Wed, 30 Oct 2019, Florian Westphal wrote:
This is a simplified version of a sendmsg ssk selection strategy,
so sendmsg or retransmit could also occur on a backup subflow.
In the initial submission it was pointed out that we should not
send more data when a retransmission is happening.
I think I had a concern about starving the retransmit worker if userspace
kept sending data, but on further thought it seems like the retransmit
worker will get a chance to send eventually. Once the non-retransmit path
in mptcp_sendmsg_frag() has to wait for memory due to unacked MPTCP-level
data, the ssk transmit queue will empty out and retransmission will
proceed.
While waiting for the queue to be completely empty at retransmit timeout
time may lead to idling the link, it should not get permanently stuck,
right? Or does the MPTCP-level accounting not work that way yet (given
what you're planning with wmem_queued accounting)?
As this isn't directly related to this I've decided to send
those two
patches now and work on sendmsg moderation next.
The plan is to add wmem_queued accounting to the mptcp socket and
then make sendmsg/mptcp_poll consider mptcp socket wmem limits to
avoid sending more data if there are no mptcp-level acknowledgements.
This however might take some time because msk->ack_seq update occurs only
at recvmsg time and not during mptcp ack processing.
For this to work properly, mptcp-level ack update should not depend on
a recvmsg call.
This sounds like a good plan.
--
Mat Martineau
Intel
11:27 p.m.
On Wed, 30 Oct 2019, Mat Martineau wrote:
On Wed, 30 Oct 2019, Florian Westphal wrote:
> This is a simplified version of a sendmsg ssk selection strategy,
> so sendmsg or retransmit could also occur on a backup subflow.
>
> In the initial submission it was pointed out that we should not
> send more data when a retransmission is happening.
>
I think I had a concern about starving the retransmit worker if userspace
kept sending data, but on further thought it seems like the retransmit worker
will get a chance to send eventually. Once the non-retransmit path in
mptcp_sendmsg_frag() has to wait for memory due to unacked MPTCP-level data,
the ssk transmit queue will empty out and retransmission will proceed.
While waiting for the queue to be completely empty at retransmit timeout time
may lead to idling the link, it should not get permanently stuck, right? Or
does the MPTCP-level accounting not work that way yet (given what you're
planning with wmem_queued accounting)?
To answer my own question: I should have taken a closer look at
mptcp_sendmsg_frag before hitting 'send', it does look like we aren't
doing that kind of MPTCP-level accounting yet. We might end up using a lot
of memory if userspace keeps sending.
> As this isn't directly related to this I've decided to send those two
> patches now and work on sendmsg moderation next.
>
> The plan is to add wmem_queued accounting to the mptcp socket and
> then make sendmsg/mptcp_poll consider mptcp socket wmem limits to
> avoid sending more data if there are no mptcp-level acknowledgements.
>
> This however might take some time because msk->ack_seq update occurs only
> at recvmsg time and not during mptcp ack processing.
> For this to work properly, mptcp-level ack update should not depend on
> a recvmsg call.
>
This sounds like a good plan.
--
Mat Martineau
Intel
11:35 p.m.
Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
To answer my own question: I should have taken a closer look at
mptcp_sendmsg_frag before hitting 'send', it does look like we aren't doing
that kind of MPTCP-level accounting yet. We might end up using a lot of
memory if userspace keeps sending.
Yes, working on it.
971
days inactive
971
days old
8 comments
3 participants
participants (3)
-
Florian Westphal -
Mat Martineau -
Paolo Abeni