On Sun, Mar 14, 2021 at 03:22:02PM +0000, Chuck Lever III wrote: > >> Anyway, I'm not arguing against a bulk allocator, nor even saying this > >> is a bad interface. It just maybe could be better. > >> > > > > I think it puts more responsibility on the caller to use the API correctly > > but I also see no value in arguing about it further because there is no > > supporting data either way (I don't have routine access to a sufficiently > > fast network to generate the data). I can add the following patch and let > > callers figure out which interface is preferred. If one of the interfaces > > is dead in a year, it can be removed. > > > > As there are a couple of ways the arrays could be used, I'm leaving it > > up to Jesper and Chuck which interface they want to use. In particular, > > it would be preferred if the array has no valid struct pages in it but > > it's up to them to judge how practical that is. > > I'm interested to hear from Jesper. > > My two cents (US): > > If svc_alloc_arg() is the /only/ consumer that wants to fill > a partially populated array of page pointers, then there's no > code-duplication benefit to changing the synopsis of > alloc_pages_bulk() at this point. > > Also, if the consumers still have to pass in the number of > pages the array needs, rather than having the bulk allocator > figure it out, then there's not much additional benefit, IMO. > > Ideally (for SUNRPC) alloc_pages_bulk() would take a pointer > to a sparsely-populated array and the total number of elements > in that array, and fill in the NULL elements. The return value > would be "success -- all elements are populated" or "failure -- > some elements remain NULL". > If the array API interface was expected to handle sparse arrays, it would make sense to define nr_pages are the number of pages that need to be in the array instead of the number of pages to allocate. The preamble would skip the first N number of allocated pages and decrement nr_pages accordingly before the watermark check. The return value would then be the last populated array element and the caller decides if that is enough to proceed or if the API needs to be called again. There is a slight risk that with a spare array that only needed 1 page in reality would fail the watermark check but on low memory, allocations take more work anyway. That definition of nr_pages would avoid the potential buffer overrun but both you and Jesper would need to agree that it's an appropriate API. -- Mel Gorman SUSE Labs
