Re: [PATCH 2/4] char: xillybus: replace __get_free_pages() with kmalloc()

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 9/3/26 14:30, Eli Billauer wrote:
> On 02/09/2026 14:09, Mike Rapoport wrote:
>>> So using vmalloc() may result in a malfunction in the data transport where
>>> the __get_free_pages() would have been successful. This outweighs the
>>> benefit of a somewhat higher probability of success in allocating very large
>>> buffers, and surely the improved aesthetics of the kernel code.
>> kmalloc() still works, just like __get_free_pages()
>>   
> 
> After these few days of discussion, this is what I make of this topic.
> 
> The memory allocation in fifo_init() is somewhat ugly, but this driver 
> has been out there for five years, and no complaints from users.
> 
> The vmalloc() option may be successful sometimes where the current 
> allocation will not. On the other hand, there is a possibility that it 
> will cause real trouble with page faults. Plus, a large allocation with 
> vmalloc() may be a weak spot on a platform to which the kernel is 
> partially or poorly ported.

vmalloc() has its limitations, yes...

> With __get_free_pages(), there is no doubt that usable memory is 
> allocated right away, and that it will work that way on every single 
> esoteric platform. vmalloc() leaves us with "we're pretty sure there is 
> probably no problem".
> 
> So I vote against vmalloc().
> 
> As for replacing __get_free_pages() with kmalloc(), it's a matter of 
> taste. I think that if you're after a buffer that has a power-of-two 
> number of pages, __get_free_pages() is the natural choice, and using it 
> makes it easier for the reader to understand what you're doing. 

If you are only after a buffer (that's physically contiguous and aligned)
and don't need the struct page itself, then kmalloc() is the natural choice.
This is where we are heading with the upcoming memdesc conversions etc.

> Replacing it with a kmalloc() is confusing in my opinion, and requires 
> that the reader is aware that kmalloc() falls back to __get_free_pages() 

Why? The reader has only to know that kmalloc() will provide such a buffer
(up to sizes that the page allocator would) and whether it falls back to the
page allocator or not is an implementation detail.
BTW, it's only at >2*PAGE_SIZE where it falls back. Which could change at
any point and nobody should care.

> in this case. So we actually want __get_free_pages(), but call kmalloc() 
> instead and let it fall back to __get_free_pages() so we don't have a 
> cast and don't need to save the order, and assume that everyone knows 
> that this is equivalent.

Oh right with __get_free_pages() you need to pass the order for freeing.
That's a real practical argument for kmalloc(). So I don't see why you'd
hold it against kmalloc().

> So I vote against the kmalloc() thing as well.
> 
> If this patch is part of a larger kernel cleanup, I suppose my votes 
> don't count for much. But for the record, this is my opinion.
> 
> Thanks and regards,
>     Eli