Re: [PATCH] perf trace beautify: Beautify flags of mount(2) and umount(2).

[Arnaldo Carvalho de Melo]

Em Wed, Oct 10, 2018 at 09:55:06AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Oct 09, 2018 at 08:52:26PM -0700, Benjamin Peterson escreveu:
> > Hi Arnaldo,
> > Did you get a chance to look at this again?
> 
> Thanks, for pinging me about it, will check.

So, this is already upstream, I'll check the other bit.

commit 3de3e8bbf302545ef9acebb9f900939ac5c3820f
Author: Benjamin Peterson <benjamin@xxxxxxxxxx>
Date:   Mon Aug 27 20:53:44 2018 -0700

    perf trace beauty: Alias 'umount' to 'umount2'
    
    Before:
    
      # perf trace -e *mount* umount /dev/mapper/fedora-home /s
        11.576 ( 0.004 ms) umount/3138 umount2(arg0: 94501956754656, arg1: 0, arg2: 1, arg3: 140051050083104, arg4: 4, arg5: 94501956755136) = -1 EINVAL Invalid argument
      #
    
    After:
    
      # perf trace -e *mount* umount /s
         0.000 ( 9.241 ms): umount/5251 umount2(name: 0x55f74a986480) = 0
    
    Signed-off-by: Benjamin Peterson <benjamin@xxxxxxxxxx>
    Tested-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
    Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
    Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
    Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
    Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
    Link: http://lkml.kernel.org/r/20180828035344.31500-1-benjamin@xxxxxxxxxx
    [ split from a larger patch ]
    Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 97ace635bed8..c106189f4066 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -825,6 +825,7 @@ static struct syscall_fmt {
 	  .arg = { [2] = { .scnprintf = SCA_SIGNUM, /* sig */ }, }, },
 	{ .name	    = "tkill",
 	  .arg = { [1] = { .scnprintf = SCA_SIGNUM, /* sig */ }, }, },
+	{ .name     = "umount2", .alias = "umount", },
 	{ .name	    = "uname", .alias = "newuname", },
 	{ .name	    = "unlinkat",
 	  .arg = { [0] = { .scnprintf = SCA_FDAT, /* dfd */ }, }, },
 
> - Arnaldo
>  
> > On Thu, Aug 30, 2018, at 14:50, Benjamin Peterson wrote:
> > > Thanks for the review.
> > > 
> > > On Thu, Aug 30, 2018, at 11:28, Arnaldo Carvalho de Melo wrote:
> > > > Em Mon, Aug 27, 2018 at 08:53:44PM -0700, Benjamin Peterson escreveu:
> > > > > Example output of perf trace -e mount,umount2:
> > > > > 
> > > > >   6243.930 ( 0.052 ms): mount/30976 mount(dev_name: 0x55dc541bb920, dir_name: 0x55dc541bc960, type: 0x55dc541b9c40, flags: BIND) = 0
> > > > >   7851.821 (26.015 ms): umount/30983 umount2(name: 0x558daa82cf50, flags: MNT_FORCE                        ) = 0
> > > > > 
> > > > > Signed-off-by: Benjamin Peterson <benjamin@xxxxxxxxxx>
> > > > 
> > > > So, I tried:
> > > > 
> > > > Before your patch:
> > > > 
> > > >   # perf trace -e *mount* mount /dev/mapper/fedora-home /s
> > > >      0.000 ( 4.335 ms): mount/3126 mount(dev_name: 0x55deecc4d480, 
> > > > dir_name: 0x55deecc4d4a0, type: 0x55deecc53880, flags: 3236757504) = 0
> > > >   #
> > > >   # perf trace -e *mount* umount /dev/mapper/fedora-home /s
> > > >          ? (         ): umount/3138  ... [continued]: umount2()) = 0
> > > >     11.576 (umount:  0.004 ms): /s: not mounted.umount/
> > > > 3138 umount2(arg0: 94501956754656, arg1: 0, arg2: 1, arg3: 
> > > > 140051050083104, arg4: 4, arg5: 94501956755136) = -1 EINVAL Invalid 
> > > > argument
> > > >   #
> > > > 
> > > > After:
> > > > 
> > > > [root@jouet ~]# perf trace -e *mount* mount /dev/mapper/fedora-home /s
> > > >      0.000 ( 1.213 ms): mount/5244 mount(dev_name: 0x5558c1169480, 
> > > > dir_name: 0x5558c11694a0, type: 0x5558c116f880, flags: ) = 0
> > > > [root@jouet ~]# perf trace -e *mount* umount /s
> > > >      0.000 ( 9.241 ms): umount/5251 umount2(name: 
> > > > 0x55f74a986480                                          ) = 0
> > > > 
> > > > The flags for mount got empty? Can you pleaes check that? I.e. using the
> > > > default mount options, not specifying a 'bind' mount like you did.
> > > 
> > > That comes from these lines in syscall_arg__scnprintf_mount_flags:
> > > 
> > > if ((flags & MS_MGC_MSK) == MS_MGC_VAL)
> > > 	flags &= ~MS_MGC_MSK;
> > > 
> > > The reason for this is explained in the comment for do_mount in fs/namespace.c:
> > > 
> > > /*
> > >  * Pre-0.97 versions of mount() didn't have a flags word.
> > >  * When the flags word was introduced its top half was required
> > >  * to have the magic value 0xC0ED, and this remained so until 2.4.0-test9.
> > >  * Therefore, if this magic number is present, it carries no information
> > >  * and must be discarded.
> > > */
> > > 
> > > (I used a bind mount for my commit message example because a "default" 
> > > mount() doesn't use any flags.)
> > > 
> > > > 
> > > > I'm splitting this patch so that the aliasing of 'umount2' to 'umount'
> > > > gets merged now,
> > > > 
> > > > Check my perf/core branch at:
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> > > 
> > > LGTM. Thanks
> > >