Re: [RFC PATCH v3 net-next 1/4] net: Add skb->mono_delivery_time to distinguish mono delivery_time from (rcv) timestamp

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Fri, Jan 21, 2022 at 2:30 AM Martin KaFai Lau <kafai@fb.com> wrote:
>
> skb->tstamp was first used as the (rcv) timestamp in real time clock base.
> The major usage is to report it to the user (e.g. SO_TIMESTAMP).
>
> Later, skb->tstamp is also set as the (future) delivery_time (e.g. EDT in TCP)
> during egress and used by the qdisc (e.g. sch_fq) to make decision on when
> the skb can be passed to the dev.
>
> Currently, there is no way to tell skb->tstamp having the (rcv) timestamp
> or the delivery_time, so it is always reset to 0 whenever forwarded
> between egress and ingress.
>
> While it makes sense to always clear the (rcv) timestamp in skb->tstamp
> to avoid confusing sch_fq that expects the delivery_time, it is a
> performance issue [0] to clear the delivery_time if the skb finally
> egress to a fq@phy-dev.  For example, when forwarding from egress to
> ingress and then finally back to egress:
>
>             tcp-sender => veth@netns => veth@hostns => fq@eth0@hostns
>                                      ^              ^
>                                      reset          rest
>
> [0] (slide 22): https://linuxplumbersconf.org/event/11/contributions/953/attachments/867/1658/LPC_2021_BPF_Datapath_Extensions.pdf
>
> This patch adds one bit skb->mono_delivery_time to flag the skb->tstamp
> is storing the mono delivery_time instead of the (rcv) timestamp.
>
> The current use case is to keep the TCP mono delivery_time (EDT) and
> to be used with sch_fq.  The later patch will also allow tc-bpf to read
> and change the mono delivery_time.
>
> In the future, another bit (e.g. skb->user_delivery_time) can be added
> for the SCM_TXTIME where the clock base is tracked by sk->sk_clockid.
>
> [ This patch is a prep work.  The following patch will
>   get the other parts of the stack ready first.  Then another patch
>   after that will finally set the skb->mono_delivery_time. ]
>
> skb_set_delivery_time() function is added.  It is used by the tcp_output.c
> and during ip[6] fragmentation to assign the delivery_time to
> the skb->tstamp and also set the skb->mono_delivery_time.
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  include/linux/skbuff.h                     | 13 +++++++++++++
>  net/bridge/netfilter/nf_conntrack_bridge.c |  5 +++--
>  net/ipv4/ip_output.c                       |  5 +++--
>  net/ipv4/tcp_output.c                      | 16 +++++++++-------
>  net/ipv6/ip6_output.c                      |  5 +++--
>  net/ipv6/netfilter.c                       |  5 +++--
>  6 files changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index bf11e1fbd69b..b9e20187242a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -720,6 +720,10 @@ typedef unsigned char *sk_buff_data_t;
>   *     @dst_pending_confirm: need to confirm neighbour
>   *     @decrypted: Decrypted SKB
>   *     @slow_gro: state present at GRO time, slower prepare step required
> + *     @mono_delivery_time: When set, skb->tstamap has the

tstamp

> + *             delivery_time in mono clock base (i.e. EDT).  Otherwise, the
> + *             skb->tstamp has the (rcv) timestamp at ingress and
> + *             delivery_time at egress.
>   *     @napi_id: id of the NAPI struct this skb came from
>   *     @sender_cpu: (aka @napi_id) source CPU in XPS
>   *     @secmark: security marking
> @@ -890,6 +894,7 @@ struct sk_buff {
>         __u8                    decrypted:1;
>  #endif
>         __u8                    slow_gro:1;
> +       __u8                    mono_delivery_time:1;

This bit fills a hole, does not change sk_buff size, right?

>
>  #ifdef CONFIG_NET_SCHED
>         __u16                   tc_index;       /* traffic control index */
> @@ -3903,6 +3908,14 @@ static inline ktime_t net_invalid_timestamp(void)
>         return 0;
>  }
>
> +static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
> +                                        bool mono)
> +{
> +       skb->tstamp = kt;
> +       /* Setting mono_delivery_time will be enabled later */
> +       /* skb->mono_delivery_time = kt && mono; */
> +}
> +
>  static inline u8 skb_metadata_len(const struct sk_buff *skb)
>  {
>         return skb_shinfo(skb)->meta_len;
> diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
> index fdbed3158555..ebfb2a5c59e4 100644
> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
> @@ -32,6 +32,7 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
>                                            struct sk_buff *))
>  {
>         int frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size;
> +       bool mono_delivery_time = skb->mono_delivery_time;

This use of a local variable is just a style choice, not needed for
correctness, correct?