Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Tue, 26 Jul 2022 13:14:09 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Oleg Nesterov <onestero@redhat.com>
Cc:     Christian Brauner <brauner@kernel.org>,
        Michal =?iso-8859-1?Q?Koutn=FD?= <mkoutny@suse.com>,
        Peter Zijlstra <peterz@infradead.org>,
        John Stultz <john.stultz@linaro.org>,
        Dmitry Shmidt <dimitrysh@google.com>,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org
Subject: Re: [PATCH RESEND 3/3 cgroup/for-5.20] cgroup: Make !percpu
 threadgroup_rwsem operations optional
Message-ID: <YuB1QW6Kce5nkBu6@slm.duckdns.org>
References: <YtDvN0wJ6CKaEPN8@slm.duckdns.org>
 <YtDvU4jRPSsarcNp@slm.duckdns.org>
 <YtDvl7Qjc5zI3e/b@slm.duckdns.org>
 <YtwFjPnCtw8ySnuv@slm.duckdns.org>
 <20220725121208.GB28662@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220725121208.GB28662@redhat.com>
Precedence: bulk

Hello, Oleg.

On Mon, Jul 25, 2022 at 02:12:09PM +0200, Oleg Nesterov wrote:
> I see no problems in this patch. But just for record, we do not need
> synchronize_rcu() in the "favor && !favoring" case, so we cab probably
> do something like
> 
> 	@@ -146,13 +146,20 @@ void rcu_sync_enter(struct rcu_sync *rsp)
> 			 * See the comment above, this simply does the "synchronous"
> 			 * call_rcu(rcu_sync_func) which does GP_ENTER -> GP_PASSED.
> 			 */
> 	+		if (wait) {
> 	+			synchronize_rcu();
> 	+			rcu_sync_func(&rsp->cb_head);
> 	+		} else {
> 	+			rcu_sync_call(rsp);
> 	+		}
> 	+	} else if (wait) {
> 	+		wait_event(rsp->gp_wait, READ_ONCE(rsp->gp_state) >= GP_PASSED);
...
> later.
> 
> __rcu_sync_enter(rsp, false) works just like rcu_sync_enter_start() but it can
> be safely called at any moment.

Yeah, I originally used rcu_sync_enter_start() but quickly found out that it
can't be reverted reliably. Given how cold the option switching path is, I
think it's fine to pay an extra synchronize_rcu() there rather than adding
more complexity to rcu_sync_enter() unless this will be useful somewhere
else too.

> And can't resist, off-topic question... Say, cgroup_attach_task_all() does
> 
> 	mutex_lock(&cgroup_mutex);
> 	percpu_down_write(&cgroup_threadgroup_rwsem);
> 
> and this means that synchronize_rcu() can be called with cgroup_mutex held.
> Perhaps it makes sense to change this code to do
> 
> 	rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
> 	mutex_lock(&cgroup_mutex);
> 	percpu_down_write(&cgroup_threadgroup_rwsem);
> 	...
> 	percpu_up_write(&cgroup_threadgroup_rwsem);
> 	mutex_unlock(&cgroup_mutex);
> 	rcu_sync_exit(&cgroup_threadgroup_rwsem.rss);
> 
> ? Just curious.

I'm not quite following. Are you saying that if we switching the rwsem into
slow mode before grabbing the locks, we can avoid inducing latencies on
other users? Hmm... assuming that I'm understanding you correctly, one
problem with that approach is that everyone would be doing synchronize_rcu()
whether they want to change favoring state. In vast majority of cases, users
won't care about this flag but most users will end up mounting cgroup and do
the rcu_sync_enter(), so we'd end up adding a grace period wait in most boot
scenarios. It's not a lot in itself but seems less desriable than making the
users who want to change the mode pay at the time of changing.

> > -	/*
> > -	 * The latency of the synchronize_rcu() is too high for cgroups,
> > -	 * avoid it at the cost of forcing all readers into the slow path.
> > -	 */
> > -	rcu_sync_enter_start(&cgroup_threadgroup_rwsem.rss);
> 
> Note that it doesn't have other users, probably you can kill it.

Ah, nice, will do that.

Thanks.

-- 
tejun