Re: [PATCH v2 8/8] pdx: introduce a new compression algorithm based on region offsets

["Roger Pau Monné" <roger.pau@citrix.com>]

On Sun, Jun 29, 2025 at 04:36:25PM +0200, Jan Beulich wrote:
> On 27.06.2025 16:51, Roger Pau Monné wrote:
> > On Thu, Jun 26, 2025 at 09:35:04AM +0200, Jan Beulich wrote:
> >> On 25.06.2025 18:24, Roger Pau Monné wrote:
> >>> On Tue, Jun 24, 2025 at 06:16:15PM +0200, Jan Beulich wrote:
> >>>> On 20.06.2025 13:11, Roger Pau Monne wrote:
> >>>>> +bool pdx_is_region_compressible(paddr_t base, unsigned long npages)
> >>>>> +{
> >>>>> +    unsigned long pfn = PFN_DOWN(base);
> >>>>> +
> >>>>> +    return pdx_to_pfn(pfn_to_pdx(pfn) + npages - 1) == (pfn + npages - 1);
> >>>>
> >>>> Aiui for this to be correct, there need to be gaps between the ranges
> >>>> covered by individual lookup table slots. In the setup logic you have a
> >>>> check commented "Avoid compression if there's no gain", but that doesn't
> >>>> look to guarantee gaps everywhere (nor would pfn_offset_sanitize_ranges()
> >>>> appear to)?
> >>>
> >>> But if there are no gaps, the full region is covered correctly, and
> >>> hence it's compressible?
> >>
> >> If there's a guarantee that such ranges would be folded into a single one,
> >> all would be fine.
> >>
> >>> Maybe I'm missing something, could you maybe provide an example that
> >>> would exhibit this issue?
> >>
> >> My understanding is that when there's no gap between regions, and when
> >> [base, base + npages) crosses as region boundary, then the expression
> >> above will yield true when, because of crossing a region boundary, it
> >> ought to be false. Or did I simply misunderstand the purpose of the
> >> pdx_is_region_compressible() invocations?
> > 
> > If there's no gap between the regions it's IMO intended for
> > pdx_is_region_compressible() to return true, as the whole region is
> > continuous in both the PFN and PDX spaces, and hence compressible
> > (even if it spans multiple regions).
> 
> My problem is that I can't make the connection between that function
> returning true and regions getting concatenated. When the function is
> invoked, concatenation (or not) has happened already, aiui.

According to my understanding, a region is compressible if there's a
contiguous PDX translation that covers the whole region.  And I agree,
concatenation or not doesn't really matter here.

> > But maybe I'm not understanding your point correctly, could you maybe
> > provide an example if you disagree with my reply above?  Sorry if I'm
> > being dull, with this compression stuff it's sometimes hard for me to
> > visualize the case you are trying to make without a concrete
> > example.
> 
> What I think I didn't take into consideration is that from two pages
> being contiguous in MFN space, it ought to follow they're also
> contiguous in PDX space. Hence [base, base + npages) crossing a region
> boundary (if, contrary to what you say, this was possible in the first
> place) would still not be encountering a discontinuity. So overall not
> an issue, irrespective of what pdx_is_region_compressible() means
> towards (non-)contiguity.

OK, so I think we are in agreement that region crossing in
pdx_is_region_compressible() is not an issue, as long as regions are
contiguous.

Thanks, Roger.