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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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 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