| Message ID | 20250303100617.223677-3-yizhou.tang@shopee.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix calculations in trace_balance_dirty_pages() for cgwb | expand |
On Mon 03-03-25 18:06:17, Tang Yizhou wrote: > From: Tang Yizhou <yizhou.tang@shopee.com> > > In the commit dcc25ae76eb7 ("writeback: move global_dirty_limit into > wb_domain") of the cgroup writeback backpressure propagation patchset, > Tejun made some adaptations to trace_balance_dirty_pages() for cgroup > writeback. However, this adaptation was incomplete and Tejun missed > further adaptation in the subsequent patches. > > In the cgroup writeback scenario, if sdtc in balance_dirty_pages() is > assigned to mdtc, then upon entering trace_balance_dirty_pages(), > __entry->limit should be assigned based on the dirty_limit of the > corresponding memcg's wb_domain, rather than global_wb_domain. > > To address this issue and simplify the implementation, introduce a 'limit' > field in struct dirty_throttle_control to store the hard_limit value > computed in wb_position_ratio() by calling hard_dirty_limit(). This field > will then be used in trace_balance_dirty_pages() to assign the value to > __entry->limit. > > Fixes: dcc25ae76eb7 ("writeback: move global_dirty_limit into wb_domain") > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com> In principle this looks fine but one nit below: > diff --git a/include/linux/writeback.h b/include/linux/writeback.h > index 32095928365c..58bda3347914 100644 > --- a/include/linux/writeback.h > +++ b/include/linux/writeback.h > @@ -326,6 +326,7 @@ struct dirty_throttle_control { > unsigned long dirty; /* file_dirty + write + nfs */ > unsigned long thresh; /* dirty threshold */ > unsigned long bg_thresh; /* dirty background threshold */ > + unsigned long limit; /* hard dirty limit */ ^^^ I'd call this dirty_limit to not invent a new name for the same thing. I've noticed the tracepoint has 'limit' as well but that is the outlier that should be modified if anything. Also I'd modify the comment to /* smoothed dirty limit */ to better explain what this is about. Honza
diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 32095928365c..58bda3347914 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -326,6 +326,7 @@ struct dirty_throttle_control { unsigned long dirty; /* file_dirty + write + nfs */ unsigned long thresh; /* dirty threshold */ unsigned long bg_thresh; /* dirty background threshold */ + unsigned long limit; /* hard dirty limit */ unsigned long wb_dirty; /* per-wb counterparts */ unsigned long wb_thresh; diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h index 3046ca6b08ea..0ff388131fc9 100644 --- a/include/trace/events/writeback.h +++ b/include/trace/events/writeback.h @@ -663,9 +663,8 @@ TRACE_EVENT(balance_dirty_pages, unsigned long freerun = (dtc->thresh + dtc->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 + - freerun) / 2; + __entry->limit = dtc->limit; + __entry->setpoint = (dtc->limit + freerun) / 2; __entry->dirty = dtc->dirty; __entry->wb_setpoint = __entry->setpoint * dtc->wb_thresh / (dtc->thresh + 1); diff --git a/mm/page-writeback.c b/mm/page-writeback.c index e980b2aec352..3147119a9a04 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -1072,7 +1072,7 @@ static void wb_position_ratio(struct dirty_throttle_control *dtc) struct bdi_writeback *wb = dtc->wb; unsigned long write_bw = READ_ONCE(wb->avg_write_bandwidth); unsigned long freerun = dirty_freerun_ceiling(dtc->thresh, dtc->bg_thresh); - unsigned long limit = hard_dirty_limit(dtc_dom(dtc), dtc->thresh); + unsigned long limit = dtc->limit = hard_dirty_limit(dtc_dom(dtc), dtc->thresh); unsigned long wb_thresh = dtc->wb_thresh; unsigned long x_intercept; unsigned long setpoint; /* dirty pages' target balance point */