Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Sebastian Andrzej Siewior]

* Steven Rostedt | 2016-03-07 09:03:47 [-0500]:

>On Sun, 6 Mar 2016 19:32:26 -0800
>"Shi, Yang"  wrote:
>
>> Hi Steven,
>> 
>> Any comment on this one? I'm supposed it will be taken by your tracing tree?
>> 
>
>Usually trace events go in through their subsystem, but since this one
>only touches the include/trace/events/ directory, I could pull it in
>too.

I took this for the next -RT release and dropped the
"!defined(CONFIG_PREEMPT_RT_FULL)" workaround we had in the meantime.

Sebastian

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Sebastian Andrzej Siewior]

* Steven Rostedt | 2016-03-07 09:03:47 [-0500]:

>On Sun, 6 Mar 2016 19:32:26 -0800
>"Shi, Yang"  wrote:
>
>> Hi Steven,
>> 
>> Any comment on this one? I'm supposed it will be taken by your tracing tree?
>> 
>
>Usually trace events go in through their subsystem, but since this one
>only touches the include/trace/events/ directory, I could pull it in
>too.

I took this for the next -RT release and dropped the
"!defined(CONFIG_PREEMPT_RT_FULL)" workaround we had in the meantime.

Sebastian

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Steven Rostedt]

On Sun, 6 Mar 2016 19:32:26 -0800
"Shi, Yang"  wrote:

> Hi Steven,
> 
> Any comment on this one? I'm supposed it will be taken by your tracing tree?
> 

Usually trace events go in through their subsystem, but since this one
only touches the include/trace/events/ directory, I could pull it in
too.

-- Steve

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Steven Rostedt]

On Sun, 6 Mar 2016 19:32:26 -0800
"Shi, Yang"  wrote:

> Hi Steven,
> 
> Any comment on this one? I'm supposed it will be taken by your tracing tree?
> 

Usually trace events go in through their subsystem, but since this one
only touches the include/trace/events/ directory, I could pull it in
too.

-- Steve

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Shi, Yang]

Hi Steven,

Any comment on this one? I'm supposed it will be taken by your tracing tree?

Thanks,
Yang


On 3/4/2016 4:52 AM, Tejun Heo wrote:

On Thu, Mar 03, 2016 at 01:08:57AM -0800, Yang Shi wrote:

commit 5634cc2aa9aebc77bc862992e7805469dcf83dac ("writeback: update writeback
tracepoints to report cgroup") made writeback tracepoints print out cgroup
path when CGROUP_WRITEBACK is enabled, but it may trigger the below bug on -rt
kernel since kernfs_path and kernfs_path_len are called by tracepoints, which
acquire spin lock that is sleepable on -rt kernel.


Acked-by: Tejun Heo 

Thanks.

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Shi, Yang]

Hi Steven,

Any comment on this one? I'm supposed it will be taken by your tracing tree?

Thanks,
Yang


On 3/4/2016 4:52 AM, Tejun Heo wrote:

On Thu, Mar 03, 2016 at 01:08:57AM -0800, Yang Shi wrote:

commit 5634cc2aa9aebc77bc862992e7805469dcf83dac ("writeback: update writeback
tracepoints to report cgroup") made writeback tracepoints print out cgroup
path when CGROUP_WRITEBACK is enabled, but it may trigger the below bug on -rt
kernel since kernfs_path and kernfs_path_len are called by tracepoints, which
acquire spin lock that is sleepable on -rt kernel.


Acked-by: Tejun Heo 

Thanks.

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Tejun Heo]

On Thu, Mar 03, 2016 at 01:08:57AM -0800, Yang Shi wrote:
> commit 5634cc2aa9aebc77bc862992e7805469dcf83dac ("writeback: update writeback
> tracepoints to report cgroup") made writeback tracepoints print out cgroup
> path when CGROUP_WRITEBACK is enabled, but it may trigger the below bug on -rt
> kernel since kernfs_path and kernfs_path_len are called by tracepoints, which
> acquire spin lock that is sleepable on -rt kernel.

Acked-by: Tejun Heo 

Thanks.

-- 
tejun

Re: [RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Tejun Heo]

On Thu, Mar 03, 2016 at 01:08:57AM -0800, Yang Shi wrote:
> commit 5634cc2aa9aebc77bc862992e7805469dcf83dac ("writeback: update writeback
> tracepoints to report cgroup") made writeback tracepoints print out cgroup
> path when CGROUP_WRITEBACK is enabled, but it may trigger the below bug on -rt
> kernel since kernfs_path and kernfs_path_len are called by tracepoints, which
> acquire spin lock that is sleepable on -rt kernel.

Acked-by: Tejun Heo 

Thanks.

-- 
tejun

[RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Yang Shi]

commit 5634cc2aa9aebc77bc862992e7805469dcf83dac ("writeback: update writeback
tracepoints to report cgroup") made writeback tracepoints print out cgroup
path when CGROUP_WRITEBACK is enabled, but it may trigger the below bug on -rt
kernel since kernfs_path and kernfs_path_len are called by tracepoints, which
acquire spin lock that is sleepable on -rt kernel.

BUG: sleeping function called from invalid context at 
kernel/locking/rtmutex.c:930
in_atomic(): 1, irqs_disabled(): 0, pid: 625, name: kworker/u16:3
INFO: lockdep is turned off.
Preemption disabled at:[] wb_writeback+0xec/0x830

CPU: 7 PID: 625 Comm: kworker/u16:3 Not tainted 4.4.1-rt5 #20
Hardware name: Freescale Layerscape 2085a RDB Board (DT)
Workqueue: writeback wb_workfn (flush-7:0)
Call trace:
[] dump_backtrace+0x0/0x200
[] show_stack+0x24/0x30
[] dump_stack+0x88/0xa8
[] ___might_sleep+0x2ec/0x300
[] rt_spin_lock+0x38/0xb8
[] kernfs_path_len+0x30/0x90
[] trace_event_raw_event_writeback_work_class+0xe8/0x2e8
[] wb_writeback+0x620/0x830
[] wb_workfn+0x61c/0x950
[] process_one_work+0x3ac/0xb30
[] worker_thread+0x9c/0x7a8
[] kthread+0x190/0x1b0
[] ret_from_fork+0x10/0x30

With unlocked kernfs_* functions, synchronize_sched() has to be called in
kernfs_rename which could be called in syscall path, but it is problematic.
So, print out cgroup ino instead of path name, which could be converted to
path name by userland.

Withouth CGROUP_WRITEBACK enabled, it just prints out root dir. But, root
dir ino vary from different filesystems, so printing out -1U to indicate
an invalid cgroup ino.

Signed-off-by: Yang Shi 
---
It should be applicable to both mainline and -rt kernel.
The change survives ftrace stress test in ltp.

v4: Acording to the discussion in v3 review, print out cgroup ino instead of
cgroup path name since it is problematic to call synchronize_sched() in
syscall.

v3:
  * Took Greg's comment to rename _kernfs_* and _cgroup_path to
kernfs_*_unlocked and cgroup_path_unlocked.

v2:
  * Adopted Steven's suggestion to call synchronize_sched in kernfs_rename_ns.

 include/trace/events/writeback.h | 121 +++
 1 file changed, 45 insertions(+), 76 deletions(-)

diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index fff846b..73614ce 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -134,58 +134,28 @@ DEFINE_EVENT(writeback_dirty_inode_template, 
writeback_dirty_inode,
 #ifdef CREATE_TRACE_POINTS
 #ifdef CONFIG_CGROUP_WRITEBACK
 
-static inline size_t __trace_wb_cgroup_size(struct bdi_writeback *wb)
+static inline unsigned int __trace_wb_assign_cgroup(struct bdi_writeback *wb)
 {
-   return kernfs_path_len(wb->memcg_css->cgroup->kn) + 1;
+   return wb->memcg_css->cgroup->kn->ino;
 }
 
-static inline void __trace_wb_assign_cgroup(char *buf, struct bdi_writeback 
*wb)
-{
-   struct cgroup *cgrp = wb->memcg_css->cgroup;
-   char *path;
-
-   path = cgroup_path(cgrp, buf, kernfs_path_len(cgrp->kn) + 1);
-   WARN_ON_ONCE(path != buf);
-}
-
-static inline size_t __trace_wbc_cgroup_size(struct writeback_control *wbc)
-{
-   if (wbc->wb)
-   return __trace_wb_cgroup_size(wbc->wb);
-   else
-   return 2;
-}
-
-static inline void __trace_wbc_assign_cgroup(char *buf,
-struct writeback_control *wbc)
+static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control 
*wbc)
 {
if (wbc->wb)
-   __trace_wb_assign_cgroup(buf, wbc->wb);
+   return __trace_wb_assign_cgroup(wbc->wb);
else
-   strcpy(buf, "/");
+   return -1U;
 }
-
 #else  /* CONFIG_CGROUP_WRITEBACK */
 
-static inline size_t __trace_wb_cgroup_size(struct bdi_writeback *wb)
-{
-   return 2;
-}
-
-static inline void __trace_wb_assign_cgroup(char *buf, struct bdi_writeback 
*wb)
-{
-   strcpy(buf, "/");
-}
-
-static inline size_t __trace_wbc_cgroup_size(struct writeback_control *wbc)
+static inline unsigned int __trace_wb_assign_cgroup(struct bdi_writeback *wb)
 {
-   return 2;
+   return -1U;
 }
 
-static inline void __trace_wbc_assign_cgroup(char *buf,
-struct writeback_control *wbc)
+static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control 
*wbc)
 {
-   strcpy(buf, "/");
+   return -1U;
 }
 
 #endif /* CONFIG_CGROUP_WRITEBACK */
@@ -201,7 +171,7 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
__array(char, name, 32)
__field(unsigned long, ino)
__field(int, sync_mode)
-   __dynamic_array(char, cgroup, __trace_wbc_cgroup_size(wbc))
+   __field(unsigned int, cgroup_ino)
),
 
TP_fast_assign(
@@ -209,14 +179,14 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,

[RFC V4 PATCH] trace: writeback: replace cgroup path to cgroup ino

[Yang Shi]

commit 5634cc2aa9aebc77bc862992e7805469dcf83dac ("writeback: update writeback
tracepoints to report cgroup") made writeback tracepoints print out cgroup
path when CGROUP_WRITEBACK is enabled, but it may trigger the below bug on -rt
kernel since kernfs_path and kernfs_path_len are called by tracepoints, which
acquire spin lock that is sleepable on -rt kernel.

BUG: sleeping function called from invalid context at 
kernel/locking/rtmutex.c:930
in_atomic(): 1, irqs_disabled(): 0, pid: 625, name: kworker/u16:3
INFO: lockdep is turned off.
Preemption disabled at:[] wb_writeback+0xec/0x830

CPU: 7 PID: 625 Comm: kworker/u16:3 Not tainted 4.4.1-rt5 #20
Hardware name: Freescale Layerscape 2085a RDB Board (DT)
Workqueue: writeback wb_workfn (flush-7:0)
Call trace:
[] dump_backtrace+0x0/0x200
[] show_stack+0x24/0x30
[] dump_stack+0x88/0xa8
[] ___might_sleep+0x2ec/0x300
[] rt_spin_lock+0x38/0xb8
[] kernfs_path_len+0x30/0x90
[] trace_event_raw_event_writeback_work_class+0xe8/0x2e8
[] wb_writeback+0x620/0x830
[] wb_workfn+0x61c/0x950
[] process_one_work+0x3ac/0xb30
[] worker_thread+0x9c/0x7a8
[] kthread+0x190/0x1b0
[] ret_from_fork+0x10/0x30

With unlocked kernfs_* functions, synchronize_sched() has to be called in
kernfs_rename which could be called in syscall path, but it is problematic.
So, print out cgroup ino instead of path name, which could be converted to
path name by userland.

Withouth CGROUP_WRITEBACK enabled, it just prints out root dir. But, root
dir ino vary from different filesystems, so printing out -1U to indicate
an invalid cgroup ino.

Signed-off-by: Yang Shi 
---
It should be applicable to both mainline and -rt kernel.
The change survives ftrace stress test in ltp.

v4: Acording to the discussion in v3 review, print out cgroup ino instead of
cgroup path name since it is problematic to call synchronize_sched() in
syscall.

v3:
  * Took Greg's comment to rename _kernfs_* and _cgroup_path to
kernfs_*_unlocked and cgroup_path_unlocked.

v2:
  * Adopted Steven's suggestion to call synchronize_sched in kernfs_rename_ns.

 include/trace/events/writeback.h | 121 +++
 1 file changed, 45 insertions(+), 76 deletions(-)

diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index fff846b..73614ce 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -134,58 +134,28 @@ DEFINE_EVENT(writeback_dirty_inode_template, 
writeback_dirty_inode,
 #ifdef CREATE_TRACE_POINTS
 #ifdef CONFIG_CGROUP_WRITEBACK
 
-static inline size_t __trace_wb_cgroup_size(struct bdi_writeback *wb)
+static inline unsigned int __trace_wb_assign_cgroup(struct bdi_writeback *wb)
 {
-   return kernfs_path_len(wb->memcg_css->cgroup->kn) + 1;
+   return wb->memcg_css->cgroup->kn->ino;
 }
 
-static inline void __trace_wb_assign_cgroup(char *buf, struct bdi_writeback 
*wb)
-{
-   struct cgroup *cgrp = wb->memcg_css->cgroup;
-   char *path;
-
-   path = cgroup_path(cgrp, buf, kernfs_path_len(cgrp->kn) + 1);
-   WARN_ON_ONCE(path != buf);
-}
-
-static inline size_t __trace_wbc_cgroup_size(struct writeback_control *wbc)
-{
-   if (wbc->wb)
-   return __trace_wb_cgroup_size(wbc->wb);
-   else
-   return 2;
-}
-
-static inline void __trace_wbc_assign_cgroup(char *buf,
-struct writeback_control *wbc)
+static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control 
*wbc)
 {
if (wbc->wb)
-   __trace_wb_assign_cgroup(buf, wbc->wb);
+   return __trace_wb_assign_cgroup(wbc->wb);
else
-   strcpy(buf, "/");
+   return -1U;
 }
-
 #else  /* CONFIG_CGROUP_WRITEBACK */
 
-static inline size_t __trace_wb_cgroup_size(struct bdi_writeback *wb)
-{
-   return 2;
-}
-
-static inline void __trace_wb_assign_cgroup(char *buf, struct bdi_writeback 
*wb)
-{
-   strcpy(buf, "/");
-}
-
-static inline size_t __trace_wbc_cgroup_size(struct writeback_control *wbc)
+static inline unsigned int __trace_wb_assign_cgroup(struct bdi_writeback *wb)
 {
-   return 2;
+   return -1U;
 }
 
-static inline void __trace_wbc_assign_cgroup(char *buf,
-struct writeback_control *wbc)
+static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control 
*wbc)
 {
-   strcpy(buf, "/");
+   return -1U;
 }
 
 #endif /* CONFIG_CGROUP_WRITEBACK */
@@ -201,7 +171,7 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
__array(char, name, 32)
__field(unsigned long, ino)
__field(int, sync_mode)
-   __dynamic_array(char, cgroup, __trace_wbc_cgroup_size(wbc))
+   __field(unsigned int, cgroup_ino)
),
 
TP_fast_assign(
@@ -209,14 +179,14 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
dev_name(inode_to_bdi(inode)->dev), 32);