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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2023 17:17, Alejandro Vallejo wrote:
> On Fri, Jul 14, 2023 at 12:36:11PM +0200, Jan Beulich wrote:
>> On 14.07.2023 12:27, Alejandro Vallejo wrote:
>>> 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.
>>
>> Hmm, then our understanding of "optional" was differing: I understood
>> "use of compressed indexes is optional", when you apparently meant
>> "the use of a directmap is optional". If this is the case, then I
>> agree with the chosen wording. (Nevertheless using the suggested "may"
>> wouldn't yield and incorrect outcome.)
> 
> Like this?
> 
> ```
>  * 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 may
>  *   have a knock-on effect on the directmap, as "compressed" pfns may not
>  *   have corresponding mapped frames.
> ```
> 
> If so, sure. I don't mind either way. I'm happy for those 2 _may_s to be 
> added. 

Yes please.

Thanks, Jan