| Message ID | 20220429064051.61552-6-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few fixup patches for z3fold | expand |
On Fri, Apr 29, 2022 at 8:41 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > > Revert commit f1549cb5ab2b ("mm/z3fold.c: allow __GFP_HIGHMEM in > z3fold_alloc"). > > z3fold can't support GFP_HIGHMEM page now. page_address is used > directly at all places. Moreover, z3fold_header is on per cpu > unbuddied list which could be access anytime. So we should rid > the support of GFP_HIGHMEM allocation for z3fold. Could you please clarify how kmem_cache is affected here? Thanks, Vitaly > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/z3fold.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/mm/z3fold.c b/mm/z3fold.c > index b3b4e65c107f..5f5d5f1556be 100644 > --- a/mm/z3fold.c > +++ b/mm/z3fold.c > @@ -212,10 +212,8 @@ static int size_to_chunks(size_t size) > static inline struct z3fold_buddy_slots *alloc_slots(struct z3fold_pool *pool, > gfp_t gfp) > { > - struct z3fold_buddy_slots *slots; > - > - slots = kmem_cache_zalloc(pool->c_handle, > - (gfp & ~(__GFP_HIGHMEM | __GFP_MOVABLE))); > + struct z3fold_buddy_slots *slots = kmem_cache_zalloc(pool->c_handle, > + gfp); > > if (slots) { > /* It will be freed separately in free_handle(). */ > @@ -1075,7 +1073,7 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, > enum buddy bud; > bool can_sleep = gfpflags_allow_blocking(gfp); > > - if (!size) > + if (!size || (gfp & __GFP_HIGHMEM)) > return -EINVAL; > > if (size > PAGE_SIZE) > -- > 2.23.0 >
On 2022/5/19 15:12, Vitaly Wool wrote: > On Fri, Apr 29, 2022 at 8:41 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> Revert commit f1549cb5ab2b ("mm/z3fold.c: allow __GFP_HIGHMEM in >> z3fold_alloc"). >> >> z3fold can't support GFP_HIGHMEM page now. page_address is used >> directly at all places. Moreover, z3fold_header is on per cpu >> unbuddied list which could be access anytime. So we should rid >> the support of GFP_HIGHMEM allocation for z3fold. > > Could you please clarify how kmem_cache is affected here? With this code changes, kmem_cache should be unaffected. HIGHMEM is still not supported for kmem_cache just like before but caller ensures __GFP_HIGHMEM is not passed in now. The issue I want to fix here is that if z3fold page is allocated from highmem, page_address can't be used directly. Did I answer your question? Or don't I get your point? Thanks! > > Thanks, > Vitaly Many thanks for your time! :) > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/z3fold.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/mm/z3fold.c b/mm/z3fold.c >> index b3b4e65c107f..5f5d5f1556be 100644 >> --- a/mm/z3fold.c >> +++ b/mm/z3fold.c >> @@ -212,10 +212,8 @@ static int size_to_chunks(size_t size) >> static inline struct z3fold_buddy_slots *alloc_slots(struct z3fold_pool *pool, >> gfp_t gfp) >> { >> - struct z3fold_buddy_slots *slots; >> - >> - slots = kmem_cache_zalloc(pool->c_handle, >> - (gfp & ~(__GFP_HIGHMEM | __GFP_MOVABLE))); >> + struct z3fold_buddy_slots *slots = kmem_cache_zalloc(pool->c_handle, >> + gfp); >> >> if (slots) { >> /* It will be freed separately in free_handle(). */ >> @@ -1075,7 +1073,7 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, >> enum buddy bud; >> bool can_sleep = gfpflags_allow_blocking(gfp); >> >> - if (!size) >> + if (!size || (gfp & __GFP_HIGHMEM)) >> return -EINVAL; >> >> if (size > PAGE_SIZE) >> -- >> 2.23.0 >> > . >
On Thu, 19 May 2022 19:34:01 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > On 2022/5/19 15:12, Vitaly Wool wrote: > > On Fri, Apr 29, 2022 at 8:41 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > >> > >> Revert commit f1549cb5ab2b ("mm/z3fold.c: allow __GFP_HIGHMEM in > >> z3fold_alloc"). > >> > >> z3fold can't support GFP_HIGHMEM page now. page_address is used > >> directly at all places. Moreover, z3fold_header is on per cpu > >> unbuddied list which could be access anytime. So we should rid > >> the support of GFP_HIGHMEM allocation for z3fold. > > > > Could you please clarify how kmem_cache is affected here? > > With this code changes, kmem_cache should be unaffected. HIGHMEM is still not supported for > kmem_cache just like before but caller ensures __GFP_HIGHMEM is not passed in now. The issue > I want to fix here is that if z3fold page is allocated from highmem, page_address can't be > used directly. Did I answer your question? Or don't I get your point? > Yes, page_address() against a highmem page only works if that page has been mapped into pagetables with kmap() or kmap_atomic(), and z3fold doesn't appear to do that. Given that other zpool_driver implementations do appear to support highmem pages, I expect that z3fold should be taught likewise. I didn't look very hard, but this particular patch is a bit worrisome. As I understand it, zswap can enable highmem: if (zpool_malloc_support_movable(entry->pool->zpool)) gfp |= __GFP_HIGHMEM | __GFP_MOVABLE; and z3fold will silently ignore the __GFP_HIGHMEM, which is OK. But with this patch, z3fold will now return -EINVAL, so existing setups will start failing?
On 2022/5/20 2:31, Andrew Morton wrote: > On Thu, 19 May 2022 19:34:01 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > >> On 2022/5/19 15:12, Vitaly Wool wrote: >>> On Fri, Apr 29, 2022 at 8:41 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >>>> >>>> Revert commit f1549cb5ab2b ("mm/z3fold.c: allow __GFP_HIGHMEM in >>>> z3fold_alloc"). >>>> >>>> z3fold can't support GFP_HIGHMEM page now. page_address is used >>>> directly at all places. Moreover, z3fold_header is on per cpu >>>> unbuddied list which could be access anytime. So we should rid >>>> the support of GFP_HIGHMEM allocation for z3fold. >>> >>> Could you please clarify how kmem_cache is affected here? >> >> With this code changes, kmem_cache should be unaffected. HIGHMEM is still not supported for >> kmem_cache just like before but caller ensures __GFP_HIGHMEM is not passed in now. The issue >> I want to fix here is that if z3fold page is allocated from highmem, page_address can't be >> used directly. Did I answer your question? Or don't I get your point? >> > > Yes, page_address() against a highmem page only works if that page has > been mapped into pagetables with kmap() or kmap_atomic(), and z3fold > doesn't appear to do that. What's more, usually z3fold page is on the percpu unbuddied list and thus can be accessed directly at any time when needed. So we can't do kunmap or kunmap_atomic until z3fold page is taken off the list. > > Given that other zpool_driver implementations do appear to support > highmem pages, I expect that z3fold should be taught likewise. > IMHO, it might be too cumbersome to support highmem pages due to above reason. > > I didn't look very hard, but this particular patch is a bit worrisome. > As I understand it, zswap can enable highmem: > > if (zpool_malloc_support_movable(entry->pool->zpool)) > gfp |= __GFP_HIGHMEM | __GFP_MOVABLE; > > and z3fold will silently ignore the __GFP_HIGHMEM, which is OK. But > with this patch, z3fold will now return -EINVAL, so existing setups > will start failing? IIUC, malloc_support_movable is never set for z3fold_zpool_driver. So zpool_malloc_support_movable will always return false, i.e. __GFP_HIGHMEM | __GFP_MOVABLE won't be set for z3fold page now (otherwise page_address will return NULL for highmem pages and null-pointer dereferencing should be witnessed already). Therefore existing setups will keep unaffected. Or am I miss something? Thanks a lot! > > . >
diff --git a/mm/z3fold.c b/mm/z3fold.c index b3b4e65c107f..5f5d5f1556be 100644 --- a/mm/z3fold.c +++ b/mm/z3fold.c @@ -212,10 +212,8 @@ static int size_to_chunks(size_t size) static inline struct z3fold_buddy_slots *alloc_slots(struct z3fold_pool *pool, gfp_t gfp) { - struct z3fold_buddy_slots *slots; - - slots = kmem_cache_zalloc(pool->c_handle, - (gfp & ~(__GFP_HIGHMEM | __GFP_MOVABLE))); + struct z3fold_buddy_slots *slots = kmem_cache_zalloc(pool->c_handle, + gfp); if (slots) { /* It will be freed separately in free_handle(). */ @@ -1075,7 +1073,7 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, enum buddy bud; bool can_sleep = gfpflags_allow_blocking(gfp); - if (!size) + if (!size || (gfp & __GFP_HIGHMEM)) return -EINVAL; if (size > PAGE_SIZE)
Revert commit f1549cb5ab2b ("mm/z3fold.c: allow __GFP_HIGHMEM in z3fold_alloc"). z3fold can't support GFP_HIGHMEM page now. page_address is used directly at all places. Moreover, z3fold_header is on per cpu unbuddied list which could be access anytime. So we should rid the support of GFP_HIGHMEM allocation for z3fold. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/z3fold.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)