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

[Willem de Bruijn <willemb@google.com>]

h

On Wed, Dec 15, 2021 at 3:12 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> The skb->skb_mstamp_ns is used as the EDT (Earliest Department Time)
> in TCP.  skb->skb_mstamp_ns is a union member of skb->tstamp.
>
> When the skb traveling veth and being forwarded like below, the skb->tstamp
> is reset to 0 at multiple points.
>
>                                                                         (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.
>
> Unlike RFC v1 trying to migrate rx tstamp to mono first,
> this patch is to preserve the EDT in skb->skb_mstamp_ns during forward.

Sucks we have to do this complex dance, because there is no room for
an skb->delivery_time.

> The idea is to temporarily store skb->skb_mstamp_ns during forward.
> skb_shinfo(skb)->hwtstamps is used as a temporary store and
> it is union-ed with the newly added "u64 tx_delivery_tstamp".
> hwtstamps should only be used when a packet is received or
> sent out of a hw device.
>
> During forward, skb->tstamp will be temporarily stored in
> skb_shinfo(skb)->tx_delivery_tstamp and a new bit
> (SKBTX_DELIVERY_TSTAMP) in skb_shinfo(skb)->tx_flags
> will also be set to tell tx_delivery_tstamp is in use.
> hwtstamps is accessed through the skb_hwtstamps() getter,
> so unlikely(tx_flags & SKBTX_DELIVERY_TSTAMP) can
> be tested in there and reset tx_delivery_tstamp to 0
> before hwtstamps is used.
>
> After moving the skb->tstamp to skb_shinfo(skb)->tx_delivery_tstamp,
> the skb->tstamp will still be reset to 0 during forward.  Thus,
> on the RX side (__netif_receive_skb_core), all existing code paths
> will still get the received time in real clock and will work as-is.
>
> When this skb finally xmit-ing out in __dev_queue_xmit(),
> it will check the SKBTX_DELIVERY_TSTAMP bit in skb_shinfo(skb)->tx_flags
> and restore the skb->tstamp from skb_shinfo(skb)->tx_delivery_tstamp
> if needed.  This bit test is done immediately after another existing
> bit test 'skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP'.
>
> Another bit SKBTX_DELIVERY_TSTAMP_ALLOW_FWD is added
> to skb_shinfo(skb)->tx_flags.  It is used to specify
> the skb->tstamp is set as a delivery time and can be
> temporarily stored during forward.  This bit is now set
> when EDT is stored in skb->skb_mstamp_ns in tcp_output.c
> This will avoid packet received from a NIC with real-clock
> in skb->tstamp being forwarded without reset.
>
> The change in af_packet.c is to avoid it calling skb_hwtstamps()
> which will reset the skb_shinfo(skb)->tx_delivery_tstamp.
> af_packet.c only wants to read the hwtstamps instead of
> storing a time in it, so a new read only getter skb_hwtstamps_ktime()
> is added.  Otherwise, a tcpdump will trigger this code path
> and unnecessarily reset the EDT stored in tx_delivery_tstamp.
>
> [Note: not all skb->tstamp=0 reset has been changed in this RFC yet]
>
> [0] (slide 22): https://linuxplumbersconf.org/event/11/contributions/953/attachments/867/1658/LPC_2021_BPF_Datapath_Extensions.pdf
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  include/linux/skbuff.h  | 52 ++++++++++++++++++++++++++++++++++++++++-
>  net/bridge/br_forward.c |  2 +-
>  net/core/dev.c          |  1 +
>  net/core/filter.c       |  6 ++---
>  net/core/skbuff.c       |  2 +-
>  net/ipv4/ip_forward.c   |  2 +-
>  net/ipv4/tcp_output.c   | 21 +++++++++++------
>  net/ipv6/ip6_output.c   |  2 +-
>  net/packet/af_packet.c  |  8 +++----
>  9 files changed, 77 insertions(+), 19 deletions(-)
>
> 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.

> +               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?

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?

> +{
> +       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?

> +       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