Jason Xing wrote:
On Thu, Sep 5, 2024 at 10:46 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_FILTER:
- Used in the receive software timestamp. Enabling the flag along with
- SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
- userspace so that it can filter out the case where one process starts
- first which turns on netstamp_needed_key through setting generation
- flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
- SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
I thought about last night as well. Like the patch [2/4] and this patch, the reason why I wanted to split is because I have to explain a lot for both hw and sw in one patch. One patch mixes different things.
No strong preference. If you still think so, I definitely can squash them as you said :)
No strong preference on 2/4. See other reply.
In this case, patch 1/4 introduces some behavior and 3/4 immediately updates it. I think it makes more sense to combine them.
Roger that. Will squash this one:)
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ int sock_set_timestamping(struct sock *sk, int optname, !(val & SOF_TIMESTAMPING_OPT_ID)) return -EINVAL;
if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&val & SOF_TIMESTAMPING_OPT_RX_FILTER)
if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTERSorry, I think my initial understanding at first read is not right. I was thinking you want this combination to pass the check in sock_set_timestamping().
If the users insist on receiving "hardware receive timestamps" with OPT_RX_FILTER enabled in this case, I think we should implement another new flag, say, OPT_RX_HARDWARE_FILTER...
My interpretation of the OPT_RX_FILTER flag is:
Only return RX timestamps if the socket also has the corresponding reporting flag set.
So it is valid to have
SOF_TIMESTAMPING_RAW_HARDWARE | SOF_TIMESTAMPING_RX_HARDWARE | SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_OPT_RX_FILTER
To filter out the software rx timestamps, but let through the hardware rx timestamps.
Oh, right, it can happen! RAW_HARDWARE is a little bit different, covering both ingress and egress path.
As said, it is a bit contrived. Feel free to disagree and keep as is too.
Well, I can keep it as is. It's easy for me, saving much energy, but...you already pointed out/ noticed this kind of use case that is not invalid.
If we want to tackle it well, we need to add a new flag for the hardware case, then we can individually control each of them, which is a more fine-grained control honestly. I'm totally fine with it as long as it will be good for users in the long run :)
If so, adding a new patch into this series (like patch [3/4]) seems inevitable. It won't take much time, I feel.
Any further thoughts?
Thanks, Jason