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: Minchan Kim <minchan.kim@gmail.com>
Date:   Tue, 9 May 2023 11:20:02 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Nhat Pham <nphamcs@gmail.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        ngupta@vflare.org, sjenning@redhat.com, ddstreet@ieee.org,
        vitaly.wool@konsulko.com, kernel-team@meta.com
Subject: Re: [PATCH] zsmalloc: move LRU update from zs_map_object() to
 zs_malloc()
Message-ID: <ZFqO0i4mWL508P0S@google.com>
References: <20230505185054.2417128-1-nphamcs@gmail.com>
 <20230506030140.GC3281499@google.com>
 <20230508140658.GA3421@cmpxchg.org>
 <CAKEwX=MtunOe6A--SG3ud-gUFg3bXFJgG4csgwHeZFAEqjCgHg@mail.gmail.com>
 <20230509030030.GD11511@google.com>
 <20230509174401.GA18828@cmpxchg.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230509174401.GA18828@cmpxchg.org>
Precedence: bulk

Hi Folks,

On Tue, May 09, 2023 at 01:44:01PM -0400, Johannes Weiner wrote:
> On Tue, May 09, 2023 at 12:00:30PM +0900, Sergey Senozhatsky wrote:
> > On (23/05/08 09:00), Nhat Pham wrote:
> > > > The deeper bug here is that zs_map_object() tries to add the page to
> > > > the LRU list while the shrinker has it isolated for reclaim. This is
> > > > way too sutble and error prone. Even if it worked now, it'll cause
> > > > corruption issues down the line.
> > > >
> > > > For example, Nhat is adding a secondary entry point to reclaim.
> > > > Reclaim expects that a page that's on the LRU is also on the fullness
> > > > list, so this would lead to a double remove_zspage() and BUG_ON().
> > > >
> > > > This patch doesn't just fix the crash, it eliminates the deeper LRU
> > > > isolation issue and makes the code more robust and simple.
> > > 
> > > I agree. IMO, less unnecessary concurrent interaction is always a
> > > win for developers' and maintainers' cognitive load.
> > 
> > Thanks for all the explanations.
> > 
> > > As a side benefit - this also gets rid of the inelegant check
> > > (mm == ZS_MM_WO). The fact that we had to include a
> > > a multi-paragraph explanation for a 3-line piece of code
> > > should have been a red flag.
> > 
> > Minchan had some strong opinion on that, so we need to hear from him
> > before we decide how do we fix it.
> 
> I'd be happy if he could validate the fix. But this fixes a crash, so
> the clock is ticking.
> 
> I will also say, his was a design preference. One we agreed to only
> very reluctantly: https://lore.kernel.org/lkml/Y3f6habiVuV9LMcu@google.com/
> 
> Now we have a crash that is a direct result of it, and which cost us
> (and apparently is still costing us) time and energy to resolve.
> 
> Unless somebody surfaces a real technical problem with the fix, I'd
> say let's do it our way this time.
> 

Sorry for being too late to review. The reason I insisted on it was
I overlookeded the bug and thought it was trivial change but better
semantic since zsmalloc provides separate API between allocation and
access unlike other allocators. Now, Nhat and Johannes provided it's
more error prone, I am totally fine with this fix and will live it
until the LRU writeback will move out of allocator.

Sorry for wasting your time to hunt this bug down and thank you for fix!

Acked-by: Minchan Kim <minchan@kernel.org>