diff mbox

[V2,2/7] mm/gup: add gup trace points

Message ID 1449096813-22436-3-git-send-email-yang.shi@linaro.org
State New
Headers show

Commit Message

Yang Shi Dec. 2, 2015, 10:53 p.m. UTC 
For slow version, just add trace point for raw __get_user_pages since all
slow variants call it to do the real work finally.

Signed-off-by: Yang Shi <yang.shi@linaro.org>

---
 mm/gup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Yang Shi Dec. 3, 2015, 12:11 a.m. UTC | #1 
On 12/2/2015 3:36 PM, Dave Hansen wrote:
> On 12/02/2015 02:53 PM, Yang Shi wrote:

>> diff --git a/mm/gup.c b/mm/gup.c

>> index deafa2c..10245a4 100644

>> --- a/mm/gup.c

>> +++ b/mm/gup.c

>> @@ -13,6 +13,9 @@

>>   #include <linux/rwsem.h>

>>   #include <linux/hugetlb.h>

>>

>> +#define CREATE_TRACE_POINTS

>> +#include <trace/events/gup.h>

>> +

>>   #include <asm/pgtable.h>

>>   #include <asm/tlbflush.h>

>

> This needs to be _the_ last thing that gets #included.  Otherwise, you

> risk colliding with any other trace header that gets implicitly included

> below.


Thanks for the suggestion, will move it to the last.

>

>> @@ -1340,6 +1346,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,

>>   					start, len)))

>>   		return 0;

>>

>> +	trace_gup_get_user_pages_fast(start, nr_pages, write, pages);

>> +

>>   	/*

>>   	 * Disable interrupts.  We use the nested form as we can already have

>>   	 * interrupts disabled by get_futex_key.

>

> It would be _really_ nice to be able to see return values from the

> various gup calls as well.  Is that feasible?


I think it should be feasible. kmem_cache_alloc trace event could show 
return value. I'm supposed gup trace events should be able to do the 
same thing.

Regards,
Yang

>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yang Shi Dec. 3, 2015, 12:52 a.m. UTC | #2 
On 12/2/2015 4:11 PM, Shi, Yang wrote:
> On 12/2/2015 3:36 PM, Dave Hansen wrote:

>> On 12/02/2015 02:53 PM, Yang Shi wrote:

>>> diff --git a/mm/gup.c b/mm/gup.c

>>> index deafa2c..10245a4 100644

>>> --- a/mm/gup.c

>>> +++ b/mm/gup.c

>>> @@ -13,6 +13,9 @@

>>>   #include <linux/rwsem.h>

>>>   #include <linux/hugetlb.h>

>>>

>>> +#define CREATE_TRACE_POINTS

>>> +#include <trace/events/gup.h>

>>> +

>>>   #include <asm/pgtable.h>

>>>   #include <asm/tlbflush.h>

>>

>> This needs to be _the_ last thing that gets #included.  Otherwise, you

>> risk colliding with any other trace header that gets implicitly included

>> below.

>

> Thanks for the suggestion, will move it to the last.

>

>>

>>> @@ -1340,6 +1346,8 @@ int __get_user_pages_fast(unsigned long start,

>>> int nr_pages, int write,

>>>                       start, len)))

>>>           return 0;

>>>

>>> +    trace_gup_get_user_pages_fast(start, nr_pages, write, pages);

>>> +

>>>       /*

>>>        * Disable interrupts.  We use the nested form as we can

>>> already have

>>>        * interrupts disabled by get_futex_key.

>>

>> It would be _really_ nice to be able to see return values from the

>> various gup calls as well.  Is that feasible?

>

> I think it should be feasible. kmem_cache_alloc trace event could show

> return value. I'm supposed gup trace events should be able to do the

> same thing.


Just did a quick test, it is definitely feasible. Please check the below 
test log:

        trace-cmd-200   [000]    99.221486: gup_get_user_pages: 
start=8000000ff0 nr_pages=1 ret=1
        trace-cmd-200   [000]    99.223215: gup_get_user_pages: 
start=8000000fdb nr_pages=1 ret=1
        trace-cmd-200   [000]    99.223298: gup_get_user_pages: 
start=8000000ed0 nr_pages=1 ret=1

nr_pages is the number of pages requested by the gup, ret is the return 
value.

If nobody has objection, I will add it into V3.

Regards,
Yang

>

> Regards,

> Yang

>

>>

>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yang Shi Dec. 3, 2015, 6:36 p.m. UTC | #3 
On 12/2/2015 8:13 PM, Steven Rostedt wrote:
> On Wed, 2 Dec 2015 15:36:50 -0800

> Dave Hansen <dave.hansen@intel.com> wrote:

>

>> On 12/02/2015 02:53 PM, Yang Shi wrote:

>>> diff --git a/mm/gup.c b/mm/gup.c

>>> index deafa2c..10245a4 100644

>>> --- a/mm/gup.c

>>> +++ b/mm/gup.c

>>> @@ -13,6 +13,9 @@

>>>   #include <linux/rwsem.h>

>>>   #include <linux/hugetlb.h>

>>>

>>> +#define CREATE_TRACE_POINTS

>>> +#include <trace/events/gup.h>

>>> +

>>>   #include <asm/pgtable.h>

>>>   #include <asm/tlbflush.h>

>>

>> This needs to be _the_ last thing that gets #included.  Otherwise, you

>> risk colliding with any other trace header that gets implicitly included

>> below.

>

> Agreed.

>

>>

>>> @@ -1340,6 +1346,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,

>>>   					start, len)))

>>>   		return 0;

>>>

>>> +	trace_gup_get_user_pages_fast(start, nr_pages, write, pages);

>>> +

>>>   	/*

>>>   	 * Disable interrupts.  We use the nested form as we can already have

>>>   	 * interrupts disabled by get_futex_key.

>>

>> It would be _really_ nice to be able to see return values from the

>> various gup calls as well.  Is that feasible?

>

> Only if you rewrite the functions to have a single return code path

> that we can add a tracepoint too. Or have a wrapper function that gets


Yes. My preliminary test just could cover the success case. gup could 
return errno from different a few code path.

> called directly that calls these functions internally and the tracepoint

> can trap the return value.


This will incur more changes in other subsystems (futex, kvm, etc), I'm 
not sure if it is worth making such changes to get return value.

> I can probably make function_graph tracer give return values, although

> it will give a return value for void functions as well. And it may give

> long long returns for int returns that may have bogus data in the

> higher bits.


If the return value requirement is not limited to gup, the approach 
sounds more reasonable.

Thanks,
Yang

>

> -- Steve

>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yang Shi Dec. 3, 2015, 10:10 p.m. UTC | #4 
On 12/3/2015 11:06 AM, Steven Rostedt wrote:
> On Thu, 03 Dec 2015 10:36:18 -0800

> "Shi, Yang" <yang.shi@linaro.org> wrote:

>

>>> called directly that calls these functions internally and the tracepoint

>>> can trap the return value.

>>

>> This will incur more changes in other subsystems (futex, kvm, etc), I'm

>> not sure if it is worth making such changes to get return value.

>

> No, it wouldn't require any changes outside of this.

>

> -long __get_user_pages(..)

> +static long __get_user_pages_internal(..)

> {

>    [..]

> }

> +

> +long __get_user_pages(..)

> +{

> +	long ret;

> +	ret = __get_user_pages_internal(..);

> +	trace_get_user_pages(.., ret)

> +}


Thanks for this. I just checked the fast version, it looks it just has 
single return path, so this should be just needed by slow version.

>

>>

>>> I can probably make function_graph tracer give return values, although

>>> it will give a return value for void functions as well. And it may give

>>> long long returns for int returns that may have bogus data in the

>>> higher bits.

>>

>> If the return value requirement is not limited to gup, the approach

>> sounds more reasonable.

>>

>

> Others have asked about it. Maybe I should do it.


If you are going to add return value in common trace code, I won't do 
the gup specific one in V3.

Thanks,
Yang

>

> -- Steve

>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/mm/gup.c b/mm/gup.c
index deafa2c..10245a4 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -13,6 +13,9 @@ 
 #include <linux/rwsem.h>
 #include <linux/hugetlb.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/gup.h>
+
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 
@@ -462,6 +465,8 @@  long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 	if (!nr_pages)
 		return 0;
 
+	trace_gup_get_user_pages(tsk, mm, start, nr_pages);
+
 	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
 	/*
@@ -599,6 +604,7 @@  int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
 	if (!(vm_flags & vma->vm_flags))
 		return -EFAULT;
 
+	trace_gup_fixup_user_fault(tsk, mm, address, fault_flags);
 	ret = handle_mm_fault(mm, vma, address, fault_flags);
 	if (ret & VM_FAULT_ERROR) {
 		if (ret & VM_FAULT_OOM)
@@ -1340,6 +1346,8 @@  int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 					start, len)))
 		return 0;
 
+	trace_gup_get_user_pages_fast(start, nr_pages, write, pages);
+
 	/*
 	 * Disable interrupts.  We use the nested form as we can already have
 	 * interrupts disabled by get_futex_key.