All of lore.kernel.org
 help / color / mirror / Atom feed
From: Shakeel Butt <shakeelb@google.com>
To: Tejun Heo <tj@kernel.org>
Cc: "Johannes Weiner" <hannes@cmpxchg.org>,
	"Michal Koutný" <mkoutny@suse.com>,
	cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Shakeel Butt" <shakeelb@google.com>
Subject: [PATCH] cgroup: rstat: optimize flush through speculative test
Date: Wed, 29 Sep 2021 16:59:36 -0700	[thread overview]
Message-ID: <20210929235936.2859271-1-shakeelb@google.com> (raw)

Currently cgroup_rstat_updated() has a speculative already-on-list test
to check if the given cgroup is already part of the rstat update tree.
This helps in reducing the contention on the rstat cpu lock. This patch
adds the similar speculative not-on-list test on the rstat flush
codepath.

Recently the commit aa48e47e3906 ("memcg: infrastructure to flush memcg
stats") added periodic rstat flush. On a large system which is not much
busy, most of the per-cpu rstat tree would be empty. So, the speculative
not-on-list test helps in eliminating unnecessary work and potentially
reducing contention on the rstat cpu lock. Please note this might
introduce temporary inaccuracy but with the frequent and periodic flush
this would not be an issue.

To evaluate the impact of this patch, an 8 GiB tmpfs file is created on
a system with swap-on-zram and the file was pushed to swap through
memory.force_empty interface. On reading the whole file, the memcg stat
flush in the refault code path is triggered. With this patch, we
observed 38% reduction in the read time of 8 GiB file.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
---
 kernel/cgroup/rstat.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index b264ab5652ba..748494fbc786 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -35,7 +35,7 @@ void cgroup_rstat_updated(struct cgroup *cgrp, int cpu)
 	 * instead of NULL, we can tell whether @cgrp is on the list by
 	 * testing the next pointer for NULL.
 	 */
-	if (cgroup_rstat_cpu(cgrp, cpu)->updated_next)
+	if (data_race(cgroup_rstat_cpu(cgrp, cpu)->updated_next))
 		return;
 
 	raw_spin_lock_irqsave(cpu_lock, flags);
@@ -157,6 +157,13 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
 						       cpu);
 		struct cgroup *pos = NULL;
 
+		/*
+		 * Speculative not-on-list test. This may lead to temporary
+		 * inaccuracies which is fine.
+		 */
+		if (!data_race(cgroup_rstat_cpu(cgrp, cpu)->updated_next))
+			goto next;
+
 		raw_spin_lock(cpu_lock);
 		while ((pos = cgroup_rstat_cpu_pop_updated(pos, cgrp, cpu))) {
 			struct cgroup_subsys_state *css;
@@ -170,7 +177,7 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
 			rcu_read_unlock();
 		}
 		raw_spin_unlock(cpu_lock);
-
+next:
 		/* if @may_sleep, play nice and yield if necessary */
 		if (may_sleep && (need_resched() ||
 				  spin_needbreak(&cgroup_rstat_lock))) {
-- 
2.33.0.685.g46640cef36-goog


WARNING: multiple messages have this Message-ID (diff)
From: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
To: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: "Johannes Weiner"
	<hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>,
	"Michal Koutný" <mkoutny-IBi9RG/b67k@public.gmane.org>,
	cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	"Shakeel Butt" <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Subject: [PATCH] cgroup: rstat: optimize flush through speculative test
Date: Wed, 29 Sep 2021 16:59:36 -0700	[thread overview]
Message-ID: <20210929235936.2859271-1-shakeelb@google.com> (raw)

Currently cgroup_rstat_updated() has a speculative already-on-list test
to check if the given cgroup is already part of the rstat update tree.
This helps in reducing the contention on the rstat cpu lock. This patch
adds the similar speculative not-on-list test on the rstat flush
codepath.

Recently the commit aa48e47e3906 ("memcg: infrastructure to flush memcg
stats") added periodic rstat flush. On a large system which is not much
busy, most of the per-cpu rstat tree would be empty. So, the speculative
not-on-list test helps in eliminating unnecessary work and potentially
reducing contention on the rstat cpu lock. Please note this might
introduce temporary inaccuracy but with the frequent and periodic flush
this would not be an issue.

To evaluate the impact of this patch, an 8 GiB tmpfs file is created on
a system with swap-on-zram and the file was pushed to swap through
memory.force_empty interface. On reading the whole file, the memcg stat
flush in the refault code path is triggered. With this patch, we
observed 38% reduction in the read time of 8 GiB file.

Signed-off-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup/rstat.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index b264ab5652ba..748494fbc786 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -35,7 +35,7 @@ void cgroup_rstat_updated(struct cgroup *cgrp, int cpu)
 	 * instead of NULL, we can tell whether @cgrp is on the list by
 	 * testing the next pointer for NULL.
 	 */
-	if (cgroup_rstat_cpu(cgrp, cpu)->updated_next)
+	if (data_race(cgroup_rstat_cpu(cgrp, cpu)->updated_next))
 		return;
 
 	raw_spin_lock_irqsave(cpu_lock, flags);
@@ -157,6 +157,13 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
 						       cpu);
 		struct cgroup *pos = NULL;
 
+		/*
+		 * Speculative not-on-list test. This may lead to temporary
+		 * inaccuracies which is fine.
+		 */
+		if (!data_race(cgroup_rstat_cpu(cgrp, cpu)->updated_next))
+			goto next;
+
 		raw_spin_lock(cpu_lock);
 		while ((pos = cgroup_rstat_cpu_pop_updated(pos, cgrp, cpu))) {
 			struct cgroup_subsys_state *css;
@@ -170,7 +177,7 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
 			rcu_read_unlock();
 		}
 		raw_spin_unlock(cpu_lock);
-
+next:
 		/* if @may_sleep, play nice and yield if necessary */
 		if (may_sleep && (need_resched() ||
 				  spin_needbreak(&cgroup_rstat_lock))) {
-- 
2.33.0.685.g46640cef36-goog


             reply	other threads:[~2021-09-30  0:00 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-29 23:59 Shakeel Butt [this message]
2021-09-29 23:59 ` [PATCH] cgroup: rstat: optimize flush through speculative test Shakeel Butt
2021-10-04 17:00 ` Tejun Heo
2021-10-04 17:00   ` Tejun Heo
2021-10-04 17:25   ` Shakeel Butt
2021-10-04 17:44     ` Tejun Heo
2021-10-04 17:44       ` Tejun Heo
2021-10-04 18:07       ` Shakeel Butt
2021-10-04 18:07         ` Shakeel Butt
2021-10-04 18:21         ` Tejun Heo
2021-10-04 18:21           ` Tejun Heo
2021-10-04 18:35           ` Shakeel Butt
2021-10-04 18:35             ` Shakeel Butt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210929235936.2859271-1-shakeelb@google.com \
    --to=shakeelb@google.com \
    --cc=cgroups@vger.kernel.org \
    --cc=hannes@cmpxchg.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mkoutny@suse.com \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.