diff mbox series

[net-next,v4,3/3] selftests: txtimestamp: add SCM_TS_OPT_ID test

Message ID 20240909165046.644417-4-vadfed@meta.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add option to provide OPT_ID value via cmsg | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 14 this patch: 14
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 3 maintainers not CCed: shuah@kernel.org edumazet@google.com linux-kselftest@vger.kernel.org
netdev/build_clang fail Errors and warnings before: 15 this patch: 15
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success net selftest script(s) already in Makefile
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 85 this patch: 31
netdev/checkpatch warning WARNING: line length of 89 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vadim Fedorenko Sept. 9, 2024, 4:50 p.m. UTC 
Extend txtimestamp test to run with fixed tskey using
SCM_TS_OPT_ID control message for all types of sockets.

Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 tools/include/uapi/asm-generic/socket.h    |  2 +
 tools/testing/selftests/net/txtimestamp.c  | 48 +++++++++++++++++-----
 tools/testing/selftests/net/txtimestamp.sh | 12 +++---
 3 files changed, 47 insertions(+), 15 deletions(-)

Comments

Willem de Bruijn Sept. 9, 2024, 5:50 p.m. UTC | #1 
Vadim Fedorenko wrote:
> Extend txtimestamp test to run with fixed tskey using
> SCM_TS_OPT_ID control message for all types of sockets.
> 
> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>  tools/include/uapi/asm-generic/socket.h    |  2 +
>  tools/testing/selftests/net/txtimestamp.c  | 48 +++++++++++++++++-----
>  tools/testing/selftests/net/txtimestamp.sh | 12 +++---
>  3 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
> index 54d9c8bf7c55..281df9139d2b 100644
> --- a/tools/include/uapi/asm-generic/socket.h
> +++ b/tools/include/uapi/asm-generic/socket.h
> @@ -124,6 +124,8 @@
>  #define SO_PASSPIDFD		76
>  #define SO_PEERPIDFD		77
>  
> +#define SCM_TS_OPT_ID		78
> +
>  #if !defined(__KERNEL__)
>  
>  #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
> diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
> index ec60a16c9307..bdd0eb74326c 100644
> --- a/tools/testing/selftests/net/txtimestamp.c
> +++ b/tools/testing/selftests/net/txtimestamp.c
> @@ -54,6 +54,10 @@
>  #define USEC_PER_SEC	1000000L
>  #define NSEC_PER_SEC	1000000000LL
>  
> +#ifndef SCM_TS_OPT_ID
> +# define SCM_TS_OPT_ID 78
> +#endif

This should not be needed. And along with the uapi change above means
the test will be broken on other platforms.

(SO|SCM)_TXTIME ostensibly has the same issue and does not do this.
Vadim Fedorenko Sept. 9, 2024, 7:59 p.m. UTC | #2 
On 09/09/2024 18:50, Willem de Bruijn wrote:
> Vadim Fedorenko wrote:
>> Extend txtimestamp test to run with fixed tskey using
>> SCM_TS_OPT_ID control message for all types of sockets.
>>
>> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
>> Reviewed-by: Willem de Bruijn <willemb@google.com>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>>   tools/include/uapi/asm-generic/socket.h    |  2 +
>>   tools/testing/selftests/net/txtimestamp.c  | 48 +++++++++++++++++-----
>>   tools/testing/selftests/net/txtimestamp.sh | 12 +++---
>>   3 files changed, 47 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
>> index 54d9c8bf7c55..281df9139d2b 100644
>> --- a/tools/include/uapi/asm-generic/socket.h
>> +++ b/tools/include/uapi/asm-generic/socket.h
>> @@ -124,6 +124,8 @@
>>   #define SO_PASSPIDFD		76
>>   #define SO_PEERPIDFD		77
>>   
>> +#define SCM_TS_OPT_ID		78
>> +
>>   #if !defined(__KERNEL__)
>>   
>>   #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
>> diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
>> index ec60a16c9307..bdd0eb74326c 100644
>> --- a/tools/testing/selftests/net/txtimestamp.c
>> +++ b/tools/testing/selftests/net/txtimestamp.c
>> @@ -54,6 +54,10 @@
>>   #define USEC_PER_SEC	1000000L
>>   #define NSEC_PER_SEC	1000000000LL
>>   
>> +#ifndef SCM_TS_OPT_ID
>> +# define SCM_TS_OPT_ID 78
>> +#endif
> 
> This should not be needed. And along with the uapi change above means
> the test will be broken on other platforms.
> 
> (SO|SCM)_TXTIME ostensibly has the same issue and does not do this.

I had the same feeling, but apparently I wasn't able to build tests
without this addition. Looks like selftests rely on system's uapi rather
the one provided in tool/include/uapi.
With SCM_TXTIME it worked because the option was added back in 2018 in
80b14dee2bea ("net: Add a new socket option for a future transmit 
time.") by Richard while tests were added in 2019 by yourself in
af5136f95045 ("selftests/net: SO_TXTIME with ETF and FQ").

Though selftests do miss uapi for other architectures, it might be a
good reason to respin the series, but fixing selftests infra is a bit
different story, I believe... I may try to fix it and post another
series.
Willem de Bruijn Sept. 9, 2024, 8:31 p.m. UTC | #3 
Vadim Fedorenko wrote:
> On 09/09/2024 18:50, Willem de Bruijn wrote:
> > Vadim Fedorenko wrote:
> >> Extend txtimestamp test to run with fixed tskey using
> >> SCM_TS_OPT_ID control message for all types of sockets.
> >>
> >> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
> >> Reviewed-by: Willem de Bruijn <willemb@google.com>
> >> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> >> ---
> >>   tools/include/uapi/asm-generic/socket.h    |  2 +
> >>   tools/testing/selftests/net/txtimestamp.c  | 48 +++++++++++++++++-----
> >>   tools/testing/selftests/net/txtimestamp.sh | 12 +++---
> >>   3 files changed, 47 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
> >> index 54d9c8bf7c55..281df9139d2b 100644
> >> --- a/tools/include/uapi/asm-generic/socket.h
> >> +++ b/tools/include/uapi/asm-generic/socket.h
> >> @@ -124,6 +124,8 @@
> >>   #define SO_PASSPIDFD		76
> >>   #define SO_PEERPIDFD		77
> >>   
> >> +#define SCM_TS_OPT_ID		78
> >> +
> >>   #if !defined(__KERNEL__)
> >>   
> >>   #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
> >> diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
> >> index ec60a16c9307..bdd0eb74326c 100644
> >> --- a/tools/testing/selftests/net/txtimestamp.c
> >> +++ b/tools/testing/selftests/net/txtimestamp.c
> >> @@ -54,6 +54,10 @@
> >>   #define USEC_PER_SEC	1000000L
> >>   #define NSEC_PER_SEC	1000000000LL
> >>   
> >> +#ifndef SCM_TS_OPT_ID
> >> +# define SCM_TS_OPT_ID 78
> >> +#endif
> > 
> > This should not be needed. And along with the uapi change above means
> > the test will be broken on other platforms.
> > 
> > (SO|SCM)_TXTIME ostensibly has the same issue and does not do this.
> 
> I had the same feeling, but apparently I wasn't able to build tests
> without this addition. Looks like selftests rely on system's uapi rather
> the one provided in tool/include/uapi.

Right, as they should.

make headers_install will install headers by default under $KSRC/usr

tools/testing/selftests/net/Makefile has

CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES)

Haven't tried, but I assume this will pick up the right header
depending on the arch.

> With SCM_TXTIME it worked because the option was added back in 2018 in
> 80b14dee2bea ("net: Add a new socket option for a future transmit 
> time.") by Richard while tests were added in 2019 by yourself in
> af5136f95045 ("selftests/net: SO_TXTIME with ETF and FQ").
> 
> Though selftests do miss uapi for other architectures, it might be a
> good reason to respin the series, but fixing selftests infra is a bit
> different story, I believe... I may try to fix it and post another
> series.
Vadim Fedorenko Sept. 9, 2024, 8:37 p.m. UTC | #4 
On 09/09/2024 21:31, Willem de Bruijn wrote:
> Vadim Fedorenko wrote:
>> On 09/09/2024 18:50, Willem de Bruijn wrote:
>>> Vadim Fedorenko wrote:
>>>> Extend txtimestamp test to run with fixed tskey using
>>>> SCM_TS_OPT_ID control message for all types of sockets.
>>>>
>>>> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
>>>> Reviewed-by: Willem de Bruijn <willemb@google.com>
>>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>>> ---
>>>>    tools/include/uapi/asm-generic/socket.h    |  2 +
>>>>    tools/testing/selftests/net/txtimestamp.c  | 48 +++++++++++++++++-----
>>>>    tools/testing/selftests/net/txtimestamp.sh | 12 +++---
>>>>    3 files changed, 47 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
>>>> index 54d9c8bf7c55..281df9139d2b 100644
>>>> --- a/tools/include/uapi/asm-generic/socket.h
>>>> +++ b/tools/include/uapi/asm-generic/socket.h
>>>> @@ -124,6 +124,8 @@
>>>>    #define SO_PASSPIDFD		76
>>>>    #define SO_PEERPIDFD		77
>>>>    
>>>> +#define SCM_TS_OPT_ID		78
>>>> +
>>>>    #if !defined(__KERNEL__)
>>>>    
>>>>    #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
>>>> diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
>>>> index ec60a16c9307..bdd0eb74326c 100644
>>>> --- a/tools/testing/selftests/net/txtimestamp.c
>>>> +++ b/tools/testing/selftests/net/txtimestamp.c
>>>> @@ -54,6 +54,10 @@
>>>>    #define USEC_PER_SEC	1000000L
>>>>    #define NSEC_PER_SEC	1000000000LL
>>>>    
>>>> +#ifndef SCM_TS_OPT_ID
>>>> +# define SCM_TS_OPT_ID 78
>>>> +#endif
>>>
>>> This should not be needed. And along with the uapi change above means
>>> the test will be broken on other platforms.
>>>
>>> (SO|SCM)_TXTIME ostensibly has the same issue and does not do this.
>>
>> I had the same feeling, but apparently I wasn't able to build tests
>> without this addition. Looks like selftests rely on system's uapi rather
>> the one provided in tool/include/uapi.
> 
> Right, as they should.
> 
> make headers_install will install headers by default under $KSRC/usr
> 
> tools/testing/selftests/net/Makefile has
> 
> CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES)
> 
> Haven't tried, but I assume this will pick up the right header
> depending on the arch.

I see, thanks!
I'll respin (in usual 24 hours timeout) the series with nits fixes and 
will remove this ifdefs in selftests.

>> With SCM_TXTIME it worked because the option was added back in 2018 in
>> 80b14dee2bea ("net: Add a new socket option for a future transmit
>> time.") by Richard while tests were added in 2019 by yourself in
>> af5136f95045 ("selftests/net: SO_TXTIME with ETF and FQ").
>>
>> Though selftests do miss uapi for other architectures, it might be a
>> good reason to respin the series, but fixing selftests infra is a bit
>> different story, I believe... I may try to fix it and post another
>> series.
> 
>
diff mbox series

Patch

diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
index 54d9c8bf7c55..281df9139d2b 100644
--- a/tools/include/uapi/asm-generic/socket.h
+++ b/tools/include/uapi/asm-generic/socket.h
@@ -124,6 +124,8 @@ 
 #define SO_PASSPIDFD		76
 #define SO_PEERPIDFD		77
 
+#define SCM_TS_OPT_ID		78
+
 #if !defined(__KERNEL__)
 
 #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
index ec60a16c9307..bdd0eb74326c 100644
--- a/tools/testing/selftests/net/txtimestamp.c
+++ b/tools/testing/selftests/net/txtimestamp.c
@@ -54,6 +54,10 @@ 
 #define USEC_PER_SEC	1000000L
 #define NSEC_PER_SEC	1000000000LL
 
+#ifndef SCM_TS_OPT_ID
+# define SCM_TS_OPT_ID 78
+#endif
+
 /* command line parameters */
 static int cfg_proto = SOCK_STREAM;
 static int cfg_ipproto = IPPROTO_TCP;
@@ -77,6 +81,8 @@  static bool cfg_epollet;
 static bool cfg_do_listen;
 static uint16_t dest_port = 9000;
 static bool cfg_print_nsec;
+static uint32_t ts_opt_id;
+static bool cfg_use_cmsg_opt_id;
 
 static struct sockaddr_in daddr;
 static struct sockaddr_in6 daddr6;
@@ -136,12 +142,13 @@  static void validate_key(int tskey, int tstype)
 	/* compare key for each subsequent request
 	 * must only test for one type, the first one requested
 	 */
-	if (saved_tskey == -1)
+	if (saved_tskey == -1 || cfg_use_cmsg_opt_id)
 		saved_tskey_type = tstype;
 	else if (saved_tskey_type != tstype)
 		return;
 
 	stepsize = cfg_proto == SOCK_STREAM ? cfg_payload_len : 1;
+	stepsize = cfg_use_cmsg_opt_id ? 0 : stepsize;
 	if (tskey != saved_tskey + stepsize) {
 		fprintf(stderr, "ERROR: key %d, expected %d\n",
 				tskey, saved_tskey + stepsize);
@@ -480,7 +487,7 @@  static void fill_header_udp(void *p, bool is_ipv4)
 
 static void do_test(int family, unsigned int report_opt)
 {
-	char control[CMSG_SPACE(sizeof(uint32_t))];
+	char control[2 * CMSG_SPACE(sizeof(uint32_t))];
 	struct sockaddr_ll laddr;
 	unsigned int sock_opt;
 	struct cmsghdr *cmsg;
@@ -620,18 +627,32 @@  static void do_test(int family, unsigned int report_opt)
 		msg.msg_iov = &iov;
 		msg.msg_iovlen = 1;
 
-		if (cfg_use_cmsg) {
+		if (cfg_use_cmsg || cfg_use_cmsg_opt_id) {
 			memset(control, 0, sizeof(control));
 
 			msg.msg_control = control;
-			msg.msg_controllen = sizeof(control);
+			msg.msg_controllen = cfg_use_cmsg * CMSG_SPACE(sizeof(uint32_t));
+			msg.msg_controllen += cfg_use_cmsg_opt_id * CMSG_SPACE(sizeof(uint32_t));
 
-			cmsg = CMSG_FIRSTHDR(&msg);
-			cmsg->cmsg_level = SOL_SOCKET;
-			cmsg->cmsg_type = SO_TIMESTAMPING;
-			cmsg->cmsg_len = CMSG_LEN(sizeof(uint32_t));
+			cmsg = NULL;
+			if (cfg_use_cmsg) {
+				cmsg = CMSG_FIRSTHDR(&msg);
+				cmsg->cmsg_level = SOL_SOCKET;
+				cmsg->cmsg_type = SO_TIMESTAMPING;
+				cmsg->cmsg_len = CMSG_LEN(sizeof(uint32_t));
+
+				*((uint32_t *)CMSG_DATA(cmsg)) = report_opt;
+			}
+			if (cfg_use_cmsg_opt_id) {
+				cmsg = cmsg ? CMSG_NXTHDR(&msg, cmsg) : CMSG_FIRSTHDR(&msg);
+				cmsg->cmsg_level = SOL_SOCKET;
+				cmsg->cmsg_type = SCM_TS_OPT_ID;
+				cmsg->cmsg_len = CMSG_LEN(sizeof(uint32_t));
+
+				*((uint32_t *)CMSG_DATA(cmsg)) = ts_opt_id;
+				saved_tskey = ts_opt_id;
+			}
 
-			*((uint32_t *) CMSG_DATA(cmsg)) = report_opt;
 		}
 
 		val = sendmsg(fd, &msg, 0);
@@ -681,6 +702,7 @@  static void __attribute__((noreturn)) usage(const char *filepath)
 			"  -L    listen on hostname and port\n"
 			"  -n:   set no-payload option\n"
 			"  -N:   print timestamps and durations in nsec (instead of usec)\n"
+			"  -o N: use SCM_TS_OPT_ID control message to provide N as tskey\n"
 			"  -p N: connect to port N\n"
 			"  -P:   use PF_PACKET\n"
 			"  -r:   use raw\n"
@@ -701,7 +723,7 @@  static void parse_opt(int argc, char **argv)
 	int c;
 
 	while ((c = getopt(argc, argv,
-				"46bc:CeEFhIl:LnNp:PrRS:t:uv:V:x")) != -1) {
+				"46bc:CeEFhIl:LnNo:p:PrRS:t:uv:V:x")) != -1) {
 		switch (c) {
 		case '4':
 			do_ipv6 = 0;
@@ -742,6 +764,10 @@  static void parse_opt(int argc, char **argv)
 		case 'N':
 			cfg_print_nsec = true;
 			break;
+		case 'o':
+			ts_opt_id = strtoul(optarg, NULL, 10);
+			cfg_use_cmsg_opt_id = true;
+			break;
 		case 'p':
 			dest_port = strtoul(optarg, NULL, 10);
 			break;
@@ -799,6 +825,8 @@  static void parse_opt(int argc, char **argv)
 		error(1, 0, "cannot ask for pktinfo over pf_packet");
 	if (cfg_busy_poll && cfg_use_epoll)
 		error(1, 0, "pass epoll or busy_poll, not both");
+	if (cfg_proto == SOCK_STREAM && cfg_use_cmsg_opt_id)
+		error(1, 0, "TCP sockets don't support SCM_TS_OPT_ID");
 
 	if (optind != argc - 1)
 		error(1, 0, "missing required hostname argument");
diff --git a/tools/testing/selftests/net/txtimestamp.sh b/tools/testing/selftests/net/txtimestamp.sh
index 25baca4b148e..fe4649bb8786 100755
--- a/tools/testing/selftests/net/txtimestamp.sh
+++ b/tools/testing/selftests/net/txtimestamp.sh
@@ -37,11 +37,13 @@  run_test_v4v6() {
 run_test_tcpudpraw() {
 	local -r args=$@
 
-	run_test_v4v6 ${args}		# tcp
-	run_test_v4v6 ${args} -u	# udp
-	run_test_v4v6 ${args} -r	# raw
-	run_test_v4v6 ${args} -R	# raw (IPPROTO_RAW)
-	run_test_v4v6 ${args} -P	# pf_packet
+	run_test_v4v6 ${args}		  # tcp
+	run_test_v4v6 ${args} -u	  # udp
+	run_test_v4v6 ${args} -u -o 42	  # udp with fixed tskey
+	run_test_v4v6 ${args} -r	  # raw
+	run_test_v4v6 ${args} -r -o 42	  # raw
+	run_test_v4v6 ${args} -R	  # raw (IPPROTO_RAW)
+	run_test_v4v6 ${args} -P	  # pf_packet
 }
 
 run_test_all() {