[PATCH] vmalloc: remove #ifdef in function body

[PATCH] vmalloc: remove #ifdef in function body

[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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Joe Perches]

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.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Minchan Kim]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Joe Perches]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Minchan Kim]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Joe Perches]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Michal Nazarewicz]

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--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Joe Perches]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Michal Nazarewicz]

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--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Ted Ts'o]

On Wed, Dec 21, 2011 at 07:47:17AM +0100, Michal Nazarewicz wrote:
> This patch that you pointed to is against a??#ifdefs are uglya?? 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Michal Nazarewicz]

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--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Minchan Kim]

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 a??flush_tlb_kenel_range()a?? 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,  MichaA? a??mina86a?? Nazarewicz    (o o)
> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] vmalloc: remove #ifdef in function body

[Michal Nazarewicz]

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--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>