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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2023 12:14, Alejandro Vallejo wrote:
> On Tue, Jul 25, 2023 at 04:34:42PM +0200, Jan Beulich wrote:
>> 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
> 
> From the look of things, I think I'm leaning on the side of Julien. The
> ranges are always ambiguous in the absence of a strong code convention, but
> it appears to be absent in the codebase. i.e: Are they meant to be
> inclusive or exclusive? Traditionally I've seen and used the later more
> often for ease of arithmetic, while from your last message you seem to be
> mentioning the latter.
> 
> If the point is to disambiguate without resorting to conventions then
> there's only realistic one option I see. Would refactoring it to
> "paddr_t base, size_t npages" satisfy you both?

If we are to refactor, then yes except it shouldn't be size_t. I can be
unsigned int (when we know more than 4 billion pages aren't possible)
or unsigned long.

Jan

> If this is something to be
> taken into account for future code we probably want a gitlab ticket to
> refactor everything else to (base,[npages|noctets]) too. e.g:
> XENPF_mem_hotadd uses the [spfn, epfn) convention, and so do a bunch of
> other pieces in x86 and common code.
> 
> Thanks,
> Alejandro
>