* [MPTCP] Re: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
@ 2019-10-30 15:58 Florian Westphal
0 siblings, 0 replies; 3+ messages in thread
From: Florian Westphal @ 2019-10-30 15:58 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]
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);
...
?
^ permalink raw reply [flat|nested] 3+ messages in thread* [MPTCP] Re: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
@ 2019-10-30 17:21 Florian Westphal
0 siblings, 0 replies; 3+ messages in thread
From: Florian Westphal @ 2019-10-30 17:21 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2371 bytes --]
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* [MPTCP] Re: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed
@ 2019-10-30 15:14 Paolo Abeni
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2019-10-30 15:14 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-30 17:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-30 15:58 [MPTCP] Re: [PATCH v2 2/2] mptcp: sendmsg: transmit on backup if other subflows have been closed Florian Westphal
-- strict thread matches above, loose matches on Subject: below --
2019-10-30 17:21 Florian Westphal
2019-10-30 15:14 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.