Re: [PATCH 4/5] Hot-remove implementation for arm64

[Laura Abbott]

On 04/18/2017 11:48 AM, Ard Biesheuvel wrote:
> On 18 April 2017 at 19:21, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Fri, Apr 14, 2017 at 03:01:58PM +0100, Andrea Reale wrote:
> > > I guess it is likely that I might have made assumptions that are true
> > > for x86_64 but do not hold for arm64. Whenever you feel this is the
> > > case, I would be really grateful if you could help identify them.
> >
> > Sure thing.
> >
> > > On Tue, Apr 11, 2017 at 06:12:11PM +0100, Mark Rutland wrote:
> > > > On Tue, Apr 11, 2017 at 03:55:42PM +0100, Andrea Reale wrote:
> >
> > > > > +static void  free_pagetable(struct page *page, int order, bool direct)
> > > >
> > > > This "direct" parameter needs a better name, and a description in a
> > > > comment block above this function.
> > >
> > > The name direct is inherited directly from the x86_64 hot remove code.
> > > It serves to distinguish if we are removing either a pagetable page that
> > > is mapping to the direct mapping space (I think it is called also linear
> > > mapping area somewhere else) or a pagetable page or a data page
> > > from vmemmap.
> >
> > FWIW, I've largely heard the folk call that the "linear mapping", and
> > x86 folk call that the "direct mapping". The two are interchangeable.
> >
> > > In this specific set of functions, the flag is needed because the various
> > > alloc_init_p*d used for allocating entries for direct memory mapping
> > > rely on pgd_pgtable_alloc, which in its turn calls  pgtable_page_ctor;
> > > hence, we need to call the dtor.
> >
> > AFAICT, that's not true for the arm64 linear map, since that's created
> > with our early_pgtable_alloc(), which doesn't call pgtable_page_ctor().
> >
> > Judging by commit:
> >
> >    1378dc3d4ba07ccd ("arm64: mm: run pgtable_page_ctor() on non-swapper
> >                       translation table pages")
> >
> > ... we only do this for non-swapper page tables.
> >
> > > On the contrary, vmemmap entries are created using vmemmap_alloc_block
> > > (from within vmemmap_populate), which does not call pgtable_page_ctor
> > > when allocating pages.
> > >
> > > I am not sure I understand why the pgtable_page_ctor is not called when
> > > allocating vmemmap page tables, but that's the current situation.
> >
> >  From a quick scan, I see that it's necessary to use pgtable_page_ctor()
> > for pages that will be used for userspace page tables, but it's not
> > clear to me if it's ever necessary for pages used for kernel page
> > tables.
> >
> > If it is, we appear to have a bug on arm64.
> >
> > Laura, Ard, thoughts?
>
> The generic apply_to_page_range() will expect the PTE lock to be
> initialized for page table pages that are not part of init_mm. For
> arm64, that is precisely efi_mm as far as I am aware. For EFI, the
> locking is unnecessary but does no harm (the permissions are set once
> via apply_to_page_range() at boot), so I added this call when adding
> support for strict permissions in EFI rt services mappings.
>
> So I think it is appropriate for create_pgd_mapping() to be in charge
> of calling the ctor(). We simply have no destroy_pgd_mapping()
> counterpart that would be the place for the dtor() call, given that we
> never take down EFI rt services mappi >
> Whether it makes sense or not to lock/unlock in apply_to_page_range()
> is something I did not spend any brain cycles on at the time.

Agreed there shouldn't be a problem right now. I do think the locking is
appropriate in apply_to_page_range given what other functions also get
locked.

I really wish this were less asymmetrical though since it get hard
to reason about. It looks like hotplug_paging will call the ctor,
so is there an issue with calling hot-remove on memory that was once
hot-added or is that not a concern?

Thanks,
Laura