Re: [PATCH net-next v3 02/14] net-timestamp: allow two features to work parallelly

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

Jason Xing wrote:
> On Thu, Oct 31, 2024 at 2:27 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >
> > On 10/30/24 5:13 PM, Jason Xing wrote:
> > > I realized that we will have some new sock_opt flags like
> > > TS_SCHED_OPT_CB in patch 4, so we can control whether to print or
> > > not... For each sock_opt point, they will be called without caring if
> > > related flags in skb are set. Well, it's meaningless to add more
> > > control of skb tsflags at each TS_xx_OPT_CB point.
> > >
> > > Am I understanding in a correct way? Now, I'm totally fine with this great idea!
> > Yes, I think so.
> >
> > The sockops prog can choose to ignore any BPF_SOCK_OPS_TS_*_CB. The are only 3:
> > SCHED, SND, and ACK. If the hwtstamp is available from a NIC, I think it would
> > be quite wasteful to throw it away. ACK can be controlled by the
> > TCP_SKB_CB(skb)->bpf_txstamp_ack.
> 
> Right, let me try this:)
> 
> > Going back to my earlier bpf_setsockopt(SOL_SOCKET, BPF_TX_TIMESTAMPING)
> > comment. I think it may as well go back to use the "u8
> > bpf_sock_ops_cb_flags;" and use the bpf_sock_ops_cb_flags_set() helper to
> > enable/disable the timestamp related callback hook. May be add one
> > BPF_SOCK_OPS_TX_TIMESTAMPING_CB_FLAG.
> 
> bpf_sock_ops_cb_flags this flag is only used in TCP condition, right?
> If that is so, it cannot be suitable for UDP.
> 
> I'm thinking of this solution:
> 1) adding a new flag in SOF_TIMESTAMPING_OPT_BPF flag (in
> include/uapi/linux/net_tstamp.h) which can be used by sk->sk_tsflags
> 2) flags =   SOF_TIMESTAMPING_OPT_BPF;    bpf_setsockopt(skops,
> SOL_SOCKET, SO_TIMESTAMPING, &flags, sizeof(flags));
> 3) test if sk->sk_tsflags has this new flag in tcp_tx_timestamp() or
> in udp_sendmsg()
> ...
> 
> >
> > For tx, one new hook should be at the sendmsg and should be around
> > tcp_tx_timestamp (?) for tcp. Another hook is __skb_tstamp_tx() which should be
> 
> I think there are two points we're supposed to record:
> 1) the moment tcp/udp_sendmsg() is triggered. It represents the syscall time.
> 2) another point in tcp_tx_timestamp(). It represents the timestamp of
> the last skb in this sendmsg() call.
> Users may happen to send a big packet.

Err on the side of fewer measurement points. It's always possible to
add more later, but not possible to remove them (depending on whether
BPF infra is ABI).

Overall great suggestion. Thanks a lot for sharing your BPF expertise
on this, Martin.

On using the raw seqno: this data is accessible to anyone root in
namespace (ns_capable) using packet sockets, so as long as it does not
open to more than that, it is logically equivalent to the current
setting.

With seqno the BPF program has to be careful that the same seqno can
be retransmitted, so for instance seeing an ACK before a (second) SND
must be anticipated. That is true for SO_TIMESTAMPING today too.

For datagrams (UDP as well as RAW and many non IP protocols), an
alternative still needs to be found.