Jason Xing wrote:
On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
introduce a new flag SOF_TIMESTAMPING_OPT_RX_FILTER in the receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter out rx software timestamp report, especially after a process turns on netstamp_needed_key which can time stamp every incoming skb.
Previously, we found out if an application starts first which turns on netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE could also get rx timestamp. Now we handle this case by introducing this new flag without breaking users.
Quoting Willem to explain why we need the flag: "why a process would want to request software timestamp reporting, but not receive software timestamp generation. The only use I see is when the application does request SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive timestamp.
Another thing about errqueue in this patch I have a few words to say: In this case, we need to handle the egress path carefully, or else reporting the tx timestamp will fail. Egress path and ingress path will finally call sock_recv_timestamp(). We have to distinguish them. Errqueue is a good indicator to reflect the flow direction.
Suggested-by: Willem de Bruijn willemb@google.com Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
Let's assume: if we remove all the report flags design, what will happen? It can work of course. I don't believe that people choose to enable the generation flag but are not willing to report it. Of course, It's another thing. I'm just saying.
Let's not debate the existing API. Its design predates both of our contributions.
Yep.
I wonder if it makes sense to you :) ?
I don't see a strong use case. I know we're on v5 while I reopen that point, sorry.
That's all right. No worries.
It seems simpler to me to not read spurious fields that are not requested, rather than to explicitly request them to be set to zero.
Adding more flags is not free. An extra option adds mental load for casual users of this alread complex API. This may certainly sound hypocritical coming from me, as I added my fair share. But I hope their functionality outweighs that cost (well.. in at least one case it was an ugly fix for a bad first attempt.. OPT_ID).
I understand what you meant here. But I'm lost...
For some users, they might use the tsflags in apps to test whether they need to receive/report the rx timestamp or not, and someday they will notice there are unexpected timestamps that come out. As we know, it's more of a design consideration about whether the users can control it by setsockopt...
In addition to the design itself, above is the only use case I know.
Ok. I'm on the fence, but not a hard no. Evidently you see value, so others may too.
A pendantic use case is if the caller expects other cmsg, but not these. Then the amount of cmsg space used will depend on a third process enabling receive timestamps. Again, can be worked around. But admittedly is surprising behavior.
I got to this point trying to condense the proposed documentation. We can add this if you feel strongly.
If the new flag is not good for future development, we can stop it and then _only_ document the special case, which we both agreed about a week ago.
Personally, I don't want to let it go easily. But It's just me. You are the maintainer, so you have to make the decision. I'm totally fine with either way. Thanks.
I was only trying to make the feature better. At least, we both have tried a lot.
But then my main feedback is that the doc has to be shorter and to
It's truly very long, to be honest. I thought I needed to list the possible combination use cases.
the point. Why would a user user this? No background on how we got here, what they might already do accidentally.
It looks like I should remove those use cases? And then clarify the reason is per socket control?
I have no idea if I should continue on this.
My attempt to document, feel free to revise:
SOF_TIMESTAMPING_OPT_RX_FILTER:
Filter out spurious receive timestamps: report a receive timestamp only if the matching timestamp generation flag is enabled.
Receive timestamps are generated early in the ingress path, before a packet's destination socket is known. If any socket enables receive timestamps, packets for all socket will receive timestamped packets. Including those that request timestamp reporting with SOF_TIMESTAMPING_SOFTWARE and/or SOF_TIMESTAMPING_RAW_HARDWARE, but do not request receive timestamp generation. This can happen when requesting transmit timestamps only.
Receiving spurious timestamps is generally benign. A process can ignore the unexpected non-zero value. But it makes behavior subtly dependent on other sockets. This flag isolates the socket for more deterministic behavior.