On Wed, Feb 5, 2025 at 1:11 AM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Jason Xing wrote: > > On Tue, Feb 4, 2025 at 10:27 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > > > > > On 1/28/25 12:46 AM, Jason Xing wrote: > > > > "Timestamping is key to debugging network stack latency. With > > > > SO_TIMESTAMPING, bugs that are otherwise incorrectly assumed to be > > > > network issues can be attributed to the kernel." This is extracted > > > > from the talk "SO_TIMESTAMPING: Powering Fleetwide RPC Monitoring" > > > > addressed by Willem de Bruijn at netdevconf 0x17). > > > > > > > > There are a few areas that need optimization with the consideration of > > > > easier use and less performance impact, which I highlighted and mainly > > > > discussed at netconf 2024 with Willem de Bruijn and John Fastabend: > > > > uAPI compatibility, extra system call overhead, and the need for > > > > application modification. I initially managed to solve these issues > > > > by writing a kernel module that hooks various key functions. However, > > > > this approach is not suitable for the next kernel release. Therefore, > > > > a BPF extension was proposed. During recent period, Martin KaFai Lau > > > > provides invaluable suggestions about BPF along the way. Many thanks > > > > here! > > > > > > > > In this series, I only support foundamental codes and tx for TCP. > > > > > > *fundamental*. > > > > > > May be just "only tx time stamping for TCP is supported..." > > > > > > > This approach mostly relies on existing SO_TIMESTAMPING feature, users > > > > only needs to pass certain flags through bpf_setsocktopt() to a separate > > > > tsflags. Please see the last selftest patch in this series. > > > > > > > > After this series, we could step by step implement more advanced > > > > functions/flags already in SO_TIMESTAMPING feature for bpf extension. > > > > > > Patch 1-4 and 6-11 can use an extra "bpf:" tag in the subject line. Patch 13 > > > should be "selftests/bpf:" instead of "bpf:" in the subject. > > > > > > Please revisit the commit messages of this patch set to check for outdated > > > comments from the earlier revisions. I may have missed some of them. > > > > Roger that, sir. Thanks for your help! > > > > > > > > Overall, it looks close. I will review at your replies later. > > > > > > Willem, could you also take a look? Thanks. > > > > Right, some related parts need reviews from netdev experts as well. > > > > Willem, please help me review this when you're available. No rush :) > > I won't have much to add for the BPF side, to be clear. > > One small high level commit message point: as submitting-patches > suggests, use imperative mood: "adds X" when the patch introduces a > feature, not "I add". And "caller gets" rather than "we get". > > Specific case, with capitalization issue: "we need to Introduce". Thanks for learning a new lesson. I will adjust them. > > I'll respond to a few inline code elements later. Nothing huge. > Also feel free to post the next version and I'll respond to that, if > you prefer. I will post v8 soon. Thanks for your precious time. Have fun with your trip :p Thanks, Jason
