On Tue, Oct 15, 2024 at 10:59 AM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Jason Xing wrote: > > On Tue, Oct 15, 2024 at 9:28 AM Willem de Bruijn > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > Jason Xing wrote: > > > > On Sun, Oct 13, 2024 at 1:48 AM Willem de Bruijn > > > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > > > > > Jason Xing wrote: > > > > > > From: Jason Xing <kernelxing@xxxxxxxxxxx> > > > > > > > > > > > > A few weeks ago, I planned to extend SO_TIMESTMAMPING feature by using > > > > > > tracepoint to print information (say, tstamp) so that we can > > > > > > transparently equip applications with this feature and require no > > > > > > modification in user side. > > > > > > > > > > > > Later, we discussed at netconf and agreed that we can use bpf for better > > > > > > extension, which is mainly suggested by John Fastabend and Willem de > > > > > > Bruijn. Many thanks here! So I post this series to see if we have a > > > > > > better solution to extend. My feeling is BPF is a good place to provide > > > > > > a way to add timestamping by administrators, without having to rebuild > > > > > > applications. > > > > > > > > > > > > This approach mostly relies on existing SO_TIMESTAMPING feature, users > > > > > > only needs to pass certain flags through bpf_setsocktop() to a separate > > > > > > tsflags. For TX timestamps, they will be printed during generation > > > > > > phase. For RX timestamps, we will wait for the moment when recvmsg() is > > > > > > called. > > > > > > > > > > > > After this series, we could step by step implement more advanced > > > > > > functions/flags already in SO_TIMESTAMPING feature for bpf extension. > > > > > > > > > > > > In this series, I only support TCP protocol which is widely used in > > > > > > SO_TIMESTAMPING feature. > > > > > > > > > > > > --- > > > > > > V2 > > > > > > Link: https://lore.kernel.org/all/20241008095109.99918-1-kerneljasonxing@xxxxxxxxx/ > > > > > > 1. Introduce tsflag requestors so that we are able to extend more in the > > > > > > future. Besides, it enables TX flags for bpf extension feature separately > > > > > > without breaking users. It is suggested by Vadim Fedorenko. > > > > > > 2. introduce a static key to control the whole feature. (Willem) > > > > > > 3. Open the gate of bpf_setsockopt for the SO_TIMESTAMPING feature in > > > > > > some TX/RX cases, not all the cases. > > > > > > > > > > > > Note: > > > > > > The main concern we've discussion in V1 thread is how to deal with the > > > > > > applications using SO_TIMESTAMPING feature? In this series, I allow both > > > > > > cases to happen at the same time, which indicates that even one > > > > > > applications setting SO_TIMESTAMPING can still be traced through BPF > > > > > > program. Please see patch [04/12]. > > > > > > > > > > This revision does not address the main concern. > > > > > > > > > > An administrator installed BPF program can affect results of a process > > > > > using SO_TIMESTAMPING in ways that break it. > > > > > > > > Sorry, I didn't get it. How the following code snippet would break users? > > > > > > The state between user and bpf timestamping needs to be separate to > > > avoid interference. > > > > Do you agree that we will use this method as following, only allow > > either of them to work? > > > > void __skb_tstamp_tx(struct sk_buff *orig_skb, > > const struct sk_buff *ack_skb, > > struct skb_shared_hwtstamps *hwtstamps, > > struct sock *sk, int tstype) > > { > > if (!sk) > > return; > > > > ret = skb_tstamp_tx_output(orig_skb, ack_skb, hwtstamps, sk, tstype); > > if (ret) > > /* Apps does set the SO_TIMESTAMPING flag, return directly */ > > return; > > > > if (static_branch_unlikely(&bpf_tstamp_control)) > > bpf_skb_tstamp_tx_output(sk, orig_skb, tstype, hwtstamps); > > } > > > > which means if the apps using non-bpf method, we will not see the > > output even if we load bpf program. > > Could the bpf setsockopt fail hard in that case? We can do this. I think I will add some if test statements to see if sk_tsflags is initialized before. > > Your current patch tries to make them work at the same time. It mostly > does work. There are just a few concerning edge cases that may result > in hard to understand bugs. Agree. > > Making only one method work per socket and fail hard if both try it is > crude, but at least the failure will be clear: the setsockopt fails. > > I think that's safer. And in practice, the use cases for BPF > timestamping probably are exactly when application timestamping is > missing? Fair enough. Let me try this way:) Thanks, Jason
