On Tue, Jun 08, 2021 at 07:27:52PM +0200, Vlastimil Babka wrote: > On 6/8/21 7:05 PM, Hyeonggon Yoo wrote: > > On Tue, Jun 08, 2021 at 09:57:18AM +0200, Vlastimil Babka wrote: > >> On 6/7/21 5:49 PM, Hyeonggon Yoo wrote: > >> > On Mon, Jun 07, 2021 at 05:27:27PM +0200, Vlastimil Babka wrote: > >> >> On 6/7/21 2:25 PM, Hyeonggon Yoo wrote: > >> >> > On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote: > >> >> >> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote: > >> >> >> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote: > >> >> >> > >> >> >> But what exactly is the gcc problem here? > >> >> >> Did you have to reproduce it with specific gcc version and/or architecture? > >> >> >> > >> >> > > >> >> > Before replying, I should say that I'm not an expert on gcc. > >> >> > I just tried some ways to fix the error, and it seemed to me that > >> >> > gcc is doing something wrong. > >> >> > >> >> I'm involving my gcc colleagues, will report results... > >> > >> Well, it seems the bot's .config included CONFIG_PROFILE_ALL_BRANCHES as the > >> main factor to trigger the problem. And (according to my colleagues) > > > > Wow, AWESOME! How did your colleague find it? that was a big hint for me. > > when CONFIG_PROFILE_ALL_BRANCHES is not set, it doesn't make an error. > > Well, we started with me doing "make kernel/bpf/local_storage.i" to create a > preprocessed C source that can be copied out and debugged outside of the whole > kernel build context. I send that to my colleague and he reduced the testcase > using cvise: > > https://gcc.gnu.org/wiki/A_guide_to_testcase_reduction > > While the reduction wasn't successful in preserving enough of the testcase, from > the result it was clear that there was lots of ftrace_branch_data stuff and so > it was easy to find this is due to > CONFIG_PROFILE_ALL_BRANCHES. > First time to hear about testcase reduction and cvise! Thank you for letting me know it :) I'm going to read it. but I ask some questions before that (see below) > > mm.. when the size passed to bpf_map_kmalloc_node is not constant, > > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is false. > > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index. > > > > what change should be made? > > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition > > doesn't make sense, because kmalloc_node is not working as expected. > > If I understood my colleagues right, the problem is that, while kmalloc_index() > seems to contains a number of *independent* "if (size <= X) conditions in a > sequence, the machinery of CONFIG_PROFILE_ALL_BRANCHES turns it to a deeply > nested if-then-else { if-then-else { if-then-else {...}}} thing (in fact using > the ternary operators, not if-then-else). > At some point of the deep nesting gcc > "forgets" that __builtin_constant_p() is true and starts behaving as if it wasn't. > > Without CONFIG_PROFILE_ALL_BRANCHES it's able to keep track of it fine. > I think you are talking about some if statements in kmalloc_index. How do you know gcc "forgets" __builtin_constant_p() is true? can it be debugged using cvise? (not offending, just because I don't know about cvise yet). Also, as I understand right, kmalloc_index doesn't have information about the value of __builtin_constant_p(size). the caller of kmalloc_index (kmalloc_node here) has that information. If I understand right, what CONFIG_PROFILE_ALL_BRANCHES does is below. replacing if(cond) { body } -> if ( __builtin_constant_p(cond) ? then (cond) else (cond, record something) ) { body } I think it does not make deep nested statements. the below is some part of preprocessor output. CONFIG_PROFILE_ALL_BRANCHES makes some ugly ternary operators, but there is no deep-nested if-then-else statements. if ( (__builtin_constant_p(!!(size <= 8)) ? (!!(size <= 8)) : ({ static struct ftrace_branch_data __attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 392, }; (!!(size <= 8)) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 3; if ( (__builtin_constant_p(!!(size <= 16)) ? (!!(size <= 16)) : ({ static struct ftrace_branch_data __attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 393, }; (!!(size <= 16)) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 4; if ( (__builtin_constant_p(!!(size <= 32)) ? (!!(size <= 32)) : ({ static struct ftrace_branch_data __attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 394, }; (!!(size <= 32)) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 5; .... and some if statements ... And I think, the problem is on kmalloc_node, not on kmalloc_index. the original code of kmalloc_node is below: static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node) { #ifndef CONFIG_SLOB if (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) { unsigned int i = kmalloc_index(size); and which is changed to below: (by CONFIG_PROFILE_ALL_BRANCHES) static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node) { if ( ( __builtin_constant_p( !!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25))) ) ? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) : ({ static struct ftrace_branch_data __attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 601, }; (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) { unsigned int i = __kmalloc_index(size, true); they are so ugly but the point is: > > It seems that gcc evaluates > > > > __builtin_constant_p( > > !!(builtin_constant_p(size) > > && size <= KMALLOC_MAX_CACHE_SIZE) > > ) > > as true. > > > > mm.. when the size passed to bpf_map_kmalloc_node is not constant, > > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is false. > > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index. > > > > what change should be made? > > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition > > doesn't make sense, because kmalloc_node is not working as expected. some evidence to add: there are 4 calls of bpf_map_kmalloc_node. if you comment out two calls of bpf_map_kmalloc_node with non-constant (in line 168, 508), it doesn't make an error. So I thought it was problem when non-constant size was passed. And if "kmalloc_index makes problem with non-constant size" is true, then it is caller's fault because it is not allowed (except in __kmalloc_index) kmalloc_node shouldn't call kmalloc_index if the size was not constant. > > void *kmalloc_node(size_t size, gfp_t flags, int node){ > > > > if ( (__builtin_constant_p( > > !!(__builtin_constant_p(size) > > && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) > > ? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) > > if ( (__builtin_constant_p(!!((0 || 110000 >= 110000) && size_is_constant)) ? (!!((0 || 110000 >= 110000) && size_is_constant)) : ({ static struct ftrace_branch_data __attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000 >= 110000) && size_is_constant)) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) > > do { > > > > > > extern void __compiletime_assert_131(void) ; > > // here - compiletime_assert_131 does NOTHING > > It doesn't seem to do nothing? see below > > > if ( (__builtin_constant_p(!!(!(!(1)))) > > ? (!!(!(!(1)))) > > : ({ static struct ftrace_branch_data __attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 417, }; (!!(!(!(1)))) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) __compiletime_assert_131(); } while (0); > > The thing above seems to be exactly the "if (!(condition)) > prefix ## suffix(); } while (0)" as the definition you posted below. > > Anyway, you can verify that clang works by commenting out the highest size > checks and passing a constant size that makes it reach the BUILD_BUG_ON_MSG() ? > I verified as below. clang didn't make an error, gcc worked as expected. then I can explain why there is no error with clang: it just did nothing with BUILD_BUG_ON. hyeyoo@hyeyoo:~/바탕화면/linux-next$ git diff diff --git a/include/linux/slab.h b/include/linux/slab.h index 8d8dd8571261..f2f9a6a7e663 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -379,6 +379,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) static __always_inline unsigned int __kmalloc_index(size_t size, bool size_is_constant) { + + BUILD_BUG_ON(1); + if (!size) return 0; hyeyoo@hyeyoo:~/바탕화면/linux-next$ make mm/kfence/kfence_test.o CC=clang-11 CALL scripts/checksyscalls.sh ... some headers omitted ... CC mm/kfence/kfence_test.o In file included from <command-line>: In function ‘__kmalloc_index’, inlined from ‘kmalloc_cache_alignment’ at mm/kfence/kfence_test.c:200:50: ././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’ declared with attribute error: BUILD_BUG_ON failed: 1 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ ././include/linux/compiler_types.h:309:4: note: in definition of macro ‘__compiletime_assert’ 309 | prefix ## suffix(); \ | ^~~~~~ ././include/linux/compiler_types.h:328:2: note: in expansion of macro ‘_compiletime_assert’ 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ 50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) | ^~~~~~~~~~~~~~~~ ./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’ 383 | BUILD_BUG_ON(1); | ^~~~~~~~~~~~ In function ‘__kmalloc_index’, inlined from ‘test_alloc’ at mm/kfence/kfence_test.c:271:47: ././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’ declared with attribute error: BUILD_BUG_ON failed: 1 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ ././include/linux/compiler_types.h:309:4: note: in definition of macro ‘__compiletime_assert’ 309 | prefix ## suffix(); \ | ^~~~~~ ././include/linux/compiler_types.h:328:2: note: in expansion of macro ‘_compiletime_assert’ 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ 50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) | ^~~~~~~~~~~~~~~~ ./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’ 383 | BUILD_BUG_ON(1); | ^~~~~~~~~~~~ make[2]: *** [scripts/Makefile.build:272: mm/kfence/kfence_test.o] 오류 1 make[1]: *** [scripts/Makefile.build:533: mm/kfence] 오류 2 make: *** [Makefile:1948: mm] 오류 2
