Re: [PATCH 1/8] x86/pdx: simplify calculation of domain struct allocation boundary

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2025 12:46, Roger Pau Monné wrote:
> On Thu, Jun 12, 2025 at 11:03:21AM +0200, Jan Beulich wrote:
>> On 11.06.2025 19:16, Roger Pau Monne wrote:
>>> @@ -498,14 +474,15 @@ struct domain *alloc_domain_struct(void)
>>>       * On systems with CONFIG_BIGMEM there's no packing, and so there's no
>>>       * such restriction.
>>>       */
>>> -#if defined(CONFIG_BIGMEM) || !defined(CONFIG_PDX_COMPRESSION)
>>> -    const unsigned int bits = IS_ENABLED(CONFIG_BIGMEM) ? 0 :
>>> -                                                          32 + PAGE_SHIFT;
>>> +#if defined(CONFIG_BIGMEM)
>>> +    const unsigned int bits = 0;
>>>  #else
>>> -    static unsigned int __read_mostly bits;
>>> +    static unsigned int __ro_after_init bits;
>>>  
>>>      if ( unlikely(!bits) )
>>> -         bits = _domain_struct_bits();
>>> +         bits = flsl(pfn_to_paddr(pdx_to_pfn(
>>> +             1UL << (sizeof(((struct page_info *)NULL)->v.inuse._domain) * 
>>> 8))))
>>> +             - 1;
>>
>> While Andrew did point you at sizeof_field(), we can have this even less 
>> verbose
>> by utilizing that frame_table is of the right type and (almost) globally in 
>> scope.
>>
>> Further, why use pfn_to_paddr()?
>>
>>          bits = flsl(pdx_to_pfn(1UL << 
>>                                 (sizeof(frame_table->v.inuse._domain) * 8))) 
>> +
>>                 PAGE_SHIFT - 1;
> 
> I've introduced and used pdx_to_paddr(), which I think it's more
> natural.  We already had a paddr_to_pdx() which was missing it's
> bidirectional equivalent.  It's now:
> 
>          bits = flsl(pdx_to_paddr(1UL << (sizeof_field(struct page_info,
>                                                        v.inuse._domain) * 8)))
>                 - 1;

Textually this is better, yes. I won't insist on the other variant, while
still noting that your way there's an extra shift whereas my way there's
merely an extra add.

>> However, it further feels like this was off by one; we had similar issues 
>> over
>> time in several places. There potentially being a gap between one less than
>> the PDX used here and that very PDX, don't we need to calculate based on the
>> "one less" value here? Hmm, there being a gap means no allocation would
>> succeed for the precise value of "bits" (in the mask-compression scheme), so
>> functionally all would be fine. Yet just to avoid setting a bad precedent I
>> think we'd still be better off using
>>
>>          bits = flsl(pdx_to_pfn((1UL << 
>>                                  (sizeof(frame_table->v.inuse._domain) * 8)) 
>> -
>>                                 1)) + PAGE_SHIFT;
>>
>> If one would log the value of bits, the result would then also be less
>> confusing in (at least) the mask-compression scheme.
> 
> 
> Is the above correct tough?
> 
> Take for example the hypothetical case where pdx_to_pfn() returns
> 0x10.

Hmm, yes - while impossible in the mask-compression scheme, it is in
principle possible with other schemes (like the offset one).

>  Then flsl() will return 5 (let's leave the PAGE_SHIFT
> adjustment out for the example here).  The allocation bit width would
> be off-by-one, because allocating using a bit width of 5 would also
> allow 0x11 to be allocated, and that won't be correct.
> 
> I think we need to get the bit width of the next pdx (so the
> non-inclusive end of the range), and then subtract 1 from it,
> otherwise the allocation bit width is possibly off-by-one.

I think you're right, and I can't really see how to (easily) get the more
precise value for the mask-compression scheme then. I would therefore
like to ask that you attach a comment clarifying the slight oddity.

Jan