All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tcp: provide tx timestamps for partial writes
@ 2017-01-02 20:20 Soheil Hassas Yeganeh
  2017-01-02 20:23 ` Soheil Hassas Yeganeh
  2017-01-03 21:21 ` Yuchung Cheng
  0 siblings, 2 replies; 6+ messages in thread
From: Soheil Hassas Yeganeh @ 2017-01-02 20:20 UTC (permalink / raw)
  To: davem, netdev
  Cc: Soheil Hassas Yeganeh, Willem de Bruijn, Yuchung Cheng,
	Eric Dumazet, Neal Cardwell, Martin KaFai Lau

From: Soheil Hassas Yeganeh <soheil@google.com>

For TCP sockets, tx timestamps are only captured when the user data
is successfully and fully written to the socket. In many cases,
however, TCP writes can be partial for which no timestamp is
collected.

Collect timestamps when the user data is partially copied into
the socket.

Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Martin KaFai Lau <kafai@fb.com>
---
 net/ipv4/tcp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2e3807d..c207b16 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -992,8 +992,10 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 	return copied;
 
 do_error:
-	if (copied)
+	if (copied) {
+		tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
 		goto out;
+	}
 out_err:
 	/* make sure we wake any epoll edge trigger waiter */
 	if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&
@@ -1329,8 +1331,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	}
 
 do_error:
-	if (copied + copied_syn)
+	if (copied + copied_syn) {
+		tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
 		goto out;
+	}
 out_err:
 	err = sk_stream_error(sk, flags, err);
 	/* make sure we wake any epoll edge trigger waiter */
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: provide tx timestamps for partial writes
  2017-01-02 20:20 [PATCH] tcp: provide tx timestamps for partial writes Soheil Hassas Yeganeh
@ 2017-01-02 20:23 ` Soheil Hassas Yeganeh
  2017-01-03 15:22   ` Soheil Hassas Yeganeh
  2017-01-03 21:21 ` Yuchung Cheng
  1 sibling, 1 reply; 6+ messages in thread
From: Soheil Hassas Yeganeh @ 2017-01-02 20:23 UTC (permalink / raw)
  To: David Miller, netdev
  Cc: Willem de Bruijn, Yuchung Cheng, Eric Dumazet, Neal Cardwell,
	Martin KaFai Lau

On Mon, Jan 2, 2017 at 3:20 PM, Soheil Hassas Yeganeh
<soheil.kdev@gmail.com> wrote:
> From: Soheil Hassas Yeganeh <soheil@google.com>
>
> For TCP sockets, tx timestamps are only captured when the user data
> is successfully and fully written to the socket. In many cases,
> however, TCP writes can be partial for which no timestamp is
> collected.
>
> Collect timestamps when the user data is partially copied into
> the socket.
>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> ---
>  net/ipv4/tcp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 2e3807d..c207b16 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -992,8 +992,10 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>         return copied;
>
>  do_error:
> -       if (copied)
> +       if (copied) {
> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
>                 goto out;
> +       }
>  out_err:
>         /* make sure we wake any epoll edge trigger waiter */
>         if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&
> @@ -1329,8 +1331,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>         }
>
>  do_error:
> -       if (copied + copied_syn)
> +       if (copied + copied_syn) {
> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
>                 goto out;
> +       }
>  out_err:
>         err = sk_stream_error(sk, flags, err);
>         /* make sure we wake any epoll edge trigger waiter */
> --
> 2.8.0.rc3.226.g39d4020
>

I'm sorry for the incomplete annotation. This is for [net-next].

Thanks,
Soheil

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: provide tx timestamps for partial writes
  2017-01-02 20:23 ` Soheil Hassas Yeganeh
@ 2017-01-03 15:22   ` Soheil Hassas Yeganeh
  2017-01-04 12:55     ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Soheil Hassas Yeganeh @ 2017-01-03 15:22 UTC (permalink / raw)
  To: David Miller, netdev
  Cc: Willem de Bruijn, Yuchung Cheng, Eric Dumazet, Neal Cardwell,
	Martin KaFai Lau

On Mon, Jan 2, 2017 at 3:23 PM, Soheil Hassas Yeganeh <soheil@google.com> wrote:
> On Mon, Jan 2, 2017 at 3:20 PM, Soheil Hassas Yeganeh
> <soheil.kdev@gmail.com> wrote:
>> From: Soheil Hassas Yeganeh <soheil@google.com>
>>
>> For TCP sockets, tx timestamps are only captured when the user data
>> is successfully and fully written to the socket. In many cases,
>> however, TCP writes can be partial for which no timestamp is
>> collected.
>>
>> Collect timestamps when the user data is partially copied into
>> the socket.
>>
>> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Yuchung Cheng <ycheng@google.com>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Neal Cardwell <ncardwell@google.com>
>> Cc: Martin KaFai Lau <kafai@fb.com>
>> ---
>>  net/ipv4/tcp.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 2e3807d..c207b16 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -992,8 +992,10 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>>         return copied;
>>
>>  do_error:
>> -       if (copied)
>> +       if (copied) {
>> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
>>                 goto out;
>> +       }
>>  out_err:
>>         /* make sure we wake any epoll edge trigger waiter */
>>         if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&
>> @@ -1329,8 +1331,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>>         }
>>
>>  do_error:
>> -       if (copied + copied_syn)
>> +       if (copied + copied_syn) {
>> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));

Thanks to Willem for noting that this should be sockc.tsflags and not
sk->sk_tsflags. I'll send V2 to fix.

Soheil

>>                 goto out;
>> +       }
>>  out_err:
>>         err = sk_stream_error(sk, flags, err);
>>         /* make sure we wake any epoll edge trigger waiter */
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>
> I'm sorry for the incomplete annotation. This is for [net-next].
>
> Thanks,
> Soheil

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: provide tx timestamps for partial writes
  2017-01-02 20:20 [PATCH] tcp: provide tx timestamps for partial writes Soheil Hassas Yeganeh
  2017-01-02 20:23 ` Soheil Hassas Yeganeh
@ 2017-01-03 21:21 ` Yuchung Cheng
  1 sibling, 0 replies; 6+ messages in thread
From: Yuchung Cheng @ 2017-01-03 21:21 UTC (permalink / raw)
  To: Soheil Hassas Yeganeh
  Cc: David Miller, netdev, Soheil Hassas Yeganeh, Willem de Bruijn,
	Eric Dumazet, Neal Cardwell, Martin KaFai Lau

On Mon, Jan 2, 2017 at 12:20 PM, Soheil Hassas Yeganeh
<soheil.kdev@gmail.com> wrote:
>
> From: Soheil Hassas Yeganeh <soheil@google.com>
>
> For TCP sockets, tx timestamps are only captured when the user data
> is successfully and fully written to the socket. In many cases,
> however, TCP writes can be partial for which no timestamp is
> collected.
>
> Collect timestamps when the user data is partially copied into
> the socket.
>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> ---
Acked-by: Yuchung Cheng <ycheng@google.com>

>  net/ipv4/tcp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 2e3807d..c207b16 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -992,8 +992,10 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>         return copied;
>
>  do_error:
> -       if (copied)
> +       if (copied) {
> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
>                 goto out;
> +       }
>  out_err:
>         /* make sure we wake any epoll edge trigger waiter */
>         if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&
> @@ -1329,8 +1331,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>         }
>
>  do_error:
> -       if (copied + copied_syn)
> +       if (copied + copied_syn) {
> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
>                 goto out;
> +       }
>  out_err:
>         err = sk_stream_error(sk, flags, err);
>         /* make sure we wake any epoll edge trigger waiter */
> --
> 2.8.0.rc3.226.g39d4020
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: provide tx timestamps for partial writes
  2017-01-03 15:22   ` Soheil Hassas Yeganeh
@ 2017-01-04 12:55     ` Eric Dumazet
  2017-01-04 15:04       ` Soheil Hassas Yeganeh
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-01-04 12:55 UTC (permalink / raw)
  To: Soheil Hassas Yeganeh
  Cc: David Miller, netdev, Willem de Bruijn, Yuchung Cheng,
	Eric Dumazet, Neal Cardwell, Martin KaFai Lau

On Tue, 2017-01-03 at 10:22 -0500, Soheil Hassas Yeganeh wrote:
> On Mon, Jan 2, 2017 at 3:23 PM, Soheil Hassas Yeganeh <soheil@google.com> wrote:
> > On Mon, Jan 2, 2017 at 3:20 PM, Soheil Hassas Yeganeh
> > <soheil.kdev@gmail.com> wrote:
> >> From: Soheil Hassas Yeganeh <soheil@google.com>
> >>
> >> For TCP sockets, tx timestamps are only captured when the user data
> >> is successfully and fully written to the socket. In many cases,
> >> however, TCP writes can be partial for which no timestamp is
> >> collected.
> >>
> >> Collect timestamps when the user data is partially copied into
> >> the socket.
> >>
> >> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> >> Cc: Willem de Bruijn <willemb@google.com>
> >> Cc: Yuchung Cheng <ycheng@google.com>
> >> Cc: Eric Dumazet <edumazet@google.com>
> >> Cc: Neal Cardwell <ncardwell@google.com>
> >> Cc: Martin KaFai Lau <kafai@fb.com>
> >> ---
> >>  net/ipv4/tcp.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> >> index 2e3807d..c207b16 100644
> >> --- a/net/ipv4/tcp.c
> >> +++ b/net/ipv4/tcp.c
> >> @@ -992,8 +992,10 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
> >>         return copied;
> >>
> >>  do_error:
> >> -       if (copied)
> >> +       if (copied) {
> >> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
> >>                 goto out;
> >> +       }
> >>  out_err:
> >>         /* make sure we wake any epoll edge trigger waiter */
> >>         if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&
> >> @@ -1329,8 +1331,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >>         }
> >>
> >>  do_error:
> >> -       if (copied + copied_syn)
> >> +       if (copied + copied_syn) {
> >> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
> 
> Thanks to Willem for noting that this should be sockc.tsflags and not
> sk->sk_tsflags. I'll send V2 to fix.

Also, why not factorizing a bit and have a single point calling
tcp_tx_timestamp() ?

This would ease code review quite a bit.

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4a044964da6670829e5c47fef52d2cd76360b59f..11357f3bd1f82fa29129dd3ecf4d270feb4a6b1d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -958,10 +958,8 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 		copied += copy;
 		offset += copy;
 		size -= copy;
-		if (!size) {
-			tcp_tx_timestamp(sk, sk->sk_tsflags, skb);
+		if (!size)
 			goto out;
-		}
 
 		if (skb->len < size_goal || (flags & MSG_OOB))
 			continue;
@@ -987,8 +985,11 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 	}
 
 out:
-	if (copied && !(flags & MSG_SENDPAGE_NOTLAST))
-		tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
+	if (copied) {
+		tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
+		if (!(flags & MSG_SENDPAGE_NOTLAST))
+			tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
+	}
 	return copied;
 
 do_error:
@@ -1281,7 +1282,6 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 
 		copied += copy;
 		if (!msg_data_left(msg)) {
-			tcp_tx_timestamp(sk, sockc.tsflags, skb);
 			if (unlikely(flags & MSG_EOR))
 				TCP_SKB_CB(skb)->eor = 1;
 			goto out;
@@ -1312,8 +1312,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	}
 
 out:
-	if (copied)
+	if (copied) {
+		tcp_tx_timestamp(sk, sockc.tsflags, tcp_write_queue_tail(sk));
 		tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
+	}
 out_nopush:
 	release_sock(sk);
 	return copied + copied_syn;

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: provide tx timestamps for partial writes
  2017-01-04 12:55     ` Eric Dumazet
@ 2017-01-04 15:04       ` Soheil Hassas Yeganeh
  0 siblings, 0 replies; 6+ messages in thread
From: Soheil Hassas Yeganeh @ 2017-01-04 15:04 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, netdev, Willem de Bruijn, Yuchung Cheng,
	Eric Dumazet, Neal Cardwell, Martin KaFai Lau

On Wed, Jan 4, 2017 at 7:55 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> On Tue, 2017-01-03 at 10:22 -0500, Soheil Hassas Yeganeh wrote:
> > On Mon, Jan 2, 2017 at 3:23 PM, Soheil Hassas Yeganeh <soheil@google.com> wrote:
> > > On Mon, Jan 2, 2017 at 3:20 PM, Soheil Hassas Yeganeh
> > > <soheil.kdev@gmail.com> wrote:
> > >> From: Soheil Hassas Yeganeh <soheil@google.com>
> > >>
> > >> For TCP sockets, tx timestamps are only captured when the user data
> > >> is successfully and fully written to the socket. In many cases,
> > >> however, TCP writes can be partial for which no timestamp is
> > >> collected.
> > >>
> > >> Collect timestamps when the user data is partially copied into
> > >> the socket.
> > >>
> > >> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> > >> Cc: Willem de Bruijn <willemb@google.com>
> > >> Cc: Yuchung Cheng <ycheng@google.com>
> > >> Cc: Eric Dumazet <edumazet@google.com>
> > >> Cc: Neal Cardwell <ncardwell@google.com>
> > >> Cc: Martin KaFai Lau <kafai@fb.com>
> > >> ---
> > >>  net/ipv4/tcp.c | 8 ++++++--
> > >>  1 file changed, 6 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > >> index 2e3807d..c207b16 100644
> > >> --- a/net/ipv4/tcp.c
> > >> +++ b/net/ipv4/tcp.c
> > >> @@ -992,8 +992,10 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
> > >>         return copied;
> > >>
> > >>  do_error:
> > >> -       if (copied)
> > >> +       if (copied) {
> > >> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
> > >>                 goto out;
> > >> +       }
> > >>  out_err:
> > >>         /* make sure we wake any epoll edge trigger waiter */
> > >>         if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&
> > >> @@ -1329,8 +1331,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >>         }
> > >>
> > >>  do_error:
> > >> -       if (copied + copied_syn)
> > >> +       if (copied + copied_syn) {
> > >> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
> >
> > Thanks to Willem for noting that this should be sockc.tsflags and not
> > sk->sk_tsflags. I'll send V2 to fix.
>
> Also, why not factorizing a bit and have a single point calling
> tcp_tx_timestamp() ?
>
> This would ease code review quite a bit.

Thanks Eric! will do in V2.

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 4a044964da6670829e5c47fef52d2cd76360b59f..11357f3bd1f82fa29129dd3ecf4d270feb4a6b1d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -958,10 +958,8 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>                 copied += copy;
>                 offset += copy;
>                 size -= copy;
> -               if (!size) {
> -                       tcp_tx_timestamp(sk, sk->sk_tsflags, skb);
> +               if (!size)
>                         goto out;
> -               }
>
>                 if (skb->len < size_goal || (flags & MSG_OOB))
>                         continue;
> @@ -987,8 +985,11 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>         }
>
>  out:
> -       if (copied && !(flags & MSG_SENDPAGE_NOTLAST))
> -               tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
> +       if (copied) {
> +               tcp_tx_timestamp(sk, sk->sk_tsflags, tcp_write_queue_tail(sk));
> +               if (!(flags & MSG_SENDPAGE_NOTLAST))
> +                       tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
> +       }
>         return copied;
>
>  do_error:
> @@ -1281,7 +1282,6 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>
>                 copied += copy;
>                 if (!msg_data_left(msg)) {
> -                       tcp_tx_timestamp(sk, sockc.tsflags, skb);
>                         if (unlikely(flags & MSG_EOR))
>                                 TCP_SKB_CB(skb)->eor = 1;
>                         goto out;
> @@ -1312,8 +1312,10 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>         }
>
>  out:
> -       if (copied)
> +       if (copied) {
> +               tcp_tx_timestamp(sk, sockc.tsflags, tcp_write_queue_tail(sk));
>                 tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
> +       }
>  out_nopush:
>         release_sock(sk);
>         return copied + copied_syn;
>
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-04 15:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-02 20:20 [PATCH] tcp: provide tx timestamps for partial writes Soheil Hassas Yeganeh
2017-01-02 20:23 ` Soheil Hassas Yeganeh
2017-01-03 15:22   ` Soheil Hassas Yeganeh
2017-01-04 12:55     ` Eric Dumazet
2017-01-04 15:04       ` Soheil Hassas Yeganeh
2017-01-03 21:21 ` Yuchung Cheng

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.