* [PATCH] vmalloc: remove #ifdef in function body
@ 2011-12-21 5:17 Minchan Kim
2011-12-21 5:31 ` Joe Perches
2011-12-21 6:13 ` Michal Nazarewicz
0 siblings, 2 replies; 13+ messages in thread
From: Minchan Kim @ 2011-12-21 5:17 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, LKML, Minchan Kim
We don't like function body which include #ifdef.
If we can, define null function to go out compile time.
It's trivial, no functional change.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
mm/vmalloc.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 0aca3ce..e1fa5a6 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -505,6 +505,7 @@ static void unmap_vmap_area(struct vmap_area *va)
vunmap_page_range(va->va_start, va->va_end);
}
+#ifdef CONFIG_DEBUG_PAGEALLOC
static void vmap_debug_free_range(unsigned long start, unsigned long end)
{
/*
@@ -520,11 +521,15 @@ static void vmap_debug_free_range(unsigned long start, unsigned long end)
* debugging doesn't do a broadcast TLB flush so it is a lot
* faster).
*/
-#ifdef CONFIG_DEBUG_PAGEALLOC
vunmap_page_range(start, end);
flush_tlb_kernel_range(start, end);
-#endif
}
+#else
+static inline void vmap_debug_free_range(unsigned long start,
+ unsigned long end)
+{
+}
+#endif
/*
* lazy_max_pages is the maximum amount of virtual address space we gather up
--
1.7.6.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 5:17 [PATCH] vmalloc: remove #ifdef in function body Minchan Kim
@ 2011-12-21 5:31 ` Joe Perches
2011-12-21 5:45 ` Minchan Kim
2011-12-21 6:13 ` Michal Nazarewicz
1 sibling, 1 reply; 13+ messages in thread
From: Joe Perches @ 2011-12-21 5:31 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML
On Wed, 2011-12-21 at 14:17 +0900, Minchan Kim wrote:
> We don't like function body which include #ifdef.
[]
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
[]
> @@ -505,6 +505,7 @@ static void unmap_vmap_area(struct vmap_area *va)
> vunmap_page_range(va->va_start, va->va_end);
> }
>
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> static void vmap_debug_free_range(unsigned long start, unsigned long end)
> {
> /*
> @@ -520,11 +521,15 @@ static void vmap_debug_free_range(unsigned long start, unsigned long end)
> * debugging doesn't do a broadcast TLB flush so it is a lot
> * faster).
> */
> -#ifdef CONFIG_DEBUG_PAGEALLOC
> vunmap_page_range(start, end);
> flush_tlb_kernel_range(start, end);
> -#endif
> }
> +#else
> +static inline void vmap_debug_free_range(unsigned long start,
> + unsigned long end)
> +{
> +}
> +#endif
I don't like this change.
I think it's perfectly good style to use:
1 void foo(args...)
2 {
3 #ifdef CONFIG_FOO
4 ...
5 #endif
6 }
instead of
1 #ifdef CONFIG_FOO
2 void foo(args...)
3 {
4 ...
5 }
6 #else
7 void foo(args...)
8 {
9 }
10 #endif
The first version is shorter and gcc optimizes
away the void func just fine. It also means
that 2 function prototypes don't need to be
kept in agreement when someone changes one
without testing CONFIG_FOO=y and CONFIG_FOO=n.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 5:31 ` Joe Perches
@ 2011-12-21 5:45 ` Minchan Kim
2011-12-21 5:58 ` Joe Perches
0 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2011-12-21 5:45 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Morton, linux-mm, LKML
On Tue, Dec 20, 2011 at 09:31:21PM -0800, Joe Perches wrote:
> On Wed, 2011-12-21 at 14:17 +0900, Minchan Kim wrote:
> > We don't like function body which include #ifdef.
> []
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> []
> > @@ -505,6 +505,7 @@ static void unmap_vmap_area(struct vmap_area *va)
> > vunmap_page_range(va->va_start, va->va_end);
> > }
> >
> > +#ifdef CONFIG_DEBUG_PAGEALLOC
> > static void vmap_debug_free_range(unsigned long start, unsigned long end)
> > {
> > /*
> > @@ -520,11 +521,15 @@ static void vmap_debug_free_range(unsigned long start, unsigned long end)
> > * debugging doesn't do a broadcast TLB flush so it is a lot
> > * faster).
> > */
> > -#ifdef CONFIG_DEBUG_PAGEALLOC
> > vunmap_page_range(start, end);
> > flush_tlb_kernel_range(start, end);
> > -#endif
> > }
> > +#else
> > +static inline void vmap_debug_free_range(unsigned long start,
> > + unsigned long end)
> > +{
> > +}
> > +#endif
>
> I don't like this change.
> I think it's perfectly good style to use:
I feel it's no problem as it is because it's very short function now
but it's not style we prefer.
>
> 1 void foo(args...)
> 2 {
> 3 #ifdef CONFIG_FOO
> 4 ...
> 5 #endif
> 6 }
>
> instead of
>
> 1 #ifdef CONFIG_FOO
> 2 void foo(args...)
> 3 {
> 4 ...
> 5 }
> 6 #else
> 7 void foo(args...)
> 8 {
> 9 }
> 10 #endif
>
> The first version is shorter and gcc optimizes
> away the void func just fine. It also means
Agree but if function would be long(but I convice
it's not long in future :)), it would be messy.
> that 2 function prototypes don't need to be
> kept in agreement when someone changes one
> without testing CONFIG_FOO=y and CONFIG_FOO=n.
The goal is not for making test easily.
Patch author should keep it consistent.
This patch is just trivial so I don't mind if who have
against this patch strongly. What I want to say is
it's not style we prefer.
>
>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 5:45 ` Minchan Kim
@ 2011-12-21 5:58 ` Joe Perches
2011-12-21 6:07 ` Minchan Kim
2011-12-21 6:21 ` Michal Nazarewicz
0 siblings, 2 replies; 13+ messages in thread
From: Joe Perches @ 2011-12-21 5:58 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML
On Wed, 2011-12-21 at 14:45 +0900, Minchan Kim wrote:
> On Tue, Dec 20, 2011 at 09:31:21PM -0800, Joe Perches wrote:
> > On Wed, 2011-12-21 at 14:17 +0900, Minchan Kim wrote:
> > > We don't like function body which include #ifdef.
[]
> > I don't like this change.
> > I think it's perfectly good style to use:
> I feel it's no problem as it is because it's very short function now
> but it's not style we prefer.
Who is this "we" you refer to?
There's nothing suggesting your patch as a preferred style
in Documentation/CodingStyle.
cheers, Joe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 5:58 ` Joe Perches
@ 2011-12-21 6:07 ` Minchan Kim
2011-12-21 6:27 ` Joe Perches
2011-12-21 6:21 ` Michal Nazarewicz
1 sibling, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2011-12-21 6:07 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Morton, linux-mm, LKML
On Tue, Dec 20, 2011 at 09:58:19PM -0800, Joe Perches wrote:
> On Wed, 2011-12-21 at 14:45 +0900, Minchan Kim wrote:
> > On Tue, Dec 20, 2011 at 09:31:21PM -0800, Joe Perches wrote:
> > > On Wed, 2011-12-21 at 14:17 +0900, Minchan Kim wrote:
> > > > We don't like function body which include #ifdef.
> []
> > > I don't like this change.
> > > I think it's perfectly good style to use:
> > I feel it's no problem as it is because it's very short function now
> > but it's not style we prefer.
>
> Who is this "we" you refer to?
>
> There's nothing suggesting your patch as a preferred style
> in Documentation/CodingStyle.
Yes. It doesn't have.
But I have thought we have done until now.
I think we can see them easily.
#> grep -nRH 'static inline void' ./ | grep {} | wc -l
936
If we consider line which don't include brace in one line, it would be many.
>
> cheers, Joe
>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 5:17 [PATCH] vmalloc: remove #ifdef in function body Minchan Kim
2011-12-21 5:31 ` Joe Perches
@ 2011-12-21 6:13 ` Michal Nazarewicz
2011-12-21 6:22 ` Minchan Kim
1 sibling, 1 reply; 13+ messages in thread
From: Michal Nazarewicz @ 2011-12-21 6:13 UTC (permalink / raw)
To: Andrew Morton, Minchan Kim; +Cc: linux-mm, LKML
On Wed, 21 Dec 2011 06:17:59 +0100, Minchan Kim <minchan@kernel.org> wrote:
> We don't like function body which include #ifdef.
> If we can, define null function to go out compile time.
> It's trivial, no functional change.
It actually adds “flush_tlb_kenel_range()” call to the function so there
is functional change.
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> mm/vmalloc.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 0aca3ce..e1fa5a6 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -505,6 +505,7 @@ static void unmap_vmap_area(struct vmap_area *va)
> vunmap_page_range(va->va_start, va->va_end);
> }
>+#ifdef CONFIG_DEBUG_PAGEALLOC
> static void vmap_debug_free_range(unsigned long start, unsigned long end)
> {
> /*
> @@ -520,11 +521,15 @@ static void vmap_debug_free_range(unsigned long start, unsigned long end)
> * debugging doesn't do a broadcast TLB flush so it is a lot
> * faster).
> */
> -#ifdef CONFIG_DEBUG_PAGEALLOC
> vunmap_page_range(start, end);
> flush_tlb_kernel_range(start, end);
> -#endif
> }
> +#else
> +static inline void vmap_debug_free_range(unsigned long start,
> + unsigned long end)
> +{
> +}
> +#endif
> /*
> * lazy_max_pages is the maximum amount of virtual address space we gather up
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 5:58 ` Joe Perches
2011-12-21 6:07 ` Minchan Kim
@ 2011-12-21 6:21 ` Michal Nazarewicz
2011-12-21 6:32 ` Joe Perches
1 sibling, 1 reply; 13+ messages in thread
From: Michal Nazarewicz @ 2011-12-21 6:21 UTC (permalink / raw)
To: Minchan Kim, Joe Perches; +Cc: Andrew Morton, linux-mm, LKML
On Wed, 21 Dec 2011 06:58:19 +0100, Joe Perches <joe@perches.com> wrote:
> On Wed, 2011-12-21 at 14:45 +0900, Minchan Kim wrote:
>> I feel it's no problem as it is because it's very short function now
>> but it's not style we prefer.
> Who is this "we" you refer to?
>
> There's nothing suggesting your patch as a preferred style
> in Documentation/CodingStyle.
There is plenty that is not documented, ;) yet it seems the community prefers
having ifdefs outside of the function.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 6:13 ` Michal Nazarewicz
@ 2011-12-21 6:22 ` Minchan Kim
2011-12-21 6:24 ` Michal Nazarewicz
0 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2011-12-21 6:22 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Andrew Morton, linux-mm, LKML
On Wed, Dec 21, 2011 at 07:13:49AM +0100, Michal Nazarewicz wrote:
> On Wed, 21 Dec 2011 06:17:59 +0100, Minchan Kim <minchan@kernel.org> wrote:
> >We don't like function body which include #ifdef.
> >If we can, define null function to go out compile time.
> >It's trivial, no functional change.
>
> It actually adds “flush_tlb_kenel_range()” call to the function so there
> is functional change.
Sorry. I can't understand your point.
Why does it add flush_tlb_kernel_range in case of !CONFIG_DEBUG_PAGEALLOC?
>
> >Signed-off-by: Minchan Kim <minchan@kernel.org>
> >---
> > mm/vmalloc.c | 9 +++++++--
> > 1 files changed, 7 insertions(+), 2 deletions(-)
> >
> >diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> >index 0aca3ce..e1fa5a6 100644
> >--- a/mm/vmalloc.c
> >+++ b/mm/vmalloc.c
> >@@ -505,6 +505,7 @@ static void unmap_vmap_area(struct vmap_area *va)
> > vunmap_page_range(va->va_start, va->va_end);
> > }
> >+#ifdef CONFIG_DEBUG_PAGEALLOC
> > static void vmap_debug_free_range(unsigned long start, unsigned long end)
> > {
> > /*
> >@@ -520,11 +521,15 @@ static void vmap_debug_free_range(unsigned long start, unsigned long end)
> > * debugging doesn't do a broadcast TLB flush so it is a lot
> > * faster).
> > */
> >-#ifdef CONFIG_DEBUG_PAGEALLOC
> > vunmap_page_range(start, end);
> > flush_tlb_kernel_range(start, end);
> >-#endif
> > }
> >+#else
> >+static inline void vmap_debug_free_range(unsigned long start,
> >+ unsigned long end)
> >+{
> >+}
> >+#endif
> >/*
> > * lazy_max_pages is the maximum amount of virtual address space we gather up
>
> --
> Best regards, _ _
> .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> ..o | Computer Science, Michał “mina86” Nazarewicz (o o)
> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 6:22 ` Minchan Kim
@ 2011-12-21 6:24 ` Michal Nazarewicz
0 siblings, 0 replies; 13+ messages in thread
From: Michal Nazarewicz @ 2011-12-21 6:24 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML
On Wed, 21 Dec 2011 07:22:32 +0100, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Dec 21, 2011 at 07:13:49AM +0100, Michal Nazarewicz wrote:
>> On Wed, 21 Dec 2011 06:17:59 +0100, Minchan Kim <minchan@kernel.org> wrote:
>> >We don't like function body which include #ifdef.
>> >If we can, define null function to go out compile time.
>> >It's trivial, no functional change.
>>
>> It actually adds “flush_tlb_kenel_range()” call to the function so there
>> is functional change.
>
> Sorry. I can't understand your point.
> Why does it add flush_tlb_kernel_range in case of !CONFIG_DEBUG_PAGEALLOC?
Uh, sorry, I've totally misread the function. Never mind my comment.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 6:07 ` Minchan Kim
@ 2011-12-21 6:27 ` Joe Perches
0 siblings, 0 replies; 13+ messages in thread
From: Joe Perches @ 2011-12-21 6:27 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML
On Wed, 2011-12-21 at 15:07 +0900, Minchan Kim wrote:
> On Tue, Dec 20, 2011 at 09:58:19PM -0800, Joe Perches wrote:
> > On Wed, 2011-12-21 at 14:45 +0900, Minchan Kim wrote:
> > > On Tue, Dec 20, 2011 at 09:31:21PM -0800, Joe Perches wrote:
> > > > On Wed, 2011-12-21 at 14:17 +0900, Minchan Kim wrote:
> > > > > We don't like function body which include #ifdef.
> > []
> > > > I don't like this change.
> > > > I think it's perfectly good style to use:
> > > I feel it's no problem as it is because it's very short function now
> > > but it's not style we prefer.
> > Who is this "we" you refer to?
> > There's nothing suggesting your patch as a preferred style
> > in Documentation/CodingStyle.
> Yes. It doesn't have.
> But I have thought we have done until now.
But whoever this "we" you're referring to hasn't
actually done so.
> I think we can see them easily.
>
> #> grep -nRH 'static inline void' ./ | grep {} | wc -l
> 936
>
> If we consider line which don't include brace in one line, it would be many.
Try:
$ grep -rP --include=*.[ch] \
'^(static|)\s*(inline|)\s*void\b[^\n;]+\n(?:{\s*|)\#\s*if' * | \
wc -l
3603
A rough approximation would be to divide by 3.
So there's maybe a 1000 or so of the other style too.
cheers, Joe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 6:21 ` Michal Nazarewicz
@ 2011-12-21 6:32 ` Joe Perches
2011-12-21 6:47 ` Michal Nazarewicz
0 siblings, 1 reply; 13+ messages in thread
From: Joe Perches @ 2011-12-21 6:32 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML
On Wed, 2011-12-21 at 07:21 +0100, Michal Nazarewicz wrote:
> it seems the community prefers
> having ifdefs outside of the function.
Some do, some don't.
http://comments.gmane.org/gmane.linux.network/214543
If it's not in coding style, I suggest
it should be changed if it doesn't
add some other useful value.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 6:32 ` Joe Perches
@ 2011-12-21 6:47 ` Michal Nazarewicz
2011-12-21 15:21 ` Ted Ts'o
0 siblings, 1 reply; 13+ messages in thread
From: Michal Nazarewicz @ 2011-12-21 6:47 UTC (permalink / raw)
To: Joe Perches; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML
On Wed, 21 Dec 2011 07:32:36 +0100, Joe Perches <joe@perches.com> wrote:
> On Wed, 2011-12-21 at 07:21 +0100, Michal Nazarewicz wrote:
>> it seems the community prefers
>> having ifdefs outside of the function.
>
> Some do, some don't.
>
> http://comments.gmane.org/gmane.linux.network/214543
This patch that you pointed to is against “#ifdefs are ugly” style
described in Documentation/SubmittingPatches.
> If it's not in coding style, I suggest
> it should be changed if it doesn't
> add some other useful value.
That my be true. I guess no one took time to adding it to the document.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] vmalloc: remove #ifdef in function body
2011-12-21 6:47 ` Michal Nazarewicz
@ 2011-12-21 15:21 ` Ted Ts'o
0 siblings, 0 replies; 13+ messages in thread
From: Ted Ts'o @ 2011-12-21 15:21 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Joe Perches, Minchan Kim, Andrew Morton, linux-mm, LKML
On Wed, Dec 21, 2011 at 07:47:17AM +0100, Michal Nazarewicz wrote:
> This patch that you pointed to is against “#ifdefs are ugly” style
> described in Documentation/SubmittingPatches.
>
> >If it's not in coding style, I suggest
> >it should be changed if it doesn't
> >add some other useful value.
>
> That my be true. I guess no one took time to adding it to the document.
Like all things, judgement is required. Some of the greatest artists
know when it's OK (and in fact, a good thing) to break the rules.
Beethoven, for example, broke the rules when he added a chorus of
singers to his 9th Symphony. Take a look at Bach's chorales,
universally acknowledged to be works of genius. Yet there Bach has
occasionally double thirds, crossed voices, and engaged in parallel
fifths --- and big no-no's which go against the textbook rules of
chorale writing.
In this case, if you have an #ifdef surrounding an entire function
body, I think common sense says that the it's fine. There's also the
rule which is says that all other things being equal, it's better not
to waste vertical space and useless boiler plate.
Worst of all is patches to change perfectly existing code because
someone is trying to be a stickler for rules, since it can break other
people's patches. If you are need to make a change, it's best that it
be checkpatch clean. But sending random cleanups just because someone
is trying to get their patch count in the kernel higher is Just
Stupid, and should be strongly discouraged.
(And that last, too, is a rule that has exceptions...)
- Ted
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-12-21 15:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-21 5:17 [PATCH] vmalloc: remove #ifdef in function body Minchan Kim
2011-12-21 5:31 ` Joe Perches
2011-12-21 5:45 ` Minchan Kim
2011-12-21 5:58 ` Joe Perches
2011-12-21 6:07 ` Minchan Kim
2011-12-21 6:27 ` Joe Perches
2011-12-21 6:21 ` Michal Nazarewicz
2011-12-21 6:32 ` Joe Perches
2011-12-21 6:47 ` Michal Nazarewicz
2011-12-21 15:21 ` Ted Ts'o
2011-12-21 6:13 ` Michal Nazarewicz
2011-12-21 6:22 ` Minchan Kim
2011-12-21 6:24 ` Michal Nazarewicz
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).