Re: [PATCH net-next 5/9] net-timestamp: ready to turn on the button to generate tx timestamps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux