From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ming Lei Subject: Re: [PATCH v6 3/3] blk-cgroup: Optimize blkcg_rstat_flush() Date: Mon, 6 Jun 2022 11:15:45 +0800 Message-ID: References: <20220602192020.166940-1-longman@redhat.com> <20220602192020.166940-4-longman@redhat.com> <9272d9ba-1b34-991b-78b5-beca5e170a23@redhat.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1654485358; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=a3GZiqjzOQU3jcJYnFlk7GQp3wzOjfJX1PgQLpdIZe0=; b=ZghtTOD/FnBjQArZuem4K6Nos/eDtLQXaplClynOX8RWszNiDk4ODf3QxKqjQE62kGJ9T4 VkCdn971L0iQcVYK5BkWr/UQfee6Frj1/hYYdTY8Npl4czVqNZcH0NAqo9iPreOLyy79cS f62AAjrEoX2iJjArjvakEKuogAj94eU= Content-Disposition: inline In-Reply-To: <9272d9ba-1b34-991b-78b5-beca5e170a23@redhat.com> List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Waiman Long Cc: Tejun Heo , Jens Axboe , cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org On Sun, Jun 05, 2022 at 10:58:15PM -0400, Waiman Long wrote: > On 6/5/22 22:23, Ming Lei wrote: > > On Sun, Jun 05, 2022 at 09:59:50PM -0400, Waiman Long wrote: > > > On 6/5/22 21:39, Ming Lei wrote: > > > > On Sun, Jun 05, 2022 at 07:15:27PM -0400, Waiman Long wrote: > > > > > On 6/3/22 23:58, Ming Lei wrote: > > > > > > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > > > index abec50f31fe6..8c4f204dbf5b 100644 > > > > > > --- a/mm/memcontrol.c > > > > > > +++ b/mm/memcontrol.c > > > > > > @@ -622,7 +622,7 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) > > > > > > { > > > > > > unsigned int x; > > > > > > - cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id()); > > > > > > + cgroup_rstat_updated(memcg->css.cgroup, smp_processor_id(), NULL); > > > > > > x = __this_cpu_add_return(stats_updates, abs(val)); > > > > > > if (x > MEMCG_CHARGE_BATCH) { > > > > > I think the rstat set of functions are doing that already. So flush will > > > > > only call CPUs that have called cgroup_rstat_updated() before. However, one > > > > Yeah, I guess the detail is in cgroup_rstat_cpu_pop_updated(), but the > > > > percpu lock(raw_spin_lock_irqsave) is still required, and cgroup_rstat_cpu_pop_updated() > > > > is still called even through there isn't any update on this CPU. > > > Yes, I think we may need to add a bitmask of what controllers have updates > > > in cgroup_rstat_cpu structure. > > > > > deficiency that I am aware of is that there is no bitmap of which controller > > > > > have update. The problem that I saw in cgroup v2 is that in a cgroup with > > > > > both memory controller and block controller enabled, a > > > > > cgroup_rstat_updated() call from memory cgroup later causes the rstat > > > > > function to call into block cgroup flush method even though there is no > > > > > update in the block controller. This is an area that needs improvement. > > > > > > > > > > Your code does allow the block controller to be aware of that and avoid > > > > > further action, but I think it has to be done in the rstat code to be > > > > > applicable to all controllers instead of just specific to block controller. > > > > I guess it can be done by adding one percpu variable to 'struct cgroup'. > > > > > > > > > There is another problem that this approach. Suppose the system have 20 > > > > > block devices and one of them has an IO operation. Now the flush method > > > > > still needs to iterate all the 20 blkg's to do an update. The block > > > > > controller is kind of special that the number of per-cgroup IO stats depends > > > > > on the number of block devices present. Other controllers just have one set > > > > > of stats per cgroup. > > > > Yeah, and this one is really blkio specific issue, and your patch does > > > > cover this one. Maybe you can add one callback to > > > > cgroup_rstat_updated(), so the "blkg_iostat_set" instance is added into > > > > percpu list under percpu lock of cgroup_rstat_cpu_lock, then the lockless > > > > list isn't needed. > > > The rstat API is generic. It may not be a good idea to put controller > > > specific information into it. Yes, cgroup_rstat_cpu_lock is taken at the > > > read side (flush). It may not taken on the write side (update). So it may > > Both cgroup_rstat_flush_locked()/cgroup_rstat_updated() take the percpu > > cgroup_rstat_cpu_lock, so the new invented lockless list can be > > replaced with plain list. > > cgroup_rstat_updated() should only take the percpu cgroup_rstat_cpu_lock the > first time it transition from "!updated" to "updated". After that, it > returns immediately without the the lock. With a regular list, you will have > to take the lock every time a new block device has an update. So there isn't > much saving on the update side. In general, the lock/unlock sequence has a > bit more overhead than the lockless insertion. On the flush side, there may > be a bit of saving, but it is not the fast path. OK, got it, looks I misunderstood cgroup_rstat_updated(), and still the point of N queue vs. 1 cgroup, then looks your patch is good. Thanks, Ming