Re: [PATCH v3] mm/pdx: Add comments throughout the codebase for pdx

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

Hi,

On Thu, Jul 13, 2023 at 05:12:09PM +0200, Jan Beulich wrote:
> On 07.07.2023 18:07, Alejandro Vallejo wrote:
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -31,6 +31,17 @@
> >   *   (i.e. all devices assigned to) a guest share a single DMA address 
> > space
> >   *   and, by default, Xen will ensure dfn == pfn.
> >   *
> > + * pdx: Page InDeX
> > + *   Indices into the frame table holding the per-page's book-keeping
> > + *   metadata. A compression scheme may be used, so there's a possibly non
> > + *   identity mapping between valid(mfn) <-> valid(pdx). See the comments
> > + *   in pdx.c for an in-depth explanation of that mapping. This also has a
> > + *   knock-on effect on the directmap, as "compressed" pfns have no
> > + *   corresponding mapped frames.
> 
> Didn't you mean to keep the directmap part optional,
I did.

> which would call for saying "may" here (twice)?
That paragraph as-is doesn't really mandate a directmap. It merely state
that there are knock-on effects on directmap indexing, not that there's
always a directmap to index.

> Yet then ...
>
> 
> > --- a/xen/include/xen/pdx.h
> > +++ b/xen/include/xen/pdx.h
> > @@ -1,6 +1,73 @@
> >  #ifndef __XEN_PDX_H__
> >  #define __XEN_PDX_H__
> >  
> > +/*
> > + * PDX (Page inDeX)
> > + *
> > + * This file deals with optimisations pertaining to frame table and
> > + * directmap indexing, A pdx is an index into the frame table, which
> > + * typically also means an index into the directmap[1]. However, having an
> > + * identity relationship between mfn and pdx could waste copious amounts of
> > + * memory in empty frame table entries and page tables. There are some
> > + * techniques to bring memory wastage down.
> > + *
> > + * [1] Some ports apply further modifications to a pdx before indexing the
> > + *     directmap. This doesn't change the fact that the same compression
> > + *     present in the frame table is also present in the directmap
> > + *     whenever said map is present.
> 
> .. you mention it here as non-optional as well. Didn't you tell me that
> Arm doesn't use compressed indexes into the directmap?
> 
> Jan

The [1] note states "whenever said map is present", meaning that it may not
be present. Saying it's optional is a stretch though. It's not like we can
choose right now.

> Didn't you tell me that Arm doesn't use compressed indexes into the directmap?
arm32 doesn't have a directmap at all. arm64 uses biased pdx as indices
(they are offset by a constant), so they are still subject to compression.

Alejandro