On Wed, Oct 05, 2022 at 11:03:39AM -0700, Yang Shi wrote: > Since v5.13 the page bulk allocator was introduced to allocate order-0 > pages in bulk. There are a few mempool allocator callers which does > order-0 page allocation in a loop, for example, dm-crypt, f2fs compress, > etc. A mempool page bulk allocator seems useful. So introduce the > mempool page bulk allocator. > > It introduces the below APIs: > - mempool_init_pages_bulk() > - mempool_create_pages_bulk() > They initialize the mempool for page bulk allocator. The pool is filled > by alloc_page() in a loop. > > - mempool_alloc_pages_bulk_list() > - mempool_alloc_pages_bulk_array() > They do bulk allocation from mempool. > They do the below conceptually: > 1. Call bulk page allocator > 2. If the allocation is fulfilled then return otherwise try to > allocate the remaining pages from the mempool > 3. If it is fulfilled then return otherwise retry from #1 with sleepable > gfp > 4. If it is still failed, sleep for a while to wait for the mempool is > refilled, then retry from #1 > The populated pages will stay on the list or array until the callers > consume them or free them. > Since mempool allocator is guaranteed to success in the sleepable context, > so the two APIs return true for success or false for fail. It is the > caller's responsibility to handle failure case (partial allocation), just > like the page bulk allocator. > > The mempool typically is an object agnostic allocator, but bulk allocation > is only supported by pages, so the mempool bulk allocator is for page > allocation only as well. > > Signed-off-by: Yang Shi <shy828301@xxxxxxxxx> Overall, I think it's an ok approach and certainly a good use case for the bulk allocator. The main concern that I have is that the dm-crypt use case doesn't really want to use lists as such and it's just a means for collecting pages to pass to bio_add_page(). bio_add_page() is working with arrays but you cannot use that array directly as any change to how that array is populated will then explode. Unfortunately, what you have is adding pages to a list to take them off the list and put them in an array and that is inefficient. How about this 1. Add a callback to __alloc_pages_bulk() that takes a page as a parameter like bulk_add_page() or whatever. 2. For page_list == NULL && page_array == NULL, the callback is used 3. Add alloc_pages_bulk_cb() that passes in the name of a callback function 4. In the dm-crypt case, use the callback to pass the page to bio_add_page for the new page allocated. It's not free because there will be an additional function call for every page bulk allocated but I suspect that's cheaper than adding a pile of pages to a list just to take them off again. It also avoids adding a user for the bulk allocator list interface that does not even want a list. It might mean that there is additional cleanup work for __alloc_pages_bulk to abstract away whether a list, array or cb is used but nothing impossible. -- Mel Gorman SUSE Labs
