On Fri, Nov 30, 2018 at 5:16 PM Deepa Dinamani deepa.kernel@gmail.com wrote:
On Sun, Nov 25, 2018 at 10:19 AM David Miller davem@davemloft.net wrote:
From: Willem de Bruijn willemdebruijn.kernel@gmail.com 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.