Message-ID: <CAL+tcoD47VfZJFPJcQOgPsQuGA=jPfKU2548fJp2NBH14gEoHA@mail.gmail.com>
Date: Wed, 9 Oct 2024 19:48:41 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, dsahern@...nel.org, 
	willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	martin.lau@...ux.dev, 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, bpf@...r.kernel.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 0/9] net-timestamp: bpf extension to equip
 applications transparently

On Wed, Oct 9, 2024 at 7:12 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> On Wed, Oct 9, 2024 at 5:28 PM Vadim Fedorenko
> <vadim.fedorenko@...ux.dev> wrote:
> >
> > On 09/10/2024 02:05, Jason Xing wrote:
> > > On Wed, Oct 9, 2024 at 7:22 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > >>
> > >> On Wed, Oct 9, 2024 at 2:44 AM Willem de Bruijn
> > >> <willemdebruijn.kernel@...il.com> wrote:
> > >>>
> > >>> Jason Xing wrote:
> > >>>> From: Jason Xing <kernelxing@...cent.com>
> > >>>>
> > >>>> 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.
> > >>>>
> > >>>> This approach relies on existing SO_TIMESTAMPING feature, for tx path,
> > >>>> users only needs to pass certain flags through bpf program to make sure
> > >>>> the last skb from each sendmsg() has timestamp related controlled flag.
> > >>>> For rx path, we have to use bpf_setsockopt() to set the sk->sk_tsflags
> > >>>> and wait for the moment when recvmsg() is called.
> > >>>
> > >>> As you mention, overall I am very supportive of having a way to add
> > >>> timestamping by adminstrators, without having to rebuild applications.
> > >>> BPF hooks seem to be the right place for this.
> > >>>
> > >>> There is existing kprobe/kretprobe/kfunc support. Supporting
> > >>> SO_TIMESTAMPING directly may be useful due to its targeted feature
> > >>> set, and correlation between measurements for the same data in the
> > >>> stream.
> > >>>
> > >>>> After this series, we could step by step implement more advanced
> > >>>> functions/flags already in SO_TIMESTAMPING feature for bpf extension.
> > >>>
> > >>> My main implementation concern is where this API overlaps with the
> > >>> existing user API, and how they might conflict. A few questions in the
> > >>> patches.
> > >>
> > >> Agreed. That's also what I'm concerned about. So I decided to ask for
> > >> related experts' help.
> > >>
> > >> How to deal with it without interfering with the existing apps in the
> > >> right way is the key problem.
> > >
> > > What I try to implement is let the bpf program have the highest
> > > precedence. It's similar to RTO min, see the commit as an example:
> > >
> > > commit f086edef71be7174a16c1ed67ac65a085cda28b1
> > > Author: Kevin Yang <yyd@...gle.com>
> > > Date:   Mon Jun 3 21:30:54 2024 +0000
> > >
> > >      tcp: add sysctl_tcp_rto_min_us
> > >
> > >      Adding a sysctl knob to allow user to specify a default
> > >      rto_min at socket init time, other than using the hard
> > >      coded 200ms default rto_min.
> > >
> > >      Note that the rto_min route option has the highest precedence
> > >      for configuring this setting, followed by the TCP_BPF_RTO_MIN
> > >      socket option, followed by the tcp_rto_min_us sysctl.
> > >
> > > It includes three cases, 1) route option, 2) bpf option, 3) sysctl.
> > > The first priority can override others. It doesn't have a good
> > > chance/point to restore the icsk_rto_min field if users want to
> > > shutdown the bpf program because it is set in
> > > bpf_sol_tcp_setsockopt().
> >
> > rto_min example is slightly different. With tcp_rto_min the doesn't
> > expect any data to come back to user space while for timestamping the
> > app may be confused directly by providing more data, or by not providing
> > expected data. I believe some hint about requestor of the data is needed
> > here. It will also help to solve the problem of populating sk_err_queue
> > mentioned by Martin.
>
> Sorry, I don't fully get it. In this patch series, this bpf extension
> feature will not rely on sk_err_queue any more to report tx timestamps
> to userspace. Bpf program can do that printing.
>
> Do you mean that it could be wrong if one skb carries the tsflags that
> are previously set due to the bpf program and then suddenly users
> detach the program? It indeed will put a new/cloned skb into the error
> queue. Interesting corner case. It seems I have to re-implement a
> totally independent tsflags for bpf extension feature. Do you have a
> better idea on this?

I feel that if I could introduce bpf new flags like
SOF_TIMESTAMPING_TX_ACK_BPF for the last skb based on this patch
series, then it will not populate skb in sk_err_queue even users
remove the bpf program all of sudden. With this kind of specific bpf
flags, we can also avoid conflicting with the apps using
SO_TIEMSTAMPING feature. Let me give it a shot unless a better
solution shows up.

Powered by blists - more mailing lists