From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 12514D2F7D1 for ; Wed, 16 Oct 2024 22:38:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 83F546B007B; Wed, 16 Oct 2024 18:38:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7EE866B0082; Wed, 16 Oct 2024 18:38:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 68EE96B0083; Wed, 16 Oct 2024 18:38:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 4424D6B007B for ; Wed, 16 Oct 2024 18:38:55 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 89619AC295 for ; Wed, 16 Oct 2024 22:38:34 +0000 (UTC) X-FDA: 82680931704.28.74C8648 Received: from mail-il1-f169.google.com (mail-il1-f169.google.com [209.85.166.169]) by imf21.hostedemail.com (Postfix) with ESMTP id 5D6351C0006 for ; Wed, 16 Oct 2024 22:38:35 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=iaEWL7sy; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf21.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.166.169 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729118174; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pQRRRq47/Hw9MX6ZtToVorakNDyg07L0TFDDDji46Mo=; b=cN+5kGCMZD/AqHJJd617ejMtLochGqZdkIBOpixtWvbVytwHTrxtULQRrJylUNq7eBNTEs JMbZuqK7iGlpUzO5aichKGO9p3qUSKsASDe9QjCDm8eREXylvDpoOwlI8vMdmcpafCVZA6 U1RacGi617rlherjO1MBFOPqbmvnE3g= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729118174; a=rsa-sha256; cv=none; b=uU7j2MeNQ87sexEJ54PGokoM2LvbU4p5AP1sOIXn/gzeRTd5dnws2HxANTVdku1uV5ZVAX M7w02jgVQRXgWx1GdTmnplEA3hJfmxjmQYdKt0S4OnKRR9SIzqUvgz0zYxnC/a9x8ITQlm xOZC5ErsWFfAKkN+KEwjahDmsk4/dSg= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=iaEWL7sy; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf21.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.166.169 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org Received: by mail-il1-f169.google.com with SMTP id e9e14a558f8ab-3a3b8b34be9so1597195ab.2 for ; Wed, 16 Oct 2024 15:38:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linuxfoundation.org; s=google; t=1729118332; x=1729723132; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=pQRRRq47/Hw9MX6ZtToVorakNDyg07L0TFDDDji46Mo=; b=iaEWL7symO3sDgRFtpKgRYxnYuVIxOgzS45L8khyxco9OhV7qPN5v02cu8ncA8/waL Puy+ySuSGhHv/BDjvYMBEZdC0v0n9O/x/bRm+LXeS0GMhVzkBqMlM4TTvFxxVbfVMteM 5t8HqU6dyKjz2GwPPDgUqfnmtg+qEKPP4FWNM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729118332; x=1729723132; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pQRRRq47/Hw9MX6ZtToVorakNDyg07L0TFDDDji46Mo=; b=sTZRQKjsabch58dsqESU86lR997zsKAuIW/kNJNMYEtXK4+3GEGbouSSKW5gZxrCdO jJBBFN8Lawi+8ltR1KbncByj+6UF0tiuwRvI2GxyqZK9JvcVimdnXGbUbM+4aYDqP7Ui JESrPe0k6+prluGkZ0TKDW5BobCtqR+W3sKcTaG4jvzZwBf81W1pFGegOyjB+Nz4laUd XjYgiFIwK6bdtEC5Z5/dkVPlrGCZWu0UZ1KdNF7wvebGACAmBIw14zG3cH2ecTmB5eQv LN34X0kBZwiRY9ebDZerZF3JKyPHna+MR/MRMvWTASdrJ/yUXeYauxcfOpaX9t93FlqU lWkA== X-Forwarded-Encrypted: i=1; AJvYcCX3wf6aSqE/tc9lLgQHxBa15znsQkIfm5rGDbQD9idCQ2/b/zq+06oTmLKiaqgRKwsmB7INnra9kg==@kvack.org X-Gm-Message-State: AOJu0Yzsli6nJWLx96+LcsMDuWsN/37uf2uNTCLJmzS2i0Oxw3SE1jTk 8EoxDzxYYMSfusTg6QEPPDuyZF6CXsfnuF4qXXLqxmRhd1tuHPFJ5m3hBc/R/T8= X-Google-Smtp-Source: AGHT+IFivQcgqrz4+LdP/xfhfGNmIqN1nL/+KVDlxQRCS03LTC4/8hYkEZhoL+4u2csjPmdOw2hZKA== X-Received: by 2002:a05:6e02:3cc2:b0:3a3:a307:6851 with SMTP id e9e14a558f8ab-3a3bce0858dmr137968295ab.22.1729118331696; Wed, 16 Oct 2024 15:38:51 -0700 (PDT) Received: from [192.168.1.128] ([38.175.170.29]) by smtp.gmail.com with ESMTPSA id 8926c6da1cb9f-4dbec9632cbsm1058985173.20.2024.10.16.15.38.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Oct 2024 15:38:51 -0700 (PDT) Message-ID: <84c0de17-899e-46fd-8b72-534d8a02c259@linuxfoundation.org> Date: Wed, 16 Oct 2024 16:38:50 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_* To: Lorenzo Stoakes Cc: Christian Brauner , Shuah Khan , "Liam R . Howlett" , Suren Baghdasaryan , Vlastimil Babka , pedro.falcato@gmail.com, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, Oliver Sang , Shuah Khan References: Content-Language: en-US From: Shuah Khan In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 5D6351C0006 X-Stat-Signature: nxz5bkifcsbmzsn3h85yhsfc536ijpjy X-Rspam-User: X-HE-Tag: 1729118315-743172 X-HE-Meta: U2FsdGVkX18WEhbm9aOJg9gvgtFBJk1rXeryC8rNJdwrHAC0YKyG/4INHDh4M6VRp36sTigbvmQWNttn9Ftd9b9m/aSrVrPQCbCENMW1mR4oojV+rQUpz6/CK3Pb+aubFBLNtdqm3zyd/9zpXY/Cf4MOjy3VjlRJfYCMzX3Z5W/N6kflFyszjVFT2uSFDmso+e4Exq5HIizp2zA/O4qt+ag/GSLauaenQNwVlGsJ5nVdOIqmTQC7pN/Aj1q+hXToL5LLP8GD/+wDyywW9jPzdfkdUZLZeJxE+SbS7eqCSaciNJf5Swx4q03pVLD2ir0ZlIX45IY9ZmT4Q7Mec0E3zdjz+mSb+1DG0ojJ2oI1My9zt/wc5GvPYypBtSqKBp6RbC4eQvPoLuocn5it0kpC3Qyk1n/v7gK1aXyQJb5JP7Tg8eAYTzB9H71pBgmJfPpva/UJt72yeW4/3AV1sz4h/f47fB87s0H5O7JQlgF5jDbq+whWhBGnmaTd2hzrphRDWQIaJLF+9xdbRCTDrkaBXlB0WZhfGiWF1N76nfCZGf8/rIeNdmGqka2vvX4UTO8wmRHppUVLWLGZnhoQoNd/DKF3CQJyKhhkb2FQZtke/e6hSf9MKGjFRg2xmNIb6dPiO+x3AQPp9iPhzxKv/W/BbOUon6HhCS8zxud+UX+aTx11gtrabuolAwVQOJ2ntZ63s70sSInHx2oNQon41gtpRswDw1x9ixQTOZeLs6n72mJOH0aqUsLmNZEUnHgpLTpbQK2K1KBcxh9CCauTqRpEf1VXxdWmn1BULd15iOQhBq2iR5eAfckkJwE/JNcr4LigWPsz4J60XMoR/gYRgr8x+jR9mvIN+/SE5/iplD7XnvWSmVCHShhOWTeOtTox2hNviq6zSEA4e1n/YOeEXFsePUTjHWpPRNtAqVzrFCJtKoTyi1q1FGJyD4QdJUJuSKYyg6OBxSx53fQBMuOkdLF SrS9wrr7 zBCpXJeVHaOS2afTnX5GmHMjQc/jVXhbB1bvgySZ/46tKcYEZuFEPUm7C2NxQjT+bh1bJ7pKllf/CqCq/e4fjOEWhqa1DYpzCb1XSkfIs1rX+nRCA1h/4ITgT4+b6bRuSspxXKQJdOyEXPH0x5dflsX5N+CHihPEukOPrU4MsphwnUiAXEEPMWo0zgAlRQT9Vq5QjRLCN985ZGukWBOvpaSXdOXHwVVGSwKRHcbUECha5a4pJ4rdrqYMp+1cjokpUhI06R2QAPxFHBEslw0LP+9tRHfbb8uGRd7xcUQ9t7UyVs7uuRpI+IcWMuwkyCMIk8viEc/8aku4qFUZBo4zRucIBcw45i0IKKZJ4rj6KK2jqlcQE2FGSOeKQRBBLLTu0Vk+0O5isST9GXPJkr+VgERVYKDVSIzhCcVZIgsZcLKK3qz7SqUKLLxhvyTHzjAGRnp9RjLTMMylj5I0k50699WrJmv9RntCGuMT+Bbfznz9S/jmdc8auKGeTUMUSreCCf0JOiIdQGVxGRqc= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: 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 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 > Does this test really need fcntl.h is another question. This is another problem with too many includes. The test built just fine on my system on 6.12-rc3 with +/* #include */ > Which prevents the problematic linux/fcntl.h header from being included and > includes the right header. > > But I'm not sure this is hugely better than what we already have > maintinability-wise? Either way if something changes to break it it'll > break the test build. > If these defines are in a header file - tests include them. Part of test development is figuring out these problems. > Let me know if this is what you want me to do. Otherwise I'm not sure how > to proceed - this header just seems broken at least on my system (arch > linux at 6.11.1). > > An aside: > > The existing code already taken the approach I take (this is partly why I > did it), I think it'd be out of the scope of my series to change that, for > instance in pidfd.h: > > #ifndef PIDFD_NONBLOCK > #define PIDFD_NONBLOCK O_NONBLOCK > #endif > > Alongside a number of other defines. So those will have to stay at least > for now for being out of scope, but obviously if people would prefer to > move the whole thing that can be followed up later. > >> I would like us to explore before giving up and saying these will stay. thanks, -- Shuah