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;
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.
Jan
