Re: [PATCH net-next v3 02/14] net-timestamp: allow two features to work parallelly

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

Jason Xing wrote:
> On Wed, Oct 30, 2024 at 9:45 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > On Wed, Oct 30, 2024 at 7:00 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> > > >
> > > > On 10/28/24 4:05 AM, Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > This patch has introduced a separate sk_tsflags_bpf for bpf
> > > > > extension, which helps us let two feature work nearly at the
> > > > > same time.
> > > > >
> > > > > Each feature will finally take effect on skb_shinfo(skb)->tx_flags,
> > > > > say, tcp_tx_timestamp() for TCP or skb_setup_tx_timestamp() for
> > > > > other types, so in __skb_tstamp_tx() we are unable to know which
> > > > > feature is turned on, unless we check each feature's own socket
> > > > > flag field.
> > > > >
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > ---
> > > > >   include/net/sock.h |  1 +
> > > > >   net/core/skbuff.c  | 39 +++++++++++++++++++++++++++++++++++++++
> > > > >   2 files changed, 40 insertions(+)
> > > > >
> > > > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > > > index 7464e9f9f47c..5384f1e49f5c 100644
> > > > > --- a/include/net/sock.h
> > > > > +++ b/include/net/sock.h
> > > > > @@ -445,6 +445,7 @@ struct sock {
> > > > >       u32                     sk_reserved_mem;
> > > > >       int                     sk_forward_alloc;
> > > > >       u32                     sk_tsflags;
> > > > > +     u32                     sk_tsflags_bpf;
> > > > >       __cacheline_group_end(sock_write_rxtx);
> > > > >
> > > > >       __cacheline_group_begin(sock_write_tx);
> > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > > > index 1cf8416f4123..39309f75e105 100644
> > > > > --- a/net/core/skbuff.c
> > > > > +++ b/net/core/skbuff.c
> > > > > @@ -5539,6 +5539,32 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
> > > > >
> > > > > +/* This function is used to test if application SO_TIMESTAMPING feature
> > > > > + * or bpf SO_TIMESTAMPING feature is loaded by checking its own socket flags.
> > > > > + */
> > > > > +static bool sk_tstamp_tx_flags(struct sock *sk, u32 tsflags, int tstype)
> > > > > +{
> > > > > +     u32 testflag;
> > > > > +
> > > > > +     switch (tstype) {
> > > > > +     case SCM_TSTAMP_SCHED:
> > > > > +             testflag = SOF_TIMESTAMPING_TX_SCHED;
> > > > > +             break;
> > > > > +     case SCM_TSTAMP_SND:
> > > > > +             testflag = SOF_TIMESTAMPING_TX_SOFTWARE;
> > > > > +             break;
> > > > > +     case SCM_TSTAMP_ACK:
> > > > > +             testflag = SOF_TIMESTAMPING_TX_ACK;
> > > > > +             break;
> > > > > +     default:
> > > > > +             return false;
> > > > > +     }
> > > > > +     if (tsflags & testflag)
> > > > > +             return true;
> > > > > +
> > > > > +     return false;
> > > > > +}
> > > > > +
> > > > >   static void skb_tstamp_tx_output(struct sk_buff *orig_skb,
> > > > >                                const struct sk_buff *ack_skb,
> > > > >                                struct skb_shared_hwtstamps *hwtstamps,
> > > > > @@ -5549,6 +5575,9 @@ static void skb_tstamp_tx_output(struct sk_buff *orig_skb,
> > > > >       u32 tsflags;
> > > > >
> > > > >       tsflags = READ_ONCE(sk->sk_tsflags);
> > > > > +     if (!sk_tstamp_tx_flags(sk, tsflags, tstype))
> > > >
> > > > I still don't get this part since v2. How does it work with cmsg only
> > > > SOF_TIMESTAMPING_TX_*?
> > > >
> > > > I tried with "./txtimestamp -6 -c 1 -C -N -L ::1" and it does not return any tx
> > > > time stamp after this patch.
> > > >
> > > > I am likely missing something
> > > > or v2 concluded that this behavior change is acceptable?
> > >
> > > Sorry, I submitted this series accidentally removing one important
> > > thing which is similar to what Vadim Fedorenko mentioned in the v1
> > > [1]:
> > > adding another member like sk_flags_bpf to handle the cmsg case.
> > >
> > > Willem, would it be acceptable to add another field in struct sock to
> > > help us recognise the case where BPF and cmsg works parallelly?
> > >
> > > [1]: https://lore.kernel.org/all/662873cb-a897-464e-bdb3-edf01363c3b2@linux.dev/
> >
> > The current timestamp flags don't need a u32. Maybe just reserve a bit
> > for this purpose?
> 
> Sure. Good suggestion.
> 
> But I think only using one bit to reflect whether the sk->sk_tsflags
> is used by normal or cmsg features is not enough. The existing
> implementation in tcp_sendmsg_locked() doesn't override the
> sk->sk_tsflags even the normal and cmsg features enabled parallelly.
> It only overrides sockc.tsflags in tcp_sendmsg_locked(). Based on
> that, even if at some point users suddenly remove the cmsg use and
> then the prior normal SO_TIMESTAMPING continues to work.
> 
> How about this, please see below:
> For now, sk->sk_tsflags only uses 17 bits (see the last one
> SOF_TIMESTAMPING_OPT_RX_FILTER). The cmsg feature only uses 4 flags
> (see SOF_TIMESTAMPING_TX_RECORD_MASK in __sock_cmsg_send()). With that
> said, we could reserve the highest four bits for cmsg use for the
> moment. Four bits represents four points where we can record the
> timestamp in the tx case.
> 
> Do you agree on this point?

I don't follow.

I probably miss the entire point.

The goal for sockcm fields is to start with the sk field and
optionally override based on cmsg. This is what sockcm_init does for
tsflags.

This information is for the skb, so these are recording flags.

Why does the new datapath need to know whether features are enabled
through setsockopt or on a per-call basis with a cmsg?

The goal was always to keep the reporting flags per socket, but make
the recording flag per packet, mainly for sampling.