Re: [PATCH net-next v2 09/12] net-timestamp: add tx OPT_ID_TCP support for bpf case

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jason Xing wrote:
> On Tue, Oct 15, 2024 at 9:38 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
> > > from each sendmsg. We only set the socket once like how we use
> > > setsockopt() with OPT_ID|OPT_ID_TCP flags.
> > >
> > > Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
> > > flag. If either of them has been set before, we will not initialize
> > > the key any more,
> >
> > Where and how is this achieved?
> 
> Please see this patch and you will find the following codes.
> +       tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
> +                   sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);

I saw that, but it's not a condition that stops reinitializing. Which
I think is the intent, based on "If either of them has been set
before, we will not initialize the key anymore"?

Reinitialization is actually supported behavior.

                if (val & SOF_TIMESTAMPING_OPT_ID &&
                    !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {

But the sk_tsflags bit may be repeatedly set and cleared.

Anyway, the current patch sets it if either requests it?

+	tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
+		    sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);
 	if (val & SOF_TIMESTAMPING_OPT_ID &&
-	    !(sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] & SOF_TIMESTAMPING_OPT_ID)) {
+	    !(tsflags & SOF_TIMESTAMPING_OPT_ID)) {

 
> But the difference/problem is that the non-bpf feature only init it
> when connect() is done, but the bpf feature could do it at the
> beginning of connect(). If running txtimestamp -l 1000, the former
> will generate 999 for turkey while the latter 1000.