diff mbox series

[RFC,2/7] memcg: do not disable interrupts when holding stats_flush_lock

Message ID 20230323040037.2389095-3-yosryahmed@google.com (mailing list archive)
State RFC
Headers show
Series Make rstat flushing IRQ and sleep friendly | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Yosry Ahmed March 23, 2023, 4 a.m. UTC
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(-)

Comments

Shakeel Butt March 23, 2023, 4:32 a.m. UTC | #1
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
>
Yosry Ahmed March 23, 2023, 5:16 a.m. UTC | #2
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 mbox series

Patch

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)