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

[Julien Grall <julien@xxxxxxx>]

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.
, 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.
So I would like to get rid of any use of range in new functions.

Cheers,

--
Julien Grall