Message-ID: <67157b7ec615_14e1829490@willemb.c.googlers.com.notmuch>
Date: Sun, 20 Oct 2024 17:51:58 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 willemdebruijn.kernel@...il.com, 
 willemb@...gle.com, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 martin.lau@...ux.dev, 
 eddyz87@...il.com, 
 song@...nel.org, 
 yonghong.song@...ux.dev, 
 john.fastabend@...il.com, 
 kpsingh@...nel.org, 
 sdf@...ichev.me, 
 haoluo@...gle.com, 
 jolsa@...nel.org
Cc: bpf@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2 04/12] net-timestamp: add static key to
 control the whole bpf extension

Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> 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@...cent.com>

Getting back to this thread. It is long, instead of responding to
multiple messages, let me combine them in a single response.


* On future extensions:

+1 that the UDP case, and datagrams more broadly, must have a clear
development path, before we can merge TCP.

Similarly, hardware timestamps need not be supported from the start,
but must clearly be supportable.


* On queueing packets to userspace:

> > the current behavior is to just queue to the sk_error_queue as long
> > as there is "SOF_TIMESTAMPING_TX_*" set in the skb's tx_flags and it
> > is regardless of the sk_tsflags. "

> Totally correct. SOF_TIMESTAMPING_SOFTWARE is a report flag while
> SOF_TIMESTAMPING_TX_* are generation flags. Without former, users can
> read the skb from the errqueue but are not able to parse the
> timestamps

Before queuing a packet to userspace on the error queue, the relevant
reporting flag is always tested. sock_recv_timestamp has:

        /*
         * generate control messages if
         * - receive time stamping in software requested
         * - software time stamp available and wanted
         * - hardware time stamps available and wanted
         */
        if (sock_flag(sk, SOCK_RCVTSTAMP) ||
            (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
            (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
            (hwtstamps->hwtstamp &&
             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
                __sock_recv_timestamp(msg, sk, skb);

Otherwise applications could get error messages queued, and
epoll/poll/select would unexpectedly behave differently.

> SOF_TIMESTAMPING_SOFTWARE is only used in traditional SO_TIMESTAMPING
> features including cmsg mode. But it will not be used in bpf mode. 

For simplicity, the two uses of the API are best kept identical. If
there is a technical reason why BPF has to diverge from established
behavior, this needs to be explicitly called out in the commit
message.

Also, if you want to extend the API for BPF in the future, good to
call this out now and ideally extensions will apply to both, to
maintain a uniform API.


* On extra measurement points, at sendmsg or tcp_write_xmit:

The first is interesting. For application timestamping, this was
never needed, as the application can just call clock_gettime before
sendmsg.

In general, additional measurement points are not only useful if the
interval between is not constant. So far, we have seen no need for
any additional points.


* On skb state:

> > For now, is there thing we can explore to share in the skb_shared_info?

skb_shinfo space is at a premium. I don't think we can justify two
extra fields just for this use case.

> My initial thought is just to reuse these fields in skb. It can work
> without interfering one another.

I'm skeptical that two methods can work at the same time. If they are
started at different times, their sk_tskey will be different, for one.

There may be workarounds. Maybe BPF can store its state in some BPF
specific field, indeed. Or perhaps it can store per-sk shadow state
that resolves the conflict. For instance, the offset between sk_tskey
and bpf_tskey.

Powered by blists - more mailing lists