Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, May 01, 2025 at 02:46:46PM +0200, Peter Zijlstra wrote: > On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote: > > On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: > > > On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: > > > > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: > > > > > On 10/16/24 04:20, Lorenzo Stoakes wrote: > > > ... > > > > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h > > > > > > b/tools/testing/selftests/pidfd/pidfd.h > > > > > > index 88d6830ee004..1640b711889b 100644 > > > > > > --- a/tools/testing/selftests/pidfd/pidfd.h > > > > > > +++ b/tools/testing/selftests/pidfd/pidfd.h > > > > > > @@ -50,6 +50,14 @@ > > > > > >#define PIDFD_NONBLOCK O_NONBLOCK > > > > > >#endif > > > > > > +/* System header file may not have this available. */ > > > > > > +#ifndef PIDFD_SELF_THREAD > > > > > > +#define PIDFD_SELF_THREAD -100 > > > > > > +#endif > > > > > > +#ifndef PIDFD_SELF_THREAD_GROUP > > > > > > +#define PIDFD_SELF_THREAD_GROUP -200 > > > > > > +#endif > > > > > > + > > > > > > > > > > As mentioned in my response to v1 patch: > > > > > > > > > > kselftest has dependency on "make headers" and tests include > > > > > headers from linux/ directory > > > > > > > > Right but that assumes you install the kernel headers on the build > > > > system, > > > > which is quite a painful thing to have to do when you are quickly > > > > iterating > > > > on a qemu setup. > > > > > > > > This is a use case I use all the time so not at all theoretical. > > > > > > > > > > This is turning out to be a fairly typical reaction from kernel > > > developers, when presented with the "you must first run make headers" > > > requirement for kselftests. > > > > > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most > > > colorful, so I'll helpfully cite it here. :) > > > > Let me re-try this. > > > > This is driving me insane. I've spend the past _TWO_ days trying to > > build KVM selftests and I'm still failing. > > > > This is absolute atrocious crap and is costing me valuable time. > > > > Please fix this fucking selftests shit to just build. This is unusable > > garbage. > > So after spending more time trying to remember how to debug Makefiles (I > hate my life), I found that not only do I need this headers shit, the > kvm selftests Makefile is actively broken if you use: make O=foo > > -INSTALL_HDR_PATH = $(top_srcdir)/usr > +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr > > > And then finally, I can do: > > make O=foo headers_install > make O=foo -C tools/testing/selftests/kvm/ > > So yeah, thank you very much for wasting my time *AGAIN*. And *AGAIN*.. this is still the state of things. Selftests are still hopelessly broken and useless. Maybe we should just delete the lot?
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 5/6/25 15:34, John Hubbard wrote: On 5/6/25 2:18 PM, Shuah Khan wrote: On 5/1/25 05:42, Peter Zijlstra wrote: On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: On 10/16/24 04:20, Lorenzo Stoakes wrote: ... Please fix this fucking selftests shit to just build. This is unusable garbage. Peter, John, There seems to be confusion regarding KHDR_INCLUDES. Tests don't have to use KHDR_INCLUDES if they don't want to. There are 4623 test Makefiles (excluding the main Makefile) under selftests/. Out of those 73 Makefiles reference KHDR_INCLUDES exported by lib.mk and selftests/Makefile. The rest are happy with system headers. The support for this KHDR_INCLUDES was added just for the case when a new test depends on header change. This is the reason why only a few test Makefiles use it. When test rings ran into issues related to dependencies between header changes, we recommended installing headers to solve the problem and introduced KHDR_INCLUDES so test Makefiles can use it in their Makefiles overriding the framework defaults. If your test doesn't need it, you can simply stop referencing it or use the approach used in mm test. It is a manual step. Works well for developers who know what they are doing. This isn't ideal for test rings. This isn't an ideal solution really. It works for the mm developers. # In order to use newer items that haven't yet been added to the user's system # header files, add $(TOOLS_INCLUDES) to the compiler invocation in each # each selftest. # You may need to add files to that location, or to refresh an existing file. In # order to do that, run "make headers" from $(top_srcdir), then copy the # header file that you want from $(top_srcdir)/usr/include/... , to the matching # subdir in $(TOOLS_INCLUDE). TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi The issues Peter is seeing regarding KHDR_INCLUDES in the following tests can be easily fixed by simply changing the test Makefile. These aren't framework related. kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) x86/Makefile:CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) futex/functional/Makefile:INCLUDES := -I../include -I../../ $(KHDR_INCLUDES) futex/functional/Makefile:CFLAGS := $(CFLAGS) -g -O2 -Wall -pthread $(INCLUDES) $(KHDR_INCLUDES) You can make the change to remove the reference to KHDR_INCLUDES. If don't have the time/bandwidth to do it, I will take care of it. If test build fails, you can then figure out how to address that. Hopefully build issues related to header changes are infrequent. thanks, -- Shuah
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 5/6/25 2:18 PM, Shuah Khan wrote:
> On 5/1/25 05:42, Peter Zijlstra wrote:
>> On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
>>> On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
> On 10/16/24 04:20, Lorenzo Stoakes wrote:
>>> ...
>> Please fix this fucking selftests shit to just build. This is unusable
>> garbage.
>
> I don't recall all the reasons why kselftests needed "make headers"
> One reason I could think of was that when a new test depends on a
> header change, the test won't build unless headers are installed.
...or until an updated copy of that updated header file is copied
somewhere, and then included in the kselftests. That's the approach
that I ultimately settled upon, after some discussion and negotion.
Details below.
>
> If this requirement is causing problems for tests that don't fall
> into the category and we probably have more of them mow, we can
> clean that up.
>
> John, you mentioned you got mm tests working without headers?
> Can you share the commit here.
>
Yes. This one sets up the general approach, which is available to
all kselftests: TOOLS_INCLUDES. It also changes selftests/mm to
set TOOLS_INCLUDES in that build:
e076eaca5906 ("selftests: break the dependency upon local header files")
And here is a representative application of the above, to selftests/mm. In
other words, taking advantage of the new file location pointed to by
TOOLS_INCLUDES:
580ea358af0a ("selftests/mm: fix additional build errors for selftests")
thanks,
--
John Hubbard
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 5/1/25 05:42, Peter Zijlstra wrote: On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: On 10/16/24 04:20, Lorenzo Stoakes wrote: ... diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif + As mentioned in my response to v1 patch: kselftest has dependency on "make headers" and tests include headers from linux/ directory Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup. This is a use case I use all the time so not at all theoretical. This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests. Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :) Let me re-try this. This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing. This is absolute atrocious crap and is costing me valuable time. Please fix this fucking selftests shit to just build. This is unusable garbage. I don't recall all the reasons why kselftests needed "make headers" One reason I could think of was that when a new test depends on a header change, the test won't build unless headers are installed. If this requirement is causing problems for tests that don't fall into the category and we probably have more of them mow, we can clean that up. John, you mentioned you got mm tests working without headers? Can you share the commit here. thanks, -- Shuah
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Mon, May 05, 2025 at 03:35:13PM +0200, Christian Brauner wrote: > I'm completely lost as to what's happening here or whether the test here > is somehow at fault for something. > > The pidfd.h head explicitly has no dependency on the pidfd uapi header > itself and I will NAK anything that makes it so. It's just a giant pain. There was a debate in my series here about my having to make things work with 'make headers', but thanks to John we resolved it the sane way by using the tools/ stuff. I _believe_ Peter is just using this thread as an example of a recent case of people being asked to do this insanity (correct me if I'm wrong Peter) and it's unrelated to pidfs in general.
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, May 01, 2025 at 02:46:46PM +0200, Peter Zijlstra wrote: > On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote: > > On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: > > > On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: > > > > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: > > > > > On 10/16/24 04:20, Lorenzo Stoakes wrote: > > > ... > > > > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h > > > > > > b/tools/testing/selftests/pidfd/pidfd.h > > > > > > index 88d6830ee004..1640b711889b 100644 > > > > > > --- a/tools/testing/selftests/pidfd/pidfd.h > > > > > > +++ b/tools/testing/selftests/pidfd/pidfd.h > > > > > > @@ -50,6 +50,14 @@ > > > > > >#define PIDFD_NONBLOCK O_NONBLOCK > > > > > >#endif > > > > > > +/* System header file may not have this available. */ > > > > > > +#ifndef PIDFD_SELF_THREAD > > > > > > +#define PIDFD_SELF_THREAD -100 > > > > > > +#endif > > > > > > +#ifndef PIDFD_SELF_THREAD_GROUP > > > > > > +#define PIDFD_SELF_THREAD_GROUP -200 > > > > > > +#endif > > > > > > + > > > > > > > > > > As mentioned in my response to v1 patch: > > > > > > > > > > kselftest has dependency on "make headers" and tests include > > > > > headers from linux/ directory > > > > > > > > Right but that assumes you install the kernel headers on the build > > > > system, > > > > which is quite a painful thing to have to do when you are quickly > > > > iterating > > > > on a qemu setup. > > > > > > > > This is a use case I use all the time so not at all theoretical. > > > > > > > > > > This is turning out to be a fairly typical reaction from kernel > > > developers, when presented with the "you must first run make headers" > > > requirement for kselftests. > > > > > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most > > > colorful, so I'll helpfully cite it here. :) > > > > Let me re-try this. > > > > This is driving me insane. I've spend the past _TWO_ days trying to > > build KVM selftests and I'm still failing. > > > > This is absolute atrocious crap and is costing me valuable time. > > > > Please fix this fucking selftests shit to just build. This is unusable > > garbage. > > So after spending more time trying to remember how to debug Makefiles (I > hate my life), I found that not only do I need this headers shit, the > kvm selftests Makefile is actively broken if you use: make O=foo > > -INSTALL_HDR_PATH = $(top_srcdir)/usr > +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr > > > And then finally, I can do: > > make O=foo headers_install > make O=foo -C tools/testing/selftests/kvm/ > > So yeah, thank you very much for wasting my time *AGAIN*. > > > Seriously, I want to be able to do: > > cd tools/testing/selftests/foo; make > > and have it just work. I would strongly suggest every subsystem to > reclaim their selftests and make it so again. > > And on that, let me go merge the fixes I need to have x86 and futex > build without this headers shit. I'm completely lost as to what's happening here or whether the test here is somehow at fault for something. The pidfd.h head explicitly has no dependency on the pidfd uapi header itself and I will NAK anything that makes it so. It's just a giant pain.
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, May 01, 2025, Peter Zijlstra wrote: > On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote: > > On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: > > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most > > > colorful, so I'll helpfully cite it here. :) > > > > Let me re-try this. > > > > This is driving me insane. I've spend the past _TWO_ days trying to > > build KVM selftests and I'm still failing. > > > > This is absolute atrocious crap and is costing me valuable time. > > > > Please fix this fucking selftests shit to just build. This is unusable > > garbage. > > So after spending more time trying to remember how to debug Makefiles (I > hate my life), I found that not only do I need this headers shit, the > kvm selftests Makefile is actively broken if you use: make O=foo > > -INSTALL_HDR_PATH = $(top_srcdir)/usr > +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr > > > And then finally, I can do: > > make O=foo headers_install > make O=foo -C tools/testing/selftests/kvm/ This doesn't actually work either, because for whatever reason, the selftests infrastructure uses OUTPUT, not O, for the output directory. And the whole top_srcdir crud doesn't work if O/OUTPUT is completely out-of-tree, e.g. I use absolute paths that have nothing to do with the source tree. I am more than happy to support any cleanup of KVM selftests, but I've more or less given up myself because so much of the ugliness is inhereted from selftests. I've resorted to hacked wrappers to make it work for my setup. E.g. I force KHDR_INCLUDES and INSTALL_HDR_PATH so that make doesn't try to grab usr/ files from the source tree.
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote: > On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: > > On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: > > > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: > > > > On 10/16/24 04:20, Lorenzo Stoakes wrote: > > ... > > > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h > > > > > b/tools/testing/selftests/pidfd/pidfd.h > > > > > index 88d6830ee004..1640b711889b 100644 > > > > > --- a/tools/testing/selftests/pidfd/pidfd.h > > > > > +++ b/tools/testing/selftests/pidfd/pidfd.h > > > > > @@ -50,6 +50,14 @@ > > > > >#define PIDFD_NONBLOCK O_NONBLOCK > > > > >#endif > > > > > +/* System header file may not have this available. */ > > > > > +#ifndef PIDFD_SELF_THREAD > > > > > +#define PIDFD_SELF_THREAD -100 > > > > > +#endif > > > > > +#ifndef PIDFD_SELF_THREAD_GROUP > > > > > +#define PIDFD_SELF_THREAD_GROUP -200 > > > > > +#endif > > > > > + > > > > > > > > As mentioned in my response to v1 patch: > > > > > > > > kselftest has dependency on "make headers" and tests include > > > > headers from linux/ directory > > > > > > Right but that assumes you install the kernel headers on the build system, > > > which is quite a painful thing to have to do when you are quickly > > > iterating > > > on a qemu setup. > > > > > > This is a use case I use all the time so not at all theoretical. > > > > > > > This is turning out to be a fairly typical reaction from kernel > > developers, when presented with the "you must first run make headers" > > requirement for kselftests. > > > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most > > colorful, so I'll helpfully cite it here. :) > > Let me re-try this. > > This is driving me insane. I've spend the past _TWO_ days trying to > build KVM selftests and I'm still failing. > > This is absolute atrocious crap and is costing me valuable time. > > Please fix this fucking selftests shit to just build. This is unusable > garbage. So after spending more time trying to remember how to debug Makefiles (I hate my life), I found that not only do I need this headers shit, the kvm selftests Makefile is actively broken if you use: make O=foo -INSTALL_HDR_PATH = $(top_srcdir)/usr +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr And then finally, I can do: make O=foo headers_install make O=foo -C tools/testing/selftests/kvm/ So yeah, thank you very much for wasting my time *AGAIN*. Seriously, I want to be able to do: cd tools/testing/selftests/foo; make and have it just work. I would strongly suggest every subsystem to reclaim their selftests and make it so again. And on that, let me go merge the fixes I need to have x86 and futex build without this headers shit.
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: > On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: > > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: > > > On 10/16/24 04:20, Lorenzo Stoakes wrote: > ... > > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h > > > > b/tools/testing/selftests/pidfd/pidfd.h > > > > index 88d6830ee004..1640b711889b 100644 > > > > --- a/tools/testing/selftests/pidfd/pidfd.h > > > > +++ b/tools/testing/selftests/pidfd/pidfd.h > > > > @@ -50,6 +50,14 @@ > > > >#define PIDFD_NONBLOCK O_NONBLOCK > > > >#endif > > > > +/* System header file may not have this available. */ > > > > +#ifndef PIDFD_SELF_THREAD > > > > +#define PIDFD_SELF_THREAD -100 > > > > +#endif > > > > +#ifndef PIDFD_SELF_THREAD_GROUP > > > > +#define PIDFD_SELF_THREAD_GROUP -200 > > > > +#endif > > > > + > > > > > > As mentioned in my response to v1 patch: > > > > > > kselftest has dependency on "make headers" and tests include > > > headers from linux/ directory > > > > Right but that assumes you install the kernel headers on the build system, > > which is quite a painful thing to have to do when you are quickly iterating > > on a qemu setup. > > > > This is a use case I use all the time so not at all theoretical. > > > > This is turning out to be a fairly typical reaction from kernel > developers, when presented with the "you must first run make headers" > requirement for kselftests. > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most > colorful, so I'll helpfully cite it here. :) Let me re-try this. This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing. This is absolute atrocious crap and is costing me valuable time. Please fix this fucking selftests shit to just build. This is unusable garbage.
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, Oct 17, 2024 at 01:37:06PM -0600, Shuah Khan wrote: > On 10/17/24 11:38, Lorenzo Stoakes wrote: > > On Thu, Oct 17, 2024 at 10:37:00AM -0700, John Hubbard wrote: > > > On 10/17/24 10:28 AM, Lorenzo Stoakes wrote: > > > > On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote: > > > > > On 10/17/24 5:06 AM, Lorenzo Stoakes wrote: > > > ... > > > > > > #ifndef __TOOLS_LINUX_PIDFD_H > > > > > > #define __TOOLS_LINUX_PIDFD_H > > > > > > > > > > > > /* > > > > > > * Some systems have issues with the linux/fcntl.h import in > > > > > > linux/pidfd.h, so > > > > > > * work around this by setting the header guard. > > > > > > */ > > > > > > #define _LINUX_FCNTL_H > > > > > > #include "../../../include/uapi/linux/pidfd.h" > > > > > > #undef _LINUX_FCNTL_H > > > > > > > > > > > > #endif /* __TOOLS_LINUX_PIDFD_H */ > > > > > > > > > > > > > > > > > > Then the test code needs only to update the pidfd.h file to #include > > > > > > and add a simple $(TOOLS_INCLUDES) to the CFLAGS += > > > > > > line in > > > > > > the pidfd self tests Makefile and we should be all good. > > > > > > > I like this solution. I should have read this message first before > handling the others. Thanks! > > > > > > Yes. > > > > > > > > > > > > > > > > > That way we always import everything in this header correctly, we > > > > > > directly > > > > > > document this issue, we include the header as you would in userland > > > > > > and we > > > > > > should cover off all the issues? > > > > > > > > > > Very nice! > > > > > > > > Thanks! > > > > > > > > I saw from your other thread the idea was to take snapshots and to run > > > > scripts > > > > to compare etc. but I suppose putting this into the known-stub directory > > > > > > Actually, I'm not running scripts, because the only time things need to > > > change is when new selftests require a new include, or when something > > > changes that selftests depend on. > > > > > > > tools/include/linux rather than tools/include/uapi/linux would avoid a > > > > conflict > > > > here. > > > > > > This is the first time I've actually looked at tools/include/linux. That > > > sounds about right, though. > > > > > > > > > > > Or would you say the wrapper should regardless be in the uapi/linux > > > > directory? > > > > > > > > > > No, not if there is already a better location, as you pointed out. > > > > OK perfect, I have a patch series ready to go with this (and addressing > > Christian's comments). > > > > Shuah - if you are open to this approach then we should be good to go! > > I am caught up with the discussion now. I am good with this change. > > Reviewed-by: Shuah Khan Perfect thanks very much, I will send out the new version of the series with this applied, much appreciated! :) > > thanks, > -- Shuah >
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/17/24 11:38, Lorenzo Stoakes wrote: On Thu, Oct 17, 2024 at 10:37:00AM -0700, John Hubbard wrote: On 10/17/24 10:28 AM, Lorenzo Stoakes wrote: On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote: On 10/17/24 5:06 AM, Lorenzo Stoakes wrote: ... #ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H /* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H #endif /* __TOOLS_LINUX_PIDFD_H */ Then the test code needs only to update the pidfd.h file to #include and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good. I like this solution. I should have read this message first before handling the others. Yes. That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues? Very nice! Thanks! I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory Actually, I'm not running scripts, because the only time things need to change is when new selftests require a new include, or when something changes that selftests depend on. tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here. This is the first time I've actually looked at tools/include/linux. That sounds about right, though. Or would you say the wrapper should regardless be in the uapi/linux directory? No, not if there is already a better location, as you pointed out. OK perfect, I have a patch series ready to go with this (and addressing Christian's comments). Shuah - if you are open to this approach then we should be good to go! I am caught up with the discussion now. I am good with this change. Reviewed-by: Shuah Khan thanks, -- Shuah
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, Oct 17, 2024 at 10:37:00AM -0700, John Hubbard wrote: > On 10/17/24 10:28 AM, Lorenzo Stoakes wrote: > > On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote: > > > On 10/17/24 5:06 AM, Lorenzo Stoakes wrote: > ... > > > > #ifndef __TOOLS_LINUX_PIDFD_H > > > > #define __TOOLS_LINUX_PIDFD_H > > > > > > > > /* > > > > * Some systems have issues with the linux/fcntl.h import in > > > > linux/pidfd.h, so > > > > * work around this by setting the header guard. > > > > */ > > > > #define _LINUX_FCNTL_H > > > > #include "../../../include/uapi/linux/pidfd.h" > > > > #undef _LINUX_FCNTL_H > > > > > > > > #endif /* __TOOLS_LINUX_PIDFD_H */ > > > > > > > > > > > > Then the test code needs only to update the pidfd.h file to #include > > > > and add a simple $(TOOLS_INCLUDES) to the CFLAGS += > > > > line in > > > > the pidfd self tests Makefile and we should be all good. > > > > > > Yes. > > > > > > > > > > > That way we always import everything in this header correctly, we > > > > directly > > > > document this issue, we include the header as you would in userland and > > > > we > > > > should cover off all the issues? > > > > > > Very nice! > > > > Thanks! > > > > I saw from your other thread the idea was to take snapshots and to run > > scripts > > to compare etc. but I suppose putting this into the known-stub directory > > Actually, I'm not running scripts, because the only time things need to > change is when new selftests require a new include, or when something > changes that selftests depend on. > > > tools/include/linux rather than tools/include/uapi/linux would avoid a > > conflict > > here. > > This is the first time I've actually looked at tools/include/linux. That > sounds about right, though. > > > > > Or would you say the wrapper should regardless be in the uapi/linux > > directory? > > > > No, not if there is already a better location, as you pointed out. OK perfect, I have a patch series ready to go with this (and addressing Christian's comments). Shuah - if you are open to this approach then we should be good to go! Thanks > > > thanks, > -- > John Hubbard >
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/17/24 10:28 AM, Lorenzo Stoakes wrote: On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote: On 10/17/24 5:06 AM, Lorenzo Stoakes wrote: ... #ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H /* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H #endif /* __TOOLS_LINUX_PIDFD_H */ Then the test code needs only to update the pidfd.h file to #include and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good. Yes. That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues? Very nice! Thanks! I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory Actually, I'm not running scripts, because the only time things need to change is when new selftests require a new include, or when something changes that selftests depend on. tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here. This is the first time I've actually looked at tools/include/linux. That sounds about right, though. Or would you say the wrapper should regardless be in the uapi/linux directory? No, not if there is already a better location, as you pointed out. thanks, -- John Hubbard
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote: > On 10/17/24 5:06 AM, Lorenzo Stoakes wrote: > > +cc John, sorry I forgot to cc you on other replies!! > > > > On Thu, Oct 17, 2024 at 09:08:19AM +0100, Lorenzo Stoakes wrote: > > [snip] > > > > > > In any case I think copying the header to the tools/ directory with this > > > linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h > > > there?) is the sensible way forward. > > > > > > A 'just include the header' is simply not an option as it breaks the > > > tests. > > I should have read this one first, this morning, but I missed it initially. > :) No worries easily done! :) > > > > > Ohhh ok I think maybe we could have a good compromise that should > > (hopefully!) > > satisfy both you and John. > > > > I can introduce tools/include/linux/pidfd.h that is a stub wrapper around > > the pidfd.h file. > > > > So it can be something like: > > > > > > #ifndef __TOOLS_LINUX_PIDFD_H > > #define __TOOLS_LINUX_PIDFD_H > > > > /* > > * Some systems have issues with the linux/fcntl.h import in > > linux/pidfd.h, so > > * work around this by setting the header guard. > > */ > > #define _LINUX_FCNTL_H > > #include "../../../include/uapi/linux/pidfd.h" > > #undef _LINUX_FCNTL_H > > > > #endif /* __TOOLS_LINUX_PIDFD_H */ > > > > > > Then the test code needs only to update the pidfd.h file to #include > > and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in > > the pidfd self tests Makefile and we should be all good. > > Yes. > > > > > That way we always import everything in this header correctly, we directly > > document this issue, we include the header as you would in userland and we > > should cover off all the issues? > > Very nice! Thanks! I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here. Or would you say the wrapper should regardless be in the uapi/linux directory? Thanks! > > > thanks, > -- > John Hubbard >
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote: +cc John, sorry I forgot to cc you on other replies!! On Thu, Oct 17, 2024 at 09:08:19AM +0100, Lorenzo Stoakes wrote: [snip] In any case I think copying the header to the tools/ directory with this linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h there?) is the sensible way forward. A 'just include the header' is simply not an option as it breaks the tests. I should have read this one first, this morning, but I missed it initially. :) Ohhh ok I think maybe we could have a good compromise that should (hopefully!) satisfy both you and John. I can introduce tools/include/linux/pidfd.h that is a stub wrapper around the pidfd.h file. So it can be something like: #ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H /* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H #endif /* __TOOLS_LINUX_PIDFD_H */ Then the test code needs only to update the pidfd.h file to #include and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good. Yes. That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues? Very nice! thanks, -- John Hubbard
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
+cc John, sorry I forgot to cc you on other replies!! On Thu, Oct 17, 2024 at 09:08:19AM +0100, Lorenzo Stoakes wrote: [snip] > > In any case I think copying the header to the tools/ directory with this > linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h > there?) is the sensible way forward. > > A 'just include the header' is simply not an option as it breaks the tests. Ohhh ok I think maybe we could have a good compromise that should (hopefully!) satisfy both you and John. I can introduce tools/include/linux/pidfd.h that is a stub wrapper around the pidfd.h file. So it can be something like: #ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H /* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H #endif /* __TOOLS_LINUX_PIDFD_H */ Then the test code needs only to update the pidfd.h file to #include and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good. That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Wed, Oct 16, 2024 at 04:38:50PM -0600, Shuah Khan wrote:
> On 10/16/24 16:06, Lorenzo Stoakes wrote:
> > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
> > > On 10/16/24 04:20, Lorenzo Stoakes wrote:
> > > > Add tests to assert that PIDFD_SELF_* correctly refers to the current
> > > > thread and process.
> > > >
> > > > This is only practically meaningful to pidfd_send_signal() and
> > > > pidfd_getfd(), but also explicitly test that we disallow this feature
> > > > for
> > > > setns() where it would make no sense.
> > > >
> > > > We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so
> > > > while in
> > > > theory PIDFD_SELF_* would work here, we'd be left blocked if we tried
> > > > it.
> > > >
> > > > We defer testing of mm-specific functionality which uses pidfd, namely
> > > > process_madvise() and process_mrelease() to mm testing (though note the
> > > > latter can not be sensibly tested as it would require the testing
> > > > process
> > > > to be dying).
> > > >
> > > > Signed-off-by: Lorenzo Stoakes
> > > > ---
> > > >tools/testing/selftests/pidfd/pidfd.h | 8 +
> > > >.../selftests/pidfd/pidfd_getfd_test.c| 141
> > > > ++
> > > >.../selftests/pidfd/pidfd_setns_test.c| 11 ++
> > > >tools/testing/selftests/pidfd/pidfd_test.c| 76 --
> > > >4 files changed, 224 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h
> > > > b/tools/testing/selftests/pidfd/pidfd.h
> > > > index 88d6830ee004..1640b711889b 100644
> > > > --- a/tools/testing/selftests/pidfd/pidfd.h
> > > > +++ b/tools/testing/selftests/pidfd/pidfd.h
> > > > @@ -50,6 +50,14 @@
> > > >#define PIDFD_NONBLOCK O_NONBLOCK
> > > >#endif
> > > > +/* System header file may not have this available. */
> > > > +#ifndef PIDFD_SELF_THREAD
> > > > +#define PIDFD_SELF_THREAD -100
> > > > +#endif
> > > > +#ifndef PIDFD_SELF_THREAD_GROUP
> > > > +#define PIDFD_SELF_THREAD_GROUP -200
> > > > +#endif
> > > > +
> > >
> > > As mentioned in my response to v1 patch:
> > >
> > > kselftest has dependency on "make headers" and tests include
> > > headers from linux/ directory
> >
> > Right but that assumes you install the kernel headers on the build system,
> > which is quite a painful thing to have to do when you are quickly iterating
> > on a qemu setup.
>
> Yes that is exactly what we do. kselftest build depends on headers
> install. The way it works for qemu is either using vitme-ng or
> building tests and installing them in your vm.. This is what CIs do.
>
> >
> > This is a use case I use all the time so not at all theoretical.
>
> This is what CIs do. Yes - it works for them to build and install
> headers. You don't have to install them on the build system. You
> run "make headers" in your repo. You could use O= option for
> relocatable build.
Right but I'm talking about my local builds in order to test the kernel. See
John's response.
>
> >
> > Unfortunately this seems broken on my system anyway :( - see below.
> >
> > >
> > > These local make it difficult to maintain these tests in the
> > > longer term. Somebody has to go clean these up later.
> >
> > I don't agree, tests have to be maintained alongside the core code, and if
> > these values change (seems unlikely) then the tests will fail and can
> > easily be updated.
> >
> > This was the approach already taken in this file with other linux
> > header-defined values, so we'll also be breaking the precendence.
>
> Some of these defines were added a while back. Often these defines
> need cleaning up. I would rather not see new ones added unless it is
> absolutely necessary.
OK, but just to note that I am now not doing a PIDFD_SELF series, I'm doing a
'PIDFD_SELF and completely change how pidfd does testing' series.
To me the right thing to do would be to send 2 series and not block this one on
this issue.
>
> >
> > >
> > > The import will be fine and you can control that with -I flag in
> > > the makefile. Remove these and try to get including linux/pidfd.h
> > > working.
> >
> > I just tried this and it's not fine :) it immediately broke the build as
> > pidfd.h imports linux/fcntl.h which conflicts horribly with system headers
> > on my machine.
> >
> > For instance f_owner_ex gets redefined among others and fails the build
> > e..g:
> >
> > /usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct
> > f_owner_ex’
> >155 | struct f_owner_ex {
> >|^~
> > In file included from /usr/include/bits/fcntl.h:61,
> > from /usr/include/fcntl.h:35,
> > from pidfd_test.c:6:
> > /usr/include/bits/fcntl-linux.h:274:8: note: originally defined here
> >274 | struct f_owner_ex
> >|^~
> >
> > It seems only one other test tries to do this as far as I can tell (I only
> > did a quick grep), so it's not at all standard it seems.
> >
> > This issue occurre
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote: > On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: > > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: > > > On 10/16/24 04:20, Lorenzo Stoakes wrote: > ... > > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h > > > > b/tools/testing/selftests/pidfd/pidfd.h > > > > index 88d6830ee004..1640b711889b 100644 > > > > --- a/tools/testing/selftests/pidfd/pidfd.h > > > > +++ b/tools/testing/selftests/pidfd/pidfd.h > > > > @@ -50,6 +50,14 @@ > > > >#define PIDFD_NONBLOCK O_NONBLOCK > > > >#endif > > > > +/* System header file may not have this available. */ > > > > +#ifndef PIDFD_SELF_THREAD > > > > +#define PIDFD_SELF_THREAD -100 > > > > +#endif > > > > +#ifndef PIDFD_SELF_THREAD_GROUP > > > > +#define PIDFD_SELF_THREAD_GROUP -200 > > > > +#endif > > > > + > > > > > > As mentioned in my response to v1 patch: > > > > > > kselftest has dependency on "make headers" and tests include > > > headers from linux/ directory > > > > Right but that assumes you install the kernel headers on the build system, > > which is quite a painful thing to have to do when you are quickly iterating > > on a qemu setup. > > > > This is a use case I use all the time so not at all theoretical. > > > > This is turning out to be a fairly typical reaction from kernel > developers, when presented with the "you must first run make headers" > requirement for kselftests. It's a typical response for good reason... :) > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most > colorful, so I'll helpfully cite it here. :) > > But seriously...user feedback is rare and valuable. We have some, to the > effect of, "lose that requirement". And we also have an agreement, and > an initial implementation in selftests/mm, on *how* to avoid it [2]. > > So...let's do it that way? Please? I'd be happy to but we can't because the uapi header is just broken with this test due to the linux/fcntl.h vs. system header fcntl.h issue. We could work around it by copying the header without the linux/fcntl.h include however... > > > [1] > https://lore.kernel.org/lkml/[email protected]/ > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e076eaca5906 > > thanks, > -- > John Hubbard >
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote: On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote: On 10/16/24 04:20, Lorenzo Stoakes wrote: ... diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif + As mentioned in my response to v1 patch: kselftest has dependency on "make headers" and tests include headers from linux/ directory Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup. This is a use case I use all the time so not at all theoretical. This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests. Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :) But seriously...user feedback is rare and valuable. We have some, to the effect of, "lose that requirement". And we also have an agreement, and an initial implementation in selftests/mm, on *how* to avoid it [2]. So...let's do it that way? Please? [1] https://lore.kernel.org/lkml/[email protected]/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e076eaca5906 thanks, -- John Hubbard
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/16/24 16:06, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
Add tests to assert that PIDFD_SELF_* correctly refers to the current
thread and process.
This is only practically meaningful to pidfd_send_signal() and
pidfd_getfd(), but also explicitly test that we disallow this feature for
setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely
process_madvise() and process_mrelease() to mm testing (though note the
latter can not be sensibly tested as it would require the testing process
to be dying).
Signed-off-by: Lorenzo Stoakes
---
tools/testing/selftests/pidfd/pidfd.h | 8 +
.../selftests/pidfd/pidfd_getfd_test.c| 141 ++
.../selftests/pidfd/pidfd_setns_test.c| 11 ++
tools/testing/selftests/pidfd/pidfd_test.c| 76 --
4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h
b/tools/testing/selftests/pidfd/pidfd.h
index 88d6830ee004..1640b711889b 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -50,6 +50,14 @@
#define PIDFD_NONBLOCK O_NONBLOCK
#endif
+/* System header file may not have this available. */
+#ifndef PIDFD_SELF_THREAD
+#define PIDFD_SELF_THREAD -100
+#endif
+#ifndef PIDFD_SELF_THREAD_GROUP
+#define PIDFD_SELF_THREAD_GROUP -200
+#endif
+
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include
headers from linux/ directory
Right but that assumes you install the kernel headers on the build system,
which is quite a painful thing to have to do when you are quickly iterating
on a qemu setup.
Yes that is exactly what we do. kselftest build depends on headers
install. The way it works for qemu is either using vitme-ng or
building tests and installing them in your vm.. This is what CIs do.
This is a use case I use all the time so not at all theoretical.
This is what CIs do. Yes - it works for them to build and install
headers. You don't have to install them on the build system. You
run "make headers" in your repo. You could use O= option for
relocatable build.
Unfortunately this seems broken on my system anyway :( - see below.
These local make it difficult to maintain these tests in the
longer term. Somebody has to go clean these up later.
I don't agree, tests have to be maintained alongside the core code, and if
these values change (seems unlikely) then the tests will fail and can
easily be updated.
This was the approach already taken in this file with other linux
header-defined values, so we'll also be breaking the precendence.
Some of these defines were added a while back. Often these defines
need cleaning up. I would rather not see new ones added unless it is
absolutely necessary.
The import will be fine and you can control that with -I flag in
the makefile. Remove these and try to get including linux/pidfd.h
working.
I just tried this and it's not fine :) it immediately broke the build as
pidfd.h imports linux/fcntl.h which conflicts horribly with system headers
on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct
f_owner_ex’
155 | struct f_owner_ex {
|^~
In file included from /usr/include/bits/fcntl.h:61,
from /usr/include/fcntl.h:35,
from pidfd_test.c:6:
/usr/include/bits/fcntl-linux.h:274:8: note: originally defined here
274 | struct f_owner_ex
|^~
It seems only one other test tries to do this as far as I can tell (I only
did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create
sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi
header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the include and that resulted in
conflicting:
In file included from /usr/include/fcntl.h:35,
from /usr/include/sys/mount.h:24,
from pidfd.h:17,
from pidfd_test.c:22:
/usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’
35 | struct flock
|^
In file included from /tmp/hdr/include/asm/fcntl.h:1,
from /tmp/hdr/include/linux/fcntl.h:5,
from /tmp/hdr/include/linux/pidfd.h:7,
from pidfd.h:6:
/usr/include/asm-generic/fcntl.h:195:8: note: originally defined here
195 | struct flock {
|^
So I don't think I can actually work around this, at least on my sys
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Wed, Oct 16, 2024 at 11:06:34PM +0100, Lorenzo Stoakes wrote:
[sniip]
> >
> > The import will be fine and you can control that with -I flag in
> > the makefile. Remove these and try to get including linux/pidfd.h
> > working.
>
> I just tried this and it's not fine :) it immediately broke the build as
> pidfd.h imports linux/fcntl.h which conflicts horribly with system headers
> on my machine.
>
> For instance f_owner_ex gets redefined among others and fails the build e..g:
>
> /usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct
> f_owner_ex’
> 155 | struct f_owner_ex {
> |^~
> In file included from /usr/include/bits/fcntl.h:61,
> from /usr/include/fcntl.h:35,
> from pidfd_test.c:6:
> /usr/include/bits/fcntl-linux.h:274:8: note: originally defined here
> 274 | struct f_owner_ex
> |^~
>
> It seems only one other test tries to do this as far as I can tell (I only
> did a quick grep), so it's not at all standard it seems.
>
> This issue occurred even when I used make headers_install to create
> sanitised user headers and added them to the include path.
>
> A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi
> header) and system fcntl.h is a known thing. Slightly bizarre...
>
> I tried removing the include and that resulted in
> conflicting:
>
> In file included from /usr/include/fcntl.h:35,
> from /usr/include/sys/mount.h:24,
> from pidfd.h:17,
> from pidfd_test.c:22:
> /usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’
>35 | struct flock
> |^
> In file included from /tmp/hdr/include/asm/fcntl.h:1,
> from /tmp/hdr/include/linux/fcntl.h:5,
> from /tmp/hdr/include/linux/pidfd.h:7,
> from pidfd.h:6:
> /usr/include/asm-generic/fcntl.h:195:8: note: originally defined here
> 195 | struct flock {
> |^
>
> So I don't think I can actually work around this, at least on my system,
> and I can't really sensibly submit a patch that I can't run on my own
> machine :)
>
> I may be missing something here.
>
[snip]
Some added data:
OK so I asked people on fedi to compile the following locally (also a
variant with _GNU_SOURCE being defined):
#include
#include
int main(void) {}
And they are all encountering the same issue as I am on a number of
different distros (ordering of includes doesn't seem to matter either).
So this seems like a known-broken thing.
And we can't really isolate inclusion of this file since all the tests
interact directly with defines from it.
So it seems the only solution is the workaround I suggested previously I
think with the header guard define hack.
[snip]
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
> On 10/16/24 04:20, Lorenzo Stoakes wrote:
> > Add tests to assert that PIDFD_SELF_* correctly refers to the current
> > thread and process.
> >
> > This is only practically meaningful to pidfd_send_signal() and
> > pidfd_getfd(), but also explicitly test that we disallow this feature for
> > setns() where it would make no sense.
> >
> > We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
> > theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
> >
> > We defer testing of mm-specific functionality which uses pidfd, namely
> > process_madvise() and process_mrelease() to mm testing (though note the
> > latter can not be sensibly tested as it would require the testing process
> > to be dying).
> >
> > Signed-off-by: Lorenzo Stoakes
> > ---
> > tools/testing/selftests/pidfd/pidfd.h | 8 +
> > .../selftests/pidfd/pidfd_getfd_test.c| 141 ++
> > .../selftests/pidfd/pidfd_setns_test.c| 11 ++
> > tools/testing/selftests/pidfd/pidfd_test.c| 76 --
> > 4 files changed, 224 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/pidfd/pidfd.h
> > b/tools/testing/selftests/pidfd/pidfd.h
> > index 88d6830ee004..1640b711889b 100644
> > --- a/tools/testing/selftests/pidfd/pidfd.h
> > +++ b/tools/testing/selftests/pidfd/pidfd.h
> > @@ -50,6 +50,14 @@
> > #define PIDFD_NONBLOCK O_NONBLOCK
> > #endif
> > +/* System header file may not have this available. */
> > +#ifndef PIDFD_SELF_THREAD
> > +#define PIDFD_SELF_THREAD -100
> > +#endif
> > +#ifndef PIDFD_SELF_THREAD_GROUP
> > +#define PIDFD_SELF_THREAD_GROUP -200
> > +#endif
> > +
>
> As mentioned in my response to v1 patch:
>
> kselftest has dependency on "make headers" and tests include
> headers from linux/ directory
Right but that assumes you install the kernel headers on the build system,
which is quite a painful thing to have to do when you are quickly iterating
on a qemu setup.
This is a use case I use all the time so not at all theoretical.
Unfortunately this seems broken on my system anyway :( - see below.
>
> These local make it difficult to maintain these tests in the
> longer term. Somebody has to go clean these up later.
I don't agree, tests have to be maintained alongside the core code, and if
these values change (seems unlikely) then the tests will fail and can
easily be updated.
This was the approach already taken in this file with other linux
header-defined values, so we'll also be breaking the precendence.
>
> The import will be fine and you can control that with -I flag in
> the makefile. Remove these and try to get including linux/pidfd.h
> working.
I just tried this and it's not fine :) it immediately broke the build as
pidfd.h imports linux/fcntl.h which conflicts horribly with system headers
on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct
f_owner_ex’
155 | struct f_owner_ex {
|^~
In file included from /usr/include/bits/fcntl.h:61,
from /usr/include/fcntl.h:35,
from pidfd_test.c:6:
/usr/include/bits/fcntl-linux.h:274:8: note: originally defined here
274 | struct f_owner_ex
|^~
It seems only one other test tries to do this as far as I can tell (I only
did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create
sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi
header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the include and that resulted in
conflicting:
In file included from /usr/include/fcntl.h:35,
from /usr/include/sys/mount.h:24,
from pidfd.h:17,
from pidfd_test.c:22:
/usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’
35 | struct flock
|^
In file included from /tmp/hdr/include/asm/fcntl.h:1,
from /tmp/hdr/include/linux/fcntl.h:5,
from /tmp/hdr/include/linux/pidfd.h:7,
from pidfd.h:6:
/usr/include/asm-generic/fcntl.h:195:8: note: originally defined here
195 | struct flock {
|^
So I don't think I can actually work around this, at least on my system,
and I can't really sensibly submit a patch that I can't run on my own
machine :)
I may be missing something here.
>
> Please revise this patch to include the header file and remove
> these local defines.
I'm a little stuck because of the above, but I _could_ do the following in
the test pidfd.h header.:
#define _LINUX_FCNTL_H
#include "../../../../include/uapi/linux/pidfd.h"
#undef _LINUX_FCNTL_H
Which prevents the problematic linux/fcntl.h header from being included
Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/16/24 04:20, Lorenzo Stoakes wrote: Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process. This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense. We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it. We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying). Signed-off-by: Lorenzo Stoakes --- tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c| 141 ++ .../selftests/pidfd/pidfd_setns_test.c| 11 ++ tools/testing/selftests/pidfd/pidfd_test.c| 76 -- 4 files changed, 224 insertions(+), 12 deletions(-) diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif + As mentioned in my response to v1 patch: kselftest has dependency on "make headers" and tests include headers from linux/ directory These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later. The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working. Please revise this patch to include the header file and remove these local defines. thanks, -- Shuah
