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

[Jason Gunthorpe <jgg@nvidia.com>]

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)

?

Thanks,
Jason