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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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.
> 
>