The commit d66acc39c7ce ("bitops: Optimise get_order()") introduced a
problem for the be2net driver as "rx_frag_size" could be a module
parameter that can be changed while loading the module. That commit
checks __builtin_constant_p() first in get_order() which cause
"adapter->big_page_size" to be assigned a value based on the
the default "rx_frag_size" value at the compilation time. It also
generate a compilation warning,
In file included from ./arch/powerpc/include/asm/page_64.h:107,
from ./arch/powerpc/include/asm/page.h:242,
from ./arch/powerpc/include/asm/mmu.h:132,
from ./arch/powerpc/include/asm/lppaca.h:47,
from ./arch/powerpc/include/asm/paca.h:17,
from ./arch/powerpc/include/asm/current.h:13,
from ./include/linux/thread_info.h:21,
from ./arch/powerpc/include/asm/processor.h:39,
from ./include/linux/prefetch.h:15,
from drivers/net/ethernet/emulex/benet/be_main.c:14:
drivers/net/ethernet/emulex/benet/be_main.c: In function
'be_rx_cqs_create':
./include/asm-generic/getorder.h:54:9: warning: comparison is always
true due to limited range of data type [-Wtype-limits]
(((n) < (1UL << PAGE_SHIFT)) ? 0 : \
^
drivers/net/ethernet/emulex/benet/be_main.c:3138:33: note: in expansion
of macro 'get_order'
adapter->big_page_size = (1 << get_order(rx_frag_size)) * PAGE_SIZE;
^~~~~~~~~
Fix it by using __get_order() instead which will calculate in runtime.
Fixes: d66acc39c7ce ("bitops: Optimise get_order()")
Signed-off-by: Qian Cai <[email protected]>
---
drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 82015c8a5ed7..db13e714df7c 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -3135,7 +3135,7 @@ static int be_rx_cqs_create(struct be_adapter *adapter)
if (adapter->num_rx_qs == 0)
adapter->num_rx_qs = 1;
- adapter->big_page_size = (1 << get_order(rx_frag_size)) * PAGE_SIZE;
+ adapter->big_page_size = (1 << __get_order(rx_frag_size)) * PAGE_SIZE;
for_all_rx_queues(adapter, rxo, i) {
rxo->adapter = adapter;
cq = &rxo->cq;
--
1.8.3.1
From: Qian Cai <[email protected]>
Date: Fri, 12 Jul 2019 15:23:21 -0400
> The commit d66acc39c7ce ("bitops: Optimise get_order()") introduced a
> problem for the be2net driver as "rx_frag_size" could be a module
> parameter that can be changed while loading the module.
Why is this a problem?
> That commit checks __builtin_constant_p() first in get_order() which
> cause "adapter->big_page_size" to be assigned a value based on the
> the default "rx_frag_size" value at the compilation time. It also
> generate a compilation warning,
rx_frag_size is not a constant, therefore the __builtin_constant_p()
test should not pass.
This explanation doesn't seem valid.
> On Jul 12, 2019, at 6:46 PM, David Miller <[email protected]> wrote:
>
> From: Qian Cai <[email protected]>
> Date: Fri, 12 Jul 2019 15:23:21 -0400
>
>> The commit d66acc39c7ce ("bitops: Optimise get_order()") introduced a
>> problem for the be2net driver as "rx_frag_size" could be a module
>> parameter that can be changed while loading the module.
>
> Why is this a problem?
Well, for example, if rx_frag_size was set to 8096 when loading the module, the kernel has already used the default value 2048 during compilation time.
>
>> That commit checks __builtin_constant_p() first in get_order() which
>> cause "adapter->big_page_size" to be assigned a value based on the
>> the default "rx_frag_size" value at the compilation time. It also
>> generate a compilation warning,
>
> rx_frag_size is not a constant, therefore the __builtin_constant_p()
> test should not pass.
>
> This explanation doesn't seem valid.
Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
# cat const.c
#include <stdio.h>
static int a = 1;
int main(void)
{
if (__builtin_constant_p(a))
printf("a is a const.\n");
return 0;
}
# gcc -O2 const.c -o const
# ./const
a is a const.
From: Qian Cai <[email protected]>
Date: Fri, 12 Jul 2019 20:27:09 -0400
> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
>
> # cat const.c
> #include <stdio.h>
>
> static int a = 1;
>
> int main(void)
> {
> if (__builtin_constant_p(a))
> printf("a is a const.\n");
>
> return 0;
> }
>
> # gcc -O2 const.c -o const
That's not a complete test case, and with a proper test case that
shows the externalization of the address of &a done by the module
parameter macros, gcc should not make this optimization or we should
define the module parameter macros in a way that makes this properly
clear to the compiler.
It makes no sense to hack around this locally in drivers and other
modules.
Thank you.
> On Jul 12, 2019, at 8:50 PM, David Miller <[email protected]> wrote:
>
> From: Qian Cai <[email protected]>
> Date: Fri, 12 Jul 2019 20:27:09 -0400
>
>> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
>>
>> # cat const.c
>> #include <stdio.h>
>>
>> static int a = 1;
>>
>> int main(void)
>> {
>> if (__builtin_constant_p(a))
>> printf("a is a const.\n");
>>
>> return 0;
>> }
>>
>> # gcc -O2 const.c -o const
>
> That's not a complete test case, and with a proper test case that
> shows the externalization of the address of &a done by the module
> parameter macros, gcc should not make this optimization or we should
> define the module parameter macros in a way that makes this properly
> clear to the compiler.
>
> It makes no sense to hack around this locally in drivers and other
> modules.
If you see the warning in the original patch,
https://lore.kernel.org/netdev/[email protected]/
GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
-O2 does not. The problem is that I have no clue about how to let GCC not to
optimize a module parameter.
Though, I have added a few people who might know more of compilers than myself.
On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <[email protected]> wrote:
>
>
>
> > On Jul 12, 2019, at 8:50 PM, David Miller <[email protected]> wrote:
> >
> > From: Qian Cai <[email protected]>
> > Date: Fri, 12 Jul 2019 20:27:09 -0400
> >
> >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
> >>
> >> # cat const.c
> >> #include <stdio.h>
> >>
> >> static int a = 1;
> >>
> >> int main(void)
> >> {
> >> if (__builtin_constant_p(a))
> >> printf("a is a const.\n");
> >>
> >> return 0;
> >> }
> >>
> >> # gcc -O2 const.c -o const
> >
> > That's not a complete test case, and with a proper test case that
> > shows the externalization of the address of &a done by the module
> > parameter macros, gcc should not make this optimization or we should
> > define the module parameter macros in a way that makes this properly
> > clear to the compiler.
> >
> > It makes no sense to hack around this locally in drivers and other
> > modules.
>
> If you see the warning in the original patch,
>
> https://lore.kernel.org/netdev/[email protected]/
>
> GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
> -O2 does not. The problem is that I have no clue about how to let GCC not to
> optimize a module parameter.
>
> Though, I have added a few people who might know more of compilers than myself.
+ Bill and James, who probably knows more than they'd like to about
__builtin_constant_p and more than other LLVM folks at this point.
--
Thanks,
~Nick Desaulniers
[My previous response was marked as spam...]
Top-of-tree clang says that it's const:
$ gcc a.c -O2 && ./a.out
a is a const.
$ clang a.c -O2 && ./a.out
a is a const.
On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <[email protected]> wrote:
> >
> >
> >
> > > On Jul 12, 2019, at 8:50 PM, David Miller <[email protected]> wrote:
> > >
> > > From: Qian Cai <[email protected]>
> > > Date: Fri, 12 Jul 2019 20:27:09 -0400
> > >
> > >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
> > >>
> > >> # cat const.c
> > >> #include <stdio.h>
> > >>
> > >> static int a = 1;
> > >>
> > >> int main(void)
> > >> {
> > >> if (__builtin_constant_p(a))
> > >> printf("a is a const.\n");
> > >>
> > >> return 0;
> > >> }
> > >>
> > >> # gcc -O2 const.c -o const
> > >
> > > That's not a complete test case, and with a proper test case that
> > > shows the externalization of the address of &a done by the module
> > > parameter macros, gcc should not make this optimization or we should
> > > define the module parameter macros in a way that makes this properly
> > > clear to the compiler.
> > >
> > > It makes no sense to hack around this locally in drivers and other
> > > modules.
> >
> > If you see the warning in the original patch,
> >
> > https://lore.kernel.org/netdev/[email protected]/
> >
> > GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
> > -O2 does not. The problem is that I have no clue about how to let GCC not to
> > optimize a module parameter.
> >
> > Though, I have added a few people who might know more of compilers than myself.
>
> + Bill and James, who probably knows more than they'd like to about
> __builtin_constant_p and more than other LLVM folks at this point.
>
> --
> Thanks,
> ~Nick Desaulniers
On Thu, Jul 18, 2019 at 2:18 PM Bill Wendling <[email protected]> wrote:
>
> Top-of-tree clang says that it's const:
>
> $ gcc a.c -O2 && ./a.out
> a is a const.
>
> $ clang a.c -O2 && ./a.out
> a is a const.
Right, so I know you (Bill) did a lot of work to refactor
__builtin_constant_p handling in Clang and LLVM in the
pre-llvm-9-release timeframe. I suspect Qian might not be using
clang-9 built from source (as clang-8 is the current release) and thus
observing differences.
>
> On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers <[email protected]> wrote:
>>
>> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <[email protected]> wrote:
>> >
>> >
>> >
>> > > On Jul 12, 2019, at 8:50 PM, David Miller <[email protected]> wrote:
>> > >
>> > > From: Qian Cai <[email protected]>
>> > > Date: Fri, 12 Jul 2019 20:27:09 -0400
>> > >
>> > >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
>> > >>
>> > >> # cat const.c
>> > >> #include <stdio.h>
>> > >>
>> > >> static int a = 1;
>> > >>
>> > >> int main(void)
>> > >> {
>> > >> if (__builtin_constant_p(a))
>> > >> printf("a is a const.\n");
>> > >>
>> > >> return 0;
>> > >> }
>> > >>
>> > >> # gcc -O2 const.c -o const
>> > >
>> > > That's not a complete test case, and with a proper test case that
>> > > shows the externalization of the address of &a done by the module
>> > > parameter macros, gcc should not make this optimization or we should
>> > > define the module parameter macros in a way that makes this properly
>> > > clear to the compiler.
>> > >
>> > > It makes no sense to hack around this locally in drivers and other
>> > > modules.
>> >
>> > If you see the warning in the original patch,
>> >
>> > https://lore.kernel.org/netdev/[email protected]/
>> >
>> > GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
>> > -O2 does not. The problem is that I have no clue about how to let GCC not to
>> > optimize a module parameter.
>> >
>> > Though, I have added a few people who might know more of compilers than myself.
>>
>> + Bill and James, who probably knows more than they'd like to about
>> __builtin_constant_p and more than other LLVM folks at this point.
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
Possibly. I'd need to ask him. :-)
On Thu, Jul 18, 2019 at 2:22 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Thu, Jul 18, 2019 at 2:18 PM Bill Wendling <[email protected]> wrote:
> >
> > Top-of-tree clang says that it's const:
> >
> > $ gcc a.c -O2 && ./a.out
> > a is a const.
> >
> > $ clang a.c -O2 && ./a.out
> > a is a const.
>
> Right, so I know you (Bill) did a lot of work to refactor
> __builtin_constant_p handling in Clang and LLVM in the
> pre-llvm-9-release timeframe. I suspect Qian might not be using
> clang-9 built from source (as clang-8 is the current release) and thus
> observing differences.
>
> >
> > On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers <[email protected]> wrote:
> >>
> >> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <[email protected]> wrote:
> >> >
> >> >
> >> >
> >> > > On Jul 12, 2019, at 8:50 PM, David Miller <[email protected]> wrote:
> >> > >
> >> > > From: Qian Cai <[email protected]>
> >> > > Date: Fri, 12 Jul 2019 20:27:09 -0400
> >> > >
> >> > >> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
> >> > >>
> >> > >> # cat const.c
> >> > >> #include <stdio.h>
> >> > >>
> >> > >> static int a = 1;
> >> > >>
> >> > >> int main(void)
> >> > >> {
> >> > >> if (__builtin_constant_p(a))
> >> > >> printf("a is a const.\n");
> >> > >>
> >> > >> return 0;
> >> > >> }
> >> > >>
> >> > >> # gcc -O2 const.c -o const
> >> > >
> >> > > That's not a complete test case, and with a proper test case that
> >> > > shows the externalization of the address of &a done by the module
> >> > > parameter macros, gcc should not make this optimization or we should
> >> > > define the module parameter macros in a way that makes this properly
> >> > > clear to the compiler.
> >> > >
> >> > > It makes no sense to hack around this locally in drivers and other
> >> > > modules.
> >> >
> >> > If you see the warning in the original patch,
> >> >
> >> > https://lore.kernel.org/netdev/[email protected]/
> >> >
> >> > GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
> >> > -O2 does not. The problem is that I have no clue about how to let GCC not to
> >> > optimize a module parameter.
> >> >
> >> > Though, I have added a few people who might know more of compilers than myself.
> >>
> >> + Bill and James, who probably knows more than they'd like to about
> >> __builtin_constant_p and more than other LLVM folks at this point.
> >>
> >> --
> >> Thanks,
> >> ~Nick Desaulniers
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
> On Jul 18, 2019, at 5:21 PM, Bill Wendling <[email protected]> wrote:
>
> [My previous response was marked as spam...]
>
> Top-of-tree clang says that it's const:
>
> $ gcc a.c -O2 && ./a.out
> a is a const.
>
> $ clang a.c -O2 && ./a.out
> a is a const.
I used clang-7.0.1. So, this is getting worse where both GCC and clang will start to suffer the
same problem.
>
>
> On Thu, Jul 18, 2019 at 2:10 PM Nick Desaulniers
> <[email protected]> wrote:
>>
>> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <[email protected]> wrote:
>>>
>>>
>>>
>>>> On Jul 12, 2019, at 8:50 PM, David Miller <[email protected]> wrote:
>>>>
>>>> From: Qian Cai <[email protected]>
>>>> Date: Fri, 12 Jul 2019 20:27:09 -0400
>>>>
>>>>> Actually, GCC would consider it a const with -O2 optimized level because it found that it was never modified and it does not understand it is a module parameter. Considering the following code.
>>>>>
>>>>> # cat const.c
>>>>> #include <stdio.h>
>>>>>
>>>>> static int a = 1;
>>>>>
>>>>> int main(void)
>>>>> {
>>>>> if (__builtin_constant_p(a))
>>>>> printf("a is a const.\n");
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> # gcc -O2 const.c -o const
>>>>
>>>> That's not a complete test case, and with a proper test case that
>>>> shows the externalization of the address of &a done by the module
>>>> parameter macros, gcc should not make this optimization or we should
>>>> define the module parameter macros in a way that makes this properly
>>>> clear to the compiler.
>>>>
>>>> It makes no sense to hack around this locally in drivers and other
>>>> modules.
>>>
>>> If you see the warning in the original patch,
>>>
>>> https://lore.kernel.org/netdev/[email protected]/
>>>
>>> GCC definitely optimize rx_frag_size to be a constant while I just confirmed clang
>>> -O2 does not. The problem is that I have no clue about how to let GCC not to
>>> optimize a module parameter.
>>>
>>> Though, I have added a few people who might know more of compilers than myself.
>>
>> + Bill and James, who probably knows more than they'd like to about
>> __builtin_constant_p and more than other LLVM folks at this point.
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
From: Qian Cai <[email protected]>
Date: Thu, 18 Jul 2019 19:26:47 -0400
>
>
>> On Jul 18, 2019, at 5:21 PM, Bill Wendling <[email protected]> wrote:
>>
>> [My previous response was marked as spam...]
>>
>> Top-of-tree clang says that it's const:
>>
>> $ gcc a.c -O2 && ./a.out
>> a is a const.
>>
>> $ clang a.c -O2 && ./a.out
>> a is a const.
>
>
> I used clang-7.0.1. So, this is getting worse where both GCC and clang will start to suffer the
> same problem.
Then rewrite the module parameter macros such that the non-constness
is evident to all compilers regardless of version.
That is the place to fix this, otherwise we will just be adding hacks
all over the place rather than in just one spot.
Thanks.
Hi Qian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.2 next-20190719]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Qian-Cai/be2net-fix-adapter-big_page_size-miscaculation/20190713-191644
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/net/ethernet/emulex/benet/be_main.c: In function 'be_rx_cqs_create':
>> drivers/net/ethernet/emulex/benet/be_main.c:3138:33: error: implicit declaration of function '__get_order'; did you mean 'get_order'? [-Werror=implicit-function-declaration]
adapter->big_page_size = (1 << __get_order(rx_frag_size)) * PAGE_SIZE;
^~~~~~~~~~~
get_order
cc1: some warnings being treated as errors
vim +3138 drivers/net/ethernet/emulex/benet/be_main.c
3116
3117 static int be_rx_cqs_create(struct be_adapter *adapter)
3118 {
3119 struct be_queue_info *eq, *cq;
3120 struct be_rx_obj *rxo;
3121 int rc, i;
3122
3123 adapter->num_rss_qs =
3124 min(adapter->num_evt_qs, adapter->cfg_num_rx_irqs);
3125
3126 /* We'll use RSS only if atleast 2 RSS rings are supported. */
3127 if (adapter->num_rss_qs < 2)
3128 adapter->num_rss_qs = 0;
3129
3130 adapter->num_rx_qs = adapter->num_rss_qs + adapter->need_def_rxq;
3131
3132 /* When the interface is not capable of RSS rings (and there is no
3133 * need to create a default RXQ) we'll still need one RXQ
3134 */
3135 if (adapter->num_rx_qs == 0)
3136 adapter->num_rx_qs = 1;
3137
> 3138 adapter->big_page_size = (1 << __get_order(rx_frag_size)) * PAGE_SIZE;
3139 for_all_rx_queues(adapter, rxo, i) {
3140 rxo->adapter = adapter;
3141 cq = &rxo->cq;
3142 rc = be_queue_alloc(adapter, cq, RX_CQ_LEN,
3143 sizeof(struct be_eth_rx_compl));
3144 if (rc)
3145 return rc;
3146
3147 u64_stats_init(&rxo->stats.sync);
3148 eq = &adapter->eq_obj[i % adapter->num_evt_qs].q;
3149 rc = be_cmd_cq_create(adapter, cq, eq, false, 3);
3150 if (rc)
3151 return rc;
3152 }
3153
3154 dev_info(&adapter->pdev->dev,
3155 "created %d RX queue(s)\n", adapter->num_rx_qs);
3156 return 0;
3157 }
3158
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 2019-07-18 at 16:29 -0700, David Miller wrote:
> From: Qian Cai <[email protected]>
> Date: Thu, 18 Jul 2019 19:26:47 -0400
>
> >
> >
> >> On Jul 18, 2019, at 5:21 PM, Bill Wendling <[email protected]> wrote:
> >>
> >> [My previous response was marked as spam...]
> >>
> >> Top-of-tree clang says that it's const:
> >>
> >> $ gcc a.c -O2 && ./a.out
> >> a is a const.
> >>
> >> $ clang a.c -O2 && ./a.out
> >> a is a const.
> >
> >
> > I used clang-7.0.1. So, this is getting worse where both GCC and clang will
> start to suffer the
> > same problem.
>
> Then rewrite the module parameter macros such that the non-constness
> is evident to all compilers regardless of version.
>
> That is the place to fix this, otherwise we will just be adding hacks
> all over the place rather than in just one spot.
The problem is that when the compiler is compiling be_main.o, it has no
knowledge about what is going to happen in load_module(). The compiler can only
see that a "const struct kernel_param_ops" "__param_ops_rx_frag_size" at the
time with
__param_ops_rx_frag_size.arg = &rx_frag_size
but only in load_module()->parse_args()->parse_one()->param_set_ushort(), it
changes "__param_ops_rx_frag_size.arg" which in-turn changes the value
of "rx_frag_size".
On Fri, 2019-07-19 at 17:47 -0400, Qian Cai wrote:
> On Thu, 2019-07-18 at 16:29 -0700, David Miller wrote:
> > From: Qian Cai <[email protected]>
> > Date: Thu, 18 Jul 2019 19:26:47 -0400
> >
> > >
> > >
> > > > On Jul 18, 2019, at 5:21 PM, Bill Wendling <[email protected]> wrote:
> > > >
> > > > [My previous response was marked as spam...]
> > > >
> > > > Top-of-tree clang says that it's const:
> > > >
> > > > $ gcc a.c -O2 && ./a.out
> > > > a is a const.
> > > >
> > > > $ clang a.c -O2 && ./a.out
> > > > a is a const.
> > >
> > >
> > >
> > > I used clang-7.0.1. So, this is getting worse where both GCC and clang
> > > will
> >
> > start to suffer the
> > > same problem.
> >
> > Then rewrite the module parameter macros such that the non-constness
> > is evident to all compilers regardless of version.
> >
> > That is the place to fix this, otherwise we will just be adding hacks
> > all over the place rather than in just one spot.
>
> The problem is that when the compiler is compiling be_main.o, it has no
> knowledge about what is going to happen in load_module(). The compiler can
> only
> see that a "const struct kernel_param_ops" "__param_ops_rx_frag_size" at the
> time with
>
> __param_ops_rx_frag_size.arg = &rx_frag_size
>
> but only in load_module()->parse_args()->parse_one()->param_set_ushort(), it
> changes "__param_ops_rx_frag_size.arg" which in-turn changes the value
> of "rx_frag_size".
Even for an obvious case, the compilers still go ahead optimizing a variable as
a constant. Maybe it is best to revert the commit d66acc39c7ce ("bitops:
Optimise get_order()") unless some compiler experts could improve the situation.
#include <stdio.h>
int a = 1;
int main(void)
{
int *p;
p = &a;
*p = 2;
if (__builtin_constant_p(a))
printf("a is a const.\n");
printf("a = %d\n", a);
return 0;
}
# gcc -O2 const.c -o const
# ./const
a is a const.
a = 2
On Mon, Jul 22, 2019 at 5:13 PM Qian Cai <[email protected]> wrote:
>
> On Fri, 2019-07-19 at 17:47 -0400, Qian Cai wrote:
> > On Thu, 2019-07-18 at 16:29 -0700, David Miller wrote:
> > > From: Qian Cai <[email protected]>
> > > Date: Thu, 18 Jul 2019 19:26:47 -0400
> > >
> > > >
> > > >
> > > > > On Jul 18, 2019, at 5:21 PM, Bill Wendling <[email protected]> wrote:
> > > > >
> > > > > [My previous response was marked as spam...]
> > > > >
> > > > > Top-of-tree clang says that it's const:
> > > > >
> > > > > $ gcc a.c -O2 && ./a.out
> > > > > a is a const.
> > > > >
> > > > > $ clang a.c -O2 && ./a.out
> > > > > a is a const.
> > > >
> > > >
> > > >
> > > > I used clang-7.0.1. So, this is getting worse where both GCC and clang
> > > > will
> > >
> > > start to suffer the
> > > > same problem.
> > >
> > > Then rewrite the module parameter macros such that the non-constness
> > > is evident to all compilers regardless of version.
> > >
> > > That is the place to fix this, otherwise we will just be adding hacks
> > > all over the place rather than in just one spot.
> >
> > The problem is that when the compiler is compiling be_main.o, it has no
> > knowledge about what is going to happen in load_module(). The compiler can
> > only
> > see that a "const struct kernel_param_ops" "__param_ops_rx_frag_size" at the
> > time with
> >
> > __param_ops_rx_frag_size.arg = &rx_frag_size
> >
> > but only in load_module()->parse_args()->parse_one()->param_set_ushort(), it
> > changes "__param_ops_rx_frag_size.arg" which in-turn changes the value
> > of "rx_frag_size".
>
> Even for an obvious case, the compilers still go ahead optimizing a variable as
> a constant. Maybe it is best to revert the commit d66acc39c7ce ("bitops:
> Optimise get_order()") unless some compiler experts could improve the situation.
>
> #include <stdio.h>
>
> int a = 1;
>
> int main(void)
> {
> int *p;
>
> p = &a;
> *p = 2;
>
> if (__builtin_constant_p(a))
> printf("a is a const.\n");
>
> printf("a = %d\n", a);
>
> return 0;
> }
>
> # gcc -O2 const.c -o const
>
> # ./const
> a is a const.
> a = 2
This example (like the former) is showing correct behavior. At the
point of invocation of __builtin_constant_p here, the compiler knows
that 'a' is 2, because you've just assigned it (through 'p', but that
indirection trivially disappears in optimization).
The original issue,
https://lore.kernel.org/netdev/[email protected]/
The debugging so far seems point to that the compilers get confused by the
module sections. During module_param(), it stores “__param_rx_frag_size"
as a “struct kernel_param” into the __param section. Later, load_module()
obtains all “kernel_param” from the __param section and compare against the
user-input module parameters from the command-line. If there is a match, it
calls params[i].ops->set(¶ms[I]) to replace the value. If compilers can’t
see that params[i].ops->set(¶ms[I]) could potentially change the value
of rx_frag_size, it will wrongly optimize it as a constant.
For example (it is not
compilable yet as I have not able to extract variable from the __param section
like find_module_sections()),
#include <stdio.h>
#include <string.h>
#define __module_param_call(name, ops, arg) \
static struct kernel_param __param_##name \
__attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) = { \
#name, ops, { arg } }
struct kernel_param {
const char *name;
const struct kernel_param_ops *ops;
union {
int *arg;
};
};
struct kernel_param_ops {
int (*set)(const struct kernel_param *kp);
};
#define STANDARD_PARAM_DEF(name) \
int param_set_##name(const struct kernel_param *kp) \
{ \
*kp->arg = 2; \
} \
const struct kernel_param_ops param_ops_##name = { \
.set = param_set_##name, \
};
STANDARD_PARAM_DEF(ushort);
static int rx = 1;
__module_param_call(rx_frag_siz, ¶m_ops_ushort, &rx_frag_size);
int main(int argc, char *argv[])
{
const struct kernel_param *params = <<< Get all kernel_param from the __param section >>>;
int i;
if (__builtin_constant_p(rx_frag_size))
printf("rx_frag_size is a const.\n");
for (i = 0; i < num_param; i++) {
if (!strcmp(params[I].name, argv[1])) {
params[i].ops->set(¶ms[i]);
break;
}
}
printf("rx_frag_size = %d\n", rx_frag_size);
return 0;
}