On Thu, Oct 31, 2024 at 8:30 PM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Jason Xing wrote: > > On Thu, Oct 31, 2024 at 2:27 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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. It seems that using the tskey for bpf extension is always correct and easy to use. Could we provide the tskey to users and then let users decide the better way to identify the call of sendmsg. We could keep the traditional use of tskey. If without it, people need to figure out a good way and may find it difficult to use the bpf extension. I will keep thinking of alternatives for UDP in the meantime. Thanks, Jason
