Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1342496 > unrolled thread

[PATCH trivial] include/linux/gfp.h: Improve the coding styles

Back to article view | Back to linux.kernel

Contents

  [PATCH trivial] include/linux/gfp.h: Improve the coding styles chengang@emindsoft.com.cn - 2016-02-24 23:30 +0100
    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles SeongJae Park <sj38.park@gmail.com> - 2016-02-25 02:10 +0100
      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 15:10 +0100
    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Michal Hocko <mhocko@kernel.org> - 2016-02-25 10:00 +0100
      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 15:30 +0100
        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Michal Hocko <mhocko@kernel.org> - 2016-02-25 15:50 +0100
          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 23:20 +0100
    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Mel Gorman <mgorman@techsingularity.net> - 2016-02-25 10:30 +0100
      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 15:40 +0100
        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jiri Kosina <jikos@kernel.org> - 2016-02-25 16:20 +0100
          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 23:20 +0100
        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Mel Gorman <mgorman@techsingularity.net> - 2016-02-25 17:10 +0100
          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 23:30 +0100
            Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jiri Kosina <jikos@kernel.org> - 2016-02-25 23:40 +0100
              Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-26 16:00 +0100
            Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles SeongJae Park <sj38.park@gmail.com> - 2016-02-26 00:20 +0100
              Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-26 16:10 +0100
            Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jianyu Zhan <nasa4836@gmail.com> - 2016-02-26 03:40 +0100
              Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-26 16:30 +0100
                Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Theodore Ts'o <tytso@mit.edu> - 2016-02-27 03:50 +0100
                  Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-27 15:30 +0100
                    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Theodore Ts'o <tytso@mit.edu> - 2016-02-27 18:00 +0100
                      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-28 01:20 +0100
                        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Mel Gorman <mgorman@techsingularity.net> - 2016-02-28 14:30 +0100
                          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-28 16:30 +0100
                    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jiri Kosina <jikos@kernel.org> - 2016-02-28 00:20 +0100
                      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-28 01:50 +0100
                        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Theodore Ts'o <tytso@mit.edu> - 2016-02-28 23:30 +0100
                          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-29 18:50 +0100

Page 1 of 2  [1] 2  Next page →

#1342496 — [PATCH trivial] include/linux/gfp.h: Improve the coding styles

From: Chen Gang <chengang@emindsoft.com.cn>

Always notice about 80 columns, and the white space near '|'.

Let the wrapped function parameters align as the same styles.

Remove redundant statement "enum zone_type z;" in function gfp_zone.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 include/linux/gfp.h | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 36e0c5e..cf904ef 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -53,8 +53,10 @@ struct vm_area_struct;
 #define __GFP_DMA	((__force gfp_t)___GFP_DMA)
 #define __GFP_HIGHMEM	((__force gfp_t)___GFP_HIGHMEM)
 #define __GFP_DMA32	((__force gfp_t)___GFP_DMA32)
-#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
-#define GFP_ZONEMASK	(__GFP_DMA|__GFP_HIGHMEM|__GFP_DMA32|__GFP_MOVABLE)
+#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE) \
+						/* ZONE_MOVABLE allowed */
+#define GFP_ZONEMASK	(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32 | \
+			 __GFP_MOVABLE)
 
 /*
  * Page mobility and placement hints
@@ -151,9 +153,12 @@ struct vm_area_struct;
  */
 #define __GFP_IO	((__force gfp_t)___GFP_IO)
 #define __GFP_FS	((__force gfp_t)___GFP_FS)
-#define __GFP_DIRECT_RECLAIM	((__force gfp_t)___GFP_DIRECT_RECLAIM) /* Caller can reclaim */
-#define __GFP_KSWAPD_RECLAIM	((__force gfp_t)___GFP_KSWAPD_RECLAIM) /* kswapd can wake */
-#define __GFP_RECLAIM ((__force gfp_t)(___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM))
+#define __GFP_DIRECT_RECLAIM ((__force gfp_t)___GFP_DIRECT_RECLAIM) \
+							/* Caller can reclaim */
+#define __GFP_KSWAPD_RECLAIM ((__force gfp_t)___GFP_KSWAPD_RECLAIM) \
+							/* kswapd can wake */
+#define __GFP_RECLAIM	((__force gfp_t)(___GFP_DIRECT_RECLAIM | \
+			 ___GFP_KSWAPD_RECLAIM))
 #define __GFP_REPEAT	((__force gfp_t)___GFP_REPEAT)
 #define __GFP_NOFAIL	((__force gfp_t)___GFP_NOFAIL)
 #define __GFP_NORETRY	((__force gfp_t)___GFP_NORETRY)
@@ -262,7 +267,7 @@ struct vm_area_struct;
 			 ~__GFP_KSWAPD_RECLAIM)
 
 /* Convert GFP flags to their corresponding migrate type */
-#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
+#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE | __GFP_MOVABLE)
 #define GFP_MOVABLE_SHIFT 3
 
 static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
@@ -377,11 +382,10 @@ static inline bool gfpflags_allow_blocking(const gfp_t gfp_flags)
 
 static inline enum zone_type gfp_zone(gfp_t flags)
 {
-	enum zone_type z;
 	int bit = (__force int) (flags & GFP_ZONEMASK);
+	enum zone_type z = (GFP_ZONE_TABLE >> (bit * GFP_ZONES_SHIFT)) &
+			    ((1 << GFP_ZONES_SHIFT) - 1);
 
-	z = (GFP_ZONE_TABLE >> (bit * GFP_ZONES_SHIFT)) &
-					 ((1 << GFP_ZONES_SHIFT) - 1);
 	VM_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
 	return z;
 }
@@ -428,8 +432,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 		       struct zonelist *zonelist, nodemask_t *nodemask);
 
 static inline struct page *
-__alloc_pages(gfp_t gfp_mask, unsigned int order,
-		struct zonelist *zonelist)
+__alloc_pages(gfp_t gfp_mask, unsigned int order, struct zonelist *zonelist)
 {
 	return __alloc_pages_nodemask(gfp_mask, order, zonelist, NULL);
 }
@@ -453,7 +456,7 @@ __alloc_pages_node(int nid, gfp_t gfp_mask, unsigned int order)
  * online.
  */
 static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
-						unsigned int order)
+					    unsigned int order)
 {
 	if (nid == NUMA_NO_NODE)
 		nid = numa_mem_id();
@@ -470,8 +473,9 @@ alloc_pages(gfp_t gfp_mask, unsigned int order)
 	return alloc_pages_current(gfp_mask, order);
 }
 extern struct page *alloc_pages_vma(gfp_t gfp_mask, int order,
-			struct vm_area_struct *vma, unsigned long addr,
-			int node, bool hugepage);
+				    struct vm_area_struct *vma,
+				    unsigned long addr, int node,
+				    bool hugepage);
 #define alloc_hugepage_vma(gfp_mask, vma, addr, order)	\
 	alloc_pages_vma(gfp_mask, order, vma, addr, numa_node_id(), true)
 #else
@@ -552,7 +556,8 @@ static inline bool pm_suspended_storage(void)
 }
 #endif /* CONFIG_PM_SLEEP */
 
-#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
+#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || \
+     defined(CONFIG_CMA)
 /* The below functions must be run on a range from a single zone. */
 extern int alloc_contig_range(unsigned long start, unsigned long end,
 			      unsigned migratetype);
-- 
1.9.3

[toc] | [next] | [standalone]

#1342558

Hello Chen,


On Thu, 25 Feb 2016, chengang@emindsoft.com.cn wrote:

> From: Chen Gang <chengang@emindsoft.com.cn>
>
> Always notice about 80 columns, and the white space near '|'.
>
> Let the wrapped function parameters align as the same styles.
>
> Remove redundant statement "enum zone_type z;" in function gfp_zone.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> include/linux/gfp.h | 35 ++++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 36e0c5e..cf904ef 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -53,8 +53,10 @@ struct vm_area_struct;
> #define __GFP_DMA	((__force gfp_t)___GFP_DMA)
> #define __GFP_HIGHMEM	((__force gfp_t)___GFP_HIGHMEM)
> #define __GFP_DMA32	((__force gfp_t)___GFP_DMA32)
> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
> -#define GFP_ZONEMASK	(__GFP_DMA|__GFP_HIGHMEM|__GFP_DMA32|__GFP_MOVABLE)
> +#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE) \
> +						/* ZONE_MOVABLE allowed */

Well, the indentation for the comment and the '\' looks odd to me.  If
the 80 column limit is necessary, how about moving the comment to above
line of the macro as below?  Because comments are usually placed before
the target they are explaining, I believe this may better to read.

  -#define __GFP_MOVABLE        ((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
  +/* ZONE_MOVABLE allowed */
  +#define __GFP_MOVABLE        ((__force gfp_t)___GFP_MOVABLE)

Maybe the opinion can be applied to below similar changes, too.


Thanks,
SeongJae Park.

> +#define GFP_ZONEMASK	(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32 | \
> +			 __GFP_MOVABLE)
>
> /*
>  * Page mobility and placement hints
> @@ -151,9 +153,12 @@ struct vm_area_struct;
>  */
> #define __GFP_IO	((__force gfp_t)___GFP_IO)
> #define __GFP_FS	((__force gfp_t)___GFP_FS)
> -#define __GFP_DIRECT_RECLAIM	((__force gfp_t)___GFP_DIRECT_RECLAIM) /* Caller can reclaim */
> -#define __GFP_KSWAPD_RECLAIM	((__force gfp_t)___GFP_KSWAPD_RECLAIM) /* kswapd can wake */
> -#define __GFP_RECLAIM ((__force gfp_t)(___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM))
> +#define __GFP_DIRECT_RECLAIM ((__force gfp_t)___GFP_DIRECT_RECLAIM) \
> +							/* Caller can reclaim */
> +#define __GFP_KSWAPD_RECLAIM ((__force gfp_t)___GFP_KSWAPD_RECLAIM) \
> +							/* kswapd can wake */
> +#define __GFP_RECLAIM	((__force gfp_t)(___GFP_DIRECT_RECLAIM | \
> +			 ___GFP_KSWAPD_RECLAIM))
> #define __GFP_REPEAT	((__force gfp_t)___GFP_REPEAT)
> #define __GFP_NOFAIL	((__force gfp_t)___GFP_NOFAIL)
> #define __GFP_NORETRY	((__force gfp_t)___GFP_NORETRY)
> @@ -262,7 +267,7 @@ struct vm_area_struct;
> 			 ~__GFP_KSWAPD_RECLAIM)
>
> /* Convert GFP flags to their corresponding migrate type */
> -#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> +#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE | __GFP_MOVABLE)
> #define GFP_MOVABLE_SHIFT 3
>
> static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> @@ -377,11 +382,10 @@ static inline bool gfpflags_allow_blocking(const gfp_t gfp_flags)
>
> static inline enum zone_type gfp_zone(gfp_t flags)
> {
> -	enum zone_type z;
> 	int bit = (__force int) (flags & GFP_ZONEMASK);
> +	enum zone_type z = (GFP_ZONE_TABLE >> (bit * GFP_ZONES_SHIFT)) &
> +			    ((1 << GFP_ZONES_SHIFT) - 1);
>
> -	z = (GFP_ZONE_TABLE >> (bit * GFP_ZONES_SHIFT)) &
> -					 ((1 << GFP_ZONES_SHIFT) - 1);
> 	VM_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> 	return z;
> }
> @@ -428,8 +432,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
> 		       struct zonelist *zonelist, nodemask_t *nodemask);
>
> static inline struct page *
> -__alloc_pages(gfp_t gfp_mask, unsigned int order,
> -		struct zonelist *zonelist)
> +__alloc_pages(gfp_t gfp_mask, unsigned int order, struct zonelist *zonelist)
> {
> 	return __alloc_pages_nodemask(gfp_mask, order, zonelist, NULL);
> }
> @@ -453,7 +456,7 @@ __alloc_pages_node(int nid, gfp_t gfp_mask, unsigned int order)
>  * online.
>  */
> static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
> -						unsigned int order)
> +					    unsigned int order)
> {
> 	if (nid == NUMA_NO_NODE)
> 		nid = numa_mem_id();
> @@ -470,8 +473,9 @@ alloc_pages(gfp_t gfp_mask, unsigned int order)
> 	return alloc_pages_current(gfp_mask, order);
> }
> extern struct page *alloc_pages_vma(gfp_t gfp_mask, int order,
> -			struct vm_area_struct *vma, unsigned long addr,
> -			int node, bool hugepage);
> +				    struct vm_area_struct *vma,
> +				    unsigned long addr, int node,
> +				    bool hugepage);
> #define alloc_hugepage_vma(gfp_mask, vma, addr, order)	\
> 	alloc_pages_vma(gfp_mask, order, vma, addr, numa_node_id(), true)
> #else
> @@ -552,7 +556,8 @@ static inline bool pm_suspended_storage(void)
> }
> #endif /* CONFIG_PM_SLEEP */
>
> -#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> +#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || \
> +     defined(CONFIG_CMA)
> /* The below functions must be run on a range from a single zone. */
> extern int alloc_contig_range(unsigned long start, unsigned long end,
> 			      unsigned migratetype);
> -- 
> 1.9.3
>
> --
> 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/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

[toc] | [prev] | [next] | [standalone]

#1343181

On 2/25/16 09:01, SeongJae Park wrote:
> 
> Well, the indentation for the comment and the '\' looks odd to me.  If
> the 80 column limit is necessary, how about moving the comment to above
> line of the macro as below?  Because comments are usually placed before
> the target they are explaining, I believe this may better to read.
> 
>  -#define __GFP_MOVABLE        ((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
>  +/* ZONE_MOVABLE allowed */
>  +#define __GFP_MOVABLE        ((__force gfp_t)___GFP_MOVABLE)
> 
> Maybe the opinion can be applied to below similar changes, too.
> 

At least for me, what you said above is OK (it is a common way).

And welcome other members' suggestions.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1342994

On Thu 25-02-16 06:26:31, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> Always notice about 80 columns, and the white space near '|'.
> 
> Let the wrapped function parameters align as the same styles.
> 
> Remove redundant statement "enum zone_type z;" in function gfp_zone.

I do not think this is an improvement. The comment placement is just odd
and artificially splitting the mask into more lines makes git grep
harder to use.

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  include/linux/gfp.h | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 36e0c5e..cf904ef 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -53,8 +53,10 @@ struct vm_area_struct;
>  #define __GFP_DMA	((__force gfp_t)___GFP_DMA)
>  #define __GFP_HIGHMEM	((__force gfp_t)___GFP_HIGHMEM)
>  #define __GFP_DMA32	((__force gfp_t)___GFP_DMA32)
> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
> -#define GFP_ZONEMASK	(__GFP_DMA|__GFP_HIGHMEM|__GFP_DMA32|__GFP_MOVABLE)
> +#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE) \
> +						/* ZONE_MOVABLE allowed */
> +#define GFP_ZONEMASK	(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32 | \
> +			 __GFP_MOVABLE)
>  
>  /*
>   * Page mobility and placement hints
> @@ -151,9 +153,12 @@ struct vm_area_struct;
>   */
>  #define __GFP_IO	((__force gfp_t)___GFP_IO)
>  #define __GFP_FS	((__force gfp_t)___GFP_FS)
> -#define __GFP_DIRECT_RECLAIM	((__force gfp_t)___GFP_DIRECT_RECLAIM) /* Caller can reclaim */
> -#define __GFP_KSWAPD_RECLAIM	((__force gfp_t)___GFP_KSWAPD_RECLAIM) /* kswapd can wake */
> -#define __GFP_RECLAIM ((__force gfp_t)(___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM))
> +#define __GFP_DIRECT_RECLAIM ((__force gfp_t)___GFP_DIRECT_RECLAIM) \
> +							/* Caller can reclaim */
> +#define __GFP_KSWAPD_RECLAIM ((__force gfp_t)___GFP_KSWAPD_RECLAIM) \
> +							/* kswapd can wake */
> +#define __GFP_RECLAIM	((__force gfp_t)(___GFP_DIRECT_RECLAIM | \
> +			 ___GFP_KSWAPD_RECLAIM))
>  #define __GFP_REPEAT	((__force gfp_t)___GFP_REPEAT)
>  #define __GFP_NOFAIL	((__force gfp_t)___GFP_NOFAIL)
>  #define __GFP_NORETRY	((__force gfp_t)___GFP_NORETRY)
> @@ -262,7 +267,7 @@ struct vm_area_struct;
>  			 ~__GFP_KSWAPD_RECLAIM)
>  
>  /* Convert GFP flags to their corresponding migrate type */
> -#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> +#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE | __GFP_MOVABLE)
>  #define GFP_MOVABLE_SHIFT 3
>  
>  static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> @@ -377,11 +382,10 @@ static inline bool gfpflags_allow_blocking(const gfp_t gfp_flags)
>  
>  static inline enum zone_type gfp_zone(gfp_t flags)
>  {
> -	enum zone_type z;
>  	int bit = (__force int) (flags & GFP_ZONEMASK);
> +	enum zone_type z = (GFP_ZONE_TABLE >> (bit * GFP_ZONES_SHIFT)) &
> +			    ((1 << GFP_ZONES_SHIFT) - 1);
>  
> -	z = (GFP_ZONE_TABLE >> (bit * GFP_ZONES_SHIFT)) &
> -					 ((1 << GFP_ZONES_SHIFT) - 1);
>  	VM_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
>  	return z;
>  }
> @@ -428,8 +432,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>  		       struct zonelist *zonelist, nodemask_t *nodemask);
>  
>  static inline struct page *
> -__alloc_pages(gfp_t gfp_mask, unsigned int order,
> -		struct zonelist *zonelist)
> +__alloc_pages(gfp_t gfp_mask, unsigned int order, struct zonelist *zonelist)
>  {
>  	return __alloc_pages_nodemask(gfp_mask, order, zonelist, NULL);
>  }
> @@ -453,7 +456,7 @@ __alloc_pages_node(int nid, gfp_t gfp_mask, unsigned int order)
>   * online.
>   */
>  static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
> -						unsigned int order)
> +					    unsigned int order)
>  {
>  	if (nid == NUMA_NO_NODE)
>  		nid = numa_mem_id();
> @@ -470,8 +473,9 @@ alloc_pages(gfp_t gfp_mask, unsigned int order)
>  	return alloc_pages_current(gfp_mask, order);
>  }
>  extern struct page *alloc_pages_vma(gfp_t gfp_mask, int order,
> -			struct vm_area_struct *vma, unsigned long addr,
> -			int node, bool hugepage);
> +				    struct vm_area_struct *vma,
> +				    unsigned long addr, int node,
> +				    bool hugepage);
>  #define alloc_hugepage_vma(gfp_mask, vma, addr, order)	\
>  	alloc_pages_vma(gfp_mask, order, vma, addr, numa_node_id(), true)
>  #else
> @@ -552,7 +556,8 @@ static inline bool pm_suspended_storage(void)
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  
> -#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> +#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || \
> +     defined(CONFIG_CMA)
>  /* The below functions must be run on a range from a single zone. */
>  extern int alloc_contig_range(unsigned long start, unsigned long end,
>  			      unsigned migratetype);
> -- 
> 1.9.3

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1343200

On 2/25/16 16:57, Michal Hocko wrote:
> On Thu 25-02-16 06:26:31, chengang@emindsoft.com.cn wrote:
>>
>> Always notice about 80 columns, and the white space near '|'.
>>
>> Let the wrapped function parameters align as the same styles.
>>
>> Remove redundant statement "enum zone_type z;" in function gfp_zone.
> 
> I do not think this is an improvement. The comment placement is just odd
> and artificially splitting the mask into more lines makes git grep
> harder to use.
> 

Excuse me, I am not quite sure your meaning is the whole contents of the
patch is worthless, or only for the "comment placement"?

For the "comment placement" the common way is below, but still make git
grep harder:

-#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
+/* ZONE_MOVABLE allowed */
+#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)

Then how about:

-#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
+#define __GFP_MOVABLE	\
		((__force gfp_t)___GFP_MOVABLE) /* ZONE_MOVABLE allowed */

or:

-#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
+#define __GFP_MOVABLE	/* ZONE_MOVABLE allowed */ \
			((__force gfp_t)___GFP_MOVABLE)


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343211

On Thu 25-02-16 22:23:38, Chen Gang wrote:
> On 2/25/16 16:57, Michal Hocko wrote:
> > On Thu 25-02-16 06:26:31, chengang@emindsoft.com.cn wrote:
> >>
> >> Always notice about 80 columns, and the white space near '|'.
> >>
> >> Let the wrapped function parameters align as the same styles.
> >>
> >> Remove redundant statement "enum zone_type z;" in function gfp_zone.
> > 
> > I do not think this is an improvement. The comment placement is just odd
> > and artificially splitting the mask into more lines makes git grep
> > harder to use.
> > 
> 
> Excuse me, I am not quite sure your meaning is the whole contents of the
> patch is worthless, or only for the "comment placement"?
> 
> For the "comment placement" the common way is below, but still make git
> grep harder:

if you did git grep ZONE_MOVABLE you would get less information

> 
> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
> +/* ZONE_MOVABLE allowed */
> +#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)
> 
> Then how about:
> 
> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
> +#define __GFP_MOVABLE	\
> 		((__force gfp_t)___GFP_MOVABLE) /* ZONE_MOVABLE allowed */
> 
> or:
> 
> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
> +#define __GFP_MOVABLE	/* ZONE_MOVABLE allowed */ \
> 			((__force gfp_t)___GFP_MOVABLE)

Now looks worse then other, really. Please try to think what would be
a benefit of such change. As Mel already pointed out git blame would
take an additional step to get back to the patch which has introduced
them. And what is the advantage? Make 80 characters-per-line rule happy?
I just do not think this is worth changes at all.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1343520

On 2/25/16 22:47, Michal Hocko wrote:
>>
>> For the "comment placement" the common way is below, but still make git
>> grep harder:
> 
> if you did git grep ZONE_MOVABLE you would get less information
> 

OK.

>>
>> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
>> +/* ZONE_MOVABLE allowed */
>> +#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)
>>
>> Then how about:
>>
>> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
>> +#define __GFP_MOVABLE	\
>> 		((__force gfp_t)___GFP_MOVABLE) /* ZONE_MOVABLE allowed */
>>
>> or:
>>
>> -#define __GFP_MOVABLE	((__force gfp_t)___GFP_MOVABLE)  /* ZONE_MOVABLE allowed */
>> +#define __GFP_MOVABLE	/* ZONE_MOVABLE allowed */ \
>> 			((__force gfp_t)___GFP_MOVABLE)
> 
> Now looks worse then other, really. Please try to think what would be
> a benefit of such change. As Mel already pointed out git blame would
> take an additional step to get back to the patch which has introduced
> them. And what is the advantage? Make 80 characters-per-line rule happy?
> I just do not think this is worth changes at all.
> 

For 80 column limitation:

 - I often use vsp (vertical split window) in vim to reading code in the
   2 files, 80 columns limitation can avoid the line wrap, which will
   let code reading better.

 - Sometimes we need copy/past the code to a pdf files (e.g. print the
   interface header file contents to a new document as appendix), or
   print the code to a physical paper (e.g. write a book).

For worth or worthless:

  The shared header files (e.g. in our case), have more chances to be
  read or printed than the normal source code files. So for me, we need
  take more care about the coding styles of them.

For git-blame:

 - It really a good feature! Originally, I did not know about it :-).

 - Can it instead of sending trivial patch? (I guess not).


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343024

On Thu, Feb 25, 2016 at 06:26:31AM +0800, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> Always notice about 80 columns, and the white space near '|'.
> 
> Let the wrapped function parameters align as the same styles.
> 
> Remove redundant statement "enum zone_type z;" in function gfp_zone.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

NAK from me at least. From my perspective, it's preferrable to preserve
blame than go through a layer of cleanup when looking for the commit
that defined particular flags. It's ok to cleanup code at the same time
definitions change for functional or performance reasons.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1343207

On 2/25/16 17:27, Mel Gorman wrote:
> On Thu, Feb 25, 2016 at 06:26:31AM +0800, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> Always notice about 80 columns, and the white space near '|'.
>>
>> Let the wrapped function parameters align as the same styles.
>>
>> Remove redundant statement "enum zone_type z;" in function gfp_zone.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> NAK from me at least. From my perspective, it's preferrable to preserve
> blame than go through a layer of cleanup when looking for the commit
> that defined particular flags. It's ok to cleanup code at the same time
> definitions change for functional or performance reasons.
> 

I can understand for your NAK, it is a trivial patch. For me, I guess
trivial@kernel.org will care about this kind of patch.

If we have another better way than sending trivial patch, that will be
OK to me. At present, I am learning mm in my free time, when I feel
something is valuable more or less, I will send related patch for it.

And excuse me, I guess my english is not quite well, I am not quite
understand the meaning below, could you provide more details?

  "it's preferable to preserve blame than go through a layer of cleanup
  when looking for the commit that defined particular flags".

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343263

On Thu, 25 Feb 2016, Chen Gang wrote:

> I can understand for your NAK, it is a trivial patch. 

Not all trivial patches are NAKed :) But they have to be generally useful.

Shuffling code around, without actually changing / improving it a bit, 
just for the sole purpose of formatting, is kind of pointless (especially 
given the fact that the current code as-is is easily readable; it's not 
like it'd be a horrible mess difficult to understand).

Sure, it might had been formatted better at the time it was actually 
merged. But changing it "just because" after being in tree for long time 
doesn't fix any problem really.

> And excuse me, I guess my english is not quite well, I am not quite
> understand the meaning below, could you provide more details?
> 
>   "it's preferable to preserve blame than go through a layer of cleanup
>   when looking for the commit that defined particular flags".

git-blame. When looking at commits touching particular lines, you add an 
extra hop to the person who is trying to find a (functional) commit that 
touched a particular line.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1343522

On 2/25/16 23:12, Jiri Kosina wrote:
> On Thu, 25 Feb 2016, Chen Gang wrote:
> 
>> I can understand for your NAK, it is a trivial patch. 
> 
> Not all trivial patches are NAKed :) But they have to be generally useful.
> 
> Shuffling code around, without actually changing / improving it a bit, 
> just for the sole purpose of formatting, is kind of pointless (especially 
> given the fact that the current code as-is is easily readable; it's not 
> like it'd be a horrible mess difficult to understand).
> 
> Sure, it might had been formatted better at the time it was actually 
> merged. But changing it "just because" after being in tree for long time 
> doesn't fix any problem really.
> 

OK, thanks. I have replied the related contents in the other thread.

Welcome any ideas, suggestions, and completions in the other related
thread.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343289

On Thu, Feb 25, 2016 at 10:38:58PM +0800, Chen Gang wrote:
> On 2/25/16 17:27, Mel Gorman wrote:
> > On Thu, Feb 25, 2016 at 06:26:31AM +0800, chengang@emindsoft.com.cn wrote:
> >> From: Chen Gang <chengang@emindsoft.com.cn>
> >>
> >> Always notice about 80 columns, and the white space near '|'.
> >>
> >> Let the wrapped function parameters align as the same styles.
> >>
> >> Remove redundant statement "enum zone_type z;" in function gfp_zone.
> >>
> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> > 
> > NAK from me at least. From my perspective, it's preferrable to preserve
> > blame than go through a layer of cleanup when looking for the commit
> > that defined particular flags. It's ok to cleanup code at the same time
> > definitions change for functional or performance reasons.
> > 
> 
> I can understand for your NAK, it is a trivial patch. For me, I guess
> trivial@kernel.org will care about this kind of patch.
> 

I do not want this patch to go through the trivial tree. It still adds
another step to identifying relevant commits through git blame and has
limited, if any, benefit to maintainability.

>   "it's preferable to preserve blame than go through a layer of cleanup
>   when looking for the commit that defined particular flags".
> 

git blame identifies what commit last altered a line. If a cleanup patch
is encountered then the tree before that commit needs to be examined
which adds time. It's rare that cleanup patches on their own are useful
and this is one of those cases.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1343528

On 2/26/16 00:07, Mel Gorman wrote:
>>> On Thu, Feb 25, 2016 at 06:26:31AM +0800, chengang@emindsoft.com.cn wrote:
> 
> I do not want this patch to go through the trivial tree. It still adds
> another step to identifying relevant commits through git blame and has
> limited, if any, benefit to maintainability.
> 
>>   "it's preferable to preserve blame than go through a layer of cleanup
>>   when looking for the commit that defined particular flags".
>>
> 
> git blame identifies what commit last altered a line. If a cleanup patch
> is encountered then the tree before that commit needs to be examined
> which adds time. It's rare that cleanup patches on their own are useful
> and this is one of those cases.
> 

git is a tool mainly for analyzing code, but not mainly for normal
reading main code.

So for me, the coding styles need not consider about git.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343538

On Fri, 26 Feb 2016, Chen Gang wrote:

> > git blame identifies what commit last altered a line. If a cleanup patch
> > is encountered then the tree before that commit needs to be examined
> > which adds time. It's rare that cleanup patches on their own are useful
> > and this is one of those cases.
> 
> git is a tool mainly for analyzing code, but not mainly for normal
> reading main code.
> 
> So for me, the coding styles need not consider about git.

You are mistaken here. It's very helpful when debugging; usually you want 
to find the commit that introduced particular change, and read its 
changelog (at least). Having to cross rather pointless changes just adds 
time (need to restart git-blame with commit~1 as a base) for no really 
good reason.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1344318

On 2/26/16 06:39, Jiri Kosina wrote:
> On Fri, 26 Feb 2016, Chen Gang wrote:
> 
>> git is a tool mainly for analyzing code, but not mainly for normal
>> reading main code.
>>
>> So for me, the coding styles need not consider about git.
> 
> You are mistaken here. It's very helpful when debugging;

For me, 'debugging' is related with debugger (e.g. kdb or kgdb), and
'tracing' is related with dumping log, and code analyzing is related
with "git diff" and "git blame".

And yes, for me, "git diff" and "git blame" is really very helpful for
code analyzing.

>                                                         usually you want 
> to find the commit that introduced particular change, and read its 
> changelog (at least). Having to cross rather pointless changes just adds 
> time (need to restart git-blame with commit~1 as a base) for no really 
> good reason.
> 

That is the reason why I am not quite care about body files, I often use
"git log -p filename", the cleanup code patch has negative effect with
code analyzing (although for me, it should still need to be cleanup).

But in our case, it is for the shared header file:

 - They are often the common base file, the main contents will not be
   changed quite often, and their contents are usually simple enough (
   e.g. gfp.h in our case), they are not often for "code analyzing".

 - But they are quite often read in normal reading ways by programmers
   (e.g. open with normal editors). For normal reading, programmers
   usually care about the contents, not the changes.

 - So for me, the common shared header files need always take care about
   coding styles, and need not consider about code analyzing.

And if we reject this kind of patch (in our case), I guess, that almost
mean: "for the common shared header files, their bad coding styles will
be remain for ever".


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343573

[Multipart message — attachments visible in raw view] — view raw

On Fri, 26 Feb 2016, Chen Gang wrote:

> On 2/26/16 00:07, Mel Gorman wrote:
>>>> On Thu, Feb 25, 2016 at 06:26:31AM +0800, chengang@emindsoft.com.cn wrote:
>>
>> I do not want this patch to go through the trivial tree. It still adds
>> another step to identifying relevant commits through git blame and has
>> limited, if any, benefit to maintainability.
>>
>>>   "it's preferable to preserve blame than go through a layer of cleanup
>>>   when looking for the commit that defined particular flags".
>>>
>>
>> git blame identifies what commit last altered a line. If a cleanup patch
>> is encountered then the tree before that commit needs to be examined
>> which adds time. It's rare that cleanup patches on their own are useful
>> and this is one of those cases.
>>
>
> git is a tool mainly for analyzing code, but not mainly for normal
> reading main code.
>
> So for me, the coding styles need not consider about git.


It is common to see reject of trivial coding style fixup patch here and
there.  Those patches usually be merged for early stage files that only
few people read / write.  However, for files that are old and lots of
people read and write, those patches are rejected in usual.  I mean, the
negative opinions for this patches are usual in this community.

I agree that coding style is important and respect your effort.  However,
because the code will be seen and written by most kernel hackers, the file
should be maintained to be easily readable and writable by most kernel
hackers, especially, maintainers.  What I want to say is, we should
respect maintainers' opinion in usual.

As far as I remember, I have seen a document that saying same with others'
opinion but couldn't find it.


Thanks,
SeongJae Park

>
>
> Thanks.
> -- 
> Chen Gang (陈刚)
>
> Managing Natural Environments is the Duty of Human Beings.
>
> --
> 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/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

[toc] | [prev] | [next] | [standalone]

#1344324

On 2/26/16 07:12, SeongJae Park wrote:
> 
> On Fri, 26 Feb 2016, Chen Gang wrote:
> 
>>
>> git is a tool mainly for analyzing code, but not mainly for normal
>> reading main code.
>>
>> So for me, the coding styles need not consider about git.
> 
> 
> It is common to see reject of trivial coding style fixup patch here and
> there.  Those patches usually be merged for early stage files that only
> few people read / write.  However, for files that are old and lots of
> people read and write, those patches are rejected in usual.  I mean, the
> negative opinions for this patches are usual in this community.
> 
> I agree that coding style is important and respect your effort.  However,
> because the code will be seen and written by most kernel hackers, the file
> should be maintained to be easily readable and writable by most kernel
> hackers, especially, maintainers.  What I want to say is, we should
> respect maintainers' opinion in usual.
> 

Yes we need consider about the maintainers' options.

And my another ideas are replied in the other thread, please check, and
welcome any ideas, suggestion, and completions.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1343735

On Fri, Feb 26, 2016 at 6:29 AM, Chen Gang <chengang@emindsoft.com.cn> wrote:
> git is a tool mainly for analyzing code, but not mainly for normal
> reading main code.
>
> So for me, the coding styles need not consider about git.

For you, maybe yes.

But for most of the developers/learners,  git blame does help a lot.
Kernel code was not as complicated as it is now, it is keeping evolving.

So basically a history chain is indispensable in studying such a complex system.
git blame fits in this role.  I benefited a lot from using it when I
started to learn the code,
And,  a pure coding style fix is sometimes really troublesome as I
have to use git blame
to go another step up along the history chain,  which is time
consuming and boring.

But after all, I bet you will be fond of using it if you dive deeper
into the kernel code studying.
And if you do,  you will know why so many developers in this thread
are so upset and allergic
to such coding-style fix.

As for coding style, actually IMHO this patch is even _not_ a coding
style, more like a code shuffle, indeed.

And for your commit history, I found actually you have already
contributed some quit good patches.
I don't think it is helpful for a non-layman contributor to keep
generating such code churn.



Thanks,
Jianyu Zhan

[toc] | [prev] | [next] | [standalone]

#1344342

On 2/26/16 10:32, Jianyu Zhan wrote:
> On Fri, Feb 26, 2016 at 6:29 AM, Chen Gang <chengang@emindsoft.com.cn> wrote:
>> git is a tool mainly for analyzing code, but not mainly for normal
>> reading main code.
>>
>> So for me, the coding styles need not consider about git.
> 
> For you, maybe yes.
> 
> But for most of the developers/learners,  git blame does help a lot.
> Kernel code was not as complicated as it is now, it is keeping evolving.
> 

Yes.

> So basically a history chain is indispensable in studying such a complex system.
> git blame fits in this role.  I benefited a lot from using it when I
> started to learn the code,
> And,  a pure coding style fix is sometimes really troublesome as I
> have to use git blame
> to go another step up along the history chain,  which is time
> consuming and boring.
> 
> But after all, I bet you will be fond of using it if you dive deeper
> into the kernel code studying.
> And if you do,  you will know why so many developers in this thread
> are so upset and allergic
> to such coding-style fix.
> 

For me, for discussion, I don't care about "so many developers", I only
focus on the proof and the contribution.


> As for coding style, actually IMHO this patch is even _not_ a coding
> style, more like a code shuffle, indeed.
> 

"80 column limitation" is about coding style, I guess, all of us agree
with it.

> And for your commit history, I found actually you have already
> contributed some quit good patches.

For me, I don't care about my history -- except some members find issues
related with my original patches, I have duty to analyze the related
issues together with the finders.

> I don't think it is helpful for a non-layman contributor to keep
> generating such code churn.
> 

For me, we are discussing, so it is not quite suitable to make an early
conclusion (code churn).

For me, I don't care about layman or non-layman, I only focus on the
proof and the contribution.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [next] | [standalone]

#1344894

On Fri, Feb 26, 2016 at 11:26:02PM +0800, Chen Gang wrote:
> > As for coding style, actually IMHO this patch is even _not_ a coding
> > style, more like a code shuffle, indeed.
> > 
> 
> "80 column limitation" is about coding style, I guess, all of us agree
> with it.

No, it's been accepted that checkpatch requiring people to reformat
code to within be 80 columns limitation was actively harmful, and it
no longer does that.

Worse, it now complains when you split a printf string across lines,
so there were patches that split a string across multiple lines to
make checkpatch shut up.  And now there are patches that join the
string back together.

And if you now start submitting patches to split them up again because
you think the 80 column restriction is so darned important, that would
be even ***more*** code churn.

Which is one of the reasons why some of us aren't terribly happy with
people who start running checkpatch -file on other people's code and
start submitting patches, either through the trivial patch portal or
not.

Mel, as an MM developer, has already NACK'ed the patch, which means
you should not send the patch to **any** upstream maintainer for
inclusion.

						- Ted
						

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web