* [PATCH] zsmalloc: fix linking bug in init_zspage
@ 2018-08-09 13:53 zhouxianrong
2018-08-09 14:41 ` Vlastimil Babka
0 siblings, 1 reply; 7+ messages in thread
From: zhouxianrong @ 2018-08-09 13:53 UTC (permalink / raw
To: linux-mm
Cc: linux-kernel, minchan, ngupta, sergey.senozhatsky.work,
zhouxianrong
The last partial object in last subpage of zspage should not be linked
in allocation list.
Signed-off-by: zhouxianrong <zhouxianrong@tom.com>
---
mm/zsmalloc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 8d87e973a4f5..24dd8da0aa59 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1040,6 +1040,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
* Reset OBJ_TAG_BITS bit to last link to tell
* whether it's allocated object or not.
*/
+ if (off > PAGE_SIZE)
+ link -= class->size / sizeof(*link);
link->next = -1UL << OBJ_TAG_BITS;
}
kunmap_atomic(vaddr);
--
2.13.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] zsmalloc: fix linking bug in init_zspage
2018-08-09 13:53 [PATCH] zsmalloc: fix linking bug in init_zspage zhouxianrong
@ 2018-08-09 14:41 ` Vlastimil Babka
0 siblings, 0 replies; 7+ messages in thread
From: Vlastimil Babka @ 2018-08-09 14:41 UTC (permalink / raw
To: zhouxianrong, linux-mm
Cc: linux-kernel, minchan, ngupta, sergey.senozhatsky.work
On 08/09/2018 03:53 PM, zhouxianrong wrote:
> The last partial object in last subpage of zspage should not be linked
> in allocation list.
Please expand the changelog. Why it should not be? What happens if it
is? Kernel panic, data corruption or whatnot? So that people not
familiar with zsmalloc internals can judge how important the patch is
for e.g. backporting.
Thanks,
Vlastimil
> Signed-off-by: zhouxianrong <zhouxianrong@tom.com>
> ---
> mm/zsmalloc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 8d87e973a4f5..24dd8da0aa59 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1040,6 +1040,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
> * Reset OBJ_TAG_BITS bit to last link to tell
> * whether it's allocated object or not.
> */
> + if (off > PAGE_SIZE)
> + link -= class->size / sizeof(*link);
> link->next = -1UL << OBJ_TAG_BITS;
> }
> kunmap_atomic(vaddr);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] zsmalloc: fix linking bug in init_zspage
@ 2018-08-10 0:28 zhouxianrong
2018-08-13 6:05 ` Minchan Kim
0 siblings, 1 reply; 7+ messages in thread
From: zhouxianrong @ 2018-08-10 0:28 UTC (permalink / raw
To: linux-mm
Cc: linux-kernel, minchan, ngupta, sergey.senozhatsky.work,
zhouxianrong, zhouxianrong
From: zhouxianrong <zhouxianrong@huawei.com>
The last partial object in last subpage of zspage should not be linked
in allocation list. Otherwise it could trigger BUG_ON explicitly at
function zs_map_object. But it happened rarely.
Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
---
mm/zsmalloc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 8d87e973a4f5..24dd8da0aa59 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1040,6 +1040,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
* Reset OBJ_TAG_BITS bit to last link to tell
* whether it's allocated object or not.
*/
+ if (off > PAGE_SIZE)
+ link -= class->size / sizeof(*link);
link->next = -1UL << OBJ_TAG_BITS;
}
kunmap_atomic(vaddr);
--
2.13.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] zsmalloc: fix linking bug in init_zspage
2018-08-10 0:28 zhouxianrong
@ 2018-08-13 6:05 ` Minchan Kim
2018-08-13 10:55 ` Sergey Senozhatsky
0 siblings, 1 reply; 7+ messages in thread
From: Minchan Kim @ 2018-08-13 6:05 UTC (permalink / raw
To: zhouxianrong
Cc: linux-mm, linux-kernel, ngupta, sergey.senozhatsky.work,
zhouxianrong
Hi,
On Thu, Aug 09, 2018 at 08:28:17PM -0400, zhouxianrong wrote:
> From: zhouxianrong <zhouxianrong@huawei.com>
>
> The last partial object in last subpage of zspage should not be linked
> in allocation list. Otherwise it could trigger BUG_ON explicitly at
> function zs_map_object. But it happened rarely.
Could you be more specific? What case did you see the problem?
Is it a real problem or one founded by review?
Thanks.
>
> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> ---
> mm/zsmalloc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 8d87e973a4f5..24dd8da0aa59 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1040,6 +1040,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
> * Reset OBJ_TAG_BITS bit to last link to tell
> * whether it's allocated object or not.
> */
> + if (off > PAGE_SIZE)
> + link -= class->size / sizeof(*link);
> link->next = -1UL << OBJ_TAG_BITS;
> }
> kunmap_atomic(vaddr);
> --
> 2.13.6
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] zsmalloc: fix linking bug in init_zspage
2018-08-13 6:05 ` Minchan Kim
@ 2018-08-13 10:55 ` Sergey Senozhatsky
2018-08-14 0:24 ` Minchan Kim
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Senozhatsky @ 2018-08-13 10:55 UTC (permalink / raw
To: Minchan Kim
Cc: zhouxianrong, linux-mm, linux-kernel, ngupta,
sergey.senozhatsky.work, zhouxianrong
On (08/13/18 15:05), Minchan Kim wrote:
> > From: zhouxianrong <zhouxianrong@huawei.com>
> >
> > The last partial object in last subpage of zspage should not be linked
> > in allocation list. Otherwise it could trigger BUG_ON explicitly at
> > function zs_map_object. But it happened rarely.
>
> Could you be more specific? What case did you see the problem?
> Is it a real problem or one founded by review?
[..]
> > Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> > ---
> > mm/zsmalloc.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 8d87e973a4f5..24dd8da0aa59 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -1040,6 +1040,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
> > * Reset OBJ_TAG_BITS bit to last link to tell
> > * whether it's allocated object or not.
> > */
> > + if (off > PAGE_SIZE)
> > + link -= class->size / sizeof(*link);
> > link->next = -1UL << OBJ_TAG_BITS;
> > }
> > kunmap_atomic(vaddr);
Hmm. This can be a real issue. Unless I'm missing something.
So... I might be wrong, but the way I see the bug report is:
When we link objects during zspage init, we do the following:
while ((off += class->size) < PAGE_SIZE) {
link->next = freeobj++ << OBJ_TAG_BITS;
link += class->size / sizeof(*link);
}
Note that we increment the link first, link += class->size / sizeof(*link),
and check for the offset only afterwards. So by the time we break out of
the while-loop the link *might* point to the partial object which starts at
the last page of zspage, but *never* ends, because we don't have next_page
in current zspage. So that's why that object should not be linked in,
because it's not a valid allocates object - we simply don't have space
for it anymore.
zspage [ page 1 ][ page 2 ]
...............................link
[..###]
therefore the last object must be "link - 1" for such cases.
I think, the following change can also do the trick:
while ((off + class->size) < PAGE_SIZE) {
link->next = freeobj++ << OBJ_TAG_BITS;
link += class->size / sizeof(*link);
off += class->size;
}
Once again, I might be wrong on this.
Any thoughts?
-ss
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] zsmalloc: fix linking bug in init_zspage
2018-08-13 10:55 ` Sergey Senozhatsky
@ 2018-08-14 0:24 ` Minchan Kim
2018-08-14 0:51 ` Sergey Senozhatsky
0 siblings, 1 reply; 7+ messages in thread
From: Minchan Kim @ 2018-08-14 0:24 UTC (permalink / raw
To: Sergey Senozhatsky
Cc: zhouxianrong, linux-mm, linux-kernel, ngupta, zhouxianrong
Hi Sergey,
On Mon, Aug 13, 2018 at 07:55:36PM +0900, Sergey Senozhatsky wrote:
> On (08/13/18 15:05), Minchan Kim wrote:
> > > From: zhouxianrong <zhouxianrong@huawei.com>
> > >
> > > The last partial object in last subpage of zspage should not be linked
> > > in allocation list. Otherwise it could trigger BUG_ON explicitly at
> > > function zs_map_object. But it happened rarely.
> >
> > Could you be more specific? What case did you see the problem?
> > Is it a real problem or one founded by review?
> [..]
> > > Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> > > ---
> > > mm/zsmalloc.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > > index 8d87e973a4f5..24dd8da0aa59 100644
> > > --- a/mm/zsmalloc.c
> > > +++ b/mm/zsmalloc.c
> > > @@ -1040,6 +1040,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
> > > * Reset OBJ_TAG_BITS bit to last link to tell
> > > * whether it's allocated object or not.
> > > */
> > > + if (off > PAGE_SIZE)
> > > + link -= class->size / sizeof(*link);
> > > link->next = -1UL << OBJ_TAG_BITS;
> > > }
> > > kunmap_atomic(vaddr);
>
> Hmm. This can be a real issue. Unless I'm missing something.
>
> So... I might be wrong, but the way I see the bug report is:
>
> When we link objects during zspage init, we do the following:
>
> while ((off += class->size) < PAGE_SIZE) {
> link->next = freeobj++ << OBJ_TAG_BITS;
> link += class->size / sizeof(*link);
> }
>
> Note that we increment the link first, link += class->size / sizeof(*link),
> and check for the offset only afterwards. So by the time we break out of
> the while-loop the link *might* point to the partial object which starts at
> the last page of zspage, but *never* ends, because we don't have next_page
> in current zspage. So that's why that object should not be linked in,
> because it's not a valid allocates object - we simply don't have space
> for it anymore.
>
> zspage [ page 1 ][ page 2 ]
> ...............................link
> [..###]
>
> therefore the last object must be "link - 1" for such cases.
>
> I think, the following change can also do the trick:
>
> while ((off + class->size) < PAGE_SIZE) {
> link->next = freeobj++ << OBJ_TAG_BITS;
> link += class->size / sizeof(*link);
> off += class->size;
> }
>
> Once again, I might be wrong on this.
> Any thoughts?
If we want a refactoring, I'm not against but description said it tiggered
BUG_ON on zs_map_object rarely. That means it should be stable material
and need more description to understand. Please be more specific with
some example. The reason I'm hesitating is zsmalloc moves ZS_FULL group
when the zspage->inuse is equal to class->objs_per_zspage so I thought
it shouldn't allocate last partial object.
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] zsmalloc: fix linking bug in init_zspage
2018-08-14 0:24 ` Minchan Kim
@ 2018-08-14 0:51 ` Sergey Senozhatsky
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Senozhatsky @ 2018-08-14 0:51 UTC (permalink / raw
To: Minchan Kim
Cc: Sergey Senozhatsky, zhouxianrong, linux-mm, linux-kernel, ngupta,
zhouxianrong
Hi Minchan,
On (08/14/18 09:24), Minchan Kim wrote:
> > Any thoughts?
>
> If we want a refactoring, I'm not against but description said it tiggered
> BUG_ON on zs_map_object rarely. That means it should be stable material
> and need more description to understand. Please be more specific with
> some example.
I don't have any BUG_ON on hands. Would be great if zhouxianrong could
post some backtraces or more info/explanation.
> The reason I'm hesitating is zsmalloc moves ZS_FULL group
> when the zspage->inuse is equal to class->objs_per_zspage so I thought
> it shouldn't allocate last partial object.
Maybe, allocating last partial object does look a bit hacky - it's not a
valid object anyway, but I'm not suggesting that we need to change it.
Let's hear from zhouxianrong.
-ss
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-14 0:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-09 13:53 [PATCH] zsmalloc: fix linking bug in init_zspage zhouxianrong
2018-08-09 14:41 ` Vlastimil Babka
-- strict thread matches above, loose matches on Subject: below --
2018-08-10 0:28 zhouxianrong
2018-08-13 6:05 ` Minchan Kim
2018-08-13 10:55 ` Sergey Senozhatsky
2018-08-14 0:24 ` Minchan Kim
2018-08-14 0:51 ` Sergey Senozhatsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.