Re: [RFC PATCH v2 net-next] net: Preserve skb delivery time during forward

On Thu, Dec 16, 2021 at 10:32:23AM -0500, Willem de Bruijn wrote:
[ ... ]

quoted

                                                                        (c: skb->tstamp = 0)
                                                                         vv
tcp-sender => veth@netns => veth@hostns(b: rx: skb->tstamp = real_clock) => fq@eth0
                         ^^
                        (a: skb->tstamp = 0)

(a) veth@netns TX to veth@hostns:
    skb->tstamp (mono clock) is a EDT and it is in future time.
    Reset to 0 so that it won't skip the net_timestamp_check at the
    RX side in (b).
(b) RX (netif_rx) in veth@hostns:
    net_timestamp_check puts a current time (real clock) in skb->tstamp.
(c) veth@hostns forward to fq@eth0:
    skb->tstamp is reset back to 0 again because fq is using
    mono clock.

This leads to an unstable TCP throughput issue described by Daniel in [0].

We also have a use case that a bpf runs at ingress@veth@hostns
to set EDT in skb->tstamp to limit the bandwidth usage
of a particular netns.  This EDT currently also gets
reset in step (c) as described above.

[ ... ]

quoted

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6535294f6a48..9bf0a1e2a1bd 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -435,9 +435,17 @@ enum {
        /* device driver is going to provide hardware time stamp */
        SKBTX_IN_PROGRESS = 1 << 2,

+       /* shinfo stores a future tx_delivery_tstamp instead of hwtstamps */
+       SKBTX_DELIVERY_TSTAMP = 1 << 3,
+
        /* generate wifi status information (where possible) */
        SKBTX_WIFI_STATUS = 1 << 4,

+       /* skb->tstamp stored a future delivery time which
+        * was set by a local sk and it can be fowarded.
+        */
+       SKBTX_DELIVERY_TSTAMP_ALLOW_FWD = 1 << 5,
+
        /* generate software time stamp when entering packet scheduling */
        SKBTX_SCHED_TSTAMP = 1 << 6,
 };

@@ -530,7 +538,14 @@ struct skb_shared_info {
        /* Warning: this field is not always filled in (UFO)! */
        unsigned short  gso_segs;
        struct sk_buff  *frag_list;
-       struct skb_shared_hwtstamps hwtstamps;
+       union {
+               /* If SKBTX_DELIVERY_TSTAMP is set in tx_flags,
+                * tx_delivery_tstamp is stored instead of
+                * hwtstamps.
+                */

Should we just encode the timebase and/or type { timestamp,
delivery_time } in th lower bits of the timestamp field? Its
resolution is higher than actual clock precision.

In skb->tstamp ?

quoted

+               struct skb_shared_hwtstamps hwtstamps;
+               u64 tx_delivery_tstamp;
+       };
        unsigned int    gso_type;
        u32             tskey;

@@ -1463,9 +1478,44 @@ static inline unsigned int skb_end_offset(const struct sk_buff *skb)

 static inline struct skb_shared_hwtstamps *skb_hwtstamps(struct sk_buff *skb)
 {
+       if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP)) {
+               skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP;
+               skb_shinfo(skb)->tx_delivery_tstamp = 0;
+       }
        return &skb_shinfo(skb)->hwtstamps;
 }

+/* Caller only needs to read the hwtstamps as ktime.
+ * To update hwtstamps,  HW device driver should call the writable
+ * version skb_hwtstamps() that returns a pointer.
+ */
+static inline ktime_t skb_hwtstamps_ktime(const struct sk_buff *skb)
+{
+       if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP))
+               return 0;
+       return skb_shinfo(skb)->hwtstamps.hwtstamp;
+}
+
+static inline void skb_scrub_tstamp(struct sk_buff *skb)

skb_save_delivery_time?

yep. ok.

is non-zero skb->tstamp test not sufficient, instead of
SKBTX_DELIVERY_TSTAMP_ALLOW_FWD.

It is if only called on the egress path. Is bpf on ingress the only
reason for this?

Ah. ic.  meaning testing non-zero skb->tstamp and then call
skb_save_delivery_time() only during the veth-egress-path:
somewhere in veth_xmit() => veth_forward_skb() but before
skb->tstamp was reset to 0 in __dev_forward_skb().

Keep *_forward() and bpf_out_*() unchanged (i.e. keep skb->tstamp = 0)
because the skb->tstamp could be stamped by net_timestamp_check().

Then SKBTX_DELIVERY_TSTAMP_ALLOW_FWD is not needed.

Did I understand your suggestion correctly?

However, we still need a bit to distinguish tx_delivery_tstamp
from hwtstamps.

quoted

+{
+       if (skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP_ALLOW_FWD) {
+               skb_shinfo(skb)->tx_delivery_tstamp = skb->tstamp;
+               skb_shinfo(skb)->tx_flags |= SKBTX_DELIVERY_TSTAMP;
+               skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP_ALLOW_FWD;
+       }

Is this only called when there are no clones/shares?

No, I don't think so.  TCP clone it.  I also started thinking about
this after noticing a mistake in the change in  __tcp_transmit_skb().

There are other places that change tx_flags, e.g. tcp_offload.c.
It is not shared at those places or there is some specific points
in the stack that is safe to change ?

quoted

+       skb->tstamp = 0;
+}
+
+static inline void skb_restore_delivery_time(struct sk_buff *skb)
+{
+       if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP)) {
+               skb->tstamp = skb_shinfo(skb)->tx_delivery_tstamp;
+               skb_shinfo(skb)->tx_delivery_tstamp = 0;
+               skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP;
+               skb_shinfo(skb)->tx_flags |= SKBTX_DELIVERY_TSTAMP_ALLOW_FWD;
+       }
+}
+
 static inline struct ubuf_info *skb_zcopy(struct sk_buff *skb)
 {
        bool is_zcopy = skb && skb_shinfo(skb)->flags & SKBFL_ZEROCOPY_ENABLE;

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index ec646656dbf1..a3ba6195f2e3 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c

@@ -62,7 +62,7 @@ EXPORT_SYMBOL_GPL(br_dev_queue_push_xmit);

 int br_forward_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
-       skb->tstamp = 0;
+       skb_scrub_tstamp(skb);
        return NF_HOOK(NFPROTO_BRIDGE, NF_BR_POST_ROUTING,
                       net, sk, skb, NULL, skb->dev,
                       br_dev_queue_push_xmit);

diff --git a/net/core/dev.c b/net/core/dev.c
index a855e41bbe39..e9e7de758cba 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c