Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 18 Feb 2021 20:13:05 +0100
From:   Michal Hocko <mhocko@suse.com>
To:     Tim Chen <tim.c.chen@linux.intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Ying Huang <ying.huang@intel.com>, linux-mm@kvack.org,
        cgroups@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] mm: Fix dropped memcg from mem cgroup soft limit
 tree
Message-ID: <YC68QRVsCONXscCl@dhcp22.suse.cz>
References: <cover.1613584277.git.tim.c.chen@linux.intel.com>
 <8d35206601ccf0e1fe021d24405b2a0c2f4e052f.1613584277.git.tim.c.chen@linux.intel.com>
 <YC4kV7dkJpxjW+df@dhcp22.suse.cz>
 <c3ffa2cb-cb2c-20b7-d722-c875934992e9@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c3ffa2cb-cb2c-20b7-d722-c875934992e9@linux.intel.com>
Precedence: bulk

On Thu 18-02-21 10:30:20, Tim Chen wrote:
> 
> 
> On 2/18/21 12:24 AM, Michal Hocko wrote:
> 
> > 
> > I have already acked this patch in the previous version along with Fixes
> > tag. It seems that my review feedback has been completely ignored also
> > for other patches in this series.
> 
> Michal,
> 
> My apology.  Our mail system screwed up and there are some mail missing
> from our mail system that I completely missed your mail.  
> Only saw them now after I looked into the lore.kernel.org.

I see. My apology for suspecting you from ignoring my review.
 
> Responding to your comment:
> 
> >Have you observed this happening in the real life? I do agree that the
> >threshold based updates of the tree is not ideal but the whole soft
> >reclaim code is far from optimal. So why do we care only now? The
> >feature is essentially dead and fine tuning it sounds like a step back
> >to me.
> 
> Yes, I did see the issue mentioned in patch 2 breaking soft limit
> reclaim for cgroup v1.  There are still some of our customers using
> cgroup v1 so we will like to fix this if possible.

It would be great to see more details.

> For patch 3 regarding the uncharge_batch, it
> is more of an observation that we should uncharge in batch of same node
> and not prompted by actual workload.
> Thinking more about this, the worst that could happen
> is we could have some entries in the soft limit tree that overestimate
> the memory used.  The worst that could happen is a soft page reclaim
> on that cgroup.  The overhead from extra memcg event update could
> be more than a soft page reclaim pass.  So let's drop patch 3
> for now.

I would still prefer to handle that in the soft limit reclaim path and
check each memcg for the soft limit reclaim excess before the reclaim.
 
> Let me know if you will like me to resend patch 1 with the fixes tag
> for commit 4e41695356fb ("memory controller: soft limit reclaim on contention")
> and if there are any changes I should make for patch 2.

I will ack and suggest Fixes.

> 
> Thanks.
> 
> Tim

-- 
Michal Hocko
SUSE Labs