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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2025 19:45, Roger Pau Monné wrote:
> 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.

FTAOD - I didn't mean to ask for any change. I merely meant to point out
that already within this series the special use of setting nr_ranges to
zero requires (a tiny bit of) extra care. But yes, since nr_ranges can
also end up being zero for other reasons, that bit of care is necessary
anyway.

Jan