| Message ID | 20230323040037.2389095-7-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make rstat flushing IRQ and sleep friendly | expand |
On Thu, Mar 23, 2023 at 04:00:36AM +0000, Yosry Ahmed wrote: > In workingset_refault(), we call mem_cgroup_flush_stats_delayed() to > flush stats within an RCU read section and with sleeping disallowed. > Move the call to mem_cgroup_flush_stats_delayed() above the RCU read > section and allow sleeping to avoid unnecessarily performing a lot of > work without sleeping. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > > A lot of code paths call into workingset_refault(), so I am not > generally sure at all whether it's okay to sleep in all contexts or not. > Feedback here would be very helpful. Yes, it's safe.
On Thu, Mar 23, 2023 at 04:00:36AM +0000, Yosry Ahmed wrote: > In workingset_refault(), we call mem_cgroup_flush_stats_delayed() to > flush stats within an RCU read section and with sleeping disallowed. > Move the call to mem_cgroup_flush_stats_delayed() above the RCU read > section and allow sleeping to avoid unnecessarily performing a lot of > work without sleeping. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > > A lot of code paths call into workingset_refault(), so I am not > generally sure at all whether it's okay to sleep in all contexts or not. > Feedback here would be very helpful. > > --- > mm/workingset.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/mm/workingset.c b/mm/workingset.c > index 042eabbb43f6..410bc6684ea7 100644 > --- a/mm/workingset.c > +++ b/mm/workingset.c > @@ -406,6 +406,8 @@ void workingset_refault(struct folio *folio, void *shadow) > unpack_shadow(shadow, &memcgid, &pgdat, &eviction, &workingset); > eviction <<= bucket_order; > > + /* Flush stats (and potentially sleep) before holding RCU read lock */ > + mem_cgroup_flush_stats_delayed(true); Btw, it might be a good time to rename this while you're in the area. delayed suggests this is using a delayed_work, but this is actually sometimes flushing directly from the callsite. What it's doing is ratelimited calls. A better name would be: mem_cgroup_flush_stats_ratelimited()
On Thu, Mar 23, 2023 at 8:50 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Thu, Mar 23, 2023 at 04:00:36AM +0000, Yosry Ahmed wrote: > > In workingset_refault(), we call mem_cgroup_flush_stats_delayed() to > > flush stats within an RCU read section and with sleeping disallowed. > > Move the call to mem_cgroup_flush_stats_delayed() above the RCU read > > section and allow sleeping to avoid unnecessarily performing a lot of > > work without sleeping. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > > > A lot of code paths call into workingset_refault(), so I am not > > generally sure at all whether it's okay to sleep in all contexts or not. > > Feedback here would be very helpful. > > Yes, it's safe. Thanks! That's very helpful!
On Thu, Mar 23, 2023 at 9:00 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Thu, Mar 23, 2023 at 04:00:36AM +0000, Yosry Ahmed wrote: > > In workingset_refault(), we call mem_cgroup_flush_stats_delayed() to > > flush stats within an RCU read section and with sleeping disallowed. > > Move the call to mem_cgroup_flush_stats_delayed() above the RCU read > > section and allow sleeping to avoid unnecessarily performing a lot of > > work without sleeping. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > > > A lot of code paths call into workingset_refault(), so I am not > > generally sure at all whether it's okay to sleep in all contexts or not. > > Feedback here would be very helpful. > > > > --- > > mm/workingset.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/mm/workingset.c b/mm/workingset.c > > index 042eabbb43f6..410bc6684ea7 100644 > > --- a/mm/workingset.c > > +++ b/mm/workingset.c > > @@ -406,6 +406,8 @@ void workingset_refault(struct folio *folio, void *shadow) > > unpack_shadow(shadow, &memcgid, &pgdat, &eviction, &workingset); > > eviction <<= bucket_order; > > > > + /* Flush stats (and potentially sleep) before holding RCU read lock */ > > + mem_cgroup_flush_stats_delayed(true); > > Btw, it might be a good time to rename this while you're in the > area. delayed suggests this is using a delayed_work, but this is > actually sometimes flushing directly from the callsite. > > What it's doing is ratelimited calls. A better name would be: > > mem_cgroup_flush_stats_ratelimited() Agreed. Will do in the next version.
diff --git a/mm/workingset.c b/mm/workingset.c index 042eabbb43f6..410bc6684ea7 100644 --- a/mm/workingset.c +++ b/mm/workingset.c @@ -406,6 +406,8 @@ void workingset_refault(struct folio *folio, void *shadow) unpack_shadow(shadow, &memcgid, &pgdat, &eviction, &workingset); eviction <<= bucket_order; + /* Flush stats (and potentially sleep) before holding RCU read lock */ + mem_cgroup_flush_stats_delayed(true); rcu_read_lock(); /* * Look up the memcg associated with the stored ID. It might @@ -461,9 +463,6 @@ void workingset_refault(struct folio *folio, void *shadow) lruvec = mem_cgroup_lruvec(memcg, pgdat); mod_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file, nr); - - /* Do not sleep with RCU lock held */ - mem_cgroup_flush_stats_delayed(false); /* * Compare the distance to the existing workingset size. We * don't activate pages that couldn't stay resident even if
In workingset_refault(), we call mem_cgroup_flush_stats_delayed() to flush stats within an RCU read section and with sleeping disallowed. Move the call to mem_cgroup_flush_stats_delayed() above the RCU read section and allow sleeping to avoid unnecessarily performing a lot of work without sleeping. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- A lot of code paths call into workingset_refault(), so I am not generally sure at all whether it's okay to sleep in all contexts or not. Feedback here would be very helpful. --- mm/workingset.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)