On Thu, Jul 25, 2024 at 3:43 PM Andi Kleen <ak@xxxxxxxxxxxxxxx> wrote:
>
> > +static int freader_get_page(struct freader *r, u64 file_off)
> > +{
> > + pgoff_t pg_off = file_off >> PAGE_SHIFT;
> > +
> > + freader_put_page(r);
> > +
> > + r->page = find_get_page(r->mapping, pg_off);
> > + if (!r->page)
> > + return -EFAULT; /* page not mapped */
> > +
> > + r->page_addr = kmap_local_page(r->page);
>
> kmaps are a limited resource on true highmem systems
> (something like 16-32)
> Can you guarantee that you don't overrun them?
Sorry, what does "overrun" mean in this case? Note, my code doesn't
change anything about kmap_local_page() usage. We used to map one page
at a time, and my changes preserve this property. We never access many
pages at the same time.
>
> Some of the callers below seem to be in a loop.
>
Note how freader_get_page() will always call freader_put_page() first,
unmapping previously mapped page. So only one page at a time will be
mapped.
> You probably won't see any failures unless you test with real highmem.
> Given it's a obscure configuration these days, but with some of the
> attempts to unmap the page cache by default it might be back in
> mainstream.
>
> Also true highmem disables preemption, I assume you took that
> into account. If the worst case run time is long enough would
> need preemption points.
I don't think I did because I'm not sure what the above means, care to
elaborate? But I'll reiterate, fundamentally my changes don't change
any behavior for all the existing cases. And for sleepable mode we
only have a read_cache_folio() call which will bring the page into
page cache, and after that the rest of the logic is exactly the same
as in non-faultable mode.
>
> -Andi
