Hello Willem, On Mon, Sep 9, 2024 at 3:41 AM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Jason Xing wrote: > > On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > Jason Xing wrote: > > > > On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn > > > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > > > > > Jason Xing wrote: > > > > > > From: Jason Xing <kernelxing@xxxxxxxxxxx> > > > > > > > > > > > > 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@xxxxxxxxxx> > > > > > > Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx> > > > > > > > > > > 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. Willem, thanks so much for your effort!!! I learn a lot these days. I'm going to completely replace it with your description. Thanks, Jason
