RE: [net-next, v3, 07/10] net: sock: extend SO_TIMESTAMPING for PHC binding

Hi Mat,

-----Original Message-----
From: Mat Martineau <redacted>
Sent: 2021年6月16日 9:01
To: Y.b. Lu <yangbo.lu@nxp.com>
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; Richard Cochran
[off-list ref]; David S . Miller [off-list ref];
Jakub Kicinski [off-list ref]; Matthieu Baerts
[off-list ref]; Shuah Khan [off-list ref]; Michal
Kubecek [off-list ref]; Florian Fainelli [off-list ref];
Andrew Lunn [off-list ref]; Rui Sousa [off-list ref]; Sebastien
Laveze [off-list ref]; Florian Westphal [off-list ref]
Subject: Re: [net-next, v3, 07/10] net: sock: extend SO_TIMESTAMPING for
PHC binding

On Tue, 15 Jun 2021, Yangbo Lu wrote:

quoted

Since PTP virtual clock support is added, there can be several PTP
virtual clocks based on one PTP physical clock for timestamping.

This patch is to extend SO_TIMESTAMPING API to support PHC (PTP
Hardware Clock) binding by adding a new flag
SOF_TIMESTAMPING_BIND_PHC. When PTP virtual clocks are in use, user
space can configure to bind one for timestamping, but PTP physical
clock is not supported and not needed to bind.

This patch is preparation for timestamp conversion from raw timestamp
to a specific PTP virtual clock time in core net.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v3:
	- Added this patch.
---
include/net/sock.h              |  8 +++-
include/uapi/linux/net_tstamp.h | 17 ++++++++-
net/core/sock.c                 | 65

+++++++++++++++++++++++++++++++--

quoted

net/ethtool/common.c            |  1 +
net/mptcp/sockopt.c             | 10 +++--
5 files changed, 91 insertions(+), 10 deletions(-)

[...]

quoted

@@ -166,7 +169,7 @@ static int

mptcp_setsockopt_sol_socket_tstamp(struct mptcp_sock *msk, int optnam

quoted

			break;
		case SO_TIMESTAMPING_NEW:
		case SO_TIMESTAMPING_OLD:
-			sock_set_timestamping(sk, optname, val);
+			sock_set_timestamping(sk, optname, val, optval, optlen);

This is inside a loop, so in cases where optlen == sizeof(struct
so_timestamping) this will end up re-copying the structure from userspace
one extra time for each MPTCP subflow: once for the MPTCP socket, plus one
time for each of the TCP subflows that are grouped under this MPTCP
connection.

Given that the extra copies only happen when using the extended bind_phc
option, it's not a huge cost. But sock_set_timestamping() was written to
avoid the extra copies for 'int' sized options, and if that was worth the
effort then the larger so_timestamping structure could be copied (once)
before the sock_set_timestamping() call and passed in.

I see now...
Let me pass so_timestamping structure in to avoid re-copying from userspace.

quoted

			break;
		}

@@ -207,7 +210,8 @@ static int mptcp_setsockopt_sol_socket_int(struct

mptcp_sock *msk, int optname,

quoted

	case SO_TIMESTAMPNS_NEW:
	case SO_TIMESTAMPING_OLD:
	case SO_TIMESTAMPING_NEW:
-		return mptcp_setsockopt_sol_socket_tstamp(msk, optname, val);
+		return mptcp_setsockopt_sol_socket_tstamp(msk, optname, val,
+							  optval, optlen);

Rather than modifying mptcp_setsockopt_sol_socket_int(), I suggest adding
a mptcp_setsockopt_sol_socket_timestamping() helper function that can
handle the special copying for so_timestamping.

Thank you. Let me do this in next version.

quoted

	}

	return -ENOPROTOOPT;
--
2.25.1

--
Mat Martineau
Intel