Re: [PATCH v2 08/20] KVM: s390: KVM page table management functions: allocation

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Tue, 16 Sep 2025 19:01:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 16.09.25 um 18:47 schrieb Claudio Imbrenda:
> > On Tue, 16 Sep 2025 18:26:53 +0200
> > Heiko Carstens <hca@linux.ibm.com> wrote:
> >   
> >> On Wed, Sep 10, 2025 at 08:07:34PM +0200, Claudio Imbrenda wrote:  
> >>> Add page table management functions to be used for KVM guest (gmap)
> >>> page tables.
> >>>
> >>> This patch adds the boilerplate and functions for the allocation and
> >>> deallocation of DAT tables.
> >>>
> >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> >>> ---
> >>>   arch/s390/kvm/Makefile     |  1 +
> >>>   arch/s390/kvm/dat.c        | 91 ++++++++++++++++++++++++++++++++++++++
> >>>   arch/s390/kvm/dat.h        |  4 ++
> >>>   arch/s390/mm/page-states.c |  1 +
> >>>   4 files changed, 97 insertions(+)
> >>>   create mode 100644 arch/s390/kvm/dat.c  
> >>
> >> ...
> >>  
> >>> +static inline struct page_table *dat_alloc_pt_noinit(void)
> >>> +{
> >>> +	struct page *page;
> >>> +	void *virt;
> >>> +
> >>> +	page = alloc_pages(GFP_ATOMIC, 0);
> >>> +	if (!page)
> >>> +		return NULL;
> >>> +
> >>> +	virt = page_to_virt(page);
> >>> +	__arch_set_page_dat(virt, 1);
> >>> +	return virt;
> >>> +}  
> >>
> >> Is GFP_ATOMIC a typo, and this should have been GFP_KERNEL?
> >>
> >> Otherwise I would guess this will cause problems in the future when
> >> under memory pressure allocating guest page tables fails easily,
> >> while before this change such allocations never failed.  
> > 
> > how so? the documentation in gfp_types.h says:
> > 
> >   * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
> >   * watermark is applied to allow access to "atomic reserves".
> >   * The current implementation doesn't support NMI and few other strict
> >   * non-preemptive contexts (e.g. raw_spin_lock). The same applies to %GFP_NOWAIT.
> >   *
> >   * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
> >   * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.
> > 
> > 
> > I think GFP_ATOMIC actually gives more guarantees?  
> 
> In real life GFP_ATOMIC can fail, GFP_KERNEL does not.All gfp allocation failures
> are usually the atomic ones.

interesting... then I guess I need GFP_KERNEL | GFP_ATOMIC ?