Re: alloc_pages_bulk()

On Mon, 22 Feb 2021 09:42:56 +0000
Mel Gorman [off-list ref] wrote:

On Mon, Feb 15, 2021 at 05:10:38PM +0100, Jesper Dangaard Brouer wrote:

quoted

On Mon, 15 Feb 2021 12:00:56 +0000
Mel Gorman [off-list ref] wrote:
  

quoted

On Thu, Feb 11, 2021 at 01:26:28PM +0100, Jesper Dangaard Brouer wrote:  

[...]  

quoted

quoted

I also suggest the API can return less pages than requested. Because I
want to to "exit"/return if it need to go into an expensive code path
(like buddy allocator or compaction).  I'm assuming we have a flags to
give us this behavior (via gfp_flags or alloc_flags)?
    

The API returns the number of pages returned on a list so policies
around how aggressive it should be allocating the requested number of
pages could be adjusted without changing the API. Passing in policy
requests via gfp_flags may be problematic as most (all?) bits are
already used.  

Well, I was just thinking that I would use GFP_ATOMIC instead of
GFP_KERNEL to "communicate" that I don't want this call to take too
long (like sleeping).  I'm not requesting any fancy policy :-)
  

The NFS use case requires opposite semantics
-- it really needs those allocations to succeed
https://lore.kernel.org/r/161340498400.7780.962495219428962117.stgit@klimt.1015granger.net.

Sorry, but that is not how I understand the code.

The code is doing exactly what I'm requesting. If the alloc_pages_bulk()
doesn't return expected number of pages, then check if others need to
run.  The old code did schedule_timeout(msecs_to_jiffies(500)), while
Chuck's patch change this to ask for cond_resched().  Thus, it tries to
avoid blocking the CPU for too long (when allocating many pages).

And the nfsd code seems to handle that the code can be interrupted (via
return -EINTR) via signal_pending(current).  Thus, the nfsd code seems
to be able to handle if the page allocations failed.

I've asked what code it's based on as it's not 5.11 and I'll iron that
out first.

Then it might be clearer what the "can fail" semantics should look like.
I think it would be best to have pairs of patches where the first patch
adjusts the semantics of the bulk allocator and the second adds a user.
That will limit the amount of code code carried in the implementation.
When the initial users are in place then the implementation can be
optimised as the optimisations will require significant refactoring and
I not want to refactor multiple times.

I guess, I should try to code-up the usage in page_pool.

What is the latest patch for adding alloc_pages_bulk() ?

The nfsd code (svc_alloc_arg) is called in a context where it can
sleep, and thus use GFP_KERNEL.  In most cases the page_pool will be
called with GFP_ATOMIC.  I don't think I/page_pool will retry the call
like Chuck did, as I cannot (re)schedule others to run.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

On Mon, Feb 22, 2021 at 12:42:46PM +0100, Jesper Dangaard Brouer wrote:

On Mon, 22 Feb 2021 09:42:56 +0000
Mel Gorman [off-list ref] wrote:

quoted

On Mon, Feb 15, 2021 at 05:10:38PM +0100, Jesper Dangaard Brouer wrote:

quoted

On Mon, 15 Feb 2021 12:00:56 +0000
Mel Gorman [off-list ref] wrote:
  

quoted

On Thu, Feb 11, 2021 at 01:26:28PM +0100, Jesper Dangaard Brouer wrote:  

[...]  

quoted

quoted

I also suggest the API can return less pages than requested. Because I
want to to "exit"/return if it need to go into an expensive code path
(like buddy allocator or compaction).  I'm assuming we have a flags to
give us this behavior (via gfp_flags or alloc_flags)?
    

The API returns the number of pages returned on a list so policies
around how aggressive it should be allocating the requested number of
pages could be adjusted without changing the API. Passing in policy
requests via gfp_flags may be problematic as most (all?) bits are
already used.  

Well, I was just thinking that I would use GFP_ATOMIC instead of
GFP_KERNEL to "communicate" that I don't want this call to take too
long (like sleeping).  I'm not requesting any fancy policy :-)
  

The NFS use case requires opposite semantics
-- it really needs those allocations to succeed
https://lore.kernel.org/r/161340498400.7780.962495219428962117.stgit@klimt.1015granger.net.

Sorry, but that is not how I understand the code.

The code is doing exactly what I'm requesting. If the alloc_pages_bulk()
doesn't return expected number of pages, then check if others need to
run.  The old code did schedule_timeout(msecs_to_jiffies(500)), while
Chuck's patch change this to ask for cond_resched().  Thus, it tries to
avoid blocking the CPU for too long (when allocating many pages).

And the nfsd code seems to handle that the code can be interrupted (via
return -EINTR) via signal_pending(current).  Thus, the nfsd code seems
to be able to handle if the page allocations failed.

I'm waiting to find out exactly what NFSD is currently doing as the code
in 5.11 is not the same as what Chuck was coding against so I'm not 100%
certain how it currently works.

quoted

I've asked what code it's based on as it's not 5.11 and I'll iron that
out first.

Then it might be clearer what the "can fail" semantics should look like.
I think it would be best to have pairs of patches where the first patch
adjusts the semantics of the bulk allocator and the second adds a user.
That will limit the amount of code code carried in the implementation.
When the initial users are in place then the implementation can be
optimised as the optimisations will require significant refactoring and
I not want to refactor multiple times.

I guess, I should try to code-up the usage in page_pool.

What is the latest patch for adding alloc_pages_bulk() ?

There isn't a usable latest version until I reconcile the nfsd caller.
The only major change in the API right now is dropping order. It handles
order-0 only.

-- 
Mel Gorman
SUSE Labs