Re: [PATCH] zsmalloc: fix a race with deferred_handles storing

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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.