On (24/02/27 03:02), Chengming Zhou wrote:
[..]
> @@ -978,10 +974,11 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
> pages[i] = page;
> }
>
> - create_page_chain(class, zspage, pages);
> init_zspage(class, zspage);
> zspage->pool = pool;
> zspage->class = class->index;
> + /* RCU set_zspage() after zspage initialized. */
> + create_page_chain(class, zspage, pages);
So this hasn't been tested, has it?
init_zspage() does not like to be invoked before create_page_chain(),
because we haven't setup required pointers yet.
So when init_zspage() calls get_first_page() it gets NULL zspage->first_page
which we then use in is_first_page(first_page)->PagePrivate(page). As far as
I can tell.
On 2024/2/28 12:33, Sergey Senozhatsky wrote:
> On (24/02/27 03:02), Chengming Zhou wrote:
> [..]
>> @@ -978,10 +974,11 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
>> pages[i] = page;
>> }
>>
>> - create_page_chain(class, zspage, pages);
>> init_zspage(class, zspage);
>> zspage->pool = pool;
>> zspage->class = class->index;
>> + /* RCU set_zspage() after zspage initialized. */
>> + create_page_chain(class, zspage, pages);
>
> So this hasn't been tested, has it?
I have tested it in my test vm, but it hasn't KASAN enabled. I tested the
kernel build in tmpfs with zswap enabled using zsmalloc pool, not sure
why the kernel didn't crash then...
>
> init_zspage() does not like to be invoked before create_page_chain(),
> because we haven't setup required pointers yet.
You're right, I can reproduce the problem with KASAN enabled this time,
create_page_chain() should be put before init_zspage(), which will iterate
over the pages to create free objects list.
>
> So when init_zspage() calls get_first_page() it gets NULL zspage->first_page
> which we then use in is_first_page(first_page)->PagePrivate(page). As far as
> I can tell.
Thanks! I will fix it and test throughly before send an update.
On (24/02/28 13:14), Chengming Zhou wrote:
> On 2024/2/28 12:33, Sergey Senozhatsky wrote:
> > On (24/02/27 03:02), Chengming Zhou wrote:
> > [..]
> >> @@ -978,10 +974,11 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
> >> pages[i] = page;
> >> }
> >>
> >> - create_page_chain(class, zspage, pages);
> >> init_zspage(class, zspage);
> >> zspage->pool = pool;
> >> zspage->class = class->index;
> >> + /* RCU set_zspage() after zspage initialized. */
> >> + create_page_chain(class, zspage, pages);
> >
> > So this hasn't been tested, has it?
> I have tested it in my test vm, but it hasn't KASAN enabled. I tested the
> kernel build in tmpfs with zswap enabled using zsmalloc pool, not sure
> why the kernel didn't crash then...
I hit the problem on non-kasan-enabled kernel. KASAN was enabled
later on.
[..]
> > So when init_zspage() calls get_first_page() it gets NULL zspage->first_page
> > which we then use in is_first_page(first_page)->PagePrivate(page). As far as
> > I can tell.
>
> Thanks! I will fix it and test throughly before send an update.
I'm curious if we want to add RCU to the picture, given that zsmalloc
is quite often run under memory pressure.
On 2024/2/28 13:29, Sergey Senozhatsky wrote:
> On (24/02/28 13:14), Chengming Zhou wrote:
>> On 2024/2/28 12:33, Sergey Senozhatsky wrote:
>>> On (24/02/27 03:02), Chengming Zhou wrote:
>>> [..]
>>>> @@ -978,10 +974,11 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
>>>> pages[i] = page;
>>>> }
>>>>
>>>> - create_page_chain(class, zspage, pages);
>>>> init_zspage(class, zspage);
>>>> zspage->pool = pool;
>>>> zspage->class = class->index;
>>>> + /* RCU set_zspage() after zspage initialized. */
>>>> + create_page_chain(class, zspage, pages);
>>>
>>> So this hasn't been tested, has it?
>> I have tested it in my test vm, but it hasn't KASAN enabled. I tested the
>> kernel build in tmpfs with zswap enabled using zsmalloc pool, not sure
>> why the kernel didn't crash then...
>
> I hit the problem on non-kasan-enabled kernel. KASAN was enabled
> later on.
Ok, It should be a problem with my process, sorry.
>
> [..]
>
>>> So when init_zspage() calls get_first_page() it gets NULL zspage->first_page
>>> which we then use in is_first_page(first_page)->PagePrivate(page). As far as
>>> I can tell.
>>
>> Thanks! I will fix it and test throughly before send an update.
>
> I'm curious if we want to add RCU to the picture, given that zsmalloc
> is quite often run under memory pressure.
Yes, it's a reasonable point. But given struct zspage size has only 56 bytes,
it maybe not a problem to delay its free to RCU?
Thanks.
On (24/02/28 13:42), Chengming Zhou wrote:
> > I'm curious if we want to add RCU to the picture, given that zsmalloc
> > is quite often run under memory pressure.
>
> Yes, it's a reasonable point. But given struct zspage size has only 56 bytes,
> it maybe not a problem to delay its free to RCU?
Hmm, yeah, probably. I think it'll make sense to wait for more
"go for it" from Cc-ed folks before we land this series.