Received-SPF: pass (google.com: domain of linux-kernel+bounces-86856-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 29 Feb 2024 08:40:31 -0600
From: Andrew Halaney <ahalaney@redhat.com>
To: Abhishek Chauhan <quic_abchauha@quicinc.com>
Cc: "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, 
	Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>, Martin KaFai Lau <martin.lau@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, kernel@quicinc.com
Subject: Re: [PATCH net-next v2] net: Modify mono_delivery_time with
 clockid_delivery_time
Message-ID: <5h23xaefpzjr544hw2lsiby7v4zokfnmxm5bye66yx7h3qn6br@te7gptkdamdj>
References: <20240228011219.1119105-1-quic_abchauha@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240228011219.1119105-1-quic_abchauha@quicinc.com>

Hey ABC,

One minor nit below.

On Tue, Feb 27, 2024 at 05:12:19PM -0800, Abhishek Chauhan wrote:
> Bridge driver today has no support to forward the userspace timestamp
> packets and ends up resetting the timestamp. ETF qdisc checks the
> packet coming from userspace and encounters to be 0 thereby dropping
> time sensitive packets. These changes will allow userspace timestamps
> packets to be forwarded from the bridge to NIC drivers.
> 
> Existing functionality of mono_delivery_time is not altered here
> instead just extended with userspace tstamp support for bridge
> forwarding path.
> 
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> ---
> Changes since v1 
> - Changed the commit subject as i am modifying the mono_delivery_time 
>   bit with clockid_delivery_time.
> - Took care of suggestion mentioned by Willem to use the same bit for 
>   userspace delivery time as there are no conflicts between TCP and 
>   SCM_TXTIME, because explicit cmsg makes no sense for TCP and only
>   RAW and DGRAM sockets interprets it. 
> - Clear explaination of why this is needed mentioned below and this 
>   is extending the work done by Martin for mono_delivery_time 
>   https://patchwork.kernel.org/project/netdevbpf/patch/20220302195525.3480280-1-kafai@fb.com/
> - Version 1 patch can be referenced with below link which states 
>   the exact problem with tc-etf and discussions which took place
>   https://lore.kernel.org/all/20240215215632.2899370-1-quic_abchauha@quicinc.com/
> 
>  include/linux/skbuff.h                     | 22 +++++++++++++---------
>  net/bridge/netfilter/nf_conntrack_bridge.c |  2 +-
>  net/core/dev.c                             |  2 +-
>  net/core/filter.c                          |  6 +++---
>  net/ieee802154/6lowpan/reassembly.c        |  2 +-
>  net/ipv4/inet_fragment.c                   |  2 +-
>  net/ipv4/ip_fragment.c                     |  2 +-
>  net/ipv4/ip_output.c                       | 13 +++++++++++--
>  net/ipv4/raw.c                             |  9 +++++++++
>  net/ipv6/ip6_output.c                      | 12 ++++++++++--
>  net/ipv6/netfilter.c                       |  2 +-
>  net/ipv6/netfilter/nf_conntrack_reasm.c    |  2 +-
>  net/ipv6/raw.c                             | 10 +++++++++-
>  net/ipv6/reassembly.c                      |  2 +-
>  net/sched/act_bpf.c                        |  4 ++--
>  net/sched/cls_bpf.c                        |  4 ++--
>  16 files changed, 67 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 2dde34c29203..24a34d56cfa3 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -816,10 +816,14 @@ 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->tstamp has the
> + *	@clockid_delivery_time: When set, skb->tstamp has the
>   *		delivery_time in mono clock base (i.e. EDT).  Otherwise, the
>   *		skb->tstamp has the (rcv) timestamp at ingress and
>   *		delivery_time at egress.
> + *		This bit is also set if the tstamp is set from userspace which
> + *		acts as an information in the bridge forwarding path to net

s/net/not/ or maybe "avoid resetting" ?

> + *		reset the tstamp value when user sets the timestamp using
> + *		SO_TXTIME sockopts