* [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
@ 2015-10-22 7:45 Zhu Yanjun
2015-10-22 7:53 ` yzhu1
2015-10-22 16:34 ` David Smith
0 siblings, 2 replies; 19+ messages in thread
From: Zhu Yanjun @ 2015-10-22 7:45 UTC (permalink / raw)
To: systemtap
-rt mode spin lock lead to __might_sleep calltrace.
Replacing spin lock with stp type raw lock and
changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
Call Trace:
[<ffffffff818d83f1>] dump_stack+0x19/0x1b
[<ffffffff81070e3f>] __might_sleep+0xef/0x160
[<ffffffff818de710>] rt_spin_lock+0x20/0x50
[<ffffffff81178699>] d_path+0x79/0x1a0
[<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffff811674b2>] search_binary_handler+0x332/0x380
[<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
[<ffffffff81168e08>] do_execve+0x18/0x20
[<ffffffff81169082>] SyS_execve+0x32/0x50
[<ffffffff818e6979>] stub_execve+0x69/0xa0
Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
---
runtime/linux/alloc.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
index 7e7e5ce..914cf2b 100644
--- a/runtime/linux/alloc.c
+++ b/runtime/linux/alloc.c
@@ -12,6 +12,7 @@
#define _STAPLINUX_ALLOC_C_
#include <linux/percpu.h>
+#include "stp_helper_lock.h"
static int _stp_allocated_net_memory = 0;
/* Default, and should be "safe" from anywhere. */
@@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
& ~__GFP_WAIT)
/* May only be used in context that can sleep. __GFP_NORETRY is to
suppress the oom-killer from kicking in. */
+#ifndef CONFIG_PREEMPT_RT_FULL
#define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
+#else
+#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
+#endif
/* #define DEBUG_MEM */
/*
@@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
static int _stp_allocated_memory = 0;
#ifdef DEBUG_MEM
-static DEFINE_SPINLOCK(_stp_mem_lock);
+static STP_DEFINE_SPINLOCK(_stp_mem_lock);
#define MEM_MAGIC 0xc11cf77f
#define MEM_FENCE_SIZE 32
@@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t size, enum _stp_memtype typ
m->type = type;
m->len = size;
m->addr = addr;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_add(p, &_stp_mem_list);
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
return addr;
}
@@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct _stp_mem_entry *m, void *addr, size_t s
m->type = MEM_PERCPU;
m->len = size;
m->addr = addr;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_add(p, &_stp_mem_list);
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
}
static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
@@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
struct list_head *p, *tmp;
struct _stp_mem_entry *m = NULL;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
if (m->addr == addr) {
@@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
break;
}
}
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
if (!found) {
printk("SYSTEMTAP ERROR: Free of unallocated memory %p type=%s\n",
addr, _stp_malloc_types[type].free);
@@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
struct list_head *p, *tmp;
struct _stp_mem_entry *m = NULL;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
if (m->addr == addr) {
@@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
break;
}
}
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
if (!found) {
printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
addr);
@@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
struct list_head *p, *tmp;
struct _stp_mem_entry *m;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
list_del(p);
@@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
}
}
done:
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
return;
--
1.7.9.5
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-22 7:45 [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS Zhu Yanjun
@ 2015-10-22 7:53 ` yzhu1
2015-10-23 4:04 ` Santosh Shukla
2015-10-22 16:34 ` David Smith
1 sibling, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-10-22 7:53 UTC (permalink / raw)
To: systemtap, sshukla
Hi, Shukla
I am a beginner. To fix bug, I made this patch. Please comment it.
Thanks a lot.
Zhu Yanjun
On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
> -rt mode spin lock lead to __might_sleep calltrace.
> Replacing spin lock with stp type raw lock and
> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>
> Call Trace:
> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
> [<ffffffff81178699>] d_path+0x79/0x1a0
> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
> [<ffffffff81168e08>] do_execve+0x18/0x20
> [<ffffffff81169082>] SyS_execve+0x32/0x50
> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
> ---
> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
> index 7e7e5ce..914cf2b 100644
> --- a/runtime/linux/alloc.c
> +++ b/runtime/linux/alloc.c
> @@ -12,6 +12,7 @@
> #define _STAPLINUX_ALLOC_C_
>
> #include <linux/percpu.h>
> +#include "stp_helper_lock.h"
>
> static int _stp_allocated_net_memory = 0;
> /* Default, and should be "safe" from anywhere. */
> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
> & ~__GFP_WAIT)
> /* May only be used in context that can sleep. __GFP_NORETRY is to
> suppress the oom-killer from kicking in. */
> +#ifndef CONFIG_PREEMPT_RT_FULL
> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
> +#else
> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
> +#endif
>
> /* #define DEBUG_MEM */
> /*
> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
> static int _stp_allocated_memory = 0;
>
> #ifdef DEBUG_MEM
> -static DEFINE_SPINLOCK(_stp_mem_lock);
> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>
> #define MEM_MAGIC 0xc11cf77f
> #define MEM_FENCE_SIZE 32
> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t size, enum _stp_memtype typ
> m->type = type;
> m->len = size;
> m->addr = addr;
> - spin_lock(&_stp_mem_lock);
> + stp_spin_lock(&_stp_mem_lock);
> list_add(p, &_stp_mem_list);
> - spin_unlock(&_stp_mem_lock);
> + stp_spin_unlock(&_stp_mem_lock);
> return addr;
> }
>
> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct _stp_mem_entry *m, void *addr, size_t s
> m->type = MEM_PERCPU;
> m->len = size;
> m->addr = addr;
> - spin_lock(&_stp_mem_lock);
> + stp_spin_lock(&_stp_mem_lock);
> list_add(p, &_stp_mem_list);
> - spin_unlock(&_stp_mem_lock);
> + stp_spin_unlock(&_stp_mem_lock);
> }
>
> static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
> struct list_head *p, *tmp;
> struct _stp_mem_entry *m = NULL;
>
> - spin_lock(&_stp_mem_lock);
> + stp_spin_lock(&_stp_mem_lock);
> list_for_each_safe(p, tmp, &_stp_mem_list) {
> m = list_entry(p, struct _stp_mem_entry, list);
> if (m->addr == addr) {
> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
> break;
> }
> }
> - spin_unlock(&_stp_mem_lock);
> + stp_spin_unlock(&_stp_mem_lock);
> if (!found) {
> printk("SYSTEMTAP ERROR: Free of unallocated memory %p type=%s\n",
> addr, _stp_malloc_types[type].free);
> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
> struct list_head *p, *tmp;
> struct _stp_mem_entry *m = NULL;
>
> - spin_lock(&_stp_mem_lock);
> + stp_spin_lock(&_stp_mem_lock);
> list_for_each_safe(p, tmp, &_stp_mem_list) {
> m = list_entry(p, struct _stp_mem_entry, list);
> if (m->addr == addr) {
> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
> break;
> }
> }
> - spin_unlock(&_stp_mem_lock);
> + stp_spin_unlock(&_stp_mem_lock);
> if (!found) {
> printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
> addr);
> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
> struct list_head *p, *tmp;
> struct _stp_mem_entry *m;
>
> - spin_lock(&_stp_mem_lock);
> + stp_spin_lock(&_stp_mem_lock);
> list_for_each_safe(p, tmp, &_stp_mem_list) {
> m = list_entry(p, struct _stp_mem_entry, list);
> list_del(p);
> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
> }
> }
> done:
> - spin_unlock(&_stp_mem_lock);
> + stp_spin_unlock(&_stp_mem_lock);
>
> return;
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-22 7:45 [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS Zhu Yanjun
2015-10-22 7:53 ` yzhu1
@ 2015-10-22 16:34 ` David Smith
2015-10-26 3:19 ` yzhu1
1 sibling, 1 reply; 19+ messages in thread
From: David Smith @ 2015-10-22 16:34 UTC (permalink / raw)
To: Zhu Yanjun, systemtap
On 10/22/2015 02:46 AM, Zhu Yanjun wrote:
> -rt mode spin lock lead to __might_sleep calltrace.
> Replacing spin lock with stp type raw lock and
> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
In general, this patch looks fine. However, I'm not too sure about the
STP_ALLOC_SLEEP_FLAGS bit. Can show us a backtrace that happens with
your spinlock changes but without the alloc flags changes or explain why
the alloc flags change is necessary? It could be that we're using the
wrong set of flags in the caller.
Thanks.
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-22 7:53 ` yzhu1
@ 2015-10-23 4:04 ` Santosh Shukla
2015-10-26 7:03 ` yzhu1
2015-11-17 7:38 ` yzhu1
0 siblings, 2 replies; 19+ messages in thread
From: Santosh Shukla @ 2015-10-23 4:04 UTC (permalink / raw)
To: yzhu1; +Cc: systemtap
Looks Ok to me.
But I like to know systemtap's autotest / regression test is fine or
not? Pl. run that regression test and let us know the feedback.
If regression passes then
Reviewed-by : Santosh Shukla <sshukla@mvista.com>
Next time pl. add me in to list incase you want me to review -rt aware
stap patches.
Thanks.
On Thu, Oct 22, 2015 at 1:22 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
> Hi, Shukla
>
> I am a beginner. To fix bug, I made this patch. Please comment it.
>
> Thanks a lot.
> Zhu Yanjun
>
> On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
>>
>> -rt mode spin lock lead to __might_sleep calltrace.
>> Replacing spin lock with stp type raw lock and
>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>
>> Call Trace:
>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>> [<ffffffff81178699>] d_path+0x79/0x1a0
>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90
>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0
>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50
>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100
>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>> [<ffffffff81168e08>] do_execve+0x18/0x20
>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
>> ---
>> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
>> index 7e7e5ce..914cf2b 100644
>> --- a/runtime/linux/alloc.c
>> +++ b/runtime/linux/alloc.c
>> @@ -12,6 +12,7 @@
>> #define _STAPLINUX_ALLOC_C_
>> #include <linux/percpu.h>
>> +#include "stp_helper_lock.h"
>> static int _stp_allocated_net_memory = 0;
>> /* Default, and should be "safe" from anywhere. */
>> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
>> & ~__GFP_WAIT)
>> /* May only be used in context that can sleep. __GFP_NORETRY is to
>> suppress the oom-killer from kicking in. */
>> +#ifndef CONFIG_PREEMPT_RT_FULL
>> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
>> +#else
>> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
>> +#endif
>> /* #define DEBUG_MEM */
>> /*
>> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
>> static int _stp_allocated_memory = 0;
>> #ifdef DEBUG_MEM
>> -static DEFINE_SPINLOCK(_stp_mem_lock);
>> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>> #define MEM_MAGIC 0xc11cf77f
>> #define MEM_FENCE_SIZE 32
>> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t
>> size, enum _stp_memtype typ
>> m->type = type;
>> m->len = size;
>> m->addr = addr;
>> - spin_lock(&_stp_mem_lock);
>> + stp_spin_lock(&_stp_mem_lock);
>> list_add(p, &_stp_mem_list);
>> - spin_unlock(&_stp_mem_lock);
>> + stp_spin_unlock(&_stp_mem_lock);
>> return addr;
>> }
>> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct
>> _stp_mem_entry *m, void *addr, size_t s
>> m->type = MEM_PERCPU;
>> m->len = size;
>> m->addr = addr;
>> - spin_lock(&_stp_mem_lock);
>> + stp_spin_lock(&_stp_mem_lock);
>> list_add(p, &_stp_mem_list);
>> - spin_unlock(&_stp_mem_lock);
>> + stp_spin_unlock(&_stp_mem_lock);
>> }
>> static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
>> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum
>> _stp_memtype type)
>> struct list_head *p, *tmp;
>> struct _stp_mem_entry *m = NULL;
>> - spin_lock(&_stp_mem_lock);
>> + stp_spin_lock(&_stp_mem_lock);
>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>> m = list_entry(p, struct _stp_mem_entry, list);
>> if (m->addr == addr) {
>> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum
>> _stp_memtype type)
>> break;
>> }
>> }
>> - spin_unlock(&_stp_mem_lock);
>> + stp_spin_unlock(&_stp_mem_lock);
>> if (!found) {
>> printk("SYSTEMTAP ERROR: Free of unallocated memory %p
>> type=%s\n",
>> addr, _stp_malloc_types[type].free);
>> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
>> struct list_head *p, *tmp;
>> struct _stp_mem_entry *m = NULL;
>> - spin_lock(&_stp_mem_lock);
>> + stp_spin_lock(&_stp_mem_lock);
>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>> m = list_entry(p, struct _stp_mem_entry, list);
>> if (m->addr == addr) {
>> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
>> break;
>> }
>> }
>> - spin_unlock(&_stp_mem_lock);
>> + stp_spin_unlock(&_stp_mem_lock);
>> if (!found) {
>> printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
>> addr);
>> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
>> struct list_head *p, *tmp;
>> struct _stp_mem_entry *m;
>> - spin_lock(&_stp_mem_lock);
>> + stp_spin_lock(&_stp_mem_lock);
>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>> m = list_entry(p, struct _stp_mem_entry, list);
>> list_del(p);
>> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
>> }
>> }
>> done:
>> - spin_unlock(&_stp_mem_lock);
>> + stp_spin_unlock(&_stp_mem_lock);
>> return;
>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-22 16:34 ` David Smith
@ 2015-10-26 3:19 ` yzhu1
2015-10-26 21:28 ` David Smith
0 siblings, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-10-26 3:19 UTC (permalink / raw)
To: David Smith, systemtap
Hi, David
I do not have this backtrace. Because STP_ALLOC_SLEEP_FLAGS will cause
kmalloc sleep,
so I replaced it with STP_ALLOC_FLAGS.
Zhu Yanjun
On 10/23/2015 12:34 AM, David Smith wrote:
> On 10/22/2015 02:46 AM, Zhu Yanjun wrote:
>> -rt mode spin lock lead to __might_sleep calltrace.
>> Replacing spin lock with stp type raw lock and
>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
> In general, this patch looks fine. However, I'm not too sure about the
> STP_ALLOC_SLEEP_FLAGS bit. Can show us a backtrace that happens with
> your spinlock changes but without the alloc flags changes or explain why
> the alloc flags change is necessary? It could be that we're using the
> wrong set of flags in the caller.
>
> Thanks.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-23 4:04 ` Santosh Shukla
@ 2015-10-26 7:03 ` yzhu1
2015-10-26 8:25 ` yzhu1
2015-11-17 7:38 ` yzhu1
1 sibling, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-10-26 7:03 UTC (permalink / raw)
To: Santosh Shukla; +Cc: systemtap
Hi, Santosh
Thanks for your review.
Do you mean that I do systemtap's autotest / regression test?
If so, would you like to let me know how to make
systemtap's autotest / regression test ? I am glad to make such tests.
Best Regards!
Zhu Yanjun
On 10/23/2015 12:04 PM, Santosh Shukla wrote:
> Looks Ok to me.
>
> But I like to know systemtap's autotest / regression test is fine or
> not? Pl. run that regression test and let us know the feedback.
>
> If regression passes then
>
> Reviewed-by : Santosh Shukla <sshukla@mvista.com>
>
> Next time pl. add me in to list incase you want me to review -rt aware
> stap patches.
>
> Thanks.
>
> On Thu, Oct 22, 2015 at 1:22 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
>> Hi, Shukla
>>
>> I am a beginner. To fix bug, I made this patch. Please comment it.
>>
>> Thanks a lot.
>> Zhu Yanjun
>>
>> On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
>>> -rt mode spin lock lead to __might_sleep calltrace.
>>> Replacing spin lock with stp type raw lock and
>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>>
>>> Call Trace:
>>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>>> [<ffffffff81178699>] d_path+0x79/0x1a0
>>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>>> [<ffffffff81168e08>] do_execve+0x18/0x20
>>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>>>
>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
>>> ---
>>> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
>>> index 7e7e5ce..914cf2b 100644
>>> --- a/runtime/linux/alloc.c
>>> +++ b/runtime/linux/alloc.c
>>> @@ -12,6 +12,7 @@
>>> #define _STAPLINUX_ALLOC_C_
>>> #include <linux/percpu.h>
>>> +#include "stp_helper_lock.h"
>>> static int _stp_allocated_net_memory = 0;
>>> /* Default, and should be "safe" from anywhere. */
>>> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
>>> & ~__GFP_WAIT)
>>> /* May only be used in context that can sleep. __GFP_NORETRY is to
>>> suppress the oom-killer from kicking in. */
>>> +#ifndef CONFIG_PREEMPT_RT_FULL
>>> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
>>> +#else
>>> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
>>> +#endif
>>> /* #define DEBUG_MEM */
>>> /*
>>> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
>>> static int _stp_allocated_memory = 0;
>>> #ifdef DEBUG_MEM
>>> -static DEFINE_SPINLOCK(_stp_mem_lock);
>>> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>>> #define MEM_MAGIC 0xc11cf77f
>>> #define MEM_FENCE_SIZE 32
>>> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t
>>> size, enum _stp_memtype typ
>>> m->type = type;
>>> m->len = size;
>>> m->addr = addr;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_add(p, &_stp_mem_list);
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> return addr;
>>> }
>>> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct
>>> _stp_mem_entry *m, void *addr, size_t s
>>> m->type = MEM_PERCPU;
>>> m->len = size;
>>> m->addr = addr;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_add(p, &_stp_mem_list);
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> }
>>> static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
>>> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>> _stp_memtype type)
>>> struct list_head *p, *tmp;
>>> struct _stp_mem_entry *m = NULL;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>> m = list_entry(p, struct _stp_mem_entry, list);
>>> if (m->addr == addr) {
>>> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>> _stp_memtype type)
>>> break;
>>> }
>>> }
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> if (!found) {
>>> printk("SYSTEMTAP ERROR: Free of unallocated memory %p
>>> type=%s\n",
>>> addr, _stp_malloc_types[type].free);
>>> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
>>> struct list_head *p, *tmp;
>>> struct _stp_mem_entry *m = NULL;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>> m = list_entry(p, struct _stp_mem_entry, list);
>>> if (m->addr == addr) {
>>> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
>>> break;
>>> }
>>> }
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> if (!found) {
>>> printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
>>> addr);
>>> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
>>> struct list_head *p, *tmp;
>>> struct _stp_mem_entry *m;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>> m = list_entry(p, struct _stp_mem_entry, list);
>>> list_del(p);
>>> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
>>> }
>>> }
>>> done:
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> return;
>>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-26 7:03 ` yzhu1
@ 2015-10-26 8:25 ` yzhu1
2015-10-26 8:43 ` Santosh Shukla
0 siblings, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-10-26 8:25 UTC (permalink / raw)
To: Santosh Shukla; +Cc: systemtap
On 10/26/2015 03:03 PM, yzhu1 wrote:
> Hi, Santosh
>
> Thanks for your review.
>
> Do you mean that I do systemtap's autotest / regression test?
>
> If so, would you like to let me know how to make
> systemtap's autotest / regression test ? I am glad to make such tests.
Hi, Santosh
Sorry, I mean that the backtrace will not appear and the whole systemtap
can work well after this patch is applied to systemtap.
That is, the whole systemtap can pass our autotest / regression tests.
The tester and I can confirm the above.
And I do not know systemtap's autotest / regression tests in systemtap
software. If the systemtap's autotest / regression tests exists in
systemtap software,
please let me know how to make this systemtap's autotest / regression test.
Thanks a lot.
Zhu Yanjun
>
> Best Regards!
> Zhu Yanjun
>
> On 10/23/2015 12:04 PM, Santosh Shukla wrote:
>> Looks Ok to me.
>>
>> But I like to know systemtap's autotest / regression test is fine or
>> not? Pl. run that regression test and let us know the feedback.
>>
>> If regression passes then
>>
>> Reviewed-by : Santosh Shukla <sshukla@mvista.com>
>>
>> Next time pl. add me in to list incase you want me to review -rt aware
>> stap patches.
>>
>> Thanks.
>>
>> On Thu, Oct 22, 2015 at 1:22 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
>>> Hi, Shukla
>>>
>>> I am a beginner. To fix bug, I made this patch. Please comment it.
>>>
>>> Thanks a lot.
>>> Zhu Yanjun
>>>
>>> On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
>>>> -rt mode spin lock lead to __might_sleep calltrace.
>>>> Replacing spin lock with stp type raw lock and
>>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>>>
>>>> Call Trace:
>>>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>>>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>>>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>>>> [<ffffffff81178699>] d_path+0x79/0x1a0
>>>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffffa004d01b>]
>>>> __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>>>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>>>> [<ffffffff81168e08>] do_execve+0x18/0x20
>>>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>>>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>>>>
>>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
>>>> ---
>>>> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
>>>> index 7e7e5ce..914cf2b 100644
>>>> --- a/runtime/linux/alloc.c
>>>> +++ b/runtime/linux/alloc.c
>>>> @@ -12,6 +12,7 @@
>>>> #define _STAPLINUX_ALLOC_C_
>>>> #include <linux/percpu.h>
>>>> +#include "stp_helper_lock.h"
>>>> static int _stp_allocated_net_memory = 0;
>>>> /* Default, and should be "safe" from anywhere. */
>>>> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
>>>> & ~__GFP_WAIT)
>>>> /* May only be used in context that can sleep. __GFP_NORETRY is to
>>>> suppress the oom-killer from kicking in. */
>>>> +#ifndef CONFIG_PREEMPT_RT_FULL
>>>> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
>>>> +#else
>>>> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
>>>> +#endif
>>>> /* #define DEBUG_MEM */
>>>> /*
>>>> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
>>>> static int _stp_allocated_memory = 0;
>>>> #ifdef DEBUG_MEM
>>>> -static DEFINE_SPINLOCK(_stp_mem_lock);
>>>> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>>>> #define MEM_MAGIC 0xc11cf77f
>>>> #define MEM_FENCE_SIZE 32
>>>> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr,
>>>> size_t
>>>> size, enum _stp_memtype typ
>>>> m->type = type;
>>>> m->len = size;
>>>> m->addr = addr;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_add(p, &_stp_mem_list);
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> return addr;
>>>> }
>>>> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct
>>>> _stp_mem_entry *m, void *addr, size_t s
>>>> m->type = MEM_PERCPU;
>>>> m->len = size;
>>>> m->addr = addr;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_add(p, &_stp_mem_list);
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> }
>>>> static void _stp_mem_debug_free(void *addr, enum _stp_memtype
>>>> type)
>>>> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>>> _stp_memtype type)
>>>> struct list_head *p, *tmp;
>>>> struct _stp_mem_entry *m = NULL;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>> if (m->addr == addr) {
>>>> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>>> _stp_memtype type)
>>>> break;
>>>> }
>>>> }
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> if (!found) {
>>>> printk("SYSTEMTAP ERROR: Free of unallocated
>>>> memory %p
>>>> type=%s\n",
>>>> addr, _stp_malloc_types[type].free);
>>>> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
>>>> struct list_head *p, *tmp;
>>>> struct _stp_mem_entry *m = NULL;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>> if (m->addr == addr) {
>>>> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
>>>> break;
>>>> }
>>>> }
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> if (!found) {
>>>> printk("SYSTEMTAP ERROR: Couldn't validate memory
>>>> %p\n",
>>>> addr);
>>>> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
>>>> struct list_head *p, *tmp;
>>>> struct _stp_mem_entry *m;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>> list_del(p);
>>>> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
>>>> }
>>>> }
>>>> done:
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> return;
>>>>
>>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-26 8:25 ` yzhu1
@ 2015-10-26 8:43 ` Santosh Shukla
0 siblings, 0 replies; 19+ messages in thread
From: Santosh Shukla @ 2015-10-26 8:43 UTC (permalink / raw)
To: yzhu1; +Cc: systemtap
On Mon, Oct 26, 2015 at 1:55 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
> On 10/26/2015 03:03 PM, yzhu1 wrote:
>>
>> Hi, Santosh
>>
>> Thanks for your review.
>>
>> Do you mean that I do systemtap's autotest / regression test?
>>
>> If so, would you like to let me know how to make
>> systemtap's autotest / regression test ? I am glad to make such tests.
>
> Hi, Santosh
>
> Sorry, I mean that the backtrace will not appear and the whole systemtap can
> work well after this patch is applied to systemtap.
> That is, the whole systemtap can pass our autotest / regression tests.
>
> The tester and I can confirm the above.
>
> And I do not know systemtap's autotest / regression tests in systemtap
> software. If the systemtap's autotest / regression tests exists in systemtap
> software,
> please let me know how to make this systemtap's autotest / regression test.
>
Refer this [1], I guess it runs all the existing testsuites and make
sure your patches not breaking other modules etc..
[1] https://sourceware.org/systemtap/wiki/TestSuites
>
> Thanks a lot.
> Zhu Yanjun
>
>>
>> Best Regards!
>> Zhu Yanjun
>>
>> On 10/23/2015 12:04 PM, Santosh Shukla wrote:
>>>
>>> Looks Ok to me.
>>>
>>> But I like to know systemtap's autotest / regression test is fine or
>>> not? Pl. run that regression test and let us know the feedback.
>>>
>>> If regression passes then
>>>
>>> Reviewed-by : Santosh Shukla <sshukla@mvista.com>
>>>
>>> Next time pl. add me in to list incase you want me to review -rt aware
>>> stap patches.
>>>
>>> Thanks.
>>>
>>> On Thu, Oct 22, 2015 at 1:22 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
>>>>
>>>> Hi, Shukla
>>>>
>>>> I am a beginner. To fix bug, I made this patch. Please comment it.
>>>>
>>>> Thanks a lot.
>>>> Zhu Yanjun
>>>>
>>>> On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
>>>>>
>>>>> -rt mode spin lock lead to __might_sleep calltrace.
>>>>> Replacing spin lock with stp type raw lock and
>>>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>>>>
>>>>> Call Trace:
>>>>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>>>>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>>>>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>>>>> [<ffffffff81178699>] d_path+0x79/0x1a0
>>>>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90
>>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>>> [<ffffffffa004d01b>]
>>>>> __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0
>>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50
>>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100
>>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>>>>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>>>>> [<ffffffff81168e08>] do_execve+0x18/0x20
>>>>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>>>>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>>>>>
>>>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
>>>>> ---
>>>>> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
>>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
>>>>> index 7e7e5ce..914cf2b 100644
>>>>> --- a/runtime/linux/alloc.c
>>>>> +++ b/runtime/linux/alloc.c
>>>>> @@ -12,6 +12,7 @@
>>>>> #define _STAPLINUX_ALLOC_C_
>>>>> #include <linux/percpu.h>
>>>>> +#include "stp_helper_lock.h"
>>>>> static int _stp_allocated_net_memory = 0;
>>>>> /* Default, and should be "safe" from anywhere. */
>>>>> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
>>>>> & ~__GFP_WAIT)
>>>>> /* May only be used in context that can sleep. __GFP_NORETRY is to
>>>>> suppress the oom-killer from kicking in. */
>>>>> +#ifndef CONFIG_PREEMPT_RT_FULL
>>>>> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
>>>>> +#else
>>>>> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
>>>>> +#endif
>>>>> /* #define DEBUG_MEM */
>>>>> /*
>>>>> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
>>>>> static int _stp_allocated_memory = 0;
>>>>> #ifdef DEBUG_MEM
>>>>> -static DEFINE_SPINLOCK(_stp_mem_lock);
>>>>> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>>>>> #define MEM_MAGIC 0xc11cf77f
>>>>> #define MEM_FENCE_SIZE 32
>>>>> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr,
>>>>> size_t
>>>>> size, enum _stp_memtype typ
>>>>> m->type = type;
>>>>> m->len = size;
>>>>> m->addr = addr;
>>>>> - spin_lock(&_stp_mem_lock);
>>>>> + stp_spin_lock(&_stp_mem_lock);
>>>>> list_add(p, &_stp_mem_list);
>>>>> - spin_unlock(&_stp_mem_lock);
>>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>>> return addr;
>>>>> }
>>>>> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct
>>>>> _stp_mem_entry *m, void *addr, size_t s
>>>>> m->type = MEM_PERCPU;
>>>>> m->len = size;
>>>>> m->addr = addr;
>>>>> - spin_lock(&_stp_mem_lock);
>>>>> + stp_spin_lock(&_stp_mem_lock);
>>>>> list_add(p, &_stp_mem_list);
>>>>> - spin_unlock(&_stp_mem_lock);
>>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>>> }
>>>>> static void _stp_mem_debug_free(void *addr, enum _stp_memtype
>>>>> type)
>>>>> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>>>> _stp_memtype type)
>>>>> struct list_head *p, *tmp;
>>>>> struct _stp_mem_entry *m = NULL;
>>>>> - spin_lock(&_stp_mem_lock);
>>>>> + stp_spin_lock(&_stp_mem_lock);
>>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>>> if (m->addr == addr) {
>>>>> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>>>> _stp_memtype type)
>>>>> break;
>>>>> }
>>>>> }
>>>>> - spin_unlock(&_stp_mem_lock);
>>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>>> if (!found) {
>>>>> printk("SYSTEMTAP ERROR: Free of unallocated memory %p
>>>>> type=%s\n",
>>>>> addr, _stp_malloc_types[type].free);
>>>>> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
>>>>> struct list_head *p, *tmp;
>>>>> struct _stp_mem_entry *m = NULL;
>>>>> - spin_lock(&_stp_mem_lock);
>>>>> + stp_spin_lock(&_stp_mem_lock);
>>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>>> if (m->addr == addr) {
>>>>> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
>>>>> break;
>>>>> }
>>>>> }
>>>>> - spin_unlock(&_stp_mem_lock);
>>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>>> if (!found) {
>>>>> printk("SYSTEMTAP ERROR: Couldn't validate memory
>>>>> %p\n",
>>>>> addr);
>>>>> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
>>>>> struct list_head *p, *tmp;
>>>>> struct _stp_mem_entry *m;
>>>>> - spin_lock(&_stp_mem_lock);
>>>>> + stp_spin_lock(&_stp_mem_lock);
>>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>>> list_del(p);
>>>>> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
>>>>> }
>>>>> }
>>>>> done:
>>>>> - spin_unlock(&_stp_mem_lock);
>>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>>> return;
>>>>>
>>>>
>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-26 3:19 ` yzhu1
@ 2015-10-26 21:28 ` David Smith
2015-10-27 13:52 ` Frank Ch. Eigler
2015-10-28 2:30 ` yzhu1
0 siblings, 2 replies; 19+ messages in thread
From: David Smith @ 2015-10-26 21:28 UTC (permalink / raw)
To: yzhu1, systemtap
On 10/25/2015 10:19 PM, yzhu1 wrote:
> Hi, David
>
> I do not have this backtrace. Because STP_ALLOC_SLEEP_FLAGS will cause
> kmalloc sleep,
> so I replaced it with STP_ALLOC_FLAGS.
Evidently I'm not explaining myself well, I'll try again in more detail.
We've got 2 sets of allocation flags in systemtap:
STP_ALLOC_FLAGS: These are the default flags and allocations using this
set of flags will not sleep.
STP_ALLOC_SLEEP_FLAGS: These flags are only supposed to be used from
contexts where it is safe to sleep (like begin probes).
Here's the big question - on the realtime kernel, is it ever safe to sleep?
If the answer to the previous question is "no", then your change is correct.
If the answer to the previous question is "yes", then your change isn't
correct. Instead, we need to look at uses of STP_ALLOC_SLEEP_FLAGS,
because we're using the wrong set of flags somewhere. We're using
STP_ALLOC_SLEEP_FLAGS in an allocation that should be using STP_ALLOC_FLAGS.
I see 6 uses of STP_ALLOC_SLEEP_FLAGS in the systemtap source. If we
can't get a backtrace, then I'll need you to change the 6 uses of
STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS one at a time to figure out
which use of STP_ALLOC_SLEEP_FLAGS isn't correct.
Thanks for continuing to work this problem.
>
> Zhu Yanjun
>
> On 10/23/2015 12:34 AM, David Smith wrote:
>> On 10/22/2015 02:46 AM, Zhu Yanjun wrote:
>>> -rt mode spin lock lead to __might_sleep calltrace.
>>> Replacing spin lock with stp type raw lock and
>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>> In general, this patch looks fine. However, I'm not too sure about the
>> STP_ALLOC_SLEEP_FLAGS bit. Can show us a backtrace that happens with
>> your spinlock changes but without the alloc flags changes or explain why
>> the alloc flags change is necessary? It could be that we're using the
>> wrong set of flags in the caller.
>>
>> Thanks.
>>
>
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-26 21:28 ` David Smith
@ 2015-10-27 13:52 ` Frank Ch. Eigler
2015-10-28 2:30 ` yzhu1
1 sibling, 0 replies; 19+ messages in thread
From: Frank Ch. Eigler @ 2015-10-27 13:52 UTC (permalink / raw)
To: David Smith; +Cc: yzhu1, systemtap
dsmith wrote:
> [...]
> Here's the big question - on the realtime kernel, is it ever safe to sleep?
> If the answer to the previous question is "no", then your change
> [switching to STP_ALLOC_FLAGS] is correct. [...]
But even then, not in general. If we use atomic memory allocation flags only,
we may suffer unnecessary alloc failures on normal kernels. If we do this
switch, perhaps it needs to be done rt-conditionally. It might be as simple
as to "#define STP_ALLOC_SLEEP_FLAGS ..." differently on rt (CONFIG_*...).
- FChE
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-26 21:28 ` David Smith
2015-10-27 13:52 ` Frank Ch. Eigler
@ 2015-10-28 2:30 ` yzhu1
2015-10-28 17:09 ` David Smith
1 sibling, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-10-28 2:30 UTC (permalink / raw)
To: David Smith, systemtap
On 10/27/2015 05:28 AM, David Smith wrote:
> On 10/25/2015 10:19 PM, yzhu1 wrote:
>> Hi, David
>>
>> I do not have this backtrace. Because STP_ALLOC_SLEEP_FLAGS will cause
>> kmalloc sleep,
>> so I replaced it with STP_ALLOC_FLAGS.
> Evidently I'm not explaining myself well, I'll try again in more detail.
>
> We've got 2 sets of allocation flags in systemtap:
>
> STP_ALLOC_FLAGS: These are the default flags and allocations using this
> set of flags will not sleep.
>
> STP_ALLOC_SLEEP_FLAGS: These flags are only supposed to be used from
> contexts where it is safe to sleep (like begin probes).
>
> Here's the big question - on the realtime kernel, is it ever safe to sleep?
>
> If the answer to the previous question is "no", then your change is correct.
>
> If the answer to the previous question is "yes", then your change isn't
> correct. Instead, we need to look at uses of STP_ALLOC_SLEEP_FLAGS,
> because we're using the wrong set of flags somewhere. We're using
> STP_ALLOC_SLEEP_FLAGS in an allocation that should be using STP_ALLOC_FLAGS.
>
> I see 6 uses of STP_ALLOC_SLEEP_FLAGS in the systemtap source. If we
> can't get a backtrace, then I'll need you to change the 6 uses of
> STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS one at a time to figure out
> which use of STP_ALLOC_SLEEP_FLAGS isn't correct.
>
> Thanks for continuing to work this problem.
Hi, All
OK. I will do.
Zhu Yanjun
>
>
>> Zhu Yanjun
>>
>> On 10/23/2015 12:34 AM, David Smith wrote:
>>> On 10/22/2015 02:46 AM, Zhu Yanjun wrote:
>>>> -rt mode spin lock lead to __might_sleep calltrace.
>>>> Replacing spin lock with stp type raw lock and
>>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>> In general, this patch looks fine. However, I'm not too sure about the
>>> STP_ALLOC_SLEEP_FLAGS bit. Can show us a backtrace that happens with
>>> your spinlock changes but without the alloc flags changes or explain why
>>> the alloc flags change is necessary? It could be that we're using the
>>> wrong set of flags in the caller.
>>>
>>> Thanks.
>>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-28 2:30 ` yzhu1
@ 2015-10-28 17:09 ` David Smith
0 siblings, 0 replies; 19+ messages in thread
From: David Smith @ 2015-10-28 17:09 UTC (permalink / raw)
To: yzhu1, systemtap
[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]
On 10/27/2015 09:30 PM, yzhu1 wrote:
> On 10/27/2015 05:28 AM, David Smith wrote:
>> On 10/25/2015 10:19 PM, yzhu1 wrote:
>>> Hi, David
>>>
>>> I do not have this backtrace. Because STP_ALLOC_SLEEP_FLAGS will cause
>>> kmalloc sleep,
>>> so I replaced it with STP_ALLOC_FLAGS.
>> Evidently I'm not explaining myself well, I'll try again in more detail.
>>
>> We've got 2 sets of allocation flags in systemtap:
>>
>> STP_ALLOC_FLAGS: These are the default flags and allocations using this
>> set of flags will not sleep.
>>
>> STP_ALLOC_SLEEP_FLAGS: These flags are only supposed to be used from
>> contexts where it is safe to sleep (like begin probes).
>>
>> Here's the big question - on the realtime kernel, is it ever safe to
>> sleep?
>>
>> If the answer to the previous question is "no", then your change is
>> correct.
>>
>> If the answer to the previous question is "yes", then your change isn't
>> correct. Instead, we need to look at uses of STP_ALLOC_SLEEP_FLAGS,
>> because we're using the wrong set of flags somewhere. We're using
>> STP_ALLOC_SLEEP_FLAGS in an allocation that should be using
>> STP_ALLOC_FLAGS.
>>
>> I see 6 uses of STP_ALLOC_SLEEP_FLAGS in the systemtap source. If we
>> can't get a backtrace, then I'll need you to change the 6 uses of
>> STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS one at a time to figure out
>> which use of STP_ALLOC_SLEEP_FLAGS isn't correct.
>>
>> Thanks for continuing to work this problem.
> Hi, All
>
> OK. I will do.
I had an idea which might help. I've attached a patch that adds a call
to 'might_sleep()' to all the functions that use STP_ALLOC_SLEEP_FLAGS.
Hopefully that might help point out where the problem lies.
Please give it a try.
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
[-- Attachment #2: might_sleep.patch --]
[-- Type: text/x-patch, Size: 2077 bytes --]
diff --git a/runtime/linux/stat_runtime.h b/runtime/linux/stat_runtime.h
index 5e27026..9491a1c 100644
--- a/runtime/linux/stat_runtime.h
+++ b/runtime/linux/stat_runtime.h
@@ -48,6 +48,7 @@ static Stat _stp_stat_alloc(size_t stat_data_size)
return NULL;
/* Called from module_init, so user context, may sleep alloc. */
+ might_sleep();
st = _stp_kmalloc_gfp (sizeof(struct _Stat), STP_ALLOC_SLEEP_FLAGS);
if (st == NULL)
return NULL;
diff --git a/runtime/linux/stp_tracepoint.c b/runtime/linux/stp_tracepoint.c
index 33fe27a..95c4e95 100644
--- a/runtime/linux/stp_tracepoint.c
+++ b/runtime/linux/stp_tracepoint.c
@@ -80,6 +80,7 @@ int add_probe(struct tracepoint_entry *e, void *probe, void *data)
}
if (found)
return -EEXIST;
+ might_sleep();
p = _stp_kmalloc_gfp(sizeof(struct stp_tp_probe),
STP_ALLOC_SLEEP_FLAGS);
if (!p)
@@ -153,6 +154,7 @@ struct tracepoint_entry *add_tracepoint(const char *name)
* Using kmalloc here to allocate a variable length element. Could
* cause some memory fragmentation if overused.
*/
+ might_sleep();
e = _stp_kmalloc_gfp(sizeof(struct tracepoint_entry) + name_len,
STP_ALLOC_SLEEP_FLAGS);
if (!e)
diff --git a/runtime/task_finder_vma.c b/runtime/task_finder_vma.c
index f0a4db9..354d424 100644
--- a/runtime/task_finder_vma.c
+++ b/runtime/task_finder_vma.c
@@ -53,6 +53,7 @@ __stp_tf_vma_new_entry(void)
struct __stp_tf_vma_entry *entry;
size_t size = sizeof (struct __stp_tf_vma_entry);
#ifdef CONFIG_UTRACE
+ might_sleep();
entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
STP_ALLOC_SLEEP_FLAGS);
#else
@@ -78,8 +79,11 @@ static int
stap_initialize_vma_map(void)
{
size_t size = sizeof(struct hlist_head) * __STP_TF_TABLE_SIZE;
- struct hlist_head *map = (struct hlist_head *) _stp_kzalloc_gfp(size,
- STP_ALLOC_SLEEP_FLAGS);
+ struct hlist_head *map;
+
+ might_sleep();
+ map = (struct hlist_head *) _stp_kzalloc_gfp(size,
+ STP_ALLOC_SLEEP_FLAGS);
if (map == NULL)
return -ENOMEM;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-10-23 4:04 ` Santosh Shukla
2015-10-26 7:03 ` yzhu1
@ 2015-11-17 7:38 ` yzhu1
2015-11-17 7:52 ` Santosh Shukla
1 sibling, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-11-17 7:38 UTC (permalink / raw)
To: Santosh Shukla; +Cc: systemtap
[-- Attachment #1: Type: text/plain, Size: 6471 bytes --]
Hi, Shukla
I run systemtap's autotest. I think this patch can pass it.
So I send this patch to you.
Best Regards!
Zhu Yanjun
On 10/23/2015 12:04 PM, Santosh Shukla wrote:
> Looks Ok to me.
>
> But I like to know systemtap's autotest / regression test is fine or
> not? Pl. run that regression test and let us know the feedback.
>
> If regression passes then
>
> Reviewed-by : Santosh Shukla <sshukla@mvista.com>
>
> Next time pl. add me in to list incase you want me to review -rt aware
> stap patches.
>
> Thanks.
>
> On Thu, Oct 22, 2015 at 1:22 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
>> Hi, Shukla
>>
>> I am a beginner. To fix bug, I made this patch. Please comment it.
>>
>> Thanks a lot.
>> Zhu Yanjun
>>
>> On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
>>> -rt mode spin lock lead to __might_sleep calltrace.
>>> Replacing spin lock with stp type raw lock and
>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>>
>>> Call Trace:
>>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>>> [<ffffffff81178699>] d_path+0x79/0x1a0
>>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100
>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>>> [<ffffffff81168e08>] do_execve+0x18/0x20
>>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>>>
>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
>>> ---
>>> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
>>> index 7e7e5ce..914cf2b 100644
>>> --- a/runtime/linux/alloc.c
>>> +++ b/runtime/linux/alloc.c
>>> @@ -12,6 +12,7 @@
>>> #define _STAPLINUX_ALLOC_C_
>>> #include <linux/percpu.h>
>>> +#include "stp_helper_lock.h"
>>> static int _stp_allocated_net_memory = 0;
>>> /* Default, and should be "safe" from anywhere. */
>>> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
>>> & ~__GFP_WAIT)
>>> /* May only be used in context that can sleep. __GFP_NORETRY is to
>>> suppress the oom-killer from kicking in. */
>>> +#ifndef CONFIG_PREEMPT_RT_FULL
>>> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
>>> +#else
>>> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
>>> +#endif
>>> /* #define DEBUG_MEM */
>>> /*
>>> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
>>> static int _stp_allocated_memory = 0;
>>> #ifdef DEBUG_MEM
>>> -static DEFINE_SPINLOCK(_stp_mem_lock);
>>> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>>> #define MEM_MAGIC 0xc11cf77f
>>> #define MEM_FENCE_SIZE 32
>>> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t
>>> size, enum _stp_memtype typ
>>> m->type = type;
>>> m->len = size;
>>> m->addr = addr;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_add(p, &_stp_mem_list);
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> return addr;
>>> }
>>> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct
>>> _stp_mem_entry *m, void *addr, size_t s
>>> m->type = MEM_PERCPU;
>>> m->len = size;
>>> m->addr = addr;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_add(p, &_stp_mem_list);
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> }
>>> static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
>>> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>> _stp_memtype type)
>>> struct list_head *p, *tmp;
>>> struct _stp_mem_entry *m = NULL;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>> m = list_entry(p, struct _stp_mem_entry, list);
>>> if (m->addr == addr) {
>>> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>> _stp_memtype type)
>>> break;
>>> }
>>> }
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> if (!found) {
>>> printk("SYSTEMTAP ERROR: Free of unallocated memory %p
>>> type=%s\n",
>>> addr, _stp_malloc_types[type].free);
>>> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
>>> struct list_head *p, *tmp;
>>> struct _stp_mem_entry *m = NULL;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>> m = list_entry(p, struct _stp_mem_entry, list);
>>> if (m->addr == addr) {
>>> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
>>> break;
>>> }
>>> }
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> if (!found) {
>>> printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
>>> addr);
>>> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
>>> struct list_head *p, *tmp;
>>> struct _stp_mem_entry *m;
>>> - spin_lock(&_stp_mem_lock);
>>> + stp_spin_lock(&_stp_mem_lock);
>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>> m = list_entry(p, struct _stp_mem_entry, list);
>>> list_del(p);
>>> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
>>> }
>>> }
>>> done:
>>> - spin_unlock(&_stp_mem_lock);
>>> + stp_spin_unlock(&_stp_mem_lock);
>>> return;
>>>
>>
[-- Attachment #2: 0001-stp-rt-replace-spin_lock-with-stp-style-lock-and-use.patch --]
[-- Type: text/x-patch, Size: 4898 bytes --]
From b75fb2f91b8daa18c5927b1254a6b11de133203e Mon Sep 17 00:00:00 2001
From: Zhu Yanjun <yanjun.zhu@windriver.com>
Date: Thu, 22 Oct 2015 15:41:57 +0800
Subject: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use
STP_ALLOC_FLAGS
-rt mode spin lock lead to __might_sleep calltrace.
Replacing spin lock with stp type raw lock and
changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
Call Trace:
[<ffffffff818d83f1>] dump_stack+0x19/0x1b
[<ffffffff81070e3f>] __might_sleep+0xef/0x160
[<ffffffff818de710>] rt_spin_lock+0x20/0x50
[<ffffffff81178699>] d_path+0x79/0x1a0
[<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffff811674b2>] search_binary_handler+0x332/0x380
[<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
[<ffffffff81168e08>] do_execve+0x18/0x20
[<ffffffff81169082>] SyS_execve+0x32/0x50
[<ffffffff818e6979>] stub_execve+0x69/0xa0
Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
Reviewed-by : Santosh Shukla <sshukla@mvista.com>
---
runtime/linux/alloc.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
index 7e7e5ce..914cf2b 100644
--- a/runtime/linux/alloc.c
+++ b/runtime/linux/alloc.c
@@ -12,6 +12,7 @@
#define _STAPLINUX_ALLOC_C_
#include <linux/percpu.h>
+#include "stp_helper_lock.h"
static int _stp_allocated_net_memory = 0;
/* Default, and should be "safe" from anywhere. */
@@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
& ~__GFP_WAIT)
/* May only be used in context that can sleep. __GFP_NORETRY is to
suppress the oom-killer from kicking in. */
+#ifndef CONFIG_PREEMPT_RT_FULL
#define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
+#else
+#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
+#endif
/* #define DEBUG_MEM */
/*
@@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
static int _stp_allocated_memory = 0;
#ifdef DEBUG_MEM
-static DEFINE_SPINLOCK(_stp_mem_lock);
+static STP_DEFINE_SPINLOCK(_stp_mem_lock);
#define MEM_MAGIC 0xc11cf77f
#define MEM_FENCE_SIZE 32
@@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t size, enum _stp_memtype typ
m->type = type;
m->len = size;
m->addr = addr;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_add(p, &_stp_mem_list);
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
return addr;
}
@@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct _stp_mem_entry *m, void *addr, size_t s
m->type = MEM_PERCPU;
m->len = size;
m->addr = addr;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_add(p, &_stp_mem_list);
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
}
static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
@@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
struct list_head *p, *tmp;
struct _stp_mem_entry *m = NULL;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
if (m->addr == addr) {
@@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
break;
}
}
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
if (!found) {
printk("SYSTEMTAP ERROR: Free of unallocated memory %p type=%s\n",
addr, _stp_malloc_types[type].free);
@@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
struct list_head *p, *tmp;
struct _stp_mem_entry *m = NULL;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
if (m->addr == addr) {
@@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
break;
}
}
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
if (!found) {
printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
addr);
@@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
struct list_head *p, *tmp;
struct _stp_mem_entry *m;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
list_del(p);
@@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
}
}
done:
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
return;
--
1.7.9.5
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-11-17 7:38 ` yzhu1
@ 2015-11-17 7:52 ` Santosh Shukla
0 siblings, 0 replies; 19+ messages in thread
From: Santosh Shukla @ 2015-11-17 7:52 UTC (permalink / raw)
To: yzhu1; +Cc: systemtap
Pl. send patch via email tools like mutt or get-send-email. Attaching
patch is not a good approach and then I will review. thanks.
On Tue, Nov 17, 2015 at 1:08 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
> Hi, Shukla
>
> I run systemtap's autotest. I think this patch can pass it.
> So I send this patch to you.
>
> Best Regards!
> Zhu Yanjun
>
> On 10/23/2015 12:04 PM, Santosh Shukla wrote:
>>
>> Looks Ok to me.
>>
>> But I like to know systemtap's autotest / regression test is fine or
>> not? Pl. run that regression test and let us know the feedback.
>>
>> If regression passes then
>>
>> Reviewed-by : Santosh Shukla <sshukla@mvista.com>
>>
>> Next time pl. add me in to list incase you want me to review -rt aware
>> stap patches.
>>
>> Thanks.
>>
>> On Thu, Oct 22, 2015 at 1:22 PM, yzhu1 <Yanjun.Zhu@windriver.com> wrote:
>>>
>>> Hi, Shukla
>>>
>>> I am a beginner. To fix bug, I made this patch. Please comment it.
>>>
>>> Thanks a lot.
>>> Zhu Yanjun
>>>
>>> On 10/22/2015 03:46 PM, Zhu Yanjun wrote:
>>>>
>>>> -rt mode spin lock lead to __might_sleep calltrace.
>>>> Replacing spin lock with stp type raw lock and
>>>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
>>>>
>>>> Call Trace:
>>>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>>>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>>>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>>>> [<ffffffff81178699>] d_path+0x79/0x1a0
>>>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100
>>>> [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>>>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>>>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>>>> [<ffffffff81168e08>] do_execve+0x18/0x20
>>>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>>>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>>>>
>>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
>>>> ---
>>>> runtime/linux/alloc.c | 27 ++++++++++++++++-----------
>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
>>>> index 7e7e5ce..914cf2b 100644
>>>> --- a/runtime/linux/alloc.c
>>>> +++ b/runtime/linux/alloc.c
>>>> @@ -12,6 +12,7 @@
>>>> #define _STAPLINUX_ALLOC_C_
>>>> #include <linux/percpu.h>
>>>> +#include "stp_helper_lock.h"
>>>> static int _stp_allocated_net_memory = 0;
>>>> /* Default, and should be "safe" from anywhere. */
>>>> @@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
>>>> & ~__GFP_WAIT)
>>>> /* May only be used in context that can sleep. __GFP_NORETRY is to
>>>> suppress the oom-killer from kicking in. */
>>>> +#ifndef CONFIG_PREEMPT_RT_FULL
>>>> #define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
>>>> +#else
>>>> +#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
>>>> +#endif
>>>> /* #define DEBUG_MEM */
>>>> /*
>>>> @@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
>>>> static int _stp_allocated_memory = 0;
>>>> #ifdef DEBUG_MEM
>>>> -static DEFINE_SPINLOCK(_stp_mem_lock);
>>>> +static STP_DEFINE_SPINLOCK(_stp_mem_lock);
>>>> #define MEM_MAGIC 0xc11cf77f
>>>> #define MEM_FENCE_SIZE 32
>>>> @@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t
>>>> size, enum _stp_memtype typ
>>>> m->type = type;
>>>> m->len = size;
>>>> m->addr = addr;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_add(p, &_stp_mem_list);
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> return addr;
>>>> }
>>>> @@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct
>>>> _stp_mem_entry *m, void *addr, size_t s
>>>> m->type = MEM_PERCPU;
>>>> m->len = size;
>>>> m->addr = addr;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_add(p, &_stp_mem_list);
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> }
>>>> static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
>>>> @@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>>> _stp_memtype type)
>>>> struct list_head *p, *tmp;
>>>> struct _stp_mem_entry *m = NULL;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>> if (m->addr == addr) {
>>>> @@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum
>>>> _stp_memtype type)
>>>> break;
>>>> }
>>>> }
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> if (!found) {
>>>> printk("SYSTEMTAP ERROR: Free of unallocated memory %p
>>>> type=%s\n",
>>>> addr, _stp_malloc_types[type].free);
>>>> @@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
>>>> struct list_head *p, *tmp;
>>>> struct _stp_mem_entry *m = NULL;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>> if (m->addr == addr) {
>>>> @@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
>>>> break;
>>>> }
>>>> }
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> if (!found) {
>>>> printk("SYSTEMTAP ERROR: Couldn't validate memory
>>>> %p\n",
>>>> addr);
>>>> @@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
>>>> struct list_head *p, *tmp;
>>>> struct _stp_mem_entry *m;
>>>> - spin_lock(&_stp_mem_lock);
>>>> + stp_spin_lock(&_stp_mem_lock);
>>>> list_for_each_safe(p, tmp, &_stp_mem_list) {
>>>> m = list_entry(p, struct _stp_mem_entry, list);
>>>> list_del(p);
>>>> @@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
>>>> }
>>>> }
>>>> done:
>>>> - spin_unlock(&_stp_mem_lock);
>>>> + stp_spin_unlock(&_stp_mem_lock);
>>>> return;
>>>>
>>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-11-18 17:14 ` David Smith
@ 2015-11-19 7:53 ` yzhu1
0 siblings, 0 replies; 19+ messages in thread
From: yzhu1 @ 2015-11-19 7:53 UTC (permalink / raw)
To: David Smith, sshukla, systemtap
On 11/19/2015 01:14 AM, David Smith wrote:
> On 11/18/2015 02:14 AM, yzhu1 wrote:
>> Hi, David
>>
>> Thanks for your comments.
>>
>> I can not reproduce this problem. And my user can reproduce this problem
>> ocassionly.
>> And I requested him to help me to make tests about your changes. But he
>> refused.
>>
>> So I made auto tests in systemtap after this patch is applied. And I can
>> not find any
>> regressions about this patch.
Hi, David
Thanks for your comments.
Zhu Yanjun
> Most of the time you won't see any difference when using STP_ALLOC_FLAGS
> in place of STP_ALLOC_SLEEP_FLAGS. You would see a difference if your
> system is under memory pressure.
>
> Let me refresh everyone's knowledge of the difference between
> STP_ALLOC_FLAGS and STP_ALLOC_SLEEP_FLAGS. When using STP_ALLOC_FLAGS,
> if memory isn't available, the allocation will fail. When using
> STP_ALLOC_SLEEP_FLAGS, the kernel will try to wait until memory is
> available. Obviously, we can only use STP_ALLOC_SLEEP_FLAGS in certain
> places in the code where sleeping is permitted.
>
> If changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS makes a difference,
> this really means that we're using STP_ALLOC_SLEEP_FLAGS in a place
> where it isn't safe to sleep.
>
> It could be that in the normal kernel it is safe to sleep in a spot
> where we're using STP_ALLOC_SLEEP_FLAGS, but in the realtime kernel it
> isn't safe to sleep at that same spot. I guess it also makes sense to
> sleep as little as possible when running systemtap on the realtime kernel.
>
> So, I'll withdraw my objection. As long as the switch from
> STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS only affects the realtime
> kernel, I'm OK with it.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-11-18 8:14 ` yzhu1
@ 2015-11-18 17:14 ` David Smith
2015-11-19 7:53 ` yzhu1
0 siblings, 1 reply; 19+ messages in thread
From: David Smith @ 2015-11-18 17:14 UTC (permalink / raw)
To: yzhu1, sshukla, systemtap
On 11/18/2015 02:14 AM, yzhu1 wrote:
> Hi, David
>
> Thanks for your comments.
>
> I can not reproduce this problem. And my user can reproduce this problem
> ocassionly.
> And I requested him to help me to make tests about your changes. But he
> refused.
>
> So I made auto tests in systemtap after this patch is applied. And I can
> not find any
> regressions about this patch.
Most of the time you won't see any difference when using STP_ALLOC_FLAGS
in place of STP_ALLOC_SLEEP_FLAGS. You would see a difference if your
system is under memory pressure.
Let me refresh everyone's knowledge of the difference between
STP_ALLOC_FLAGS and STP_ALLOC_SLEEP_FLAGS. When using STP_ALLOC_FLAGS,
if memory isn't available, the allocation will fail. When using
STP_ALLOC_SLEEP_FLAGS, the kernel will try to wait until memory is
available. Obviously, we can only use STP_ALLOC_SLEEP_FLAGS in certain
places in the code where sleeping is permitted.
If changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS makes a difference,
this really means that we're using STP_ALLOC_SLEEP_FLAGS in a place
where it isn't safe to sleep.
It could be that in the normal kernel it is safe to sleep in a spot
where we're using STP_ALLOC_SLEEP_FLAGS, but in the realtime kernel it
isn't safe to sleep at that same spot. I guess it also makes sense to
sleep as little as possible when running systemtap on the realtime kernel.
So, I'll withdraw my objection. As long as the switch from
STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS only affects the realtime
kernel, I'm OK with it.
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-11-17 16:45 ` David Smith
@ 2015-11-18 8:14 ` yzhu1
2015-11-18 17:14 ` David Smith
0 siblings, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-11-18 8:14 UTC (permalink / raw)
To: David Smith, sshukla, systemtap
Hi, David
Thanks for your comments.
I can not reproduce this problem. And my user can reproduce this problem
ocassionly.
And I requested him to help me to make tests about your changes. But he
refused.
So I made auto tests in systemtap after this patch is applied. And I can
not find any
regressions about this patch.
Best Regards!
Zhu Yanjun
On 11/18/2015 12:45 AM, David Smith wrote:
> On 11/17/2015 02:09 AM, yzhu1 wrote:
>> From: Zhu Yanjun <yanjun.zhu@windriver.com>
>>
>> -rt mode spin lock lead to __might_sleep calltrace.
>> Replacing spin lock with stp type raw lock and
>> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
> I have the same comment I had before. The raw lock changes are fine, but
> I still don't believe the STP_ALLOC_SLEEP_FLAGS change is correct, at
> least not without some further explanation. In the call trace below,
> STP_ALLOC_SLEEP_FLAGS wasn't used from what I can tell.
>
> Did you get a chance to try running systemtap with your raw lock changes
> and the added might_sleep() call patch I sent you?
>
>> Call Trace:
>> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
>> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
>> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
>> [<ffffffff81178699>] d_path+0x79/0x1a0
>> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
>> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
>> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
>> [<ffffffff81168e08>] do_execve+0x18/0x20
>> [<ffffffff81169082>] SyS_execve+0x32/0x50
>> [<ffffffff818e6979>] stub_execve+0x69/0xa0
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-11-17 8:09 ` [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS yzhu1
@ 2015-11-17 16:45 ` David Smith
2015-11-18 8:14 ` yzhu1
0 siblings, 1 reply; 19+ messages in thread
From: David Smith @ 2015-11-17 16:45 UTC (permalink / raw)
To: yzhu1, sshukla, systemtap
On 11/17/2015 02:09 AM, yzhu1 wrote:
> From: Zhu Yanjun <yanjun.zhu@windriver.com>
>
> -rt mode spin lock lead to __might_sleep calltrace.
> Replacing spin lock with stp type raw lock and
> changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
I have the same comment I had before. The raw lock changes are fine, but
I still don't believe the STP_ALLOC_SLEEP_FLAGS change is correct, at
least not without some further explanation. In the call trace below,
STP_ALLOC_SLEEP_FLAGS wasn't used from what I can tell.
Did you get a chance to try running systemtap with your raw lock changes
and the added might_sleep() call patch I sent you?
> Call Trace:
> [<ffffffff818d83f1>] dump_stack+0x19/0x1b
> [<ffffffff81070e3f>] __might_sleep+0xef/0x160
> [<ffffffff818de710>] rt_spin_lock+0x20/0x50
> [<ffffffff81178699>] d_path+0x79/0x1a0
> [<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
> [<ffffffff811674b2>] search_binary_handler+0x332/0x380
> [<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
> [<ffffffff81168e08>] do_execve+0x18/0x20
> [<ffffffff81169082>] SyS_execve+0x32/0x50
> [<ffffffff818e6979>] stub_execve+0x69/0xa0
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS
2015-11-17 8:09 stp: rt: replace spin_lock with stp style lock and use yzhu1
@ 2015-11-17 8:09 ` yzhu1
2015-11-17 16:45 ` David Smith
0 siblings, 1 reply; 19+ messages in thread
From: yzhu1 @ 2015-11-17 8:09 UTC (permalink / raw)
To: sshukla, systemtap
From: Zhu Yanjun <yanjun.zhu@windriver.com>
-rt mode spin lock lead to __might_sleep calltrace.
Replacing spin lock with stp type raw lock and
changing STP_ALLOC_SLEEP_FLAGS to STP_ALLOC_FLAGS solves the problem.
Call Trace:
[<ffffffff818d83f1>] dump_stack+0x19/0x1b
[<ffffffff81070e3f>] __might_sleep+0xef/0x160
[<ffffffff818de710>] rt_spin_lock+0x20/0x50
[<ffffffff81178699>] d_path+0x79/0x1a0
[<ffffffffa0047be9>] __stp_get_mm_path.constprop.79+0x49/0x90 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa004d01b>] __stp_utrace_attach_match_tsk.isra.53+0x7b/0x1b0 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa004d18c>] __stp_utrace_task_finder_report_exec+0x3c/0x50 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffffa0047b59>] utrace_report_exec+0xb9/0x100 [stap_f5bb3e3c9b162aab5a51afc2375fe9cf_2073]
[<ffffffff811674b2>] search_binary_handler+0x332/0x380
[<ffffffff81168d0c>] do_execve_common.isra.24+0x55c/0x640
[<ffffffff81168e08>] do_execve+0x18/0x20
[<ffffffff81169082>] SyS_execve+0x32/0x50
[<ffffffff818e6979>] stub_execve+0x69/0xa0
Signed-off-by: Zhu Yanjun <yanjun.zhu@windriver.com>
Reviewed-by : Santosh Shukla <sshukla@mvista.com>
---
runtime/linux/alloc.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/runtime/linux/alloc.c b/runtime/linux/alloc.c
index 7e7e5ce..914cf2b 100644
--- a/runtime/linux/alloc.c
+++ b/runtime/linux/alloc.c
@@ -12,6 +12,7 @@
#define _STAPLINUX_ALLOC_C_
#include <linux/percpu.h>
+#include "stp_helper_lock.h"
static int _stp_allocated_net_memory = 0;
/* Default, and should be "safe" from anywhere. */
@@ -19,7 +20,11 @@ static int _stp_allocated_net_memory = 0;
& ~__GFP_WAIT)
/* May only be used in context that can sleep. __GFP_NORETRY is to
suppress the oom-killer from kicking in. */
+#ifndef CONFIG_PREEMPT_RT_FULL
#define STP_ALLOC_SLEEP_FLAGS (GFP_KERNEL | __GFP_NORETRY)
+#else
+#define STP_ALLOC_SLEEP_FLAGS STP_ALLOC_FLAGS
+#endif
/* #define DEBUG_MEM */
/*
@@ -40,7 +45,7 @@ static int _stp_allocated_net_memory = 0;
static int _stp_allocated_memory = 0;
#ifdef DEBUG_MEM
-static DEFINE_SPINLOCK(_stp_mem_lock);
+static STP_DEFINE_SPINLOCK(_stp_mem_lock);
#define MEM_MAGIC 0xc11cf77f
#define MEM_FENCE_SIZE 32
@@ -108,9 +113,9 @@ static void *_stp_mem_debug_setup(void *addr, size_t size, enum _stp_memtype typ
m->type = type;
m->len = size;
m->addr = addr;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_add(p, &_stp_mem_list);
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
return addr;
}
@@ -122,9 +127,9 @@ static void _stp_mem_debug_percpu(struct _stp_mem_entry *m, void *addr, size_t s
m->type = MEM_PERCPU;
m->len = size;
m->addr = addr;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_add(p, &_stp_mem_list);
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
}
static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
@@ -133,7 +138,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
struct list_head *p, *tmp;
struct _stp_mem_entry *m = NULL;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
if (m->addr == addr) {
@@ -142,7 +147,7 @@ static void _stp_mem_debug_free(void *addr, enum _stp_memtype type)
break;
}
}
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
if (!found) {
printk("SYSTEMTAP ERROR: Free of unallocated memory %p type=%s\n",
addr, _stp_malloc_types[type].free);
@@ -184,7 +189,7 @@ static void _stp_mem_debug_validate(void *addr)
struct list_head *p, *tmp;
struct _stp_mem_entry *m = NULL;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
if (m->addr == addr) {
@@ -192,7 +197,7 @@ static void _stp_mem_debug_validate(void *addr)
break;
}
}
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
if (!found) {
printk("SYSTEMTAP ERROR: Couldn't validate memory %p\n",
addr);
@@ -551,7 +556,7 @@ static void _stp_mem_debug_done(void)
struct list_head *p, *tmp;
struct _stp_mem_entry *m;
- spin_lock(&_stp_mem_lock);
+ stp_spin_lock(&_stp_mem_lock);
list_for_each_safe(p, tmp, &_stp_mem_list) {
m = list_entry(p, struct _stp_mem_entry, list);
list_del(p);
@@ -583,7 +588,7 @@ static void _stp_mem_debug_done(void)
}
}
done:
- spin_unlock(&_stp_mem_lock);
+ stp_spin_unlock(&_stp_mem_lock);
return;
--
1.7.9.5
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-11-19 7:53 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-22 7:45 [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS Zhu Yanjun
2015-10-22 7:53 ` yzhu1
2015-10-23 4:04 ` Santosh Shukla
2015-10-26 7:03 ` yzhu1
2015-10-26 8:25 ` yzhu1
2015-10-26 8:43 ` Santosh Shukla
2015-11-17 7:38 ` yzhu1
2015-11-17 7:52 ` Santosh Shukla
2015-10-22 16:34 ` David Smith
2015-10-26 3:19 ` yzhu1
2015-10-26 21:28 ` David Smith
2015-10-27 13:52 ` Frank Ch. Eigler
2015-10-28 2:30 ` yzhu1
2015-10-28 17:09 ` David Smith
2015-11-17 8:09 stp: rt: replace spin_lock with stp style lock and use yzhu1
2015-11-17 8:09 ` [PATCH 1/1] stp: rt: replace spin_lock with stp style lock and use STP_ALLOC_FLAGS yzhu1
2015-11-17 16:45 ` David Smith
2015-11-18 8:14 ` yzhu1
2015-11-18 17:14 ` David Smith
2015-11-19 7:53 ` yzhu1
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).