On Tue, Mar 19, 2024 at 10:00 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > [..] > > > > I guess that is because on arm32 , we have highmem but > > > > sg_init_one supports lowmem only. the below should be > > > > able to fix? > > > > > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > > index 9dec853647c8..47c0386caba2 100644 > > > > --- a/mm/zswap.c > > > > +++ b/mm/zswap.c > > > > @@ -1086,7 +1086,8 @@ static void zswap_decompress(struct zswap_entry > > > > *entry, struct page *page) > > > > zpool_unmap_handle(zpool, entry->handle); > > > > } > > > > > > > > - sg_init_one(&input, src, entry->length); > > > > + sg_init_table(&input, 1); > > > > + sg_set_page(&input, kmap_to_page(src), entry->length, > > > > offset_in_page(src)); > > > > > > Is this working around the debug check in sg_init_one()? IIUC, only > > > > I wouldn't characterize it as a workaround; it's more of a solution. > > I assumed that the debug check in sg_set_buf() is because > sg_set_page() cannot handle highmem pages, sorry if that isn't the > case. Apparently we are hitting a warning with kmap_to_page() though > as syzbot just reported. > > > > > > lowmem pages are supported. We may be passing in a highmem page to > > > sg_set_page() now, right? > > > > we can pass highmem to sg_set_page(). This is perfectly fine. > > So the debug check is only because we are using virt_to_page() in sg_set_buf()? yes. it is checking if linear_mapping can apply on the buffer. > > > > > > > > > Also, it seems like if src is a lowmem address kmap_to_page() will be > > > doing unnecessary checks (assuming it's working correctly)? > > > > In practice, we consistently use kmap and kunmap even on systems with > > low memory. > > However, it's worth noting that for low memory scenarios, kmap > > essentially returns > > page_to_virt(page_address). Thus, the overhead of kmap_to_page shouldn't be > > significant on low memory systems, especially considering that it simplifies to > > virt_to_page(). > > > > Another approach is to consistently employ page_to_virt() for low > > memory situations > > and reserve kmap for high memory scenarios. However, since we always > > utilize kmap > > regardless of whether the page is low or high memory, we don't need to concern > > ourselves with this distinction > > I see. Thanks for elaborating. > > > > > > > > > Would it be more robust to just use the temporary buffer if src is a > > > kmap address? > > > > I don't think so because we will need a memcpy then. > > I thought that was necessary because sg_set_page() cannot take in > highmem pages, but you mentioned that this isn't the case. I think both sg_init_one() and sg_set_page() lack docs. as apparently sg_init_one() can't take highmem. sg_set_page() can definitely take highmem as crypto/scompress.c takes care of both high and low. and scatterwalk_map_and_copy() can handle both. Thanks Barry
