On Wed, Oct 13, 2021 at 04:45:35PM -0700, Shakeel Butt wrote:

On Wed, Oct 13, 2021 at 4:15 PM Roman Gushchin [off-list ref] wrote:

quoted

[...]

quoted

quoted

quoted

Isn't it a bit too aggressive?

How about
    if (WARN_ON_ONCE(gfp & __GFP_ACCOUNT))

We actually know that kvmalloc(__GFP_ACCOUNT) users exist and can
trigger bulk page allocator through vmalloc, so I don't think the
warning would be any helpful.

quoted

       gfp &= ~__GFP_ACCOUNT;

Bulk allocator is best effort, so callers have adequate fallbacks.
Transparently disabling accounting would be unexpected.

I see...

Shouldn't we then move this check to an upper level?

E.g.:

if (!(gfp & __GFP_ACCOUNT))
   call_into_bulk_allocator();
else
   call_into_per_page_allocator();

If we add this check in the upper level (e.g. in vm_area_alloc_pages()
) then I think we would need WARN_ON_ONCE(gfp & __GFP_ACCOUNT) in the
bulk allocator to detect future users.

At the moment I am more inclined towards this patch's approach. Let's
say in future we find there is a __GFP_ACCOUNT allocation which can
benefit from bulk allocator and we decide to add such support in bulk
allocator then we would not need to change the bulk allocator callers
at that time just the bulk allocator.

I agree with you.  Let's apply the patch as-is.