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.
