diff mbox series

[RFC,3/9] writeback: tracing: pass global_wb_domain as tracepoint parameter

Message ID 20200409193543.18115-4-mathieu.desnoyers@efficios.com
State RFC
Delegated to: BPF Maintainers
Headers show
Series None | expand

Commit Message

Mathieu Desnoyers April 9, 2020, 7:35 p.m. UTC 
The symbol global_wb_domain is not GPL-exported to modules. In order
to allow kernel tracers implemented as GPL modules to access the
global writeback domain dirty limit, as used within the
global_dirty_state and balance_dirty_pages trace events, pass
a pointer to the global writeback domain as a new parameter for
those tracepoints.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: bpf@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 include/trace/events/writeback.h | 17 ++++++++++-------
 mm/page-writeback.c              |  9 +++++----
 2 files changed, 15 insertions(+), 11 deletions(-)

Comments

Alexei Starovoitov April 9, 2020, 10:33 p.m. UTC | #1 
On Thu, Apr 09, 2020 at 03:35:37PM -0400, Mathieu Desnoyers wrote:
>  		if (pause < min_pause) {
> -			trace_balance_dirty_pages(wb,
> +			trace_balance_dirty_pages(&global_wb_domain,
> +						  wb,
>  						  sdtc->thresh,
>  						  sdtc->bg_thresh,
>  						  sdtc->dirty,

argh. 13 arguments to single function ?!
Currently the call site looks like:
                        trace_balance_dirty_pages(wb,
                                                  sdtc->thresh,
                                                  sdtc->bg_thresh,
                                                  sdtc->dirty,
                                                  sdtc->wb_thresh,
                                                  sdtc->wb_dirty,
                                                  dirty_ratelimit,
                                                  task_ratelimit,
                                                  pages_dirtied,
                                                  period,
                                                  min(pause, 0L),
                                                  start_time);
Just pass sdtc as a pointer instead.
Then another wb argument will be fine.
Mathieu Desnoyers April 10, 2020, 11:54 a.m. UTC | #2 
----- On Apr 9, 2020, at 6:33 PM, Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

> On Thu, Apr 09, 2020 at 03:35:37PM -0400, Mathieu Desnoyers wrote:
>>  		if (pause < min_pause) {
>> -			trace_balance_dirty_pages(wb,
>> +			trace_balance_dirty_pages(&global_wb_domain,
>> +						  wb,
>>  						  sdtc->thresh,
>>  						  sdtc->bg_thresh,
>>  						  sdtc->dirty,
> 
> argh. 13 arguments to single function ?!
> Currently the call site looks like:
>                        trace_balance_dirty_pages(wb,
>                                                  sdtc->thresh,
>                                                  sdtc->bg_thresh,
>                                                  sdtc->dirty,
>                                                  sdtc->wb_thresh,
>                                                  sdtc->wb_dirty,
>                                                  dirty_ratelimit,
>                                                  task_ratelimit,
>                                                  pages_dirtied,
>                                                  period,
>                                                  min(pause, 0L),
>                                                  start_time);
> Just pass sdtc as a pointer instead.
> Then another wb argument will be fine.

Good point! In order to do that I would need to move the declaration
of struct dirty_throttle_control from mm/page-writeback.c to
include/linux/writeback.h so it can be used from the tracepoint.

Seems fair ?

Thanks,

Mathieu
diff mbox series

Patch

diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index d94def25e4dc..8fd774108c9c 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -531,11 +531,13 @@  TRACE_EVENT(writeback_queue_io,
 
 TRACE_EVENT(global_dirty_state,
 
-	TP_PROTO(unsigned long background_thresh,
+	TP_PROTO(struct wb_domain *domain,
+		 unsigned long background_thresh,
 		 unsigned long dirty_thresh
 	),
 
-	TP_ARGS(background_thresh,
+	TP_ARGS(domain,
+		background_thresh,
 		dirty_thresh
 	),
 
@@ -558,7 +560,7 @@  TRACE_EVENT(global_dirty_state,
 		__entry->nr_written	= global_node_page_state(NR_WRITTEN);
 		__entry->background_thresh = background_thresh;
 		__entry->dirty_thresh	= dirty_thresh;
-		__entry->dirty_limit	= global_wb_domain.dirty_limit;
+		__entry->dirty_limit	= domain->dirty_limit;
 	),
 
 	TP_printk("dirty=%lu writeback=%lu unstable=%lu "
@@ -625,7 +627,8 @@  TRACE_EVENT(bdi_dirty_ratelimit,
 
 TRACE_EVENT(balance_dirty_pages,
 
-	TP_PROTO(struct bdi_writeback *wb,
+	TP_PROTO(struct wb_domain *domain,
+		 struct bdi_writeback *wb,
 		 unsigned long thresh,
 		 unsigned long bg_thresh,
 		 unsigned long dirty,
@@ -638,7 +641,7 @@  TRACE_EVENT(balance_dirty_pages,
 		 long pause,
 		 unsigned long start_time),
 
-	TP_ARGS(wb, thresh, bg_thresh, dirty, bdi_thresh, bdi_dirty,
+	TP_ARGS(domain, wb, thresh, bg_thresh, dirty, bdi_thresh, bdi_dirty,
 		dirty_ratelimit, task_ratelimit,
 		dirtied, period, pause, start_time),
 
@@ -664,8 +667,8 @@  TRACE_EVENT(balance_dirty_pages,
 		unsigned long freerun = (thresh + bg_thresh) / 2;
 		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 
-		__entry->limit		= global_wb_domain.dirty_limit;
-		__entry->setpoint	= (global_wb_domain.dirty_limit +
+		__entry->limit		= domain->dirty_limit;
+		__entry->setpoint	= (domain->dirty_limit +
 						freerun) / 2;
 		__entry->dirty		= dirty;
 		__entry->bdi_setpoint	= __entry->setpoint *
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 7326b54ab728..c76aae305360 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -443,9 +443,8 @@  static void domain_dirty_limits(struct dirty_throttle_control *dtc)
 	dtc->thresh = thresh;
 	dtc->bg_thresh = bg_thresh;
 
-	/* we should eventually report the domain in the TP */
 	if (!gdtc)
-		trace_global_dirty_state(bg_thresh, thresh);
+		trace_global_dirty_state(&global_wb_domain, bg_thresh, thresh);
 }
 
 /**
@@ -1736,7 +1735,8 @@  static void balance_dirty_pages(struct bdi_writeback *wb,
 		 * do a reset, as it may be a light dirtier.
 		 */
 		if (pause < min_pause) {
-			trace_balance_dirty_pages(wb,
+			trace_balance_dirty_pages(&global_wb_domain,
+						  wb,
 						  sdtc->thresh,
 						  sdtc->bg_thresh,
 						  sdtc->dirty,
@@ -1765,7 +1765,8 @@  static void balance_dirty_pages(struct bdi_writeback *wb,
 		}
 
 pause:
-		trace_balance_dirty_pages(wb,
+		trace_balance_dirty_pages(&global_wb_domain,
+					  wb,
 					  sdtc->thresh,
 					  sdtc->bg_thresh,
 					  sdtc->dirty,