* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
[not found] <202401230837.TXro0PHi-lkp@intel.com>
@ 2024-01-23 7:07 ` Baokun Li
2024-01-23 16:24 ` Christian Brauner
2024-01-23 17:46 ` Linus Torvalds
0 siblings, 2 replies; 9+ messages in thread
From: Baokun Li @ 2024-01-23 7:07 UTC (permalink / raw)
To: kernel test robot, sfr
Cc: llvm, oe-kbuild-all, Christian Brauner, Christian Brauner,
Baokun Li, yangerkun, zhangyi (F),
Linus Torvalds, linux-next
On 2024/1/23 8:12, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc
> head: 297983dc9011461cba6278bfe03f4305c4a2caa0
> commit: 4bbd51d0f0ad709c0f02c100439d6c9ba6811d4b [12/13] fs: make the i_size_read/write helpers be smp_load_acquire/store_release()
> config: i386-randconfig-015-20240123 (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/config)
> compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202401230837.TXro0PHi-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> In file included from fs/netfs/buffered_read.c:10:
> In file included from fs/netfs/internal.h:9:
> In file included from include/linux/seq_file.h:12:
>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> 911 | return smp_load_acquire(&inode->i_size);
> | ^
> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> 206 | compiletime_assert_atomic_type(*p); \
> | ^
> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> 438 | compiletime_assert(__native_word(t), \
> | ^
> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> 423 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^
> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> 416 | prefix ## suffix(); \
> | ^
> <scratch space>:38:1: note: expanded from here
> 38 | __compiletime_assert_207
> | ^
> 1 error generated.
> --
> In file included from fs/netfs/buffered_write.c:9:
>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> 911 | return smp_load_acquire(&inode->i_size);
> | ^
> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> 206 | compiletime_assert_atomic_type(*p); \
> | ^
> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> 438 | compiletime_assert(__native_word(t), \
> | ^
> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> 423 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^
> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> 416 | prefix ## suffix(); \
> | ^
> <scratch space>:253:1: note: expanded from here
> 253 | __compiletime_assert_207
> | ^
> In file included from fs/netfs/buffered_write.c:9:
>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> 206 | compiletime_assert_atomic_type(*p); \
> | ^
> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> 438 | compiletime_assert(__native_word(t), \
> | ^
> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> 423 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^
> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> 416 | prefix ## suffix(); \
> | ^
> <scratch space>:253:1: note: expanded from here
> 253 | __compiletime_assert_207
> | ^
> In file included from fs/netfs/buffered_write.c:9:
>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> 206 | compiletime_assert_atomic_type(*p); \
> | ^
> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> 438 | compiletime_assert(__native_word(t), \
> | ^
> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> 423 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^
> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> 416 | prefix ## suffix(); \
> | ^
> <scratch space>:253:1: note: expanded from here
> 253 | __compiletime_assert_207
> | ^
> In file included from fs/netfs/buffered_write.c:9:
>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> 206 | compiletime_assert_atomic_type(*p); \
> | ^
> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> 438 | compiletime_assert(__native_word(t), \
> | ^
> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> 423 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^
> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> 416 | prefix ## suffix(); \
> | ^
> <scratch space>:253:1: note: expanded from here
> 253 | __compiletime_assert_207
> | ^
> 4 errors generated.
> --
> In file included from fs/netfs/misc.c:8:
> In file included from include/linux/swap.h:9:
> In file included from include/linux/memcontrol.h:13:
> In file included from include/linux/cgroup.h:17:
>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> 911 | return smp_load_acquire(&inode->i_size);
> | ^
> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> 206 | compiletime_assert_atomic_type(*p); \
> | ^
> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> 438 | compiletime_assert(__native_word(t), \
> | ^
> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> 423 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^
> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> 416 | prefix ## suffix(); \
> | ^
> <scratch space>:251:1: note: expanded from here
> 251 | __compiletime_assert_207
> | ^
> 1 error generated.
>
> Kconfig warnings: (for reference only)
> WARNING: unmet direct dependencies detected for DRM_I915_DEBUG_GEM
> Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && DRM_I915_WERROR [=n]
> Selected by [y]:
> - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && !COMPILE_TEST [=n]
>
>
> vim +911 include/linux/fs.h
>
> 874
> 875 void filemap_invalidate_lock_two(struct address_space *mapping1,
> 876 struct address_space *mapping2);
> 877 void filemap_invalidate_unlock_two(struct address_space *mapping1,
> 878 struct address_space *mapping2);
> 879
> 880
> 881 /*
> 882 * NOTE: in a 32bit arch with a preemptable kernel and
> 883 * an UP compile the i_size_read/write must be atomic
> 884 * with respect to the local cpu (unlike with preempt disabled),
> 885 * but they don't need to be atomic with respect to other cpus like in
> 886 * true SMP (so they need either to either locally disable irq around
> 887 * the read or for example on x86 they can be still implemented as a
> 888 * cmpxchg8b without the need of the lock prefix). For SMP compiles
> 889 * and 64bit archs it makes no difference if preempt is enabled or not.
> 890 */
> 891 static inline loff_t i_size_read(const struct inode *inode)
> 892 {
> 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> 894 loff_t i_size;
> 895 unsigned int seq;
> 896
> 897 do {
> 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
> 899 i_size = inode->i_size;
> 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
> 901 return i_size;
> 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
> 903 loff_t i_size;
> 904
> 905 preempt_disable();
> 906 i_size = inode->i_size;
> 907 preempt_enable();
> 908 return i_size;
> 909 #else
> 910 /* Pairs with smp_store_release() in i_size_write() */
> > 911 return smp_load_acquire(&inode->i_size);
> 912 #endif
> 913 }
> 914
Sorry to cause this compilation error, I have not encountered this
before when testing compilation with gcc on x86 and arm64.
Is there a special config required for this, or is it just clang that
triggers this compile error?
The compile error seems to be that some architectural implementations
of smp_load_acquire() do not support pointers to long long data
types. Can't think of a good way to avoid this problem, any ideas
from linus and christian?
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-23 7:07 ` [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity Baokun Li
@ 2024-01-23 16:24 ` Christian Brauner
2024-01-24 7:52 ` Baokun Li
2024-01-23 17:46 ` Linus Torvalds
1 sibling, 1 reply; 9+ messages in thread
From: Christian Brauner @ 2024-01-23 16:24 UTC (permalink / raw)
To: Baokun Li
Cc: kernel test robot, sfr, llvm, oe-kbuild-all, Christian Brauner,
yangerkun, zhangyi (F),
Linus Torvalds, linux-next
On Tue, Jan 23, 2024 at 03:07:50PM +0800, Baokun Li wrote:
> On 2024/1/23 8:12, kernel test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc
> > head: 297983dc9011461cba6278bfe03f4305c4a2caa0
> > commit: 4bbd51d0f0ad709c0f02c100439d6c9ba6811d4b [12/13] fs: make the i_size_read/write helpers be smp_load_acquire/store_release()
> > config: i386-randconfig-015-20240123 (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/config)
> > compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202401230837.TXro0PHi-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> > In file included from fs/netfs/buffered_read.c:10:
> > In file included from fs/netfs/internal.h:9:
> > In file included from include/linux/seq_file.h:12:
> > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > 911 | return smp_load_acquire(&inode->i_size);
> > | ^
> > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > 206 | compiletime_assert_atomic_type(*p); \
> > | ^
> > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > 438 | compiletime_assert(__native_word(t), \
> > | ^
> > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^
> > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > 416 | prefix ## suffix(); \
> > | ^
> > <scratch space>:38:1: note: expanded from here
> > 38 | __compiletime_assert_207
> > | ^
> > 1 error generated.
> > --
> > In file included from fs/netfs/buffered_write.c:9:
> > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > 911 | return smp_load_acquire(&inode->i_size);
> > | ^
> > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > 206 | compiletime_assert_atomic_type(*p); \
> > | ^
> > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > 438 | compiletime_assert(__native_word(t), \
> > | ^
> > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^
> > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > 416 | prefix ## suffix(); \
> > | ^
> > <scratch space>:253:1: note: expanded from here
> > 253 | __compiletime_assert_207
> > | ^
> > In file included from fs/netfs/buffered_write.c:9:
> > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > 206 | compiletime_assert_atomic_type(*p); \
> > | ^
> > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > 438 | compiletime_assert(__native_word(t), \
> > | ^
> > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^
> > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > 416 | prefix ## suffix(); \
> > | ^
> > <scratch space>:253:1: note: expanded from here
> > 253 | __compiletime_assert_207
> > | ^
> > In file included from fs/netfs/buffered_write.c:9:
> > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > 206 | compiletime_assert_atomic_type(*p); \
> > | ^
> > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > 438 | compiletime_assert(__native_word(t), \
> > | ^
> > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^
> > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > 416 | prefix ## suffix(); \
> > | ^
> > <scratch space>:253:1: note: expanded from here
> > 253 | __compiletime_assert_207
> > | ^
> > In file included from fs/netfs/buffered_write.c:9:
> > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > 206 | compiletime_assert_atomic_type(*p); \
> > | ^
> > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > 438 | compiletime_assert(__native_word(t), \
> > | ^
> > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^
> > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > 416 | prefix ## suffix(); \
> > | ^
> > <scratch space>:253:1: note: expanded from here
> > 253 | __compiletime_assert_207
> > | ^
> > 4 errors generated.
> > --
> > In file included from fs/netfs/misc.c:8:
> > In file included from include/linux/swap.h:9:
> > In file included from include/linux/memcontrol.h:13:
> > In file included from include/linux/cgroup.h:17:
> > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > 911 | return smp_load_acquire(&inode->i_size);
> > | ^
> > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > 206 | compiletime_assert_atomic_type(*p); \
> > | ^
> > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > 438 | compiletime_assert(__native_word(t), \
> > | ^
> > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^
> > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > 416 | prefix ## suffix(); \
> > | ^
> > <scratch space>:251:1: note: expanded from here
> > 251 | __compiletime_assert_207
> > | ^
> > 1 error generated.
> >
> > Kconfig warnings: (for reference only)
> > WARNING: unmet direct dependencies detected for DRM_I915_DEBUG_GEM
> > Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && DRM_I915_WERROR [=n]
> > Selected by [y]:
> > - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && !COMPILE_TEST [=n]
> >
> >
> > vim +911 include/linux/fs.h
> >
> > 874
> > 875 void filemap_invalidate_lock_two(struct address_space *mapping1,
> > 876 struct address_space *mapping2);
> > 877 void filemap_invalidate_unlock_two(struct address_space *mapping1,
> > 878 struct address_space *mapping2);
> > 879
> > 880
> > 881 /*
> > 882 * NOTE: in a 32bit arch with a preemptable kernel and
> > 883 * an UP compile the i_size_read/write must be atomic
> > 884 * with respect to the local cpu (unlike with preempt disabled),
> > 885 * but they don't need to be atomic with respect to other cpus like in
> > 886 * true SMP (so they need either to either locally disable irq around
> > 887 * the read or for example on x86 they can be still implemented as a
> > 888 * cmpxchg8b without the need of the lock prefix). For SMP compiles
> > 889 * and 64bit archs it makes no difference if preempt is enabled or not.
> > 890 */
> > 891 static inline loff_t i_size_read(const struct inode *inode)
> > 892 {
> > 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> > 894 loff_t i_size;
> > 895 unsigned int seq;
> > 896
> > 897 do {
> > 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
> > 899 i_size = inode->i_size;
> > 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
> > 901 return i_size;
> > 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
> > 903 loff_t i_size;
> > 904
> > 905 preempt_disable();
> > 906 i_size = inode->i_size;
> > 907 preempt_enable();
> > 908 return i_size;
> > 909 #else
> > 910 /* Pairs with smp_store_release() in i_size_write() */
> > > 911 return smp_load_acquire(&inode->i_size);
> > 912 #endif
> > 913 }
> > 914
> Sorry to cause this compilation error, I have not encountered this
> before when testing compilation with gcc on x86 and arm64.
> Is there a special config required for this, or is it just clang that
> triggers this compile error?
>
> The compile error seems to be that some architectural implementations
> of smp_load_acquire() do not support pointers to long long data
> types. Can't think of a good way to avoid this problem, any ideas
> from linus and christian?
That code in i_size_{read,write}() is terrible to look at.
Pragmatically, we can probably just do READ_ONCE() + smp_rmb() and
smp_wmb() + WRITE_ONCE(). This guarantees the ordering and it works
(compiles) on 32bit as well. I think it's still possible that on 32bit
the READ_ONCE()/WRITE_ONCE() are compiled as two accesses. But I'm not
sure that this matters because iiuc that problem would've already been
there with the barrier in mm/filemap.c instead of here.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-23 7:07 ` [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity Baokun Li
2024-01-23 16:24 ` Christian Brauner
@ 2024-01-23 17:46 ` Linus Torvalds
2024-01-24 7:31 ` Baokun Li
1 sibling, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2024-01-23 17:46 UTC (permalink / raw)
To: Baokun Li
Cc: kernel test robot, sfr, llvm, oe-kbuild-all, Christian Brauner,
Christian Brauner, yangerkun, zhangyi (F),
linux-next
On Mon, 22 Jan 2024 at 23:08, Baokun Li <libaokun1@huawei.com> wrote:
>
> >>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > 911 | return smp_load_acquire(&inode->i_size);
Ahh, yes. We used to allow READ_ONCE() on non-atomic data structures
(we still do, but we used to too), but with truly atomic cases it's
wrong, since reading a 64-bit value with two 32-bit instructions is
clearly ever atomic.
So your patch is hitting the "these atomic and/or ordered accesses
need to be able to actually be done as *one* access" sanity check.
And the reason is that while we have special-cases 32-bit AMP and
preemption code to avoid this:
> > 891 static inline loff_t i_size_read(const struct inode *inode)
> > 892 {
> > 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> > 894 loff_t i_size;
> > 895 unsigned int seq;
> > 896
> > 897 do {
> > 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
> > 899 i_size = inode->i_size;
> > 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
> > 901 return i_size;
> > 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
> > 903 loff_t i_size;
> > 904
> > 905 preempt_disable();
> > 906 i_size = inode->i_size;
> > 907 preempt_enable();
> > 908 return i_size;
We have *not* special-cased the UP and non-preempt code, because in
that case doing a 64-bit load with two 32-bit accesses is obviously
fine (modulo interrupts, which *really* shouldn't be changing i_size.
So this last case:
> > 909 #else
> > 910 /* Pairs with smp_store_release() in i_size_write() */
> > > 911 return smp_load_acquire(&inode->i_size);
> > 912 #endif
used to be just a simple
return inode->i_size;
but now that you changed it to smp_load_acquire(), our "native size"
sanity checks kick in.
The solution is either to add a new case here (saying "if we're not
SMP, the smp_load_acquire() is pointless"), or to just remove the size
check from the non-SMP version of smp_load_acquire().
Honestly, that sounds like the right thing to do anyway. Our non-SMP
case looks like this:
#ifndef smp_load_acquire
#define smp_load_acquire(p) \
({ \
__unqual_scalar_typeof(*p) ___p1 = READ_ONCE(*p); \
compiletime_assert_atomic_type(*p); \
barrier(); \
(typeof(*p))___p1; \
})
#endif
and that compiletime_assert_atomic_type() might as well go away. Yes,
it removes some type-checking coverage, but honestly, the non-SMP case
simply isn't relevant. No developer uses a UP build for testing
anyway, so the "less coverage" is pretty much completely theoretical.
So I *think* the fix is as trivial as something like this:
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -193,7 +193,6 @@ do { \
#ifndef smp_store_release
#define smp_store_release(p, v) \
do { \
- compiletime_assert_atomic_type(*p); \
barrier(); \
WRITE_ONCE(*p, v); \
} while (0)
@@ -203,7 +202,6 @@ do { \
#define smp_load_acquire(p) \
({ \
__unqual_scalar_typeof(*p) ___p1 = READ_ONCE(*p); \
- compiletime_assert_atomic_type(*p); \
barrier(); \
(typeof(*p))___p1; \
})
because the extra type checking here only makes for extra work, not
for extra safety.
Hmm?
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-23 17:46 ` Linus Torvalds
@ 2024-01-24 7:31 ` Baokun Li
0 siblings, 0 replies; 9+ messages in thread
From: Baokun Li @ 2024-01-24 7:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: kernel test robot, sfr, llvm, oe-kbuild-all, Christian Brauner,
Christian Brauner, yangerkun, zhangyi (F),
linux-next
On 2024/1/24 1:46, Linus Torvalds wrote:
> On Mon, 22 Jan 2024 at 23:08, Baokun Li <libaokun1@huawei.com> wrote:
>>>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
>>> 911 | return smp_load_acquire(&inode->i_size);
> Ahh, yes. We used to allow READ_ONCE() on non-atomic data structures
> (we still do, but we used to too), but with truly atomic cases it's
> wrong, since reading a 64-bit value with two 32-bit instructions is
> clearly ever atomic.
>
> So your patch is hitting the "these atomic and/or ordered accesses
> need to be able to actually be done as *one* access" sanity check.
>
> And the reason is that while we have special-cases 32-bit AMP and
> preemption code to avoid this:
>
>>> 891 static inline loff_t i_size_read(const struct inode *inode)
>>> 892 {
>>> 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
>>> 894 loff_t i_size;
>>> 895 unsigned int seq;
>>> 896
>>> 897 do {
>>> 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
>>> 899 i_size = inode->i_size;
>>> 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
>>> 901 return i_size;
>>> 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
>>> 903 loff_t i_size;
>>> 904
>>> 905 preempt_disable();
>>> 906 i_size = inode->i_size;
>>> 907 preempt_enable();
>>> 908 return i_size;
> We have *not* special-cased the UP and non-preempt code, because in
> that case doing a 64-bit load with two 32-bit accesses is obviously
> fine (modulo interrupts, which *really* shouldn't be changing i_size.
>
> So this last case:
>
>>> 909 #else
>>> 910 /* Pairs with smp_store_release() in i_size_write() */
>>> > 911 return smp_load_acquire(&inode->i_size);
>>> 912 #endif
> used to be just a simple
>
> return inode->i_size;
>
> but now that you changed it to smp_load_acquire(), our "native size"
> sanity checks kick in.
>
> The solution is either to add a new case here (saying "if we're not
> SMP, the smp_load_acquire() is pointless"), or to just remove the size
> check from the non-SMP version of smp_load_acquire().
>
> Honestly, that sounds like the right thing to do anyway. Our non-SMP
> case looks like this:
>
> #ifndef smp_load_acquire
> #define smp_load_acquire(p) \
> ({ \
> __unqual_scalar_typeof(*p) ___p1 = READ_ONCE(*p); \
> compiletime_assert_atomic_type(*p); \
> barrier(); \
> (typeof(*p))___p1; \
> })
> #endif
>
> and that compiletime_assert_atomic_type() might as well go away. Yes,
> it removes some type-checking coverage, but honestly, the non-SMP case
> simply isn't relevant. No developer uses a UP build for testing
> anyway, so the "less coverage" is pretty much completely theoretical.
>
> So I *think* the fix is as trivial as something like this:
>
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -193,7 +193,6 @@ do { \
> #ifndef smp_store_release
> #define smp_store_release(p, v) \
> do { \
> - compiletime_assert_atomic_type(*p); \
> barrier(); \
> WRITE_ONCE(*p, v); \
> } while (0)
> @@ -203,7 +202,6 @@ do { \
> #define smp_load_acquire(p) \
> ({ \
> __unqual_scalar_typeof(*p) ___p1 = READ_ONCE(*p); \
> - compiletime_assert_atomic_type(*p); \
> barrier(); \
> (typeof(*p))___p1; \
> })
>
> because the extra type checking here only makes for extra work, not
> for extra safety.
>
> Hmm?
>
> Linus
Thank you very much for the detailed explanation! 😊
Now I understand that the cause of this problem is that on 32-bit
architectures the size of the long type and the long long type are
not equal, so this compilation error is triggered when both
CONFIG_SMP and CONFIG_PREEMPTION are not enabled, which
is also why I don't have it triggered on x86 and arm64. I will
include the patch in the next version of the patch set.
Thanks again!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-23 16:24 ` Christian Brauner
@ 2024-01-24 7:52 ` Baokun Li
2024-01-24 10:30 ` Christian Brauner
0 siblings, 1 reply; 9+ messages in thread
From: Baokun Li @ 2024-01-24 7:52 UTC (permalink / raw)
To: Christian Brauner
Cc: kernel test robot, sfr, llvm, oe-kbuild-all, Christian Brauner,
yangerkun, zhangyi (F),
Linus Torvalds, linux-next, Baokun Li
On 2024/1/24 0:24, Christian Brauner wrote:
> On Tue, Jan 23, 2024 at 03:07:50PM +0800, Baokun Li wrote:
>> On 2024/1/23 8:12, kernel test robot wrote:
>>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc
>>> head: 297983dc9011461cba6278bfe03f4305c4a2caa0
>>> commit: 4bbd51d0f0ad709c0f02c100439d6c9ba6811d4b [12/13] fs: make the i_size_read/write helpers be smp_load_acquire/store_release()
>>> config: i386-randconfig-015-20240123 (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/config)
>>> compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202401230837.TXro0PHi-lkp@intel.com/
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>> In file included from fs/netfs/buffered_read.c:10:
>>> In file included from fs/netfs/internal.h:9:
>>> In file included from include/linux/seq_file.h:12:
>>>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
>>> 911 | return smp_load_acquire(&inode->i_size);
>>> | ^
>>> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
>>> 206 | compiletime_assert_atomic_type(*p); \
>>> | ^
>>> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
>>> 438 | compiletime_assert(__native_word(t), \
>>> | ^
>>> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
>>> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>> | ^
>>> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
>>> 423 | __compiletime_assert(condition, msg, prefix, suffix)
>>> | ^
>>> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
>>> 416 | prefix ## suffix(); \
>>> | ^
>>> <scratch space>:38:1: note: expanded from here
>>> 38 | __compiletime_assert_207
>>> | ^
>>> 1 error generated.
>>>
>>>
>>> vim +911 include/linux/fs.h
>>>
>>> 874
>>> 875 void filemap_invalidate_lock_two(struct address_space *mapping1,
>>> 876 struct address_space *mapping2);
>>> 877 void filemap_invalidate_unlock_two(struct address_space *mapping1,
>>> 878 struct address_space *mapping2);
>>> 879
>>> 880
>>> 881 /*
>>> 882 * NOTE: in a 32bit arch with a preemptable kernel and
>>> 883 * an UP compile the i_size_read/write must be atomic
>>> 884 * with respect to the local cpu (unlike with preempt disabled),
>>> 885 * but they don't need to be atomic with respect to other cpus like in
>>> 886 * true SMP (so they need either to either locally disable irq around
>>> 887 * the read or for example on x86 they can be still implemented as a
>>> 888 * cmpxchg8b without the need of the lock prefix). For SMP compiles
>>> 889 * and 64bit archs it makes no difference if preempt is enabled or not.
>>> 890 */
>>> 891 static inline loff_t i_size_read(const struct inode *inode)
>>> 892 {
>>> 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
>>> 894 loff_t i_size;
>>> 895 unsigned int seq;
>>> 896
>>> 897 do {
>>> 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
>>> 899 i_size = inode->i_size;
>>> 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
>>> 901 return i_size;
>>> 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
>>> 903 loff_t i_size;
>>> 904
>>> 905 preempt_disable();
>>> 906 i_size = inode->i_size;
>>> 907 preempt_enable();
>>> 908 return i_size;
>>> 909 #else
>>> 910 /* Pairs with smp_store_release() in i_size_write() */
>>> > 911 return smp_load_acquire(&inode->i_size);
>>> 912 #endif
>>> 913 }
>>> 914
>> Sorry to cause this compilation error, I have not encountered this
>> before when testing compilation with gcc on x86 and arm64.
>> Is there a special config required for this, or is it just clang that
>> triggers this compile error?
>>
>> The compile error seems to be that some architectural implementations
>> of smp_load_acquire() do not support pointers to long long data
>> types. Can't think of a good way to avoid this problem, any ideas
>> from linus and christian?
> That code in i_size_{read,write}() is terrible to look at.
> Pragmatically, we can probably just do READ_ONCE() + smp_rmb() and
> smp_wmb() + WRITE_ONCE(). This guarantees the ordering and it works
> (compiles) on 32bit as well. I think it's still possible that on 32bit
> the READ_ONCE()/WRITE_ONCE() are compiled as two accesses. But I'm not
> sure that this matters because iiuc that problem would've already been
> there with the barrier in mm/filemap.c instead of here.
Thank you very much for your suggestion!
READ_ONCE()/WRITE_ONCE() now allows 64-bit accesses on 32-bit
architectures. Linus suggests smp_load_acquire()/smp_store_release()
to do the same in non-SMP case. I think this is better, what do you think?
Thanks again!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-24 7:52 ` Baokun Li
@ 2024-01-24 10:30 ` Christian Brauner
2024-01-24 11:53 ` Baokun Li
0 siblings, 1 reply; 9+ messages in thread
From: Christian Brauner @ 2024-01-24 10:30 UTC (permalink / raw)
To: Baokun Li, Linus Torvalds
Cc: kernel test robot, sfr, llvm, oe-kbuild-all, Christian Brauner,
yangerkun, zhangyi (F),
linux-next
On Wed, Jan 24, 2024 at 03:52:16PM +0800, Baokun Li wrote:
> On 2024/1/24 0:24, Christian Brauner wrote:
> > On Tue, Jan 23, 2024 at 03:07:50PM +0800, Baokun Li wrote:
> > > On 2024/1/23 8:12, kernel test robot wrote:
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc
> > > > head: 297983dc9011461cba6278bfe03f4305c4a2caa0
> > > > commit: 4bbd51d0f0ad709c0f02c100439d6c9ba6811d4b [12/13] fs: make the i_size_read/write helpers be smp_load_acquire/store_release()
> > > > config: i386-randconfig-015-20240123 (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/config)
> > > > compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
> > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/reproduce)
> > > >
> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > the same patch/commit), kindly add following tags
> > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202401230837.TXro0PHi-lkp@intel.com/
> > > >
> > > > All errors (new ones prefixed by >>):
> > > >
> > > > In file included from fs/netfs/buffered_read.c:10:
> > > > In file included from fs/netfs/internal.h:9:
> > > > In file included from include/linux/seq_file.h:12:
> > > > > > include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > > > 911 | return smp_load_acquire(&inode->i_size);
> > > > | ^
> > > > include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
> > > > 206 | compiletime_assert_atomic_type(*p); \
> > > > | ^
> > > > include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
> > > > 438 | compiletime_assert(__native_word(t), \
> > > > | ^
> > > > include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
> > > > 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > > > | ^
> > > > include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
> > > > 423 | __compiletime_assert(condition, msg, prefix, suffix)
> > > > | ^
> > > > include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
> > > > 416 | prefix ## suffix(); \
> > > > | ^
> > > > <scratch space>:38:1: note: expanded from here
> > > > 38 | __compiletime_assert_207
> > > > | ^
> > > > 1 error generated.
> > > >
> > > >
> > > > vim +911 include/linux/fs.h
> > > >
> > > > 874
> > > > 875 void filemap_invalidate_lock_two(struct address_space *mapping1,
> > > > 876 struct address_space *mapping2);
> > > > 877 void filemap_invalidate_unlock_two(struct address_space *mapping1,
> > > > 878 struct address_space *mapping2);
> > > > 879
> > > > 880
> > > > 881 /*
> > > > 882 * NOTE: in a 32bit arch with a preemptable kernel and
> > > > 883 * an UP compile the i_size_read/write must be atomic
> > > > 884 * with respect to the local cpu (unlike with preempt disabled),
> > > > 885 * but they don't need to be atomic with respect to other cpus like in
> > > > 886 * true SMP (so they need either to either locally disable irq around
> > > > 887 * the read or for example on x86 they can be still implemented as a
> > > > 888 * cmpxchg8b without the need of the lock prefix). For SMP compiles
> > > > 889 * and 64bit archs it makes no difference if preempt is enabled or not.
> > > > 890 */
> > > > 891 static inline loff_t i_size_read(const struct inode *inode)
> > > > 892 {
> > > > 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> > > > 894 loff_t i_size;
> > > > 895 unsigned int seq;
> > > > 896
> > > > 897 do {
> > > > 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
> > > > 899 i_size = inode->i_size;
> > > > 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
> > > > 901 return i_size;
> > > > 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
> > > > 903 loff_t i_size;
> > > > 904
> > > > 905 preempt_disable();
> > > > 906 i_size = inode->i_size;
> > > > 907 preempt_enable();
> > > > 908 return i_size;
> > > > 909 #else
> > > > 910 /* Pairs with smp_store_release() in i_size_write() */
> > > > > 911 return smp_load_acquire(&inode->i_size);
> > > > 912 #endif
> > > > 913 }
> > > > 914
> > > Sorry to cause this compilation error, I have not encountered this
> > > before when testing compilation with gcc on x86 and arm64.
> > > Is there a special config required for this, or is it just clang that
> > > triggers this compile error?
> > >
> > > The compile error seems to be that some architectural implementations
> > > of smp_load_acquire() do not support pointers to long long data
> > > types. Can't think of a good way to avoid this problem, any ideas
> > > from linus and christian?
> > That code in i_size_{read,write}() is terrible to look at.
> > Pragmatically, we can probably just do READ_ONCE() + smp_rmb() and
> > smp_wmb() + WRITE_ONCE(). This guarantees the ordering and it works
> > (compiles) on 32bit as well. I think it's still possible that on 32bit
> > the READ_ONCE()/WRITE_ONCE() are compiled as two accesses. But I'm not
> > sure that this matters because iiuc that problem would've already been
> > there with the barrier in mm/filemap.c instead of here.
> Thank you very much for your suggestion!
>
> READ_ONCE()/WRITE_ONCE() now allows 64-bit accesses on 32-bit
> architectures. Linus suggests smp_load_acquire()/smp_store_release()
> to do the same in non-SMP case. I think this is better, what do you think?
Practically it doesn't matter what option we choose.
If we want to remove the compile time assert from smp_load_acquire() and
smp_store_release() we should do so from all architectures that define
it. So that's arm, arm64, loongarch, parisc, powerpc, riscv, s390,
sparc, x86.
Otherwise I think this might be a bit messy. And we can do that but I'm
not sure that should be an accompanying patch to this change?
Linus, if you prefer to remove the compile time asserts from
smp_load_acquire() and smp_store_release() do you just want to apply a
patch to the tree directly? Otherwise maybe a patch separate from this
series might be better?
In the meantime or if we don't choose to remove it we should just be
able to do?
Thoughts?
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ed5966a70495..e46fe5d0dfc0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -890,8 +890,8 @@ void filemap_invalidate_unlock_two(struct address_space *mapping1,
*/
static inline loff_t i_size_read(const struct inode *inode)
{
-#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
loff_t i_size;
+#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
unsigned int seq;
do {
@@ -900,14 +900,16 @@ static inline loff_t i_size_read(const struct inode *inode)
} while (read_seqcount_retry(&inode->i_size_seqcount, seq));
return i_size;
#elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
- loff_t i_size;
preempt_disable();
i_size = inode->i_size;
preempt_enable();
return i_size;
#else
- return inode->i_size;
+
+ i_size = READ_ONCE(inode->i_size);
+ smp_rmb();
+ return i_size;
#endif
}
@@ -929,7 +931,8 @@ static inline void i_size_write(struct inode *inode, loff_t i_size)
inode->i_size = i_size;
preempt_enable();
#else
- inode->i_size = i_size;
+ smp_wmb();
+ WRITE_ONCE(inode->i_size, i_size);
#endif
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-24 10:30 ` Christian Brauner
@ 2024-01-24 11:53 ` Baokun Li
2024-01-24 13:27 ` Christian Brauner
0 siblings, 1 reply; 9+ messages in thread
From: Baokun Li @ 2024-01-24 11:53 UTC (permalink / raw)
To: Christian Brauner, Linus Torvalds
Cc: kernel test robot, sfr, llvm, oe-kbuild-all, Christian Brauner,
yangerkun, zhangyi (F),
linux-next, linux-fsdevel, Baokun Li
On 2024/1/24 18:30, Christian Brauner wrote:
> On Wed, Jan 24, 2024 at 03:52:16PM +0800, Baokun Li wrote:
>> On 2024/1/24 0:24, Christian Brauner wrote:
>>> On Tue, Jan 23, 2024 at 03:07:50PM +0800, Baokun Li wrote:
>>>> On 2024/1/23 8:12, kernel test robot wrote:
>>>>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc
>>>>> head: 297983dc9011461cba6278bfe03f4305c4a2caa0
>>>>> commit: 4bbd51d0f0ad709c0f02c100439d6c9ba6811d4b [12/13] fs: make the i_size_read/write helpers be smp_load_acquire/store_release()
>>>>> config: i386-randconfig-015-20240123 (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/config)
>>>>> compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>>>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/reproduce)
>>>>>
>>>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>>>> the same patch/commit), kindly add following tags
>>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202401230837.TXro0PHi-lkp@intel.com/
>>>>>
>>>>> All errors (new ones prefixed by >>):
>>>>>
>>>>> In file included from fs/netfs/buffered_read.c:10:
>>>>> In file included from fs/netfs/internal.h:9:
>>>>> In file included from include/linux/seq_file.h:12:
>>>>>>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
>>>>> 911 | return smp_load_acquire(&inode->i_size);
>>>>> | ^
>>>>> include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
>>>>> 206 | compiletime_assert_atomic_type(*p); \
>>>>> | ^
>>>>> include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
>>>>> 438 | compiletime_assert(__native_word(t), \
>>>>> | ^
>>>>> include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
>>>>> 435 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>>> | ^
>>>>> include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
>>>>> 423 | __compiletime_assert(condition, msg, prefix, suffix)
>>>>> | ^
>>>>> include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
>>>>> 416 | prefix ## suffix(); \
>>>>> | ^
>>>>> <scratch space>:38:1: note: expanded from here
>>>>> 38 | __compiletime_assert_207
>>>>> | ^
>>>>> 1 error generated.
>>>>>
>>>>>
>>>>> vim +911 include/linux/fs.h
>>>>>
>>>>> 874
>>>>> 875 void filemap_invalidate_lock_two(struct address_space *mapping1,
>>>>> 876 struct address_space *mapping2);
>>>>> 877 void filemap_invalidate_unlock_two(struct address_space *mapping1,
>>>>> 878 struct address_space *mapping2);
>>>>> 879
>>>>> 880
>>>>> 881 /*
>>>>> 882 * NOTE: in a 32bit arch with a preemptable kernel and
>>>>> 883 * an UP compile the i_size_read/write must be atomic
>>>>> 884 * with respect to the local cpu (unlike with preempt disabled),
>>>>> 885 * but they don't need to be atomic with respect to other cpus like in
>>>>> 886 * true SMP (so they need either to either locally disable irq around
>>>>> 887 * the read or for example on x86 they can be still implemented as a
>>>>> 888 * cmpxchg8b without the need of the lock prefix). For SMP compiles
>>>>> 889 * and 64bit archs it makes no difference if preempt is enabled or not.
>>>>> 890 */
>>>>> 891 static inline loff_t i_size_read(const struct inode *inode)
>>>>> 892 {
>>>>> 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
>>>>> 894 loff_t i_size;
>>>>> 895 unsigned int seq;
>>>>> 896
>>>>> 897 do {
>>>>> 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
>>>>> 899 i_size = inode->i_size;
>>>>> 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
>>>>> 901 return i_size;
>>>>> 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
>>>>> 903 loff_t i_size;
>>>>> 904
>>>>> 905 preempt_disable();
>>>>> 906 i_size = inode->i_size;
>>>>> 907 preempt_enable();
>>>>> 908 return i_size;
>>>>> 909 #else
>>>>> 910 /* Pairs with smp_store_release() in i_size_write() */
>>>>> > 911 return smp_load_acquire(&inode->i_size);
>>>>> 912 #endif
>>>>> 913 }
>>>>> 914
>>>> Sorry to cause this compilation error, I have not encountered this
>>>> before when testing compilation with gcc on x86 and arm64.
>>>> Is there a special config required for this, or is it just clang that
>>>> triggers this compile error?
>>>>
>>>> The compile error seems to be that some architectural implementations
>>>> of smp_load_acquire() do not support pointers to long long data
>>>> types. Can't think of a good way to avoid this problem, any ideas
>>>> from linus and christian?
>>> That code in i_size_{read,write}() is terrible to look at.
>>> Pragmatically, we can probably just do READ_ONCE() + smp_rmb() and
>>> smp_wmb() + WRITE_ONCE(). This guarantees the ordering and it works
>>> (compiles) on 32bit as well. I think it's still possible that on 32bit
>>> the READ_ONCE()/WRITE_ONCE() are compiled as two accesses. But I'm not
>>> sure that this matters because iiuc that problem would've already been
>>> there with the barrier in mm/filemap.c instead of here.
>> Thank you very much for your suggestion!
>>
>> READ_ONCE()/WRITE_ONCE() now allows 64-bit accesses on 32-bit
>> architectures. Linus suggests smp_load_acquire()/smp_store_release()
>> to do the same in non-SMP case. I think this is better, what do you think?
> Practically it doesn't matter what option we choose.
>
> If we want to remove the compile time assert from smp_load_acquire() and
> smp_store_release() we should do so from all architectures that define
> it. So that's arm, arm64, loongarch, parisc, powerpc, riscv, s390,
> sparc, x86.
Hello, Christian!
If CONFIG_SMP is not enabled in include/asm-generic/barrier.h,
then smp_load_acquire/smp_store_release is implemented as
READ_ONCE/READ_ONCE and barrier() and type checking.
READ_ONCE/READ_ONCE already checks the pointer type,
but then checks it more stringently outside, but here the
more stringent checking seems unnecessary, so it is removed
and only the type checking in READ_ONCE/READ_ONCE is kept
to avoid compilation errors.
When CONFIG_SMP is enabled on 32-bit architectures,
smp_load_acquire/smp_store_release is not called in i_size_read/write,
so there is no compilation problem. On 64-bit architectures, there
is no compilation problem because sizeof(long long) == sizeof(long),
regardless of whether CONFIG_SMP is enabled or not.
Also the arm64 implementation of __smp_load_acquire does not call
READ_ONCE but only uses compiletime_assert_atomic_type to check
the length of the data, so we can't remove compile-time assertions
from all architectures.
> Otherwise I think this might be a bit messy. And we can do that but I'm
> not sure that should be an accompanying patch to this change?
>
> Linus, if you prefer to remove the compile time asserts from
> smp_load_acquire() and smp_store_release() do you just want to apply a
> patch to the tree directly? Otherwise maybe a patch separate from this
> series might be better?
>
> In the meantime or if we don't choose to remove it we should just be
> able to do?
>
> Thoughts?
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ed5966a70495..e46fe5d0dfc0 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -890,8 +890,8 @@ void filemap_invalidate_unlock_two(struct address_space *mapping1,
> */
> static inline loff_t i_size_read(const struct inode *inode)
> {
> -#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> loff_t i_size;
> +#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> unsigned int seq;
>
> do {
> @@ -900,14 +900,16 @@ static inline loff_t i_size_read(const struct inode *inode)
> } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
> return i_size;
> #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
> - loff_t i_size;
>
> preempt_disable();
> i_size = inode->i_size;
> preempt_enable();
> return i_size;
> #else
> - return inode->i_size;
> +
> + i_size = READ_ONCE(inode->i_size);
> + smp_rmb();
> + return i_size;
> #endif
> }
>
> @@ -929,7 +931,8 @@ static inline void i_size_write(struct inode *inode, loff_t i_size)
> inode->i_size = i_size;
> preempt_enable();
> #else
> - inode->i_size = i_size;
> + smp_wmb();
> + WRITE_ONCE(inode->i_size, i_size);
> #endif
> }
Yes, using smp_rmb()/smp_wmb() would also solve the problem, but
the initial purpose of this patch was to replace smp_rmb() in filemap_read()
with the clearer smp_load_acquire/smp_store_release, and that's where
the community is going with this evolution. Later on, buffer and page/folio
will also switch to acquire/release, which is why I think Linus' suggestion
is better.
No offense. 😅
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-24 11:53 ` Baokun Li
@ 2024-01-24 13:27 ` Christian Brauner
2024-01-24 13:38 ` Baokun Li
0 siblings, 1 reply; 9+ messages in thread
From: Christian Brauner @ 2024-01-24 13:27 UTC (permalink / raw)
To: Baokun Li
Cc: Linus Torvalds, kernel test robot, sfr, llvm, oe-kbuild-all,
Christian Brauner, yangerkun, zhangyi (F),
linux-next, linux-fsdevel
> If CONFIG_SMP is not enabled in include/asm-generic/barrier.h,
> then smp_load_acquire/smp_store_release is implemented as
> READ_ONCE/READ_ONCE and barrier() and type checking.
> READ_ONCE/READ_ONCE already checks the pointer type,
> but then checks it more stringently outside, but here the
> more stringent checking seems unnecessary, so it is removed
> and only the type checking in READ_ONCE/READ_ONCE is kept
> to avoid compilation errors.
Maha, brainfart on my end, I missed the !CONFIG_SMP case.
Sorry about that.
>
> When CONFIG_SMP is enabled on 32-bit architectures,
> smp_load_acquire/smp_store_release is not called in i_size_read/write,
> so there is no compilation problem. On 64-bit architectures, there
> is no compilation problem because sizeof(long long) == sizeof(long),
> regardless of whether CONFIG_SMP is enabled or not.
Yes, of course.
> Yes, using smp_rmb()/smp_wmb() would also solve the problem, but
> the initial purpose of this patch was to replace smp_rmb() in filemap_read()
> with the clearer smp_load_acquire/smp_store_release, and that's where
> the community is going with this evolution. Later on, buffer and page/folio
> will also switch to acquire/release, which is why I think Linus' suggestion
> is better.
Ah ok, thanks for the context. Can you send an updated series then, please?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
2024-01-24 13:27 ` Christian Brauner
@ 2024-01-24 13:38 ` Baokun Li
0 siblings, 0 replies; 9+ messages in thread
From: Baokun Li @ 2024-01-24 13:38 UTC (permalink / raw)
To: Christian Brauner
Cc: Linus Torvalds, kernel test robot, sfr, llvm, oe-kbuild-all,
Christian Brauner, yangerkun, zhangyi (F),
linux-next, linux-fsdevel, Baokun Li
On 2024/1/24 21:27, Christian Brauner wrote:
>> If CONFIG_SMP is not enabled in include/asm-generic/barrier.h,
>> then smp_load_acquire/smp_store_release is implemented as
>> READ_ONCE/READ_ONCE and barrier() and type checking.
>> READ_ONCE/READ_ONCE already checks the pointer type,
>> but then checks it more stringently outside, but here the
>> more stringent checking seems unnecessary, so it is removed
>> and only the type checking in READ_ONCE/READ_ONCE is kept
>> to avoid compilation errors.
> Maha, brainfart on my end, I missed the !CONFIG_SMP case.
> Sorry about that.
Never mind. 😊
>> When CONFIG_SMP is enabled on 32-bit architectures,
>> smp_load_acquire/smp_store_release is not called in i_size_read/write,
>> so there is no compilation problem. On 64-bit architectures, there
>> is no compilation problem because sizeof(long long) == sizeof(long),
>> regardless of whether CONFIG_SMP is enabled or not.
> Yes, of course.
>
>> Yes, using smp_rmb()/smp_wmb() would also solve the problem, but
>> the initial purpose of this patch was to replace smp_rmb() in filemap_read()
>> with the clearer smp_load_acquire/smp_store_release, and that's where
>> the community is going with this evolution. Later on, buffer and page/folio
>> will also switch to acquire/release, which is why I think Linus' suggestion
>> is better.
> Ah ok, thanks for the context. Can you send an updated series then, please?
>
No problem, I'll send a new version soon!
Cheers!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-01-24 13:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <202401230837.TXro0PHi-lkp@intel.com>
2024-01-23 7:07 ` [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity Baokun Li
2024-01-23 16:24 ` Christian Brauner
2024-01-24 7:52 ` Baokun Li
2024-01-24 10:30 ` Christian Brauner
2024-01-24 11:53 ` Baokun Li
2024-01-24 13:27 ` Christian Brauner
2024-01-24 13:38 ` Baokun Li
2024-01-23 17:46 ` Linus Torvalds
2024-01-24 7:31 ` Baokun Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).