Jason Xing wrote:

On Wed, Aug 28, 2024 at 9:44 PM Willem de Bruijn
[off-list ref] wrote:

quoted

quoted

quoted

quoted

I can see what you mean here: you don't like combining the reporting
flag and generation flag, right? But If we don't check whether those
two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
protocols like udp will fail as we talked before.

netstamp_needed_key cannot be implemented as per socket feature (at
that time when the driver just pass the skb to the rx stack, we don't
know which socket the skb belongs to). Since we cannot prevent this
from happening during its generation period, I suppose we can delay
the check and try to stop it when it has to report, I mean, in
sock_recv_timestamp().

Or am I missing something? What would you suggest?

quoted

        /*
         * 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);

I evidently already noticed this back in 2014, when I left a note in
commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
sk_flags"):

    SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
    timestamp logic (netstamp_needed). That can be simplified and this
    last key removed, but will leave that for a separate patch.

But I do not see __sock_recv_timestamp toggling the feature either
then or now, so I think this is vestigial and can be removed.

After investigating more of it, as your previous commit said, the
legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
flag from enum sock_flags {}, right? Do you expect me to do that? If
so, I would love to do it :)

I did not say that. I said that the specific line here appears
vestigial.

quoted

quoted

quoted

quoted

        if (sock_flag(sk, SOCK_RCVTSTAMP) ||
            (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||

Thanks for your patience.

I think I will remove the above two lines as one patch with your
suggested-by tag in the series so that later we can easily review each
of them.

Please don't. As I said right below this:

One thing at a time. Let's focus on the change you proposed to me.

The above is just a hunch on first read. I would have to spend more
time to convince myself that it is indeed correct and safe. At which
point I might as well send it myself too..

More importantly, let's not expand what is intended to be a stand
alone improvement with tangential changes. It just makes reviewing
harder and slows it down.
 

quoted

One thing at a time. Let's focus on the change you proposed to me.

quoted

But I still don't get it when you say "__sock_recv_timestamp toggling
the feature", could you say more, please? I'm not sure if it has
something to do with the above line.

SOF_TIMESTAMPING_RX_SOFTWARE is a request to enable software receive
timestamp *generation*. This is done by calling net_enable_timestamp.

I did not immediately see a path from __sock_recv_timestamp to
net_enable_timestamp, so I don't see a point in entering that function
based on this flag.

I see. It does make sense. We don't need the generation flag to test
if we need to report here.

I will remove them by quoting your saying.

See above.

quoted

quoted

quoted

quoted

quoted

I can see the value of your extra filter. Given the above examples, it
won't be the first subtle variance from the API design, either.

Really appreciate that you understand me :)

quoted

So either way is fine with me: change it or leave it.

But in both ways, yes: please update the documentation accordingly.

Roger that, sir. I will do it.

quoted

And if you do choose to change it, please be ready to revert on report
of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
because that always worked as they subtly relied on another daemon to
enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.

Yes, I still chose to change it and try to make it in the correct
direction. So if there are future reports, please let me know, I will
surely keep a close eye on it.

Sounds good, thanks.

So let me organize my thoughts here.

In the next move, I would do such things:
1) keep two patches in this series as they are.
2) add some descriptions about "this commit introduces subtle
variance, if the application that only pass
SOF_TIMESTAMPING_SOFTWARE..." something like this in the Documentation
file.

Make it crystal clear that the distinction between timestamp
generation and timestamp reporting, which this document goes out of
its way to explain, does not hold for receive timestamping.

Sure, I will add it into the documentation as well.

Thanks for your help.