Re: [PATCH 6/8] mm/pdx: Standardize region validation wrt pdx compression

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2023 16:27, Julien Grall wrote:
> Hi,
> 
> On 25/07/2023 07:51, Jan Beulich wrote:
>> On 24.07.2023 20:20, Julien Grall wrote:
>>> On 24/07/2023 13:18, Alejandro Vallejo wrote:
>>>> On Fri, Jul 21, 2023 at 06:05:51PM +0100, Julien Grall wrote:
>>>>> Hi Alejandro,
>>>>>
>>>>> On 17/07/2023 17:03, Alejandro Vallejo wrote:
>>>>>> +bool pdx_is_region_compressible(unsigned long smfn, unsigned long emfn)
>>>>>
>>>>> For newer interface, I would rather prefer if we use start + size. It is
>>>>> easier to reason (you don't have to wonder whether 'emfn' is inclusive or
>>>>> not) and avoid issue in the case you are trying to handle a region right 
>>>>> at
>>>>> the end of the address space as emfn would be 0 in the non-inclusive case
>>>>> (not much a concern for MFNs as the last one should be invalid, but it 
>>>>> makes
>>>>> harder to reason).
>>>> I could agree on this, but every single caller is based on (smfn, emfn),
>>>> so it needlessly forces every caller to perform conversions where the
>>>> callee can do it just once.
>>>
>>> That's indeed one way to see it. The problem is that...
>>>
>>>> That said, I think your point makes sense and
>>>> it ought to be done. Probably as as part of a bigger refactor where
>>>> (smfn, emfn)-based functions are turned into (base, len) variants.
>>>
>>> ... clean-up tends to be put in the back-burner and we just continue to
>>> add new use. This makes the task to remove every use a lot more
>>> difficult. So there is a point when one has to say no more.
>>>
>>> Therefore, I would strongly prefer if each callers are doing the
>>> computation. The rest can be removed leisurely.
>>>
>>> Let see what the opinion of the other maintainers.
>>
>> I think [a,b] ranges are fine to pass, and may even be preferable over
>> passing a size. I'm specifically using that term that you also used:
>> "size" (or "length") is ambiguous when talking about page granular
>> items - is it in bytes or number of pages?
> 
> I was referring to the number of pages. I don't think it make sense to 
> have it in bytes given the start is a frame.
> 
>> Especially in the former
>> case calculations at the call sites would be quite a bit more cumbersome.
>> I could agree with (mfn,nr) tuples
> 
> Ok. So your objection of my proposal is just about the name, right? If 
> so, I didn't put too much thought behind the naming when I sent my 
> original e-mail. I am open to any.

Something like "nr" would be fine with me, for example.

> , but as said I think inclusive
>> ranges are also fine to use (and would be less of a problem at the call
>> sites here, afaics).
> 
> The problem with range is that it can lead to confusion on whether the 
> end is inclusive or exclusive. We had one bug recently in the Arm PCI 
> code because of that.

It's a matter of properly writing it down, I would say.

> So I would like to get rid of any use of range in new functions.

Hmm, seems to need further input from other maintainers / committers
then.

Jan