Re: [PATCH v2 4/8] pdx: introduce command line compression toggle

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 25, 2025 at 06:00:48PM +0200, Jan Beulich wrote:
> On 25.06.2025 17:46, Roger Pau Monné wrote:
> > On Tue, Jun 24, 2025 at 03:40:16PM +0200, Jan Beulich wrote:
> >> On 20.06.2025 13:11, Roger Pau Monne wrote:
> >>> Introduce a command line option to allow disabling PDX compression.  The
> >>> disabling is done by turning pfn_pdx_add_region() into a no-op, so when
> >>> attempting to initialize the selected compression algorithm the array of
> >>> ranges to compress is empty.
> >>
> >> While neat, this also feels fragile. It's not obvious that for any
> >> algorithm pfn_pdx_compression_setup() would leave compression disabled
> >> when there are zero ranges. In principle, if it was written differently
> >> for mask compression, there being no ranges could result in compression
> >> simply squeezing out all of the address bits. Yet as long as we think
> >> we're going to keep this in mind ...
> > 
> > It seemed to me that nr_rages == 0 (so no ranges reported) should
> > result in no compression, for example on x86 this means there's no
> > SRAT.
> 
> Just to mention it: While in the pfn_pdx_compression_setup() flavor in
> patch 3 there's no explicit check (hence the logic is assumed to be
> coping with that situation),

If you prefer I can leave the pfn_pdx_compression_setup() as-is in
patch 3, as AFAICT that implementation does cope with nr_ranges == 0,
that would result in a mask with just the low bits set, and hence
hole_shift will be 0.

> the one introduced in the last patch does
> have such an explicit check. Apparently there the logic doesn't cleanly
> cover that case all by itself.

No, I don't think the logic in patch 8 will cope nicely with nr_ranges
== 0, it seems to me at least the flsl() against a 0 pdx size mask
would result in an invalid pdx_index_shift given the current logic.

IMO it's best to short-circuit the nr_ranges == 0 case early in the
function, as that avoids complexity.

Thanks, Roger.