[PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table

[PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table

[Arnaldo Carvalho de Melo]

Namhyung,

	Can you take a look and perhaps give me your Acked-by?

- Arnaldo

From: Arnaldo Carvalho de Melo <acme@redhat.com>

No need to traverse it all again using the filename extension to
disambiguate kernel from kernel modules, just cache it when
reading the buildid table.

This also fixes a refcount bug in the error path, i.e. in the error path
for the __machine__create_kernel_maps() call we do a dso__put(), which
is ok and expected if that dso came from machine__findnew_dso(), but
wasn't when we found it by traversing the machine dso list without
grabbing a dso refcount (dso__get()) before dropping the list lock.

Fixes: b837a8bdc489 ("perf tools: Fix build-id matching on vmlinux")
Link: http://lkml.kernel.org/n/tip-48fh3cf6pvs4zs2fj4nhc4b5@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c  |  7 +++++++
 tools/perf/util/machine.c | 34 +++-------------------------------
 tools/perf/util/machine.h |  1 +
 3 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 43838003c1a1..6bc92ae1e99a 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1260,6 +1260,13 @@ static int __event_process_build_id(struct build_id_event *bev,
 
 		if (!is_kernel_module(filename, cpumode))
 			dso->kernel = dso_type;
+		/*
+		 * We don't need to grab a reference as long as
+		 * the kernel dso is in the machine dso list.
+		 */
+		if (cpumode == PERF_RECORD_MISC_KERNEL ||
+		    cpumode == PERF_RECORD_MISC_GUEST_KERNEL)
+			machine->kernel_dso = dso;
 
 		build_id__sprintf(dso->build_id, sizeof(dso->build_id),
 				  sbuild_id);
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d07a38678e14..f6e689b2c83c 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -62,6 +62,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
 	}
 
 	machine->current_tid = NULL;
+	machine->kernel_dso = NULL;
 
 	return 0;
 }
@@ -1180,39 +1181,10 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
 		 * Should be there already, from the build-id table in
 		 * the header.
 		 */
-		struct dso *kernel = NULL;
-		struct dso *dso;
+		struct dso *kernel;
 
 		pthread_rwlock_rdlock(&machine->dsos.lock);
-
-		list_for_each_entry(dso, &machine->dsos.head, node) {
-
-			/*
-			 * The cpumode passed to is_kernel_module is not the
-			 * cpumode of *this* event. If we insist on passing
-			 * correct cpumode to is_kernel_module, we should
-			 * record the cpumode when we adding this dso to the
-			 * linked list.
-			 *
-			 * However we don't really need passing correct
-			 * cpumode.  We know the correct cpumode must be kernel
-			 * mode (if not, we should not link it onto kernel_dsos
-			 * list).
-			 *
-			 * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN.
-			 * is_kernel_module() treats it as a kernel cpumode.
-			 */
-
-			if (!dso->kernel ||
-			    is_kernel_module(dso->long_name,
-					     PERF_RECORD_MISC_CPUMODE_UNKNOWN))
-				continue;
-
-
-			kernel = dso;
-			break;
-		}
-
+		kernel = dso__get(machine->kernel_dso);
 		pthread_rwlock_unlock(&machine->dsos.lock);
 
 		if (kernel == NULL)
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index 9dfc4281f940..d7b044a1046f 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -38,6 +38,7 @@ struct machine {
 	struct dsos	  dsos;
 	struct map_groups kmaps;
 	struct map	  *vmlinux_maps[MAP__NR_TYPES];
+	struct dso	  *kernel_dso;
 	u64		  kernel_start;
 	symbol_filter_t	  symbol_filter;
 	pid_t		  *current_tid;
-- 
2.1.0

Re: [PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table

[Namhyung Kim]

Hi Arnaldo,

On Thu, Sep 24, 2015 at 10:42:28AM -0300, Arnaldo Carvalho de Melo wrote:
> Namhyung,
> 
> 	Can you take a look and perhaps give me your Acked-by?

The dso->kernel and event->cpumode is always confusing for modules..
In this case, it seems that mmap events set kernel cpumode but
build-id events don't.  So kernel cpumode in a bulid-id event
indicates that it is a kernel (vmlinux) dso, right?

I'll test this tomorrow..

Thanks,
Namhyung


> 
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> No need to traverse it all again using the filename extension to
> disambiguate kernel from kernel modules, just cache it when
> reading the buildid table.
> 
> This also fixes a refcount bug in the error path, i.e. in the error path
> for the __machine__create_kernel_maps() call we do a dso__put(), which
> is ok and expected if that dso came from machine__findnew_dso(), but
> wasn't when we found it by traversing the machine dso list without
> grabbing a dso refcount (dso__get()) before dropping the list lock.
> 
> Fixes: b837a8bdc489 ("perf tools: Fix build-id matching on vmlinux")
> Link: http://lkml.kernel.org/n/tip-48fh3cf6pvs4zs2fj4nhc4b5@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/header.c  |  7 +++++++
>  tools/perf/util/machine.c | 34 +++-------------------------------
>  tools/perf/util/machine.h |  1 +
>  3 files changed, 11 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 43838003c1a1..6bc92ae1e99a 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1260,6 +1260,13 @@ static int __event_process_build_id(struct build_id_event *bev,
>  
>  		if (!is_kernel_module(filename, cpumode))
>  			dso->kernel = dso_type;
> +		/*
> +		 * We don't need to grab a reference as long as
> +		 * the kernel dso is in the machine dso list.
> +		 */
> +		if (cpumode == PERF_RECORD_MISC_KERNEL ||
> +		    cpumode == PERF_RECORD_MISC_GUEST_KERNEL)
> +			machine->kernel_dso = dso;
>  
>  		build_id__sprintf(dso->build_id, sizeof(dso->build_id),
>  				  sbuild_id);
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index d07a38678e14..f6e689b2c83c 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -62,6 +62,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
>  	}
>  
>  	machine->current_tid = NULL;
> +	machine->kernel_dso = NULL;
>  
>  	return 0;
>  }
> @@ -1180,39 +1181,10 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
>  		 * Should be there already, from the build-id table in
>  		 * the header.
>  		 */
> -		struct dso *kernel = NULL;
> -		struct dso *dso;
> +		struct dso *kernel;
>  
>  		pthread_rwlock_rdlock(&machine->dsos.lock);
> -
> -		list_for_each_entry(dso, &machine->dsos.head, node) {
> -
> -			/*
> -			 * The cpumode passed to is_kernel_module is not the
> -			 * cpumode of *this* event. If we insist on passing
> -			 * correct cpumode to is_kernel_module, we should
> -			 * record the cpumode when we adding this dso to the
> -			 * linked list.
> -			 *
> -			 * However we don't really need passing correct
> -			 * cpumode.  We know the correct cpumode must be kernel
> -			 * mode (if not, we should not link it onto kernel_dsos
> -			 * list).
> -			 *
> -			 * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN.
> -			 * is_kernel_module() treats it as a kernel cpumode.
> -			 */
> -
> -			if (!dso->kernel ||
> -			    is_kernel_module(dso->long_name,
> -					     PERF_RECORD_MISC_CPUMODE_UNKNOWN))
> -				continue;
> -
> -
> -			kernel = dso;
> -			break;
> -		}
> -
> +		kernel = dso__get(machine->kernel_dso);
>  		pthread_rwlock_unlock(&machine->dsos.lock);
>  
>  		if (kernel == NULL)
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index 9dfc4281f940..d7b044a1046f 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -38,6 +38,7 @@ struct machine {
>  	struct dsos	  dsos;
>  	struct map_groups kmaps;
>  	struct map	  *vmlinux_maps[MAP__NR_TYPES];
> +	struct dso	  *kernel_dso;
>  	u64		  kernel_start;
>  	symbol_filter_t	  symbol_filter;
>  	pid_t		  *current_tid;
> -- 
> 2.1.0
> 

Re: [PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table

[Arnaldo Carvalho de Melo]

Em Fri, Sep 25, 2015 at 12:01:12AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Thu, Sep 24, 2015 at 10:42:28AM -0300, Arnaldo Carvalho de Melo wrote:
> > Namhyung,
> > 
> > 	Can you take a look and perhaps give me your Acked-by?
> 
> The dso->kernel and event->cpumode is always confusing for modules..

Right, my idea is to delete dso->kernel, to remove this confusion.

Please see my tmp.perf/core branch, there are a few patches there using
alternative methods in places previously using the botched dso->kernel.

> In this case, it seems that mmap events set kernel cpumode but
> build-id events don't.

in the previous sentence you think build-id events don't set
perf_event_attr.misc bits related to cpumode...

> So kernel cpumode in a bulid-id event
> indicates that it is a kernel (vmlinux) dso, right?

While here you ask if it does?

All these are synthesized, it is a matter of looking at the routines
synthesizing them :-)

> I'll test this tomorrow..

Thanks, over time there were areas where multiple people touched and
added different semantics that are biting now, I'm trying to, while
fixing a bug, the one Wang Nan reported, clarify those things.

It is taking more time than I antecipated, as I'm stumbling in what look
like other bugs in the perf-probe codebase :-/

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> > 
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > No need to traverse it all again using the filename extension to
> > disambiguate kernel from kernel modules, just cache it when
> > reading the buildid table.
> > 
> > This also fixes a refcount bug in the error path, i.e. in the error path
> > for the __machine__create_kernel_maps() call we do a dso__put(), which
> > is ok and expected if that dso came from machine__findnew_dso(), but
> > wasn't when we found it by traversing the machine dso list without
> > grabbing a dso refcount (dso__get()) before dropping the list lock.
> > 
> > Fixes: b837a8bdc489 ("perf tools: Fix build-id matching on vmlinux")
> > Link: http://lkml.kernel.org/n/tip-48fh3cf6pvs4zs2fj4nhc4b5@git.kernel.org
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/util/header.c  |  7 +++++++
> >  tools/perf/util/machine.c | 34 +++-------------------------------
> >  tools/perf/util/machine.h |  1 +
> >  3 files changed, 11 insertions(+), 31 deletions(-)
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 43838003c1a1..6bc92ae1e99a 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -1260,6 +1260,13 @@ static int __event_process_build_id(struct build_id_event *bev,
> >  
> >  		if (!is_kernel_module(filename, cpumode))
> >  			dso->kernel = dso_type;
> > +		/*
> > +		 * We don't need to grab a reference as long as
> > +		 * the kernel dso is in the machine dso list.
> > +		 */
> > +		if (cpumode == PERF_RECORD_MISC_KERNEL ||
> > +		    cpumode == PERF_RECORD_MISC_GUEST_KERNEL)
> > +			machine->kernel_dso = dso;
> >  
> >  		build_id__sprintf(dso->build_id, sizeof(dso->build_id),
> >  				  sbuild_id);
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index d07a38678e14..f6e689b2c83c 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -62,6 +62,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
> >  	}
> >  
> >  	machine->current_tid = NULL;
> > +	machine->kernel_dso = NULL;
> >  
> >  	return 0;
> >  }
> > @@ -1180,39 +1181,10 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
> >  		 * Should be there already, from the build-id table in
> >  		 * the header.
> >  		 */
> > -		struct dso *kernel = NULL;
> > -		struct dso *dso;
> > +		struct dso *kernel;
> >  
> >  		pthread_rwlock_rdlock(&machine->dsos.lock);
> > -
> > -		list_for_each_entry(dso, &machine->dsos.head, node) {
> > -
> > -			/*
> > -			 * The cpumode passed to is_kernel_module is not the
> > -			 * cpumode of *this* event. If we insist on passing
> > -			 * correct cpumode to is_kernel_module, we should
> > -			 * record the cpumode when we adding this dso to the
> > -			 * linked list.
> > -			 *
> > -			 * However we don't really need passing correct
> > -			 * cpumode.  We know the correct cpumode must be kernel
> > -			 * mode (if not, we should not link it onto kernel_dsos
> > -			 * list).
> > -			 *
> > -			 * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN.
> > -			 * is_kernel_module() treats it as a kernel cpumode.
> > -			 */
> > -
> > -			if (!dso->kernel ||
> > -			    is_kernel_module(dso->long_name,
> > -					     PERF_RECORD_MISC_CPUMODE_UNKNOWN))
> > -				continue;
> > -
> > -
> > -			kernel = dso;
> > -			break;
> > -		}
> > -
> > +		kernel = dso__get(machine->kernel_dso);
> >  		pthread_rwlock_unlock(&machine->dsos.lock);
> >  
> >  		if (kernel == NULL)
> > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> > index 9dfc4281f940..d7b044a1046f 100644
> > --- a/tools/perf/util/machine.h
> > +++ b/tools/perf/util/machine.h
> > @@ -38,6 +38,7 @@ struct machine {
> >  	struct dsos	  dsos;
> >  	struct map_groups kmaps;
> >  	struct map	  *vmlinux_maps[MAP__NR_TYPES];
> > +	struct dso	  *kernel_dso;
> >  	u64		  kernel_start;
> >  	symbol_filter_t	  symbol_filter;
> >  	pid_t		  *current_tid;
> > -- 
> > 2.1.0
> > 

Re: [PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table

[Namhyung Kim]

On Thu, Sep 24, 2015 at 12:56:31PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 25, 2015 at 12:01:12AM +0900, Namhyung Kim escreveu:
> > Hi Arnaldo,
> > 
> > On Thu, Sep 24, 2015 at 10:42:28AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Namhyung,
> > > 
> > > 	Can you take a look and perhaps give me your Acked-by?
> > 
> > The dso->kernel and event->cpumode is always confusing for modules..
> 
> Right, my idea is to delete dso->kernel, to remove this confusion.

I'm fine with that. :)


> 
> Please see my tmp.perf/core branch, there are a few patches there using
> alternative methods in places previously using the botched dso->kernel.
> 
> > In this case, it seems that mmap events set kernel cpumode but
> > build-id events don't.
> 
> in the previous sentence you think build-id events don't set
> perf_event_attr.misc bits related to cpumode...
> 
> > So kernel cpumode in a bulid-id event
> > indicates that it is a kernel (vmlinux) dso, right?
> 
> While here you ask if it does?

I mean that cpumode is not set for modules, so if you see cpumode set,
it should be the kernel dso..


> 
> All these are synthesized, it is a matter of looking at the routines
> synthesizing them :-)

Right.


> 
> > I'll test this tomorrow..
> 
> Thanks, over time there were areas where multiple people touched and
> added different semantics that are biting now, I'm trying to, while
> fixing a bug, the one Wang Nan reported, clarify those things.
> 
> It is taking more time than I antecipated, as I'm stumbling in what look
> like other bugs in the perf-probe codebase :-/

OK, after applying this patch, perf cannot find vmlinux anymore.  It
finds scsi_mod.ko.gz instead, resulting in no kernel symbols..

Before:

  $ perf report | grep -F '[k]' | head
   0.44%  cc1              [kernel.vmlinux]               [k] clear_page_c
   0.34%  cc1              [kernel.vmlinux]               [k] page_fault
   0.22%  cc1              [kernel.vmlinux]               [k] get_page_from_freelist
   0.22%  cc1              [kernel.vmlinux]               [k] handle_mm_fault
   0.15%  cc1              [kernel.vmlinux]               [k] __do_page_fault
   0.13%  cc1              [kernel.vmlinux]               [k] unmap_single_vma
   0.12%  cc1              [kernel.vmlinux]               [k] __mem_cgroup_count_vm_event
   0.11%  cc1              [kernel.vmlinux]               [k] mem_cgroup_try_charge
   0.11%  cc1              [kernel.vmlinux]               [k] __d_lookup_rcu
   0.11%  cc1              [kernel.vmlinux]               [k] copy_user_generic_string    

After: (same data file but having no symbol affects the result)

  $ perf report | grep -F '[k]' | head
   0.42%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff812b4077
   0.33%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff8153f9d0
   0.11%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff8117ec0c
   0.10%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff812b443c
   0.10%  as               scsi_mod.ko.gz                 [k] 0xffffffff812b4077
   0.08%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff811bbd90
   0.07%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff811bf188
   0.07%  as               scsi_mod.ko.gz                 [k] 0xffffffff8153f9d0
   0.06%  sh               scsi_mod.ko.gz                 [k] 0xffffffff8153f9d0
   0.06%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff811e0838                       


Unfortunately, I don't have to time to look at it now.  It's one of
the biggest national holiday in Korea so maybe I can have a look later
in next week.

Thanks,
Namhyung

Re: [PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table

[Arnaldo Carvalho de Melo]

Em Fri, Sep 25, 2015 at 02:54:52PM +0900, Namhyung Kim escreveu:
> On Thu, Sep 24, 2015 at 12:56:31PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Sep 25, 2015 at 12:01:12AM +0900, Namhyung Kim escreveu:
> > > Hi Arnaldo,
> > > 
> > > On Thu, Sep 24, 2015 at 10:42:28AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Namhyung,
> > > > 
> > > > 	Can you take a look and perhaps give me your Acked-by?
> > > 
> > > The dso->kernel and event->cpumode is always confusing for modules..
> > 
> > Right, my idea is to delete dso->kernel, to remove this confusion.
> 
> I'm fine with that. :)
> 
> 
> > 
> > Please see my tmp.perf/core branch, there are a few patches there using
> > alternative methods in places previously using the botched dso->kernel.
> > 
> > > In this case, it seems that mmap events set kernel cpumode but
> > > build-id events don't.
> > 
> > in the previous sentence you think build-id events don't set
> > perf_event_attr.misc bits related to cpumode...
> > 
> > > So kernel cpumode in a bulid-id event
> > > indicates that it is a kernel (vmlinux) dso, right?
> > 
> > While here you ask if it does?
> 
> I mean that cpumode is not set for modules, so if you see cpumode set,
> it should be the kernel dso..
> 
> 
> > 
> > All these are synthesized, it is a matter of looking at the routines
> > synthesizing them :-)
> 
> Right.
> 
> 
> > 
> > > I'll test this tomorrow..
> > 
> > Thanks, over time there were areas where multiple people touched and
> > added different semantics that are biting now, I'm trying to, while
> > fixing a bug, the one Wang Nan reported, clarify those things.
> > 
> > It is taking more time than I antecipated, as I'm stumbling in what look
> > like other bugs in the perf-probe codebase :-/
> 
> OK, after applying this patch, perf cannot find vmlinux anymore.  It
> finds scsi_mod.ko.gz instead, resulting in no kernel symbols..

Thanks for checking, will reproduce and add a 'perf test' entry before
trying to continue the work in this patch...

- Arnaldo
 
> Before:
> 
>   $ perf report | grep -F '[k]' | head
>    0.44%  cc1              [kernel.vmlinux]               [k] clear_page_c
>    0.34%  cc1              [kernel.vmlinux]               [k] page_fault
>    0.22%  cc1              [kernel.vmlinux]               [k] get_page_from_freelist
>    0.22%  cc1              [kernel.vmlinux]               [k] handle_mm_fault
>    0.15%  cc1              [kernel.vmlinux]               [k] __do_page_fault
>    0.13%  cc1              [kernel.vmlinux]               [k] unmap_single_vma
>    0.12%  cc1              [kernel.vmlinux]               [k] __mem_cgroup_count_vm_event
>    0.11%  cc1              [kernel.vmlinux]               [k] mem_cgroup_try_charge
>    0.11%  cc1              [kernel.vmlinux]               [k] __d_lookup_rcu
>    0.11%  cc1              [kernel.vmlinux]               [k] copy_user_generic_string    
> 
> After: (same data file but having no symbol affects the result)
> 
>   $ perf report | grep -F '[k]' | head
>    0.42%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff812b4077
>    0.33%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff8153f9d0
>    0.11%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff8117ec0c
>    0.10%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff812b443c
>    0.10%  as               scsi_mod.ko.gz                 [k] 0xffffffff812b4077
>    0.08%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff811bbd90
>    0.07%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff811bf188
>    0.07%  as               scsi_mod.ko.gz                 [k] 0xffffffff8153f9d0
>    0.06%  sh               scsi_mod.ko.gz                 [k] 0xffffffff8153f9d0
>    0.06%  cc1              scsi_mod.ko.gz                 [k] 0xffffffff811e0838                       
> 
> 
> Unfortunately, I don't have to time to look at it now.  It's one of
> the biggest national holiday in Korea so maybe I can have a look later
> in next week.
> 
> Thanks,
> Namhyung