| Message ID | 20230323040037.2389095-3-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Make rstat flushing IRQ and sleep friendly | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Mar 22, 2023 at 9:00 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > The rstat flushing code was modified so that we do not disable interrupts > when we hold the global rstat lock. Do the same for stats_flush_lock on > the memcg side to avoid unnecessarily disabling interrupts throughout > flushing. > > Since the code exclusively uses trylock to acquire this lock, it should > be fine to hold from interrupt contexts or normal contexts without > disabling interrupts as a deadlock cannot occur. For interrupt contexts > we will return immediately without flushing anyway. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > mm/memcontrol.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 5abffe6f8389..e0e92b38fa51 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -636,15 +636,17 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) > > static void __mem_cgroup_flush_stats(void) > { > - unsigned long flag; > - > - if (!spin_trylock_irqsave(&stats_flush_lock, flag)) > + /* > + * This lock can be acquired from interrupt context, How? What's the code path? > but we only acquire > + * using trylock so it should be fine as we cannot cause a deadlock. > + */ > + if (!spin_trylock(&stats_flush_lock)) > return; > > flush_next_time = jiffies_64 + 2*FLUSH_TIME; > cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup); > atomic_set(&stats_flush_threshold, 0); > - spin_unlock_irqrestore(&stats_flush_lock, flag); > + spin_unlock(&stats_flush_lock); > } > > void mem_cgroup_flush_stats(void) > -- > 2.40.0.rc1.284.g88254d51c5-goog >
On Wed, Mar 22, 2023 at 9:32 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, Mar 22, 2023 at 9:00 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > The rstat flushing code was modified so that we do not disable interrupts > > when we hold the global rstat lock. Do the same for stats_flush_lock on > > the memcg side to avoid unnecessarily disabling interrupts throughout > > flushing. > > > > Since the code exclusively uses trylock to acquire this lock, it should > > be fine to hold from interrupt contexts or normal contexts without > > disabling interrupts as a deadlock cannot occur. For interrupt contexts > > we will return immediately without flushing anyway. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > mm/memcontrol.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 5abffe6f8389..e0e92b38fa51 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -636,15 +636,17 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) > > > > static void __mem_cgroup_flush_stats(void) > > { > > - unsigned long flag; > > - > > - if (!spin_trylock_irqsave(&stats_flush_lock, flag)) > > + /* > > + * This lock can be acquired from interrupt context, > > How? What's the code path? I believe through the charge/uncharge path we can do memcg_check_events()->mem_cgroup_threshold()->mem_cgroup_usage()->mem_cgroup_flush_stats(), right? I am assuming we can charge/uncharge memory in an interrupt context. Also the current code always disables interrupts before calling memcg_check_events(), which made me suspect the percpu variables that are modified by that call can also be modified in interrupt context. > > > but we only acquire > > + * using trylock so it should be fine as we cannot cause a deadlock. > > + */ > > + if (!spin_trylock(&stats_flush_lock)) > > return; > > > > flush_next_time = jiffies_64 + 2*FLUSH_TIME; > > cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup); > > atomic_set(&stats_flush_threshold, 0); > > - spin_unlock_irqrestore(&stats_flush_lock, flag); > > + spin_unlock(&stats_flush_lock); > > } > > > > void mem_cgroup_flush_stats(void) > > -- > > 2.40.0.rc1.284.g88254d51c5-goog > >
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 5abffe6f8389..e0e92b38fa51 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -636,15 +636,17 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) static void __mem_cgroup_flush_stats(void) { - unsigned long flag; - - if (!spin_trylock_irqsave(&stats_flush_lock, flag)) + /* + * This lock can be acquired from interrupt context, but we only acquire + * using trylock so it should be fine as we cannot cause a deadlock. + */ + if (!spin_trylock(&stats_flush_lock)) return; flush_next_time = jiffies_64 + 2*FLUSH_TIME; cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup); atomic_set(&stats_flush_threshold, 0); - spin_unlock_irqrestore(&stats_flush_lock, flag); + spin_unlock(&stats_flush_lock); } void mem_cgroup_flush_stats(void)
The rstat flushing code was modified so that we do not disable interrupts when we hold the global rstat lock. Do the same for stats_flush_lock on the memcg side to avoid unnecessarily disabling interrupts throughout flushing. Since the code exclusively uses trylock to acquire this lock, it should be fine to hold from interrupt contexts or normal contexts without disabling interrupts as a deadlock cannot occur. For interrupt contexts we will return immediately without flushing anyway. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- mm/memcontrol.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)