| Message ID | 20230328061638.203420-5-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | memcg: make rstat flushing irq and sleep friendly | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Mar 27, 2023 at 11:16 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > rstat flushing is too expensive to perform in irq context. > The previous patch removed the only context that may invoke an rstat > flush from irq context, add a WARN_ON_ONCE() to detect future > violations, or those that we are not aware of. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> Reviewed-by: Shakeel Butt <shakeelb@google.com>
On Tue, Mar 28, 2023 at 06:16:33AM +0000, Yosry Ahmed wrote: > rstat flushing is too expensive to perform in irq context. > The previous patch removed the only context that may invoke an rstat > flush from irq context, add a WARN_ON_ONCE() to detect future > violations, or those that we are not aware of. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > kernel/cgroup/rstat.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > index d3252b0416b6..c2571939139f 100644 > --- a/kernel/cgroup/rstat.c > +++ b/kernel/cgroup/rstat.c > @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep) > { > int cpu; > > + /* rstat flushing is too expensive for irq context */ > + WARN_ON_ONCE(!in_task()); > lockdep_assert_held(&cgroup_rstat_lock); This seems a bit arbitrary. Why is an irq caller forbidden, but an irq-disabled, non-preemptible section caller is allowed? The latency impact on the system would be the same, right?
On Tue, Mar 28, 2023 at 10:49 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Tue, Mar 28, 2023 at 06:16:33AM +0000, Yosry Ahmed wrote: > > rstat flushing is too expensive to perform in irq context. > > The previous patch removed the only context that may invoke an rstat > > flush from irq context, add a WARN_ON_ONCE() to detect future > > violations, or those that we are not aware of. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > kernel/cgroup/rstat.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > > index d3252b0416b6..c2571939139f 100644 > > --- a/kernel/cgroup/rstat.c > > +++ b/kernel/cgroup/rstat.c > > @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep) > > { > > int cpu; > > > > + /* rstat flushing is too expensive for irq context */ > > + WARN_ON_ONCE(!in_task()); > > lockdep_assert_held(&cgroup_rstat_lock); > > This seems a bit arbitrary. Why is an irq caller forbidden, but an > irq-disabled, non-preemptible section caller is allowed? The latency > impact on the system would be the same, right? Thanks for taking a look. So in the first patch series the initial purpose was to make sure cgroup_rstat_lock was never acquired in an irq context, so that we can stop disabling irqs while holding it. Tejun disagreed with this approach though. We currently have one caller that calls flushing with irqs disabled (mem_cgroup_usage()) -- so we cannot forbid such callers (yet), but I thought we can at least forbid callers from irq context now (or catch those that we are not aware of), and then maybe forbid irqs_disabled() contexts as well we can get rid of that callsite. WDYT?
On Tue, Mar 28, 2023 at 11:59 AM Yosry Ahmed <yosryahmed@google.com> wrote: > > On Tue, Mar 28, 2023 at 10:49 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > On Tue, Mar 28, 2023 at 06:16:33AM +0000, Yosry Ahmed wrote: > > > rstat flushing is too expensive to perform in irq context. > > > The previous patch removed the only context that may invoke an rstat > > > flush from irq context, add a WARN_ON_ONCE() to detect future > > > violations, or those that we are not aware of. > > > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > > --- > > > kernel/cgroup/rstat.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > > > index d3252b0416b6..c2571939139f 100644 > > > --- a/kernel/cgroup/rstat.c > > > +++ b/kernel/cgroup/rstat.c > > > @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep) > > > { > > > int cpu; > > > > > > + /* rstat flushing is too expensive for irq context */ > > > + WARN_ON_ONCE(!in_task()); > > > lockdep_assert_held(&cgroup_rstat_lock); > > > > This seems a bit arbitrary. Why is an irq caller forbidden, but an > > irq-disabled, non-preemptible section caller is allowed? The latency > > impact on the system would be the same, right? > > Thanks for taking a look. > > So in the first patch series the initial purpose was to make sure > cgroup_rstat_lock was never acquired in an irq context, so that we can > stop disabling irqs while holding it. Tejun disagreed with this > approach though. > > We currently have one caller that calls flushing with irqs disabled > (mem_cgroup_usage()) -- so we cannot forbid such callers (yet), but I > thought we can at least forbid callers from irq context now (or catch > those that we are not aware of), and then maybe forbid irqs_disabled() > contexts as well we can get rid of that callsite. > > WDYT? I added more context in the commit log in the v2 respin [1]. Let me know if you want me to change something else, rephrase the comment, or drop the patch entirely. [1]https://lore.kernel.org/linux-mm/20230328221644.803272-5-yosryahmed@google.com/
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c index d3252b0416b6..c2571939139f 100644 --- a/kernel/cgroup/rstat.c +++ b/kernel/cgroup/rstat.c @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep) { int cpu; + /* rstat flushing is too expensive for irq context */ + WARN_ON_ONCE(!in_task()); lockdep_assert_held(&cgroup_rstat_lock); for_each_possible_cpu(cpu) {
rstat flushing is too expensive to perform in irq context. The previous patch removed the only context that may invoke an rstat flush from irq context, add a WARN_ON_ONCE() to detect future violations, or those that we are not aware of. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- kernel/cgroup/rstat.c | 2 ++ 1 file changed, 2 insertions(+)