On 10/02/2024 2:21 am, Pasha Tatashin wrote:
[...]
>>> +/**
>>> + * 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
>>> + */
>>> +static inline void *iommu_alloc_pages_node(int nid, gfp_t gfp, int order)
>>> +{
>>> + struct page *page = __iommu_alloc_pages_node(nid, gfp, order);
>>> +
>>> + if (unlikely(!page))
>>> + return NULL;
>>
>> As a general point I'd prefer to fold these checks into the accounting
>> function itself rather than repeat them all over.
>
> For the free functions this saves a few cycles by not repeating this
> check again inside __free_pages(), to keep things symmetrical it makes
> sense to keep __iomu_free_account and __iomu_alloc_account the same.
> With the other clean-up there are not that many of these checks left.
__free_pages() doesn't accept NULL, so __iommu_free_pages() shouldn't
need a check; free_pages() does, but correspondingly iommu_free_pages()
needs its own check up-front to avoid virt_to_page(NULL); either way it
means there are no callers of iommu_free_account() who should be passing
NULL.
The VA-returning allocators of course need to avoid page_address(NULL),
so I clearly made this comment in the wrong place to begin with, oops.
In the end I guess that will leave __iommu_alloc_pages() as the only
caller of iommu_alloc_account() who doesn't already need to handle their
own NULL. OK, I'm convinced, apologies for having to bounce it off you
to work it through :)
>>> + */
>>> +static inline void *iommu_alloc_page_node(int nid, gfp_t gfp)
>>> +{
>>> + return iommu_alloc_pages_node(nid, gfp, 0);
>>> +}
>>
>> TBH I'm not entirely convinced that saving 4 characters per invocation
>> times 11 invocations makes this wrapper worthwhile :/
>
> Let's keep them. After the clean-up that you suggested, there are
> fewer functions left in this file, but I think that it is cleaner to
> keep these remaining, as it is beneficial to easily distinguish when
> exactly one page is allocated vs when multiple are allocated via code
> search.
But is it, really? It's not at all obvious to me *why* it would be
significantly interesting to distinguish fixed order-0 allocations from
higher-order or variable-order (which may still be 0) ones. After all,
there's no regular alloc_page_node() wrapper, yet plenty more callers of
alloc_pages_node(..., 0) :/
Thanks,
Robin.