Yosry Ahmed wrote: > On Wed, Feb 26, 2025 at 10:01:29AM -0600, Ira Weiny wrote: > > Yosry Ahmed wrote: > > > On Wed, Feb 12, 2025 at 09:20:24AM -0800, Yosry Ahmed wrote: > > > > On Mon, Mar 18, 2024 at 2:03 PM Barry Song <21cnbao@xxxxxxxxx> wrote: > > > > > > > > > > On Tue, Mar 19, 2024 at 9:52 AM syzbot > > > > > <syzbot+adbc983a1588b7805de3@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > Hello, > > > > > > > > > > > > syzbot has tested the proposed patch but the reproducer is still triggering an issue: > > > > > > WARNING in __kmap_to_page > > > > > > > > > > > > ------------[ cut here ]------------ > > > > > > WARNING: CPU: 0 PID: 3529 at mm/highmem.c:167 __kmap_to_page+0x100/0x194 mm/highmem.c:167 > > > > > > Modules linked in: > > > > > > > > > > + Ira > > > > > > > > > > Hi Ira, > > > > > > > > > > I noticed this warning is coming from commit ef6e06b2ef87077. > > > > > > > > > > you have a commit message like > > > > > " Because it is intended to remove kmap_to_page() add a warn on once to > > > > > the kmap checks to flag potential issues early. > > > > > " > > > > > > > > > > Do we have a replacement for kmap_to_page()? The background is that we > > > > > want to pass a highmem buffer to sg_set_page() but we only know its virt > > > > > address. > > > > > > > > I am reviving this thread because new zsmalloc changes will make > > > > mappings sleepable, which will allow zswap to drop the memcpy() in > > > > zswap_decompress() -- except for the !virt_addr_valid() case. We can > > > > get rid of that too if we can use kmap_tp_page() in the scatterlist > > > > code. > > > > > > > > Ira, could you please answer Barry's question above about > > > > kmap_to_page()? It has been a year and kmap_to_page() is still around. > > > > > > (Trying again with Ira as the main recepient just in case) > > > > > > Ira, could you please help us out here? :) > > > > Apologies, > > > > No there is no alternative to kmap_to_page(). The work I was doing has > > stalled out and I really don't know if it will resume. > > > > There are a few folks, like me, who would like to remove highmem but every > > time the subject comes up someone speaks up about a rare (mostly embedded) > > platform which really needs it. So I don't see it going away soon. > > > > Removing the warning could be justified by saying that highmem removal can > > be done completely within the kmap calls and only when that has been > > completed can these calls go away. But generally kmap_to_page() is not a > > popular call and it might be seen as a step backwards by some. > > > > For example: > > https://lore.kernel.org/linux-mm/20221216070621.GA24832@xxxxxx/ > > > > The patch with the warnings was a stop gap to ensure current users did not > > break. > > > > Do you have evidence that the extra memcpy is bad enough performance that > > you could justify using kmap_to_page? > > It's not about performance, it's about cleaning up the code. Currently > we have a special memcpy path [1] with a huge comment in > zswap_decompress() to handle zsmalloc not being sleepable and the kmap > case. The zsmalloc case is going away soon, and we'd like to remove the > special handling completely. > > Using kmap_to_page() will allow us to do so. As you mentioned, this > would not be blocking to removing highmem. The call can just be replaced > with virt_to_page() once this is done. > > If this is okay with you I can send a patch removing the warnings in > __kmap_to_page() when I start using the function. It is fine with me. I was just thinking it would be easier to justify adding a caller if there was something like a performance boost. Perhaps Christoph is not as concerned these days. :-D Ira > > [1]https://elixir.bootlin.com/linux/v6.13.4/source/mm/zswap.c#L1012 > > > Ira > > > > > > > > > > > > > Thanks. > > > > > > >
