2008-10-17 13:05:44

by Rakib Mullick

[permalink] [raw]
Subject: [PATCH] init: Properly placing noinline keyword.

Here, noinline keyword should be placed between storage class and type.
Thanks.

Signed-off-by: Md.Rakib H. Mullick([email protected])

--- linux-2.6-stable.orig/init/main.c 2008-10-16 20:04:15.000000000 +0600
+++ linux-2.6-stable/init/main.c 2008-10-16 20:59:29.562096784 +0600
@@ -457,7 +457,7 @@ static void __init setup_command_line(ch
* gcc-3.4 accidentally inlines this function, so use noinline.
*/

-static void noinline __init_refok rest_init(void)
+static noinline void __init_refok rest_init(void)
__releases(kernel_lock)
{
int pid;
@@ -792,7 +792,7 @@ static void run_init_process(char *init_
/* This is a non __init function. Force it to be noinline otherwise gcc
* makes it inline to init() and it becomes part of init.text section
*/
-static int noinline init_post(void)
+static noinline int init_post(void)
{
free_initmem();
unlock_kernel();


2008-10-17 14:11:03

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH] init: Properly placing noinline keyword.

On Fri, Oct 17, 2008 at 07:05:32PM +0600, Rakib Mullick wrote:
> Here, noinline keyword should be placed between storage class and type.

Why?

> --- linux-2.6-stable.orig/init/main.c
> +++ linux-2.6-stable/init/main.c
> @@ -457,7 +457,7 @@ static void __init setup_command_line(ch
> * gcc-3.4 accidentally inlines this function, so use noinline.
> */
>
> -static void noinline __init_refok rest_init(void)
> +static noinline void __init_refok rest_init(void)
> __releases(kernel_lock)
> {
> int pid;
> @@ -792,7 +792,7 @@ static void run_init_process(char *init_
> /* This is a non __init function. Force it to be noinline otherwise gcc
> * makes it inline to init() and it becomes part of init.text section
> */
> -static int noinline init_post(void)
> +static noinline int init_post(void)

2008-10-17 14:17:44

by Rakib Mullick

[permalink] [raw]
Subject: Re: [PATCH] init: Properly placing noinline keyword.

On 10/17/08, Alexey Dobriyan <[email protected]> wrote:
> On Fri, Oct 17, 2008 at 07:05:32PM +0600, Rakib Mullick wrote:
> > Here, noinline keyword should be placed between storage class and type.
>
>
> Why?
Because, scripts/checkpatch.pl warned with following warning:
ERROR: inline keyword should sit between storage class and type
>
>
> > --- linux-2.6-stable.orig/init/main.c
>
> > +++ linux-2.6-stable/init/main.c
>
> > @@ -457,7 +457,7 @@ static void __init setup_command_line(ch
> > * gcc-3.4 accidentally inlines this function, so use noinline.
> > */
> >
> > -static void noinline __init_refok rest_init(void)
> > +static noinline void __init_refok rest_init(void)
> > __releases(kernel_lock)
> > {
> > int pid;
> > @@ -792,7 +792,7 @@ static void run_init_process(char *init_
> > /* This is a non __init function. Force it to be noinline otherwise gcc
> > * makes it inline to init() and it becomes part of init.text section
> > */
> > -static int noinline init_post(void)
> > +static noinline int init_post(void)
>

2008-10-17 14:26:47

by Cong Wang

[permalink] [raw]
Subject: Re: [PATCH] init: Properly placing noinline keyword.

On Fri, Oct 17, 2008 at 08:17:33PM +0600, Rakib Mullick wrote:
>On 10/17/08, Alexey Dobriyan <[email protected]> wrote:
>> On Fri, Oct 17, 2008 at 07:05:32PM +0600, Rakib Mullick wrote:
>> > Here, noinline keyword should be placed between storage class and type.
>>
>>
>> Why?
>Because, scripts/checkpatch.pl warned with following warning:
> ERROR: inline keyword should sit between storage class and type

Well, 'noinline' is different from 'inline'.

'noinline' is defined as:

#define noinline __attribute__((noinline))

in include/linux/compiler-gcc.h. But 'inline' is a _keyword_ defined
by C standard. If checkpatch.pl complains about 'noinline', you should
fix checkpatch.pl. :)


--
"Sometimes the only way to stay sane is to go a little crazy."

2008-10-17 15:14:50

by Rakib Mullick

[permalink] [raw]
Subject: Re: [PATCH] init: Properly placing noinline keyword.

On 10/17/08, Am?rico Wang <[email protected]> wrote:
> On Fri, Oct 17, 2008 at 08:17:33PM +0600, Rakib Mullick wrote:
> >On 10/17/08, Alexey Dobriyan <[email protected]> wrote:
> >> On Fri, Oct 17, 2008 at 07:05:32PM +0600, Rakib Mullick wrote:
> >> > Here, noinline keyword should be placed between storage class and type.
> >>
> >>
> >> Why?
> >Because, scripts/checkpatch.pl warned with following warning:
> > ERROR: inline keyword should sit between storage class and type
>
>
> Well, 'noinline' is different from 'inline'.
>
> 'noinline' is defined as:
>
> #define noinline __attribute__((noinline))
>
> in include/linux/compiler-gcc.h. But 'inline' is a _keyword_ defined
> by C standard. If checkpatch.pl complains about 'noinline', you should
> fix checkpatch.pl. :)
Thanks, for explanation. But isn't it nice to place it between storage
class and type ?
>
>
> --
> "Sometimes the only way to stay sane is to go a little crazy."
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2008-10-17 17:31:59

by Cong Wang

[permalink] [raw]
Subject: Re: [PATCH] init: Properly placing noinline keyword.

On Fri, Oct 17, 2008 at 09:10:07PM +0600, Rakib Mullick wrote:
>On 10/17/08, Américo Wang <[email protected]> wrote:
>> On Fri, Oct 17, 2008 at 08:17:33PM +0600, Rakib Mullick wrote:
>> >On 10/17/08, Alexey Dobriyan <[email protected]> wrote:
>> >> On Fri, Oct 17, 2008 at 07:05:32PM +0600, Rakib Mullick wrote:
>> >> > Here, noinline keyword should be placed between storage class and type.
>> >>
>> >>
>> >> Why?
>> >Because, scripts/checkpatch.pl warned with following warning:
>> > ERROR: inline keyword should sit between storage class and type
>>
>>
>> Well, 'noinline' is different from 'inline'.
>>
>> 'noinline' is defined as:
>>
>> #define noinline __attribute__((noinline))
>>
>> in include/linux/compiler-gcc.h. But 'inline' is a _keyword_ defined
>> by C standard. If checkpatch.pl complains about 'noinline', you should
>> fix checkpatch.pl. :)
>Thanks, for explanation. But isn't it nice to place it between storage
>class and type ?

I don't think so, I don't know why checkpatch.pl prefers that style.
I think probably only because that is more readable?

Anyway, gcc attribute is another different thing.

--
"Sometimes the only way to stay sane is to go a little crazy."

2008-10-20 22:13:22

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] init: Properly placing noinline keyword.

On Fri, 17 Oct 2008 18:31:23 +0100
Am__rico Wang <[email protected]> wrote:

> On Fri, Oct 17, 2008 at 09:10:07PM +0600, Rakib Mullick wrote:
> >On 10/17/08, Am__rico Wang <[email protected]> wrote:
> >> On Fri, Oct 17, 2008 at 08:17:33PM +0600, Rakib Mullick wrote:
> >> >On 10/17/08, Alexey Dobriyan <[email protected]> wrote:
> >> >> On Fri, Oct 17, 2008 at 07:05:32PM +0600, Rakib Mullick wrote:
> >> >> > Here, noinline keyword should be placed between storage class and type.
> >> >>
> >> >>
> >> >> Why?
> >> >Because, scripts/checkpatch.pl warned with following warning:
> >> > ERROR: inline keyword should sit between storage class and type
> >>
> >>
> >> Well, 'noinline' is different from 'inline'.
> >>
> >> 'noinline' is defined as:
> >>
> >> #define noinline __attribute__((noinline))
> >>
> >> in include/linux/compiler-gcc.h. But 'inline' is a _keyword_ defined
> >> by C standard. If checkpatch.pl complains about 'noinline', you should
> >> fix checkpatch.pl. :)
> >Thanks, for explanation. But isn't it nice to place it between storage
> >class and type ?
>
> I don't think so, I don't know why checkpatch.pl prefers that style.
> I think probably only because that is more readable?
>

I think it's good for consistency reasons. Yes, we _could_ have a
random sprinkling of different keyword orderings, but what benefit is
there in that? In the great majority of places the kernel uses `static
inline void' and `static noinline void' ordering, and that's a good
thing, no?

So I merged the patch and I'd support retaining the checkpatch warning.

My one concern is that the patch is too small. Probably there are
other codesites which get the keywords in a non-standard order, and I'd
rather fix them up in a single big pass rather than in a long series of
little patches.