Message-ID: <67a24a4f8af27_bb566294bd@willemb.c.googlers.com.notmuch>
Date: Tue, 04 Feb 2025 12:11:43 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 willemdebruijn.kernel@...il.com, 
 willemb@...gle.com, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 eddyz87@...il.com, 
 song@...nel.org, 
 yonghong.song@...ux.dev, 
 john.fastabend@...il.com, 
 kpsingh@...nel.org, 
 sdf@...ichev.me, 
 haoluo@...gle.com, 
 jolsa@...nel.org, 
 horms@...nel.org, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v7 00/13] net-timestamp: bpf extension to equip
 applications transparently

Jason Xing wrote:
> On Tue, Feb 4, 2025 at 10:27 AM Martin KaFai Lau <martin.lau@...ux.dev> 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".

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.

Powered by blists - more mailing lists