* Re: [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption
@ 2022-06-04 12:02 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-06-04 12:02 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 19666 bytes --]
::::::
:::::: Manual check reason: "low confidence static check warning: include/linux/cpumask.h:134:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]"
::::::
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <017b97698ba58d33bf45d30317d5a73c5b93d2a0.1654201862.git.sander@svanheule.net>
References: <017b97698ba58d33bf45d30317d5a73c5b93d2a0.1654201862.git.sander@svanheule.net>
TO: Sander Vanheule <sander@svanheule.net>
TO: Peter Zijlstra <peterz@infradead.org>
TO: Yury Norov <yury.norov@gmail.com>
TO: Andrew Morton <akpm@linux-foundation.org>
CC: Linux Memory Management List <linux-mm@kvack.org>
TO: Valentin Schneider <vschneid@redhat.com>
TO: Thomas Gleixner <tglx@linutronix.de>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
TO: Marco Elver <elver@google.com>
TO: Barry Song <song.bao.hua@hisilicon.com>
CC: linux-kernel(a)vger.kernel.org
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: Sander Vanheule <sander@svanheule.net>
Hi Sander,
I love your patch! Perhaps something to improve:
[auto build test WARNING on akpm-mm/mm-everything]
[also build test WARNING on linus/master v5.18 next-20220603]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Sander-Vanheule/cpumask-Fix-invalid-uniprocessor-assumptions/20220603-050659
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220604/202206041950.Be4CObej-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b364c76683f8ef241025a9556300778c07b590c2)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4bf6a27a30fc5847a5fc6e6dae56e5716c2625ad
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sander-Vanheule/cpumask-Fix-invalid-uniprocessor-assumptions/20220603-050659
git checkout 4bf6a27a30fc5847a5fc6e6dae56e5716c2625ad
# save the config file
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 clang-analyzer
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
^
block/blk-mq.c:3012:17: note: Assuming the condition is false
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
block/blk-mq.c:3012:2: note: '?' condition is false
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
block/blk-mq.c:3012:6: note: Assuming field 'bio' is non-null
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
block/blk-mq.c:3012:6: note: Left side of '&&' is true
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
block/blk-mq.c:3012:17: note: Assuming the condition is true
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
block/blk-mq.c:3012:2: note: '?' condition is true
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:69: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
(cond) ? \
^
block/blk-mq.c:3012:2: note: Taking true branch
if (rq->bio && blk_crypto_rq_bio_prep(rq, rq->bio, gfp_mask) < 0)
^
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
block/blk-mq.c:3013:3: note: Control jumps to line 3018
goto free_and_out;
^
block/blk-mq.c:3018:2: note: 1st function call argument is an uninitialized value
if (bio)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^ ~~~~
block/blk-mq.c:4234:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memcpy(new_tags, set->tags, cur_nr_hw_queues *
^
arch/x86/include/asm/string_32.h:150:25: note: expanded from macro 'memcpy'
#define memcpy(t, f, n) __builtin_memcpy(t, f, n)
^~~~~~~~~~~~~~~~
block/blk-mq.c:4234:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
memcpy(new_tags, set->tags, cur_nr_hw_queues *
^
arch/x86/include/asm/string_32.h:150:25: note: expanded from macro 'memcpy'
#define memcpy(t, f, n) __builtin_memcpy(t, f, n)
^~~~~~~~~~~~~~~~
block/blk-mq.c:4344:2: warning: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memset(set, 0, sizeof(*set));
^
arch/x86/include/asm/string_32.h:195:29: note: expanded from macro 'memset'
#define memset(s, c, count) __builtin_memset(s, c, count)
^~~~~~~~~~~~~~~~
block/blk-mq.c:4344:2: note: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11
memset(set, 0, sizeof(*set));
^
arch/x86/include/asm/string_32.h:195:29: note: expanded from macro 'memset'
#define memset(s, c, count) __builtin_memset(s, c, count)
^~~~~~~~~~~~~~~~
>> include/linux/cpumask.h:134:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
return !(*cpumask_bits(srcp1) & *cpumask_bits(srcp2) & 1);
^
block/blk-mq.c:3335:31: note: Assuming '____ptr' is null
struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node,
^
include/linux/list.h:1029:5: note: expanded from macro 'hlist_entry_safe'
____ptr ? hlist_entry(____ptr, type, member) : NULL; \
^~~~~~~
block/blk-mq.c:3335:31: note: '?' condition is false
struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node,
^
include/linux/list.h:1029:5: note: expanded from macro 'hlist_entry_safe'
____ptr ? hlist_entry(____ptr, type, member) : NULL; \
^
block/blk-mq.c:3338:6: note: Assuming the condition is false
if (!cpumask_test_cpu(cpu, hctx->cpumask) ||
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
block/blk-mq.c:3338:6: note: Left side of '||' is false
if (!cpumask_test_cpu(cpu, hctx->cpumask) ||
^
block/blk-mq.c:3339:7: note: Calling 'blk_mq_last_cpu_in_hctx'
!blk_mq_last_cpu_in_hctx(cpu, hctx))
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
block/blk-mq.c:3326:24: note: Passing null pointer value via 1st parameter 'srcp1'
if (cpumask_first_and(hctx->cpumask, cpu_online_mask) != cpu)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
block/blk-mq.c:3326:6: note: Calling 'cpumask_first_and'
if (cpumask_first_and(hctx->cpumask, cpu_online_mask) != cpu)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/cpumask.h:134:11: note: Dereference of null pointer
return !(*cpumask_bits(srcp1) & *cpumask_bits(srcp2) & 1);
^~~~~~~~~~~~~~~~~~~~
Suppressed 47 warnings (47 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
46 warnings generated.
drivers/nvme/host/multipath.c:40:9: warning: Call to function 'sprintf' is insecure as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'sprintf_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
return sprintf(buf, "%s\n", nvme_iopolicy_names[iopolicy]);
^~~~~~~
drivers/nvme/host/multipath.c:40:9: note: Call to function 'sprintf' is insecure as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'sprintf_s' in case of C11
return sprintf(buf, "%s\n", nvme_iopolicy_names[iopolicy]);
^~~~~~~
drivers/nvme/host/multipath.c:498:2: warning: Call to function 'sprintf' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'sprintf_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
sprintf(head->disk->disk_name, "nvme%dn%d",
^~~~~~~
drivers/nvme/host/multipath.c:498:2: note: Call to function 'sprintf' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'sprintf_s' in case of C11
sprintf(head->disk->disk_name, "nvme%dn%d",
^~~~~~~
Suppressed 44 warnings (44 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
65 warnings generated.
drivers/nvme/host/fabrics.c:70:2: warning: Call to function 'snprintf' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'snprintf_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
snprintf(host->nqn, NVMF_NQN_SIZE,
^~~~~~~~
drivers/nvme/host/fabrics.c:70:2: note: Call to function 'snprintf' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'snprintf_s' in case of C11
snprintf(host->nqn, NVMF_NQN_SIZE,
^~~~~~~~
drivers/nvme/host/fabrics.c:388:2: warning: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
^~~~~~~
drivers/nvme/host/fabrics.c:388:2: note: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11
strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
^~~~~~~
drivers/nvme/host/fabrics.c:389:2: warning: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
^~~~~~~
drivers/nvme/host/fabrics.c:389:2: note: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11
strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
^~~~~~~
drivers/nvme/host/fabrics.c:449:2: warning: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
^~~~~~~
drivers/nvme/host/fabrics.c:449:2: note: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11
strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
^~~~~~~
drivers/nvme/host/fabrics.c:450:2: warning: Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'strncpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
vim +134 include/linux/cpumask.h
9b51d9d866482a Yury Norov 2021-08-14 130
93ba139ba8190c Yury Norov 2021-08-14 131 static inline unsigned int cpumask_first_and(const struct cpumask *srcp1,
93ba139ba8190c Yury Norov 2021-08-14 132 const struct cpumask *srcp2)
93ba139ba8190c Yury Norov 2021-08-14 133 {
4bf6a27a30fc58 Sander Vanheule 2022-06-02 @134 return !(*cpumask_bits(srcp1) & *cpumask_bits(srcp2) & 1);
93ba139ba8190c Yury Norov 2021-08-14 135 }
93ba139ba8190c Yury Norov 2021-08-14 136
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption
@ 2022-06-03 5:35 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-06-03 5:35 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]
::::::
:::::: Manual check reason: "low confidence static check first_new_problem: cacheinfo.c:(.text+0x451): undefined reference to `cpu_llc_shared_map'"
::::::
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <017b97698ba58d33bf45d30317d5a73c5b93d2a0.1654201862.git.sander@svanheule.net>
References: <017b97698ba58d33bf45d30317d5a73c5b93d2a0.1654201862.git.sander@svanheule.net>
TO: Sander Vanheule <sander@svanheule.net>
TO: Peter Zijlstra <peterz@infradead.org>
TO: Yury Norov <yury.norov@gmail.com>
TO: Andrew Morton <akpm@linux-foundation.org>
CC: Linux Memory Management List <linux-mm@kvack.org>
TO: Valentin Schneider <vschneid@redhat.com>
TO: Thomas Gleixner <tglx@linutronix.de>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
TO: Marco Elver <elver@google.com>
TO: Barry Song <song.bao.hua@hisilicon.com>
CC: linux-kernel(a)vger.kernel.org
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: Sander Vanheule <sander@svanheule.net>
Hi Sander,
I love your patch! Yet something to improve:
[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master v5.18 next-20220602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Sander-Vanheule/cpumask-Fix-invalid-uniprocessor-assumptions/20220603-050659
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
:::::: branch date: 8 hours ago
:::::: commit date: 8 hours ago
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220603/202206031301.CXhGloF7-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-14-g5a0004b5-dirty
# https://github.com/intel-lab-lkp/linux/commit/4bf6a27a30fc5847a5fc6e6dae56e5716c2625ad
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sander-Vanheule/cpumask-Fix-invalid-uniprocessor-assumptions/20220603-050659
git checkout 4bf6a27a30fc5847a5fc6e6dae56e5716c2625ad
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: arch/x86/kernel/cpu/cacheinfo.o: in function `__cache_cpumap_setup':
>> cacheinfo.c:(.text+0x451): undefined reference to `cpu_llc_shared_map'
>> ld: cacheinfo.c:(.text+0x475): undefined reference to `cpu_llc_shared_map'
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v1 0/2] cpumask: Fix invalid uniprocessor assumptions
@ 2022-06-02 21:04 Sander Vanheule
2022-06-02 21:04 ` [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption Sander Vanheule
0 siblings, 1 reply; 5+ messages in thread
From: Sander Vanheule @ 2022-06-02 21:04 UTC (permalink / raw)
To: Peter Zijlstra, Yury Norov, Andrew Morton, Valentin Schneider,
Thomas Gleixner, Greg Kroah-Hartman, Marco Elver, Barry Song
Cc: linux-kernel, Andy Shevchenko, Sander Vanheule
On uniprocessor builds, it is currently assumed that any cpumask will
contain the single CPU: cpu0. This assumption is used to provide
optimised implementations.
The current assumption also appears to be wrong, by ignoring the fact
that users can provide empty cpumask-s. This can result in bugs as
explained in [1].
This series seeks to fix this assumption, allowing for masks that
contain at most cpu0, i.e. are "1" or "0".
[1] https://lore.kernel.org/all/20220530082552.46113-1-sander@svanheule.net/
Best,
Sander
---
To test these changes, I used the following code:
static void cpumask_test(const struct cpumask *mask)
{
int cpu;
if (cpumask_empty(mask))
pr_info("testing empty mask\n");
else
pr_info("testing non-empty mask\n");
pr_info("first cpu: %d\n", cpumask_first(mask));
pr_info("first zero: %d\n", cpumask_first_zero(mask));
pr_info("first and: %d\n", cpumask_first_and(mask, cpu_possible_mask));
pr_info("last cpu: %d\n", cpumask_last(mask));
pr_info("next cpu at -1: %d\n", cpumask_next(-1, mask));
pr_info("next cpu at 0: %d\n", cpumask_next(0, mask));
pr_info("next zero at -1: %d\n", cpumask_next_zero(-1, mask));
pr_info("next zero at 0: %d\n", cpumask_next_zero(0, mask));
pr_info("next and at -1: %d\n", cpumask_next_and(-1, mask, cpu_possible_mask));
pr_info("next and at 0: %d\n", cpumask_next_and(0, mask, cpu_possible_mask));
pr_info("next wrap at -1,false: %d\n", cpumask_next_wrap(-1, mask, 0, false));
pr_info("next wrap at 0,false: %d\n", cpumask_next_wrap(-1, mask, 0, false));
pr_info("next wrap at -1,true: %d\n", cpumask_next_wrap(-1, mask, 0, true));
pr_info("next wrap at 0,true: %d\n", cpumask_next_wrap(-1, mask, 0, true));
for_each_cpu(cpu, mask)
pr_info("for each: %d\n", cpu);
for_each_cpu_not(cpu, mask)
pr_info("for each not: %d\n", cpu);
for_each_cpu_wrap(cpu, mask, 0)
pr_info("for each wrap: %d\n", cpu);
for_each_cpu_and(cpu, mask, cpu_possible_mask)
pr_info("for each and: %d\n", cpu);
}
static void run_tests()
{
cpumask_clear(&empty_mask);
cpumask_test(&empty_mask);
cpumask_test(cpu_possible_mask);
}
On an unpatched build, with NR_CPUS=1:
[...] testing empty mask
[...] first cpu: 0
[...] first zero: 0
[...] first and: 0
[...] last cpu: 0
[...] next cpu at -1: 0
[...] next cpu at 0: 1
[...] next zero at -1: 0
[...] next zero at 0: 1
[...] next and at -1: 0
[...] next and at 0: 1
[...] next wrap at -1,false: 0
[...] next wrap at 0,false: 0
[...] next wrap at -1,true: 0
[...] next wrap at 0,true: 0
[...] for each: 0
[...] for each not: 0
[...] for each wrap: 0
[...] for each and: 0
[...] testing non-empty mask
[...] first cpu: 0
[...] first zero: 0
[...] first and: 0
[...] last cpu: 0
[...] next cpu at -1: 0
[...] next cpu at 0: 1
[...] next zero at -1: 0
[...] next zero at 0: 1
[...] next and at -1: 0
[...] next and at 0: 1
[...] next wrap at -1,false: 0
[...] next wrap at 0,false: 0
[...] next wrap at -1,true: 0
[...] next wrap at 0,true: 0
[...] for each: 0
[...] for each not: 0
[...] for each wrap: 0
[...] for each and: 0
On a patched build, with NR_CPUS=1:
[...] testing empty mask
[...] first cpu: 1
[...] first zero: 0
[...] first and: 1
[...] last cpu: 1
[...] next cpu at -1: 1
[...] next cpu at 0: 1
[...] next zero at -1: 0
[...] next zero at 0: 1
[...] next and at -1: 1
[...] next and at 0: 1
[...] next wrap at -1,false: 1
[...] next wrap at 0,false: 1
[...] next wrap at -1,true: 1
[...] next wrap at 0,true: 1
[...] for each not: 0
[...] testing non-empty mask
[...] first cpu: 0
[...] first zero: 1
[...] first and: 0
[...] last cpu: 0
[...] next cpu at -1: 0
[...] next cpu at 0: 1
[...] next zero at -1: 1
[...] next zero at 0: 1
[...] next and at -1: 0
[...] next and at 0: 1
[...] next wrap at -1,false: 0
[...] next wrap at 0,false: 0
[...] next wrap at -1,true: 0
[...] next wrap at 0,true: 0
[...] for each: 0
[...] for each wrap: 0
For reference, the generic implementation with NR_CPUS=2, CONFIG_SMP=y
[...] testing empty mask
[...] first cpu: 2
[...] first zero: 0
[...] first and: 2
[...] last cpu: 2
[...] next cpu at -1: 2
[...] next cpu at 0: 2
[...] next zero at -1: 0
[...] next zero at 0: 1
[...] next and at -1: 2
[...] next and at 0: 2
[...] next wrap at -1,false: 2
[...] next wrap at 0,false: 2
[...] next wrap at -1,true: 2
[...] next wrap at 0,true: 2
[...] for each not: 0
[...] testing non-empty mask
[...] first cpu: 0
[...] first zero: 1
[...] first and: 0
[...] last cpu: 0
[...] next cpu at -1: 0
[...] next cpu at 0: 2
[...] next zero at -1: 1
[...] next zero at 0: 1
[...] next and at -1: 0
[...] next and at 0: 2
[...] next wrap at -1,false: 0
[...] next wrap at 0,false: 0
[...] next wrap at -1,true: 2
[...] next wrap at 0,true: 2
[...] for each: 0
[...] for each wrap: 0
[...] for each and: 0
Sander Vanheule (2):
cpumask: Fix invalid uniprocessor mask assumption
cpumask: Add UP optimised for_each_*_cpu loops
include/linux/cpumask.h | 42 +++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
--
2.36.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption
2022-06-02 21:04 [PATCH v1 0/2] cpumask: Fix invalid uniprocessor assumptions Sander Vanheule
@ 2022-06-02 21:04 ` Sander Vanheule
2022-06-02 22:48 ` Yury Norov
0 siblings, 1 reply; 5+ messages in thread
From: Sander Vanheule @ 2022-06-02 21:04 UTC (permalink / raw)
To: Peter Zijlstra, Yury Norov, Andrew Morton, Valentin Schneider,
Thomas Gleixner, Greg Kroah-Hartman, Marco Elver, Barry Song
Cc: linux-kernel, Andy Shevchenko, Sander Vanheule
On uniprocessor builds, any CPU mask is assumed to contain exactly one
CPU (cpu0). This ignores the existence of empty masks, resulting in
incorrect behaviour for most of the implemented optimisations.
Replace the uniprocessor optimisations with implementations that also
take into account empty masks. Also drop the incorrectly optimised
for_each_cpu implementations and use the generic implementations in all
cases.
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
include/linux/cpumask.h | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index fe29ac7cc469..ce8c7b27f6c9 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -117,51 +117,54 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
}
#if NR_CPUS == 1
-/* Uniprocessor. Assume all masks are "1". */
+/* Uniprocessor. Assume all valid masks are "1" or empty. */
static inline unsigned int cpumask_first(const struct cpumask *srcp)
{
- return 0;
+ return !(*cpumask_bits(srcp) & 1);
}
static inline unsigned int cpumask_first_zero(const struct cpumask *srcp)
{
- return 0;
+ return *cpumask_bits(srcp) & 1;
}
static inline unsigned int cpumask_first_and(const struct cpumask *srcp1,
const struct cpumask *srcp2)
{
- return 0;
+ return !(*cpumask_bits(srcp1) & *cpumask_bits(srcp2) & 1);
}
static inline unsigned int cpumask_last(const struct cpumask *srcp)
{
- return 0;
+ return cpumask_first(srcp);
}
/* Valid inputs for n are -1 and 0. */
static inline unsigned int cpumask_next(int n, const struct cpumask *srcp)
{
- return n+1;
+ return !!(n + 1 + cpumask_first(srcp));
}
static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
{
- return n+1;
+ return !!(n + 1 + cpumask_first_zero(srcp));
}
static inline unsigned int cpumask_next_and(int n,
const struct cpumask *srcp,
const struct cpumask *andp)
{
- return n+1;
+ return !!(n + 1 + cpumask_first_and(srcp, andp));
}
static inline unsigned int cpumask_next_wrap(int n, const struct cpumask *mask,
int start, bool wrap)
{
- /* cpu0 unless stop condition, wrap and at cpu0, then nr_cpumask_bits */
- return (wrap && n == 0);
+ /*
+ * nr_cpumask_bits at stop condition, wrap and at cpu0, or empty mask
+ * otherwise cpu0
+ */
+ return (wrap && n == 0) || cpumask_first(mask);
}
/* cpu must be a valid cpu, ie 0, so there's no other choice. */
@@ -186,14 +189,6 @@ static inline int cpumask_any_distribute(const struct cpumask *srcp)
return cpumask_first(srcp);
}
-#define for_each_cpu(cpu, mask) \
- for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
-#define for_each_cpu_not(cpu, mask) \
- for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
-#define for_each_cpu_wrap(cpu, mask, start) \
- for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask, (void)(start))
-#define for_each_cpu_and(cpu, mask1, mask2) \
- for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask1, (void)mask2)
#else
/**
* cpumask_first - get the first cpu in a cpumask
@@ -259,11 +254,13 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
}
int __pure cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
+extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
int __pure cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
unsigned int cpumask_local_spread(unsigned int i, int node);
int cpumask_any_and_distribute(const struct cpumask *src1p,
const struct cpumask *src2p);
int cpumask_any_distribute(const struct cpumask *srcp);
+#endif /* SMP */
/**
* for_each_cpu - iterate over every cpu in a mask
@@ -289,7 +286,6 @@ int cpumask_any_distribute(const struct cpumask *srcp);
(cpu) = cpumask_next_zero((cpu), (mask)), \
(cpu) < nr_cpu_ids;)
-extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
/**
* for_each_cpu_wrap - iterate over every cpu in a mask, starting at a specified location
@@ -324,7 +320,6 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
for ((cpu) = -1; \
(cpu) = cpumask_next_and((cpu), (mask1), (mask2)), \
(cpu) < nr_cpu_ids;)
-#endif /* SMP */
#define CPU_BITS_NONE \
{ \
--
2.36.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption
2022-06-02 21:04 ` [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption Sander Vanheule
@ 2022-06-02 22:48 ` Yury Norov
2022-06-03 15:13 ` Sander Vanheule
0 siblings, 1 reply; 5+ messages in thread
From: Yury Norov @ 2022-06-02 22:48 UTC (permalink / raw)
To: Sander Vanheule
Cc: Peter Zijlstra, Andrew Morton, Valentin Schneider,
Thomas Gleixner, Greg Kroah-Hartman, Marco Elver, Barry Song,
linux-kernel, Andy Shevchenko
On Thu, Jun 02, 2022 at 11:04:19PM +0200, Sander Vanheule wrote:
> On uniprocessor builds, any CPU mask is assumed to contain exactly one
> CPU (cpu0). This ignores the existence of empty masks, resulting in
> incorrect behaviour for most of the implemented optimisations.
>
> Replace the uniprocessor optimisations with implementations that also
> take into account empty masks. Also drop the incorrectly optimised
> for_each_cpu implementations and use the generic implementations in all
> cases.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> include/linux/cpumask.h | 35 +++++++++++++++--------------------
> 1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index fe29ac7cc469..ce8c7b27f6c9 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -117,51 +117,54 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
> }
>
> #if NR_CPUS == 1
> -/* Uniprocessor. Assume all masks are "1". */
> +/* Uniprocessor. Assume all valid masks are "1" or empty. */
> static inline unsigned int cpumask_first(const struct cpumask *srcp)
> {
> - return 0;
> + return !(*cpumask_bits(srcp) & 1);
> }
I think, you can just drop this '#if NR_CPUS == 1' part and rely on SMP
versions. find_first_bit() is optimized for short bitmaps, so I expect
no overhead comparing to this.
> static inline unsigned int cpumask_first_zero(const struct cpumask *srcp)
> {
> - return 0;
> + return *cpumask_bits(srcp) & 1;
> }
>
> static inline unsigned int cpumask_first_and(const struct cpumask *srcp1,
> const struct cpumask *srcp2)
> {
> - return 0;
> + return !(*cpumask_bits(srcp1) & *cpumask_bits(srcp2) & 1);
> }
>
> static inline unsigned int cpumask_last(const struct cpumask *srcp)
> {
> - return 0;
> + return cpumask_first(srcp);
> }
>
> /* Valid inputs for n are -1 and 0. */
> static inline unsigned int cpumask_next(int n, const struct cpumask *srcp)
> {
> - return n+1;
> + return !!(n + 1 + cpumask_first(srcp));
> }
>
> static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
> {
> - return n+1;
> + return !!(n + 1 + cpumask_first_zero(srcp));
> }
>
> static inline unsigned int cpumask_next_and(int n,
> const struct cpumask *srcp,
> const struct cpumask *andp)
> {
> - return n+1;
> + return !!(n + 1 + cpumask_first_and(srcp, andp));
> }
>
> static inline unsigned int cpumask_next_wrap(int n, const struct cpumask *mask,
> int start, bool wrap)
> {
> - /* cpu0 unless stop condition, wrap and at cpu0, then nr_cpumask_bits */
> - return (wrap && n == 0);
> + /*
> + * nr_cpumask_bits at stop condition, wrap and at cpu0, or empty mask
> + * otherwise cpu0
> + */
> + return (wrap && n == 0) || cpumask_first(mask);
> }
>
> /* cpu must be a valid cpu, ie 0, so there's no other choice. */
> @@ -186,14 +189,6 @@ static inline int cpumask_any_distribute(const struct cpumask *srcp)
> return cpumask_first(srcp);
> }
>
> -#define for_each_cpu(cpu, mask) \
> - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> -#define for_each_cpu_not(cpu, mask) \
> - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> -#define for_each_cpu_wrap(cpu, mask, start) \
> - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask, (void)(start))
> -#define for_each_cpu_and(cpu, mask1, mask2) \
> - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask1, (void)mask2)
> #else
> /**
> * cpumask_first - get the first cpu in a cpumask
> @@ -259,11 +254,13 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
> }
>
> int __pure cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
> +extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
is this extern needed?
Thanks,
Yury
> int __pure cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
> unsigned int cpumask_local_spread(unsigned int i, int node);
> int cpumask_any_and_distribute(const struct cpumask *src1p,
> const struct cpumask *src2p);
> int cpumask_any_distribute(const struct cpumask *srcp);
> +#endif /* SMP */
>
> /**
> * for_each_cpu - iterate over every cpu in a mask
> @@ -289,7 +286,6 @@ int cpumask_any_distribute(const struct cpumask *srcp);
> (cpu) = cpumask_next_zero((cpu), (mask)), \
> (cpu) < nr_cpu_ids;)
>
> -extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
>
> /**
> * for_each_cpu_wrap - iterate over every cpu in a mask, starting at a specified location
> @@ -324,7 +320,6 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
> for ((cpu) = -1; \
> (cpu) = cpumask_next_and((cpu), (mask1), (mask2)), \
> (cpu) < nr_cpu_ids;)
> -#endif /* SMP */
>
> #define CPU_BITS_NONE \
> { \
> --
> 2.36.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption
2022-06-02 22:48 ` Yury Norov
@ 2022-06-03 15:13 ` Sander Vanheule
0 siblings, 0 replies; 5+ messages in thread
From: Sander Vanheule @ 2022-06-03 15:13 UTC (permalink / raw)
To: Yury Norov
Cc: Peter Zijlstra, Andrew Morton, Valentin Schneider,
Thomas Gleixner, Greg Kroah-Hartman, Marco Elver, linux-kernel,
Andy Shevchenko
On Thu, 2022-06-02 at 15:48 -0700, Yury Norov wrote:
> On Thu, Jun 02, 2022 at 11:04:19PM +0200, Sander Vanheule wrote:
> > On uniprocessor builds, any CPU mask is assumed to contain exactly one
> > CPU (cpu0). This ignores the existence of empty masks, resulting in
> > incorrect behaviour for most of the implemented optimisations.
> >
> > Replace the uniprocessor optimisations with implementations that also
> > take into account empty masks. Also drop the incorrectly optimised
> > for_each_cpu implementations and use the generic implementations in all
> > cases.
> >
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> > include/linux/cpumask.h | 35 +++++++++++++++--------------------
> > 1 file changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index fe29ac7cc469..ce8c7b27f6c9 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -117,51 +117,54 @@ static __always_inline unsigned int
> > cpumask_check(unsigned int cpu)
> > }
> >
> > #if NR_CPUS == 1
> > -/* Uniprocessor. Assume all masks are "1". */
> > +/* Uniprocessor. Assume all valid masks are "1" or empty. */
> > static inline unsigned int cpumask_first(const struct cpumask *srcp)
> > {
> > - return 0;
> > + return !(*cpumask_bits(srcp) & 1);
> > }
>
> I think, you can just drop this '#if NR_CPUS == 1' part and rely on SMP
> versions. find_first_bit() is optimized for short bitmaps, so I expect
> no overhead comparing to this.
I was initially planning to just drop the UP versions, but that means I have to
include lib/cpumask.o on all builds. I'll have another look at this.
[...]
> >
> > static inline unsigned int cpumask_next_wrap(int n, const struct cpumask
> > *mask,
> > int start, bool wrap)
> > {
> > - /* cpu0 unless stop condition, wrap and at cpu0, then
> > nr_cpumask_bits */
> > - return (wrap && n == 0);
> > + /*
> > + * nr_cpumask_bits at stop condition, wrap and at cpu0, or empty
> > mask
> > + * otherwise cpu0
> > + */
> > + return (wrap && n == 0) || cpumask_first(mask);
> > }
My tests contained a copy-paste error, so I missed a case. The results currently
still differ from the SMP implementation though, so this looks like another
reason to just drop the UP optimised version.
> >
> > /* cpu must be a valid cpu, ie 0, so there's no other choice. */
> > @@ -186,14 +189,6 @@ static inline int cpumask_any_distribute(const struct
> > cpumask *srcp)
> > return cpumask_first(srcp);
> > }
> >
> > -#define for_each_cpu(cpu, mask) \
> > - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> > -#define for_each_cpu_not(cpu, mask) \
> > - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> > -#define for_each_cpu_wrap(cpu, mask, start) \
> > - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask, (void)(start))
> > -#define for_each_cpu_and(cpu, mask1, mask2) \
> > - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask1, (void)mask2)
> > #else
> > /**
> > * cpumask_first - get the first cpu in a cpumask
> > @@ -259,11 +254,13 @@ static inline unsigned int cpumask_next_zero(int n,
> > const struct cpumask *srcp)
> > }
> >
> > int __pure cpumask_next_and(int n, const struct cpumask *, const struct
> > cpumask *);
> > +extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start,
> > bool wrap);
>
> is this extern needed?
I don't think it is, but this is a line that I just moved around in the header.
Like for the other functions, I could even add the __pure qualifier.
Thanks for the feedback,
Sander
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-04 12:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-04 12:02 [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2022-06-03 5:35 kernel test robot
2022-06-02 21:04 [PATCH v1 0/2] cpumask: Fix invalid uniprocessor assumptions Sander Vanheule
2022-06-02 21:04 ` [PATCH v1 1/2] cpumask: Fix invalid uniprocessor mask assumption Sander Vanheule
2022-06-02 22:48 ` Yury Norov
2022-06-03 15:13 ` Sander Vanheule
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.