On Tue, Apr 23, 2024 at 11:43 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Apr 23, 2024, Yosry Ahmed wrote:
> > On Tue, Apr 23, 2024 at 6:30 AM Peter Gonda <pgonda@xxxxxxxxxx> wrote:
> > > > struct page *snp_safe_alloc_page(int cpu)
> > > > {
> > > > unsigned long pfn;
> > > > struct page *p;
> > > > gfp_t gpf;
> > > > int node;
> > > >
> > > > if (cpu >= 0) {
> > > > node = cpu_to_node(cpu);
> > > > gfp = GFP_KERNEL;
> > > > } else {
> > > > node = NUMA_NO_NODE;
> > > > gfp = GFP_KERNEL_ACCOUNT
> > > > }
> >
> > FWIW, from the pov of someone who has zero familiarity with this code,
> > passing @cpu only to make inferences about the GFP flags and numa
> > nodes is confusing tbh.
> >
> > Would it be clearer to pass in the gfp flags and node id directly? I
> > think it would make it clearer why we choose to account the allocation
> > and/or specify a node in some cases but not others.
>
> Hmm, yeah, passing GFP directly makes sense, if only to align with alloc_page()
> and not reinvent the wheel. But forcing all callers to explicitly provide a node
> ID is a net negative, i.e. having snp_safe_alloc_page() and snp_safe_alloc_page_node()
> as originally suggested makes sense.
Yeah if most callers do not need to provide a node then it makes sense
to have a specialized snp_safe_alloc_page_node() variant.
>
> But snp_safe_alloc_page() should again flow alloc_pages() and pass numa_node_id(),
> not NUMA_NO_NODE.
alloc_pages_node() will use numa_node_id() if you pass in
NUMA_NO_NODE. That's the documented behavior and it seems to be widely
used. I don't see anyone using numa_node_id() directly with
alloc_pages_node().
