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
