Date:   Wed, 1 Feb 2023 12:29:16 +0900
From:   Sergey Senozhatsky <senozhatsky@chromium.org>
To:     Nhat Pham <nphamcs@gmail.com>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        akpm@linux-foundation.org, hannes@cmpxchg.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, minchan@kernel.org,
        ngupta@vflare.org, sjenning@redhat.com, ddstreet@ieee.org,
        vitaly.wool@konsulko.com, kernel-team@meta.com
Subject: Re: [PATCH] zsmalloc: fix a race with deferred_handles storing
Message-ID: <Y9ncjPEFEU33ENVq@google.com>
References: <20230110231701.326724-1-nphamcs@gmail.com>
 <Y9nDXBt2OR3hg5X7@google.com>
 <CAKEwX=My-B=KkocO0heMm=7e+Qxkg2RdRJ8pRHm9dBk+Cceyzw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKEwX=My-B=KkocO0heMm=7e+Qxkg2RdRJ8pRHm9dBk+Cceyzw@mail.gmail.com>
Precedence: bulk

On (23/01/31 18:28), Nhat Pham wrote:
> > On (23/01/10 15:17), Nhat Pham wrote:
> > [..]
> > >  #ifdef CONFIG_ZPOOL
> > > +static void restore_freelist(struct zs_pool *pool, struct size_class *class,
> > > +             struct zspage *zspage)
> > > +{
> > > +     unsigned int obj_idx = 0;
> > > +     unsigned long handle, off = 0; /* off is within-page offset */
> > > +     struct page *page = get_first_page(zspage);
> > > +     struct link_free *prev_free = NULL;
> > > +     void *prev_page_vaddr = NULL;
> > > +
> > > +     /* in case no free object found */
> > > +     set_freeobj(zspage, (unsigned int)(-1UL));
> >
> > I'm not following this. I see how -1UL works for link_free, but this
> > cast of -1UL to 4 bytes looks suspicious.
> 
> (resending this since I forgot to forward this to other recipients)
> 
> It is a bit convoluted indeed. But the idea is that for the last object,
> the last link is given by:
> 
> link->next = -1UL << OBJ_TAG_BITS
> 
> And at malloc time, we update freeobj as follows
> set_freeobj(zspage, link->next >> OBJ_TAG_BITS);
> 
> Which means the freeobj value would be set to something like this:
> (-1UL << OBJ_TAG_BITS) >> OBJ_TAG_BITS

Oh, good point. I see what you did there.

> I want to emulate this here (i.e in the case we have no free object).

Makes sense.

> As for the casting, I believe set_freeobj requires an unsigned int for
> the second field.
> 
> Alternatively, to be 100% safe, we can do something like this:
> (unsigned int)((-1UL << OBJ_TAG_BITS) >> OBJ_TAG_BITS)
> 
> But I think I got the same result as just (unsigned int)(-1UL)

Yeah, I guess they should be the same, as we take the lower 4 bytes
only.