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.
But snp_safe_alloc_page() should again flow alloc_pages() and pass numa_node_id(),
not NUMA_NO_NODE.
