On Wed, Oct 16, 2024 at 9:04 AM Jason Xing <kerneljasonxing@xxxxxxxxx> wrote:
>
> On Wed, Oct 16, 2024 at 8:10 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> >
> > On 10/11/24 9:06 PM, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@xxxxxxxxxxx>
> > >
> > > Willem suggested that we use a static key to control. The advantage
> > > is that we will not affect the existing applications at all if we
> > > don't load BPF program.
> > >
> > > In this patch, except the static key, I also add one logic that is
> > > used to test if the socket has enabled its tsflags in order to
> > > support bpf logic to allow both cases to happen at the same time.
> > > Or else, the skb carring related timestamp flag doesn't know which
> > > way of printing is desirable.
> > >
> > > One thing important is this patch allows print from both applications
> > > and bpf program at the same time. Now we have three kinds of print:
> > > 1) only BPF program prints
> > > 2) only application program prints
> > > 3) both can print without side effect
> > >
> > > Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx>
> > > ---
> > > include/net/sock.h | 1 +
> > > net/core/filter.c | 3 +++
> > > net/core/skbuff.c | 38 ++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 42 insertions(+)
> > >
> > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > index 66ecd78f1dfe..b7c51b95c92d 100644
> > > --- a/include/net/sock.h
> > > +++ b/include/net/sock.h
> > > @@ -2889,6 +2889,7 @@ static inline bool sk_dev_equal_l3scope(struct sock *sk, int dif)
> > > void sock_def_readable(struct sock *sk);
> > >
> > > int sock_bindtoindex(struct sock *sk, int ifindex, bool lock_sk);
> > > +DECLARE_STATIC_KEY_FALSE(bpf_tstamp_control);
> > > void sock_set_timestamp(struct sock *sk, int optname, bool valbool);
> > > int sock_get_timestamping(struct so_timestamping *timestamping,
> > > sockptr_t optval, unsigned int optlen);
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 996426095bd9..08135f538c99 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -5204,6 +5204,8 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
> > > .arg1_type = ARG_PTR_TO_CTX,
> > > };
> > >
> > > +DEFINE_STATIC_KEY_FALSE(bpf_tstamp_control);
> > > +
> > > static int bpf_sock_set_timestamping(struct sock *sk,
> > > struct so_timestamping *timestamping)
> > > {
> > > @@ -5217,6 +5219,7 @@ static int bpf_sock_set_timestamping(struct sock *sk,
> > > return -EINVAL;
> > >
> > > WRITE_ONCE(sk->sk_tsflags[BPFPROG_TS_REQUESTOR], flags);
> > > + static_branch_enable(&bpf_tstamp_control);
> >
> > Not sure when is a good time to do static_branch_disable().
>
> Thanks for the review.
>
> To be honest, I considered how to disable the static key. Like you
> said, I failed to find a good chance that I can accurately disable it.
>
> >
> > The bpf prog may be detached also. (IF) it ends up staying with the
> > cgroup/sockops interface, it should depend on the existing static key in
> > cgroup_bpf_enabled(CGROUP_SOCK_OPS) instead of adding another one.
>
> Are you suggesting that we need to remove the current static key? In
> the previous thread, the reason why Willem came up with this idea is,
> I think, to avoid affect the non-bpf timestamping feature.
>
> >
> > >
> > > return 0;
> > > }
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index f36eb9daa31a..d0f912f1ff7b 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > > @@ -5540,6 +5540,29 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
> > > }
> > > EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
> > >
> > > +static bool sk_tstamp_tx_flags(struct sock *sk, u32 tsflags, int tstype)
> >
> > sk is unused.
>
> Thanks for the careful check.
>
> >
> > > +{
> > > + u32 testflag;
> > > +
> > > + switch (tstype) {
> > > + case SCM_TSTAMP_SCHED:
> >
> > Instead of doing this translation,
> > is it easier to directly store the bpf prog desired ts"type" (i.e. the
> > SCM_TSTAMP_*) in the sk->sk_tsflags_bpf?
> > or there is a specific need to keep the SOF_TIMESTAMPING_* value in
> > sk->sk_tsflags_bpf?
>
> We have to reuse SOF_TIMESTAMPING_* because there are more flags, say,
> SOF_TIMESTAMPING_OPT_ID, that we need to support.
>
> >
> > > + 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,
> > > @@ -5558,6 +5581,9 @@ static void skb_tstamp_tx_output(struct sk_buff *orig_skb,
> > > if (!skb_may_tx_timestamp(sk, tsonly))
> > > return;
> > >
> > > + if (!sk_tstamp_tx_flags(sk, tsflags, tstype))
> >
> > This is a new test. tsflags is the sk->sk_tsflags here if I read it correctly.
>
> This test will be used in bpf and non-bpf cases. Because of this, we
> can support BPF extension. In this function, if skb has tsflags but we
> don't know which approach the user expects, sk_tstamp_tx_flags() can
> help us.
>
> >
> > My understanding is the sendmsg can provide SOF_TIMESTAMPING_* for individual
> > skb. Would it break?
>
> Oh, you're right. I didn't support cmsg mode...
I think I only need to test if it's in the bpf mode, or else let the
original way print the timestamp, which can solve the issue.
