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

[Y.b. Lu]

Hi Mat,

> -----Original Message-----
> From: Mat Martineau <mathew.j.martineau@xxxxxxxxxxxxxxx>
> Sent: 2021年6月16日 9:01
> To: Y.b. Lu <yangbo.lu@xxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-kselftest@xxxxxxxxxxxxxxx; mptcp@xxxxxxxxxxxxxxx; Richard Cochran
> <richardcochran@xxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> Jakub Kicinski <kuba@xxxxxxxxxx>; Matthieu Baerts
> <matthieu.baerts@xxxxxxxxxxxx>; Shuah Khan <shuah@xxxxxxxxxx>; Michal
> Kubecek <mkubecek@xxxxxxx>; Florian Fainelli <f.fainelli@xxxxxxxxx>;
> Andrew Lunn <andrew@xxxxxxx>; Rui Sousa <rui.sousa@xxxxxxx>; Sebastien
> Laveze <sebastien.laveze@xxxxxxx>; Florian Westphal <fw@xxxxxxxxx>
> Subject: Re: [net-next, v3, 07/10] net: sock: extend SO_TIMESTAMPING for
> PHC binding
> 
> On Tue, 15 Jun 2021, Yangbo Lu wrote:
> 
> > 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@xxxxxxx>
> > ---
> > Changes for v3:
> > 	- Added this patch.
> > ---
> > include/net/sock.h              |  8 +++-
> > include/uapi/linux/net_tstamp.h | 17 ++++++++-
> > net/core/sock.c                 | 65
> +++++++++++++++++++++++++++++++--
> > net/ethtool/common.c            |  1 +
> > net/mptcp/sockopt.c             | 10 +++--
> > 5 files changed, 91 insertions(+), 10 deletions(-)
> >
[...]
> > @@ -166,7 +169,7 @@ static int
> mptcp_setsockopt_sol_socket_tstamp(struct mptcp_sock *msk, int optnam
> > 			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.

> 
> > 			break;
> > 		}
> >
> > @@ -207,7 +210,8 @@ static int mptcp_setsockopt_sol_socket_int(struct
> mptcp_sock *msk, int optname,
> > 	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.

> 
> > 	}
> >
> > 	return -ENOPROTOOPT;
> > --
> > 2.25.1
> >
> >
> 
> --
> Mat Martineau
> Intel