On Fri, Nov 30, 2018 at 5:16 PM Deepa Dinamani [off-list ref] wrote:

On Sun, Nov 25, 2018 at 10:19 AM David Miller [off-list ref] wrote:

quoted

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Sun, 25 Nov 2018 09:18:55 -0500

quoted

The existing logic is as is for a reason. There is no need to change
it to satisfy the main purpose of your patchset?

It is structured as one bit to test whether a timestamp is requested
and another to select among two variants usec/nsec. Just add another
layer of branching between new/old in cases where this distinction is
needed.

Please avoid code churn unless needed.

+1

This patch makes it easier to add logic for 2 new socket time options.
But, if you prefer for all of the options to depend on SOCK_RCVTSTAMP
then I will drop it.

Yes, please keep as is.

I don't see how this change is needed to significantly simplify the
main patchset, and an unnecessary change can cause an unforeseen
regression (as was the case with doubling the tests in the hot path).

The current approach has one branch in the hot path where timestamps
are disabled and then selects from two variants where it is enabled:

    if (rcvtstamp) {
      if (rcvtstamp_ns)
        ..
      else
        ..
    }

Both of these need to be split into new and old variants. The way to
achieve that with minimal code perturbation is

    if (rcvtstamp) {
  +    if (sk_timestamping_new)
  +     return __sock_recv_timestamp_new(..)
  +
      if (rcvtstamp_ns)
        ..
      else
        ..
   }

Or alternatively add a check for new in each of the inner branches. In
any case, please be consistent between sock_recv_sw_timestamp and
tcp_recv_sw_timestamp. The current patchset alternates them.