Date: Tue, 13 Feb 2024 17:26:26 +0000
From: Robin Murphy <robin.murphy@....com>
To: Pasha Tatashin <pasha.tatashin@...een.com>
Cc: akpm@...ux-foundation.org, alim.akhtar@...sung.com, alyssa@...enzweig.io,
 asahi@...ts.linux.dev, baolu.lu@...ux.intel.com, bhelgaas@...gle.com,
 cgroups@...r.kernel.org, corbet@....net, david@...hat.com,
 dwmw2@...radead.org, hannes@...xchg.org, heiko@...ech.de,
 iommu@...ts.linux.dev, jernej.skrabec@...il.com, jonathanh@...dia.com,
 joro@...tes.org, krzysztof.kozlowski@...aro.org, linux-doc@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, linux-rockchip@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org, linux-sunxi@...ts.linux.dev,
 linux-tegra@...r.kernel.org, lizefan.x@...edance.com, marcan@...can.st,
 mhiramat@...nel.org, m.szyprowski@...sung.com, paulmck@...nel.org,
 rdunlap@...radead.org, samuel@...lland.org, suravee.suthikulpanit@....com,
 sven@...npeter.dev, thierry.reding@...il.com, tj@...nel.org,
 tomas.mudrunka@...il.com, vdumpa@...dia.com, wens@...e.org, will@...nel.org,
 yu-cheng.yu@...el.com, rientjes@...gle.com, bagasdotme@...il.com,
 mkoutny@...e.com
Subject: Re: [PATCH v4 01/10] iommu/vt-d: add wrapper functions for page
 allocations

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.

Powered by blists - more mailing lists