Re: [PATCH 3/8] socket: Disentangle SOCK_RCVTSTAMPNS from SOCK_RCVTSTAMP

[Willem de Bruijn]

On Fri, Nov 30, 2018 at 5:16 PM Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
>
> On Sun, Nov 25, 2018 at 10:19 AM David Miller <davem@xxxxxxxxxxxxx> wrote:
> >
> > From: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> > Date: Sun, 25 Nov 2018 09:18:55 -0500
> >
> > > 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.