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. > > 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. Thanks, Jason
