On Wed, Oct 30, 2024 at 9:45 AM Willem de Bruijn
<willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> Jason Xing wrote:
> > On Wed, Oct 30, 2024 at 7:00 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> > >
> > > On 10/28/24 4:05 AM, Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxx/
>
> 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?
Thanks,
Jason
