[PATCH net v4] net/udp_gso: Allow TX timestamp with UDP GSO

[PATCH net v4] net/udp_gso: Allow TX timestamp with UDP GSO

[Fred Klassen]

Fixes an issue where TX Timestamps are not arriving on the error queue
when UDP_SEGMENT CMSG type is combined with CMSG type SO_TIMESTAMPING.
This can be illustrated with an updated updgso_bench_tx program which
includes the '-T' option to test for this condition. It also introduces
the '-P' option which will call poll() before reading the error queue.

    ./udpgso_bench_tx -4ucTPv -S 1472 -l2 -D 172.16.120.18
    poll timeout
    udp tx:      0 MB/s        1 calls/s      1 msg/s

The "poll timeout" message above indicates that TX timestamp never
arrived.

This patch preserves tx_flags for the first UDP GSO segment. Only the
first segment is timestamped, even though in some cases there may be
benefital in timestamping both the first and last segment.

Factors in deciding on first segment timestamp only:

- Timestamping both first and last segmented is not feasible. Hardware
can only have one outstanding TS request at a time.

- Timestamping last segment may under report network latency of the
previous segments. Even though the doorbell is suppressed, the ring
producer counter has been incremented.

- Timestamping the first segment has the upside in that it reports
timestamps from the application's view, e.g. RTT.

- Timestamping the first segment has the downside that it may
underreport tx host network latency. It appears that we have to pick
one or the other. And possibly follow-up with a config flag to choose
behavior.

v2: Remove tests as noted by Willem de Bruijn <willemb@google.com>
    Moving tests from net to net-next

v3: Update only relevant tx_flag bits as per
    Willem de Bruijn <willemb@google.com>

v4: Update comments and commit message as per
    Willem de Bruijn <willemb@google.com>

Fixes: ee80d1ebe5ba ("udp: add udp gso")
Signed-off-by: Fred Klassen <fklassen@appneta.com>
---
 net/ipv4/udp_offload.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 06b3e2c1fcdc..9763464a75d7 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -224,6 +224,11 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	seg = segs;
 	uh = udp_hdr(seg);
 
+	/* preserve TX timestamp flags and TS key for first segment */
+	skb_shinfo(seg)->tskey = skb_shinfo(gso_skb)->tskey;
+	skb_shinfo(seg)->tx_flags |=
+			(skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP);
+
 	/* compute checksum adjustment based on old length versus new */
 	newlen = htons(sizeof(*uh) + mss);
 	check = csum16_add(csum16_sub(uh->check, uh->len), newlen);
-- 
2.11.0

Re: [PATCH net v4] net/udp_gso: Allow TX timestamp with UDP GSO

[Willem de Bruijn]

On Mon, Jun 17, 2019 at 3:05 PM Fred Klassen <fklassen@appneta.com> wrote:
>
> Fixes an issue where TX Timestamps are not arriving on the error queue
> when UDP_SEGMENT CMSG type is combined with CMSG type SO_TIMESTAMPING.
> This can be illustrated with an updated updgso_bench_tx program which
> includes the '-T' option to test for this condition. It also introduces
> the '-P' option which will call poll() before reading the error queue.
>
>     ./udpgso_bench_tx -4ucTPv -S 1472 -l2 -D 172.16.120.18
>     poll timeout
>     udp tx:      0 MB/s        1 calls/s      1 msg/s
>
> The "poll timeout" message above indicates that TX timestamp never
> arrived.
>
> This patch preserves tx_flags for the first UDP GSO segment. Only the
> first segment is timestamped, even though in some cases there may be
> benefital in timestamping both the first and last segment.
>
> Factors in deciding on first segment timestamp only:
>
> - Timestamping both first and last segmented is not feasible. Hardware
> can only have one outstanding TS request at a time.
>
> - Timestamping last segment may under report network latency of the
> previous segments. Even though the doorbell is suppressed, the ring
> producer counter has been incremented.
>
> - Timestamping the first segment has the upside in that it reports
> timestamps from the application's view, e.g. RTT.
>
> - Timestamping the first segment has the downside that it may
> underreport tx host network latency. It appears that we have to pick
> one or the other. And possibly follow-up with a config flag to choose
> behavior.
>
> v2: Remove tests as noted by Willem de Bruijn <willemb@google.com>
>     Moving tests from net to net-next
>
> v3: Update only relevant tx_flag bits as per
>     Willem de Bruijn <willemb@google.com>
>
> v4: Update comments and commit message as per
>     Willem de Bruijn <willemb@google.com>
>
> Fixes: ee80d1ebe5ba ("udp: add udp gso")
> Signed-off-by: Fred Klassen <fklassen@appneta.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net v4] net/udp_gso: Allow TX timestamp with UDP GSO

[David Miller]

From: Fred Klassen <fklassen@appneta.com>
Date: Mon, 17 Jun 2019 12:05:07 -0700

> Fixes an issue where TX Timestamps are not arriving on the error queue
> when UDP_SEGMENT CMSG type is combined with CMSG type SO_TIMESTAMPING.
> This can be illustrated with an updated updgso_bench_tx program which
> includes the '-T' option to test for this condition. It also introduces
> the '-P' option which will call poll() before reading the error queue.
 ...
> The "poll timeout" message above indicates that TX timestamp never
> arrived.
> 
> This patch preserves tx_flags for the first UDP GSO segment. Only the
> first segment is timestamped, even though in some cases there may be
> benefital in timestamping both the first and last segment.
> 
> Factors in deciding on first segment timestamp only:
> 
> - Timestamping both first and last segmented is not feasible. Hardware
> can only have one outstanding TS request at a time.
> 
> - Timestamping last segment may under report network latency of the
> previous segments. Even though the doorbell is suppressed, the ring
> producer counter has been incremented.
> 
> - Timestamping the first segment has the upside in that it reports
> timestamps from the application's view, e.g. RTT.
> 
> - Timestamping the first segment has the downside that it may
> underreport tx host network latency. It appears that we have to pick
> one or the other. And possibly follow-up with a config flag to choose
> behavior.
 ...
> Fixes: ee80d1ebe5ba ("udp: add udp gso")
> Signed-off-by: Fred Klassen <fklassen@appneta.com>

Applied and queued up for -stable, thanks.