On Wed, Oct 9, 2024 at 5:17 PM Vadim Fedorenko
<vadim.fedorenko@xxxxxxxxx> wrote:
>
> On 09/10/2024 00:48, Jason Xing wrote:
> > On Wed, Oct 9, 2024 at 3:18 AM Vadim Fedorenko
> > <vadim.fedorenko@xxxxxxxxx> wrote:
> >>
> >> On 08/10/2024 10:51, Jason Xing wrote:
> >>> From: Jason Xing <kernelxing@xxxxxxxxxxx>
> >>>
> >>> Once we set BPF_SOCK_OPS_TX_TIMESTAMP_OPT_CB_FLAG flag here, there
> >>> are three points in the previous patches where generating timestamps
> >>> works. Let us make the basic bpf mechanism for timestamping feature
> >>> work finally.
> >>>
> >>> We can use like this as a simple example in bpf program:
> >>> __section("sockops")
> >>>
> >>> case BPF_SOCK_OPS_TX_TIMESTAMP_OPT_CB:
> >>> dport = bpf_ntohl(skops->remote_port);
> >>> sport = skops->local_port;
> >>> skops->reply = SOF_TIMESTAMPING_TX_SCHED;
> >>> bpf_sock_ops_cb_flags_set(skops, BPF_SOCK_OPS_TX_TIMESTAMP_OPT_CB_FLAG);
> >>> case BPF_SOCK_OPS_TS_SCHED_OPT_CB:
> >>> bpf_printk(...);
> >>>
> >>> Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx>
> >>> ---
> >>> include/uapi/linux/bpf.h | 8 ++++++++
> >>> net/ipv4/tcp.c | 27 ++++++++++++++++++++++++++-
> >>> tools/include/uapi/linux/bpf.h | 8 ++++++++
> >>> 3 files changed, 42 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 1b478ec18ac2..6bf3f2892776 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -7034,6 +7034,14 @@ enum {
> >>> * feature is on. It indicates the
> >>> * recorded timestamp.
> >>> */
> >>> + BPF_SOCK_OPS_TX_TS_OPT_CB, /* Called when the last skb from
> >>> + * sendmsg is going to push when
> >>> + * SO_TIMESTAMPING feature is on.
> >>> + * Let user have a chance to switch
> >>> + * on BPF_SOCK_OPS_TX_TIMESTAMPING_OPT_CB_FLAG
> >>> + * flag for other three tx timestamp
> >>> + * use.
> >>> + */
> >>> };
> >>>
> >>> /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> >>> index 82cc4a5633ce..ddf4089779b5 100644
> >>> --- a/net/ipv4/tcp.c
> >>> +++ b/net/ipv4/tcp.c
> >>> @@ -477,12 +477,37 @@ void tcp_init_sock(struct sock *sk)
> >>> }
> >>> EXPORT_SYMBOL(tcp_init_sock);
> >>>
> >>> +static u32 bpf_tcp_tx_timestamp(struct sock *sk)
> >>> +{
> >>> + u32 flags;
> >>> +
> >>> + flags = tcp_call_bpf(sk, BPF_SOCK_OPS_TX_TS_OPT_CB, 0, NULL);
> >>> + if (flags <= 0)
> >>> + return 0;
> >>> +
> >>> + if (flags & ~SOF_TIMESTAMPING_MASK)
> >>> + return 0;
> >>> +
> >>> + if (!(flags & SOF_TIMESTAMPING_TX_RECORD_MASK))
> >>> + return 0;
> >>> +
> >>> + return flags;
> >>> +}
> >>> +
> >>> static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
> >>> {
> >>> struct sk_buff *skb = tcp_write_queue_tail(sk);
> >>> u32 tsflags = sockc->tsflags;
> >>> + u32 flags;
> >>> +
> >>> + if (!skb)
> >>> + return;
> >>> +
> >>> + flags = bpf_tcp_tx_timestamp(sk);
> >>> + if (flags)
> >>> + tsflags = flags;
> >>
> >> In this case it's impossible to clear timestamping flags from bpf
> >
> > It cannot be cleared only from the last skb until the next round of
> > recvmsg. Since the last skb is generated and bpf program is attached,
> > I would like to know why we need to clear the related fields in the
> > skb? Please note that I didn't hack the sk_tstflags in struct sock :)
>
> >> program, but it may be very useful. Consider providing flags from
> >> socket cookie to the program or maybe add an option to combine them?
> >
> > Thanks for this idea. May I ask what the benefits are through adding
> > an option because the bpf test statement (BPF_SOCK_OPS_TEST_FLAG) is a
> > good option to take a whole control? Or could you provide more details
> > about how you expect to do so?
>
> Well, as Willem mentioned, you are overriding flags completely. But what
> if an application is waiting for some type of timestamp to arrive, but
> bpf program rewrites flags and disables this type of timestamp? It will
> confuse application.
Indeed, this series doesn't handle the conflict very well. Initially,
I tried so hard to avoid implementing the feature again. But now, it
seems inevitable. Let me dig into it more.
>
> Thinking twice, clearing flags might not be useful because of the very
> same issue though.
Yes.
Thanks,
Jason
