* [PATCH net-next] tcp: tcp_transmit_skb() optimizations
@ 2013-10-10 15:43 Eric Dumazet
2013-10-10 15:54 ` Yuchung Cheng
2013-10-11 21:48 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2013-10-10 15:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, MF Nowlan, Yuchung Cheng, Neal Cardwell
From: Eric Dumazet <edumazet@google.com>
1) We need to take a timestamp only for skb that should be cloned.
Other skbs are not in write queue and no rtt estimation is done on them.
2) the unlikely() hint is wrong for receivers (they send pure ACK)
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: MF Nowlan <fitz@cs.yale.edu>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
---
net/ipv4/tcp_output.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index faec813..c40f608 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -850,15 +850,15 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
BUG_ON(!skb || !tcp_skb_pcount(skb));
- /* If congestion control is doing timestamping, we must
- * take such a timestamp before we potentially clone/copy.
- */
- if (icsk->icsk_ca_ops->flags & TCP_CONG_RTT_STAMP)
- __net_timestamp(skb);
-
- if (likely(clone_it)) {
+ if (clone_it) {
const struct sk_buff *fclone = skb + 1;
+ /* If congestion control is doing timestamping, we must
+ * take such a timestamp before we potentially clone/copy.
+ */
+ if (icsk->icsk_ca_ops->flags & TCP_CONG_RTT_STAMP)
+ __net_timestamp(skb);
+
if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
fclone->fclone == SKB_FCLONE_CLONE))
NET_INC_STATS_BH(sock_net(sk),
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] tcp: tcp_transmit_skb() optimizations
2013-10-10 15:43 [PATCH net-next] tcp: tcp_transmit_skb() optimizations Eric Dumazet
@ 2013-10-10 15:54 ` Yuchung Cheng
2013-10-11 21:48 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: Yuchung Cheng @ 2013-10-10 15:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, MF Nowlan, Neal Cardwell
On Thu, Oct 10, 2013 at 8:43 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> 1) We need to take a timestamp only for skb that should be cloned.
>
> Other skbs are not in write queue and no rtt estimation is done on them.
>
> 2) the unlikely() hint is wrong for receivers (they send pure ACK)
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: MF Nowlan <fitz@cs.yale.edu>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
Acked-By: Yuchung Cheng <ycheng@google.com>
> ---
> net/ipv4/tcp_output.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index faec813..c40f608 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -850,15 +850,15 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
>
> BUG_ON(!skb || !tcp_skb_pcount(skb));
>
> - /* If congestion control is doing timestamping, we must
> - * take such a timestamp before we potentially clone/copy.
> - */
> - if (icsk->icsk_ca_ops->flags & TCP_CONG_RTT_STAMP)
> - __net_timestamp(skb);
> -
> - if (likely(clone_it)) {
> + if (clone_it) {
> const struct sk_buff *fclone = skb + 1;
>
> + /* If congestion control is doing timestamping, we must
> + * take such a timestamp before we potentially clone/copy.
> + */
> + if (icsk->icsk_ca_ops->flags & TCP_CONG_RTT_STAMP)
> + __net_timestamp(skb);
> +
> if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
> fclone->fclone == SKB_FCLONE_CLONE))
> NET_INC_STATS_BH(sock_net(sk),
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] tcp: tcp_transmit_skb() optimizations
2013-10-10 15:43 [PATCH net-next] tcp: tcp_transmit_skb() optimizations Eric Dumazet
2013-10-10 15:54 ` Yuchung Cheng
@ 2013-10-11 21:48 ` David Miller
2013-10-11 22:43 ` Eric Dumazet
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2013-10-11 21:48 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, fitz, ycheng, ncardwell
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 10 Oct 2013 08:43:00 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> 1) We need to take a timestamp only for skb that should be cloned.
>
> Other skbs are not in write queue and no rtt estimation is done on them.
>
> 2) the unlikely() hint is wrong for receivers (they send pure ACK)
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
This patch is correct, so I've applied it, but it points out a bug.
The __tcp_retransmit_skb() code that does a __pskb_copy() to handle
NET_IP_ALIGN violations and skb_headroom() overflows is buggy because
it needs to store a congestion control timestamp in the original 'skb'
since that's what we'll look at in the retransmit queue.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] tcp: tcp_transmit_skb() optimizations
2013-10-11 21:48 ` David Miller
@ 2013-10-11 22:43 ` Eric Dumazet
2013-10-12 1:20 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-10-11 22:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, fitz, ycheng, ncardwell
On Fri, 2013-10-11 at 17:48 -0400, David Miller wrote:
> This patch is correct, so I've applied it, but it points out a bug.
>
> The __tcp_retransmit_skb() code that does a __pskb_copy() to handle
> NET_IP_ALIGN violations and skb_headroom() overflows is buggy because
> it needs to store a congestion control timestamp in the original 'skb'
> since that's what we'll look at in the retransmit queue.
Yes, I saw that, indeed.
I added it as low priority bug for the moment, as the default congestion
module do not really care, and this case is really unlikely ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] tcp: tcp_transmit_skb() optimizations
2013-10-11 22:43 ` Eric Dumazet
@ 2013-10-12 1:20 ` Eric Dumazet
2013-10-12 5:06 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-10-12 1:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev, fitz, ycheng, ncardwell
On Fri, 2013-10-11 at 15:43 -0700, Eric Dumazet wrote:
> On Fri, 2013-10-11 at 17:48 -0400, David Miller wrote:
>
> > This patch is correct, so I've applied it, but it points out a bug.
> >
> > The __tcp_retransmit_skb() code that does a __pskb_copy() to handle
> > NET_IP_ALIGN violations and skb_headroom() overflows is buggy because
> > it needs to store a congestion control timestamp in the original 'skb'
> > since that's what we'll look at in the retransmit queue.
>
> Yes, I saw that, indeed.
>
> I added it as low priority bug for the moment, as the default congestion
> module do not really care, and this case is really unlikely ;)
>
Ah, I remember now that the conclusion was :
the timestamp is not taken into account for retransmits.
( FLAG_RETRANS_DATA_ACKED )
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] tcp: tcp_transmit_skb() optimizations
2013-10-12 1:20 ` Eric Dumazet
@ 2013-10-12 5:06 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-10-12 5:06 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, fitz, ycheng, ncardwell
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 11 Oct 2013 18:20:19 -0700
> On Fri, 2013-10-11 at 15:43 -0700, Eric Dumazet wrote:
>> On Fri, 2013-10-11 at 17:48 -0400, David Miller wrote:
>>
>> > This patch is correct, so I've applied it, but it points out a bug.
>> >
>> > The __tcp_retransmit_skb() code that does a __pskb_copy() to handle
>> > NET_IP_ALIGN violations and skb_headroom() overflows is buggy because
>> > it needs to store a congestion control timestamp in the original 'skb'
>> > since that's what we'll look at in the retransmit queue.
>>
>> Yes, I saw that, indeed.
>>
>> I added it as low priority bug for the moment, as the default congestion
>> module do not really care, and this case is really unlikely ;)
>>
>
> Ah, I remember now that the conclusion was :
> the timestamp is not taken into account for retransmits.
>
> ( FLAG_RETRANS_DATA_ACKED )
Great, thanks for the clarification.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-12 5:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-10 15:43 [PATCH net-next] tcp: tcp_transmit_skb() optimizations Eric Dumazet
2013-10-10 15:54 ` Yuchung Cheng
2013-10-11 21:48 ` David Miller
2013-10-11 22:43 ` Eric Dumazet
2013-10-12 1:20 ` Eric Dumazet
2013-10-12 5:06 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).