[PATCH] be2net: fix adapter->big_page_size miscaculation

[PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

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 <cai@lca.pw>
---
 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[David Miller]

From: Qian Cai <cai@lca.pw>
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.

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

> On Jul 12, 2019, at 6:46 PM, David Miller <davem@davemloft.net> wrote:
> 
> From: Qian Cai <cai@lca.pw>
> 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.

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[David Miller]

From: Qian Cai <cai@lca.pw>
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.

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

> On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> 
> From: Qian Cai <cai@lca.pw>
> 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/1562959401-19815-1-git-send-email-cai@lca.pw/

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.

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Nick Desaulniers]

On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
>
>
>
> > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> >
> > From: Qian Cai <cai@lca.pw>
> > 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/1562959401-19815-1-git-send-email-cai@lca.pw/
>
> 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Bill Wendling]

[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
<ndesaulniers@google.com> wrote:
>
> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
> >
> >
> >
> > > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> > >
> > > From: Qian Cai <cai@lca.pw>
> > > 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/1562959401-19815-1-git-send-email-cai@lca.pw/
> >
> > 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Nick Desaulniers]

On Thu, Jul 18, 2019 at 2:18 PM Bill Wendling <morbo@google.com> 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 <ndesaulniers@google.com> wrote:
>>
>> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
>> >
>> >
>> >
>> > > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
>> > >
>> > > From: Qian Cai <cai@lca.pw>
>> > > 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/1562959401-19815-1-git-send-email-cai@lca.pw/
>> >
>> > 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Bill Wendling]

Possibly. I'd need to ask him. :-)

On Thu, Jul 18, 2019 at 2:22 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Jul 18, 2019 at 2:18 PM Bill Wendling <morbo@google.com> 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 <ndesaulniers@google.com> wrote:
> >>
> >> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
> >> >
> >> >
> >> >
> >> > > On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> >> > >
> >> > > From: Qian Cai <cai@lca.pw>
> >> > > 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/1562959401-19815-1-git-send-email-cai@lca.pw/
> >> >
> >> > 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

> On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> 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
> <ndesaulniers@google.com> wrote:
>> 
>> On Thu, Jul 18, 2019 at 2:01 PM Qian Cai <cai@lca.pw> wrote:
>>> 
>>> 
>>> 
>>>> On Jul 12, 2019, at 8:50 PM, David Miller <davem@davemloft.net> wrote:
>>>> 
>>>> From: Qian Cai <cai@lca.pw>
>>>> 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/1562959401-19815-1-git-send-email-cai@lca.pw/
>>> 
>>> 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[David Miller]

From: Qian Cai <cai@lca.pw>
Date: Thu, 18 Jul 2019 19:26:47 -0400

> 
> 
>> On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> 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.

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

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 <lkp@intel.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54343 bytes --]

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

On Thu, 2019-07-18 at 16:29 -0700, David Miller wrote:
> From: Qian Cai <cai@lca.pw>
> Date: Thu, 18 Jul 2019 19:26:47 -0400
> 
> > 
> > 
> >> On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> 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".

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

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 <cai@lca.pw>
> > Date: Thu, 18 Jul 2019 19:26:47 -0400
> > 
> > >  
> > >  
> > > > On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> 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

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[James Y Knight]

On Mon, Jul 22, 2019 at 5:13 PM Qian Cai <cai@lca.pw> 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 <cai@lca.pw>
> > > Date: Thu, 18 Jul 2019 19:26:47 -0400
> > >
> > > >
> > > >
> > > > > On Jul 18, 2019, at 5:21 PM, Bill Wendling <morbo@google.com> 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).

Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[Qian Cai]

The original issue,

https://lore.kernel.org/netdev/1562959401-19815-1-git-send-email-cai@lca.pw/

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(&params[I]) to replace the value.  If compilers can’t
see that params[i].ops->set(&params[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, &param_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(&params[i]);
                        break;
                }
        }

        printf("rx_frag_size = %d\n", rx_frag_size);

        return 0;
}