Re: [PATCH net-next v2 04/12] net-timestamp: add static key to control the whole bpf extension

Willem de Bruijn wrote:

Martin KaFai Lau wrote:

quoted

On 10/20/24 2:51 PM, Willem de Bruijn wrote:

quoted

Jason Xing wrote:

quoted

From: Jason Xing <kernelxing@tencent.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@tencent.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:

quoted

quoted

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. "

quoted

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.

I just tried the following diff to remove setsockopt from txtimestamp.c and run 
"./txtimestamp -6 -c 1 -C -N -L ::1". It is getting the skb from the error queue 
with only cmsg flag.

That it surprising and against the API intent as I understand it.
Let me reproduce and take a closer look.

Interesting. I guess my interpretation was wrong.

The reporting flags prevent reporting of the timestamp, but not
queuing of the skb on the error queue. Even if the only purpose is to
report a timestamp.

It goes back until well before all the API extensions. At least v3.6.

It still does suppress the timestamp itself if the relevant reporting
flag, SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE, is
not set. So BPF should really still match that, I guess.