Re: [PATCH v2 07/23] iommu/pages: De-inline the substantial functions

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2/19/25 04:19, Jason Gunthorpe wrote:
> On Sat, Feb 15, 2025 at 04:48:04PM +0800, Baolu Lu wrote:
> 
>>> +#include <linux/gfp.h>
>>> +#include <linux/mm.h>
>>> +
>>> +/**
>>> + * iommu_alloc_pages_node - Allocate a zeroed page of a given order from
>>> + *                          specific NUMA node
>>> + * @nid: memory NUMA node id
>>> + * @gfp: buddy allocator flags
>>> + * @order: page order
>>> + *
>>> + * Returns the virtual address of the allocated page. The page must be
>>> + * freed either by calling iommu_free_page() or via iommu_put_pages_list().
>> nit: ... by calling iommu_free_pages() ...
> Got it
> 
>> and
>>
>>   s/page/pages/g in above comments?
> There is alot of historical confusion here because it was all designed
> around alloc_pages() which allocated a list of contiguous pages that
> could be subdivided. When this moved to GFP_COMP and later to
> folio_alloc() the subdivision is no longer possible. So it is not
> "pages" at all anymore, but a single "[compound] page".
> 
> So the module name is called "iommu-pages" but aside from the free
> list functions everything else acts on a single [compound] page only.
> 
> If you think about it too much it makes no sense but I didn't want to
> rename every function. I tried to keep it so that "iommu pages" was
> part of othe module name, and function designators, but the comments
> talk about a singular [compound] page
> 
>>> +static void __iommu_free_page(struct page *page)
>> It's more readable if renaming it to __iommu_free_pages()?
> Ah.. Well, it captures the module name but nothing it does acts on
> multiple things, since it is internal I used the other name
> 
> How about I rename it later on to:
> 
> static void __iommu_free_desc(struct ioptdesc *iopt)
> 
> ?

No problem. It actually depends on you. Since this is only used locally,
the iommu drivers won't use it. So, anything that helps understand what
it does is okay. :-)

Thanks,
baolu