Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Cyrill Gorcunov]

On Tue, Oct 23, 2012 at 03:02:59PM -0700, akpm@linux-foundation.org wrote:
> 
> The patch titled
>      Subject: procfs-add-vmflags-field-in-smaps-output-v3-fix-2
> has been added to the -mm tree.  Its filename is
>      procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch
> 
> ------------------------------------------------------
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: procfs-add-vmflags-field-in-smaps-output-v3-fix-2
> 
> Use designated init to assign "??" mnemonic on unknown flags.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Hi Andrew, could you stack the patch below on top, this one
should be a final for sure ;-)
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: procfs-add-vmflags-field-in-smaps-output-v3-fix-on-top-2

Make assignment more readable [by peterz@] and fix bit
tests (we need unsigned long here explicitly stated).

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/proc/task_mmu.c |   63 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================
--- linux-2.6.git.orig/fs/proc/task_mmu.c
+++ linux-2.6.git/fs/proc/task_mmu.c
@@ -482,6 +482,8 @@ static int smaps_pte_range(pmd_t *pmd, u
 
 static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
 {
+#define __VM_FLAG(_f, _s)	[ilog2(_f)] = {(const char [2]){_s}}
+
 	/*
 	 * Don't forget to update Documentation/ on changes.
 	 */
@@ -491,46 +493,49 @@ static void show_smap_vma_flags(struct s
 		/*
 		 * In case if we meet a flag we don't know about.
 		 */
-		[0 ... (BITS_PER_LONG-1)] = { {'?', '?'} },
+		[0 ... (BITS_PER_LONG-1)] = { (const char [2]){"??"} },
 
-		[ilog2(VM_READ)]	= { {'r', 'd'} },
-		[ilog2(VM_WRITE)]	= { {'w', 'r'} },
-		[ilog2(VM_EXEC)]	= { {'e', 'x'} },
-		[ilog2(VM_SHARED)]	= { {'s', 'h'} },
-		[ilog2(VM_MAYREAD)]	= { {'m', 'r'} },
-		[ilog2(VM_MAYWRITE)]	= { {'m', 'w'} },
-		[ilog2(VM_MAYEXEC)]	= { {'m', 'e'} },
-		[ilog2(VM_MAYSHARE)]	= { {'m', 's'} },
-		[ilog2(VM_GROWSDOWN)]	= { {'g', 'd'} },
-		[ilog2(VM_PFNMAP)]	= { {'p', 'f'} },
-		[ilog2(VM_DENYWRITE)]	= { {'d', 'w'} },
-		[ilog2(VM_LOCKED)]	= { {'l', 'o'} },
-		[ilog2(VM_IO)]		= { {'i', 'o'} },
-		[ilog2(VM_SEQ_READ)]	= { {'s', 'r'} },
-		[ilog2(VM_RAND_READ)]	= { {'r', 'r'} },
-		[ilog2(VM_DONTCOPY)]	= { {'d', 'c'} },
-		[ilog2(VM_DONTEXPAND)]	= { {'d', 'e'} },
-		[ilog2(VM_ACCOUNT)]	= { {'a', 'c'} },
-		[ilog2(VM_NORESERVE)]	= { {'n', 'r'} },
-		[ilog2(VM_HUGETLB)]	= { {'h', 't'} },
-		[ilog2(VM_NONLINEAR)]	= { {'n', 'l'} },
-		[ilog2(VM_ARCH_1)]	= { {'a', 'r'} },
-		[ilog2(VM_DONTDUMP)]	= { {'d', 'd'} },
-		[ilog2(VM_MIXEDMAP)]	= { {'m', 'm'} },
-		[ilog2(VM_HUGEPAGE)]	= { {'h', 'g'} },
-		[ilog2(VM_NOHUGEPAGE)]	= { {'n', 'h'} },
-		[ilog2(VM_MERGEABLE)]	= { {'m', 'g'} },
+		__VM_FLAG(VM_READ,	"rd"),
+		__VM_FLAG(VM_WRITE,	"wr"),
+		__VM_FLAG(VM_EXEC,	"ex"),
+		__VM_FLAG(VM_SHARED,	"sh"),
+		__VM_FLAG(VM_MAYREAD,	"mr"),
+		__VM_FLAG(VM_MAYWRITE,	"mw"),
+		__VM_FLAG(VM_MAYEXEC,	"me"),
+		__VM_FLAG(VM_MAYSHARE,	"ms"),
+		__VM_FLAG(VM_GROWSDOWN,	"gd"),
+		__VM_FLAG(VM_PFNMAP,	"pf"),
+		__VM_FLAG(VM_DENYWRITE,	"dw"),
+		__VM_FLAG(VM_LOCKED,	"lo"),
+		__VM_FLAG(VM_IO,	"io"),
+		__VM_FLAG(VM_SEQ_READ,	"sr"),
+		__VM_FLAG(VM_RAND_READ,	"rr"),
+		__VM_FLAG(VM_DONTCOPY,	"dc"),
+		__VM_FLAG(VM_DONTEXPAND,"de"),
+		__VM_FLAG(VM_ACCOUNT,	"ac"),
+		__VM_FLAG(VM_NORESERVE,	"nr"),
+		__VM_FLAG(VM_HUGETLB,	"ht"),
+		__VM_FLAG(VM_NONLINEAR,	"nl"),
+		__VM_FLAG(VM_ARCH_1,	"ar"),
+		__VM_FLAG(VM_DONTDUMP,	"dd"),
+		__VM_FLAG(VM_MIXEDMAP,	"mm"),
+		__VM_FLAG(VM_HUGEPAGE,	"hg"),
+		__VM_FLAG(VM_NOHUGEPAGE,"nh"),
+		__VM_FLAG(VM_MERGEABLE,	"mg"),
 	};
 	size_t i;
 
 	seq_puts(m, "VmFlags: ");
 	for (i = 0; i < BITS_PER_LONG; i++) {
-		if (vma->vm_flags & (1 << i))
+		if (vma->vm_flags & (1ul << i)) {
 			seq_printf(m, "%c%c ",
 				   mnemonics[i].l[0],
 				   mnemonics[i].l[1]);
+		}
 	}
 	seq_putc(m, '\n');
+
+#undef __VM_FLAG
 }
 
 static int show_smap(struct seq_file *m, void *v, int is_pid)

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Peter Zijlstra]

On Wed, 2012-10-24 at 12:45 +0400, Cyrill Gorcunov wrote:
>         for (i = 0; i < BITS_PER_LONG; i++) {
> -               if (vma->vm_flags & (1 << i))
> +               if (vma->vm_flags & (1ul << i)) {

	for_each_set_bit(i, &vma->vm_flags, BITS_PER_LONG) {

>                         seq_printf(m, "%c%c ",
>                                    mnemonics[i].l[0],
>                                    mnemonics[i].l[1]);
> +               }
>         } 

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Stephen Rothwell]

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

Hi Cyrill,

On Wed, 24 Oct 2012 12:45:15 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>
>  static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
>  {
> +#define __VM_FLAG(_f, _s)	[ilog2(_f)] = {(const char [2]){_s}}

I really don't think you need the cast (and it may hide bad usages) or
the second set of braces.  Thus:

#define __VM_FLAG(_f, _s)	[ilog2(_f)] = {_s}

> +
>  	/*
>  	 * Don't forget to update Documentation/ on changes.
>  	 */
> @@ -491,46 +493,49 @@ static void show_smap_vma_flags(struct s
>  		/*
>  		 * In case if we meet a flag we don't know about.
>  		 */
> -		[0 ... (BITS_PER_LONG-1)] = { {'?', '?'} },
> +		[0 ... (BITS_PER_LONG-1)] = { (const char [2]){"??"} },

And here.

Further is there any reason for the struct?

#define __VM_FLAG(_f, _s)	[ilog2(_f)] = _s

static const char mnemonics[BITS_PER_LONG][2] = {
...
};

>  	seq_puts(m, "VmFlags: ");
>  	for (i = 0; i < BITS_PER_LONG; i++) {
> -		if (vma->vm_flags & (1 << i))
> +		if (vma->vm_flags & (1ul << i)) {
>  			seq_printf(m, "%c%c ",
>  				   mnemonics[i].l[0],
>  				   mnemonics[i].l[1]);

				   mnemonics[i][0], mnemonics[i][1]

/me looks for another bike shed :-)
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Cyrill Gorcunov]

On Wed, Oct 24, 2012 at 11:24:04AM +0200, Peter Zijlstra wrote:
> On Wed, 2012-10-24 at 12:45 +0400, Cyrill Gorcunov wrote:
> >         for (i = 0; i < BITS_PER_LONG; i++) {
> > -               if (vma->vm_flags & (1 << i))
> > +               if (vma->vm_flags & (1ul << i)) {
> 
> 	for_each_set_bit(i, &vma->vm_flags, BITS_PER_LONG) {

Yeah, thanks (for some reason forgot that we have such helper)
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: procfs-add-vmflags-field-in-smaps-output-v3-fix-on-top-3

[by a.p.zijlstra@] make assignment more readable and use for_each_set_bit

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/proc/task_mmu.c |   69 +++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================
--- linux-2.6.git.orig/fs/proc/task_mmu.c
+++ linux-2.6.git/fs/proc/task_mmu.c
@@ -482,6 +482,8 @@ static int smaps_pte_range(pmd_t *pmd, u
 
 static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
 {
+#define __VM_FLAG(_f, _s)	[ilog2(_f)] = {(const char [2]){_s}}
+
 	/*
 	 * Don't forget to update Documentation/ on changes.
 	 */
@@ -491,46 +493,47 @@ static void show_smap_vma_flags(struct s
 		/*
 		 * In case if we meet a flag we don't know about.
 		 */
-		[0 ... (BITS_PER_LONG-1)] = { {'?', '?'} },
+		[0 ... (BITS_PER_LONG-1)] = { (const char [2]){"??"} },
 
-		[ilog2(VM_READ)]	= { {'r', 'd'} },
-		[ilog2(VM_WRITE)]	= { {'w', 'r'} },
-		[ilog2(VM_EXEC)]	= { {'e', 'x'} },
-		[ilog2(VM_SHARED)]	= { {'s', 'h'} },
-		[ilog2(VM_MAYREAD)]	= { {'m', 'r'} },
-		[ilog2(VM_MAYWRITE)]	= { {'m', 'w'} },
-		[ilog2(VM_MAYEXEC)]	= { {'m', 'e'} },
-		[ilog2(VM_MAYSHARE)]	= { {'m', 's'} },
-		[ilog2(VM_GROWSDOWN)]	= { {'g', 'd'} },
-		[ilog2(VM_PFNMAP)]	= { {'p', 'f'} },
-		[ilog2(VM_DENYWRITE)]	= { {'d', 'w'} },
-		[ilog2(VM_LOCKED)]	= { {'l', 'o'} },
-		[ilog2(VM_IO)]		= { {'i', 'o'} },
-		[ilog2(VM_SEQ_READ)]	= { {'s', 'r'} },
-		[ilog2(VM_RAND_READ)]	= { {'r', 'r'} },
-		[ilog2(VM_DONTCOPY)]	= { {'d', 'c'} },
-		[ilog2(VM_DONTEXPAND)]	= { {'d', 'e'} },
-		[ilog2(VM_ACCOUNT)]	= { {'a', 'c'} },
-		[ilog2(VM_NORESERVE)]	= { {'n', 'r'} },
-		[ilog2(VM_HUGETLB)]	= { {'h', 't'} },
-		[ilog2(VM_NONLINEAR)]	= { {'n', 'l'} },
-		[ilog2(VM_ARCH_1)]	= { {'a', 'r'} },
-		[ilog2(VM_DONTDUMP)]	= { {'d', 'd'} },
-		[ilog2(VM_MIXEDMAP)]	= { {'m', 'm'} },
-		[ilog2(VM_HUGEPAGE)]	= { {'h', 'g'} },
-		[ilog2(VM_NOHUGEPAGE)]	= { {'n', 'h'} },
-		[ilog2(VM_MERGEABLE)]	= { {'m', 'g'} },
+		__VM_FLAG(VM_READ,	"rd"),
+		__VM_FLAG(VM_WRITE,	"wr"),
+		__VM_FLAG(VM_EXEC,	"ex"),
+		__VM_FLAG(VM_SHARED,	"sh"),
+		__VM_FLAG(VM_MAYREAD,	"mr"),
+		__VM_FLAG(VM_MAYWRITE,	"mw"),
+		__VM_FLAG(VM_MAYEXEC,	"me"),
+		__VM_FLAG(VM_MAYSHARE,	"ms"),
+		__VM_FLAG(VM_GROWSDOWN,	"gd"),
+		__VM_FLAG(VM_PFNMAP,	"pf"),
+		__VM_FLAG(VM_DENYWRITE,	"dw"),
+		__VM_FLAG(VM_LOCKED,	"lo"),
+		__VM_FLAG(VM_IO,	"io"),
+		__VM_FLAG(VM_SEQ_READ,	"sr"),
+		__VM_FLAG(VM_RAND_READ,	"rr"),
+		__VM_FLAG(VM_DONTCOPY,	"dc"),
+		__VM_FLAG(VM_DONTEXPAND,"de"),
+		__VM_FLAG(VM_ACCOUNT,	"ac"),
+		__VM_FLAG(VM_NORESERVE,	"nr"),
+		__VM_FLAG(VM_HUGETLB,	"ht"),
+		__VM_FLAG(VM_NONLINEAR,	"nl"),
+		__VM_FLAG(VM_ARCH_1,	"ar"),
+		__VM_FLAG(VM_DONTDUMP,	"dd"),
+		__VM_FLAG(VM_MIXEDMAP,	"mm"),
+		__VM_FLAG(VM_HUGEPAGE,	"hg"),
+		__VM_FLAG(VM_NOHUGEPAGE,"nh"),
+		__VM_FLAG(VM_MERGEABLE,	"mg"),
 	};
 	size_t i;
 
 	seq_puts(m, "VmFlags: ");
-	for (i = 0; i < BITS_PER_LONG; i++) {
-		if (vma->vm_flags & (1 << i))
-			seq_printf(m, "%c%c ",
-				   mnemonics[i].l[0],
-				   mnemonics[i].l[1]);
+	for_each_set_bit(i, &vma->vm_flags, BITS_PER_LONG) {
+		seq_printf(m, "%c%c ",
+			   mnemonics[i].l[0],
+			   mnemonics[i].l[1]);
 	}
 	seq_putc(m, '\n');
+
+#undef __VM_FLAG
 }
 
 static int show_smap(struct seq_file *m, void *v, int is_pid)

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Cyrill Gorcunov]

On Wed, Oct 24, 2012 at 08:47:46PM +1100, Stephen Rothwell wrote:
> Hi Cyrill,
> 
> On Wed, 24 Oct 2012 12:45:15 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> >
> >  static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> >  {
> > +#define __VM_FLAG(_f, _s)	[ilog2(_f)] = {(const char [2]){_s}}
> 
> I really don't think you need the cast (and it may hide bad usages) or
> the second set of braces.  Thus:

Hi Stephen, could you please elaborate, which bad usage hiding could be there?

> Further is there any reason for the struct?
> 
> #define __VM_FLAG(_f, _s)	[ilog2(_f)] = _s
> 
> static const char mnemonics[BITS_PER_LONG][2] = {
> ...
> };

Well, good point, though the benefit of using stucture here
could be easier way for extension if needed. The compiled result
is the same as for plain array or structure, so I would rather
stick with struct here, until contrary proved (I mean it's not
a problem to update the patch and use array here but I still
think struct it better in long term). If you guys think that I
should move it to array -- no problem, i may update ;)

> >  	seq_puts(m, "VmFlags: ");
> >  	for (i = 0; i < BITS_PER_LONG; i++) {
> > -		if (vma->vm_flags & (1 << i))
> > +		if (vma->vm_flags & (1ul << i)) {
> >  			seq_printf(m, "%c%c ",
> >  				   mnemonics[i].l[0],
> >  				   mnemonics[i].l[1]);
> 
> 				   mnemonics[i][0], mnemonics[i][1]
> 
> /me looks for another bike shed :-)

Heh :)

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Stephen Rothwell]

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

Hi Cyrill,

On Wed, 24 Oct 2012 13:59:59 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>
> On Wed, Oct 24, 2012 at 08:47:46PM +1100, Stephen Rothwell wrote:
> > 
> > On Wed, 24 Oct 2012 12:45:15 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > >
> > >  static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> > >  {
> > > +#define __VM_FLAG(_f, _s)	[ilog2(_f)] = {(const char [2]){_s}}
> > 
> > I really don't think you need the cast (and it may hide bad usages) or
> > the second set of braces.  Thus:
> 
> Hi Stephen, could you please elaborate, which bad usage hiding could be there?

I though it might suppress a warning if someone added an entry with a >2
character string, but testing shows that it does not, so that is OK.
There is still no need for the cast, though, or the second set of
braces.  The C standard allows fixed length char arrays to be initialised
by string constants and discards the trailing NUL if necessary.

> > Further is there any reason for the struct?
> > 
> > #define __VM_FLAG(_f, _s)	[ilog2(_f)] = _s
> > 
> > static const char mnemonics[BITS_PER_LONG][2] = {
> > ...
> > };
> 
> Well, good point, though the benefit of using stucture here
> could be easier way for extension if needed. The compiled result
> is the same as for plain array or structure, so I would rather
> stick with struct here, until contrary proved (I mean it's not
> a problem to update the patch and use array here but I still
> think struct it better in long term). If you guys think that I
> should move it to array -- no problem, i may update ;)

One thing we learn about early (especially working at IBM) is to resist
the temptation to over engineer :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: + procfs-add-vmflags-field-in-smaps-output-v3-fix-2.patch added to -mm tree

[Cyrill Gorcunov]

On Wed, Oct 24, 2012 at 09:26:20PM +1100, Stephen Rothwell wrote:
> > Hi Stephen, could you please elaborate, which bad usage hiding could be there?
> 
> I though it might suppress a warning if someone added an entry with a >2
> character string, but testing shows that it does not, so that is OK.
> There is still no need for the cast, though, or the second set of
> braces.  The C standard allows fixed length char arrays to be initialised
> by string constants and discards the trailing NUL if necessary.

OK, fair enough. I'll post a new patch with all concerns fixed in one
pass. Thanks all for comments!