Re: [PATCH v2 2/5] cxl: Introduce to_{ram,pmem}_{res,perf}() helpers

[Dan Williams <dan.j.williams@intel.com>]

Alejandro Lucero Palau wrote:
> 
> On 1/23/25 21:04, Dan Williams wrote:
> 
> <snip>
> 
> > xa_for_each(dsmas_xa, index, dent) {
> >>> -		if (resource_size(&cxlds->ram_res) &&
> >>> -		    range_contains(&ram_range, &dent->dpa_range))
> >>> -			update_perf_entry(dev, dent, &mds->ram_perf);
> >>> -		else if (resource_size(&cxlds->pmem_res) &&
> >>> -			 range_contains(&pmem_range, &dent->dpa_range))
> >>> -			update_perf_entry(dev, dent, &mds->pmem_perf);
> >>> -		else
> >>> -			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
> >>> -				&dent->dpa_range);
> >>> +		for (int i = 0; i < ARRAY_SIZE(partition); i++) {
> >>> +			const struct resource *res = partition[i];
> >>> +			struct range range = {
> >>> +				.start = res->start,
> >>> +				.end = res->end,
> >>> +			};
> >>> +
> >>> +			if (range_contains(&range, &dent->dpa_range))
> >>> +				update_perf_entry(dev, dent, perf[i]);
> >> This is checking if range contains dent->dpa_range.
> >>
> >> I think it is just the opposite.
> > This looks like an equivalent conversion to me, what am I missing?
> 
> 
> I guess I was confused with dpa_range in dent and assumed that was the 
> full device DPA. But now after studying the code in more detail, I see 
> it is right ... but confusing :-)
> 
> 
> If I'm not wrong again, that range_contains should really be ranges 
> being equal, what range_contains does also cover ...
> 
> 
> It would help some comment explaining what the function is doing with 
> the information coming from two different sources, one being the device 
> CDAT, with potentially performance numbers per partition, and the other 
> being the mailbox GET_PARTITION_INFO command.
> 
> 
> Anyway, apologies for the noise.

Not noise at all, a good review of this function's decisions after the
fact. Dave, do you want to follow on with some commentary for this
function?

I recall part of the motivation for range_contains() was the fact that
theoretically the CDAT DSMAS range could be less than a partition, or
cross a partition boundary. The policy here is about explicitly not
entertaining devices that want to get fancy with the CDAT. So, the
kernel just picks any DSMAS in the partition and uses that for the
performance information. If somebody screams and says the kernel needs
to be more precise (force mainline to carry more maintenance burden for
their use case) then we can revisit.

> 
> >>> @@ -567,6 +578,9 @@ static bool dpa_perf_contains(struct cxl_dpa_perf *perf,
> >>>    		.end = dpa_res->end,
> >>>    	};
> >>>    
> >>> +	if (!perf)
> >>> +		return false;
> >>> +
> >> This change seems to be an improvement or hardening. Not against doing
> >> it, but seizing the change, the function can be simplified using the
> >> parameter without any local variable.
> > No, it's not pure hardening, it is actively avoiding NULL pointer
> > de-references introduced by the new scheme to not track empty
> > partitions.
> >
> > I.e the new to_{ram,pmem}_perf() helpers return NULL when the partition
> > is zero-sized. Previously this code path would do range checks on empty
> > partitions.
> 
> 
> That's true.
> 
> 
> But I do not see the reason for using that local variable that I know is 
> not related with your change.

@perf was never NULL before this patch, it may be NULL afterwards, so I
am not understanding the "not related" charge?

I.e. previously, cxled_get_dpa_perf() would pass perf entries related to zero
ranges, but now to_ram_perf() returns NULL for those cases.

However, I could move that decision outside of dpa_perf_contains()
rather than deleting this NULL check in patch5. I'll do that.

[..]
> >>>    
> >>>    	if (!cxlds->media_ready) {
> >>>    		cxlds->dpa_res = DEFINE_RES_MEM(0, 0);
> >>> -		cxlds->ram_res = DEFINE_RES_MEM(0, 0);
> >>> -		cxlds->pmem_res = DEFINE_RES_MEM(0, 0);
> >>> +		*ram_res = DEFINE_RES_MEM(0, 0);
> >>> +		*pmem_res = DEFINE_RES_MEM(0, 0);
> >>
> >> This is a good example for the discussion about the  patch hardening
> >> resource_contains. The initialization seems fine but IORESOURCE_UNSET
> >> not used.
> > To the contrary, I think these changes are an example of "updating
> > resource_contains() to check for zero is a band-aid that does not fix
> > the root problem".
> >
> 
> I agree, but it does not preclude some resource initialization as you do 
> here not containing the IORESOURCE_UNSET flag and some other code 
> triggering the same problem I faced, so the hardening of 
> resource_contains still useful, IMO. But I'll tell Bjorn Helgaas that 
> patch is not required anymore for CXL leaving to him the decision to 
> apply it if he considers so, following the concerns you expressed there.
> 
> >> it could be argued the resource is set, but it is a zero-size resource
> >> leading to problems in current CXL code.
> > I challenge you to find problems in current CXL code after these
> > partition reworks.
> 
> That's an unfair challenge ... you removed resource_contains ... :-)
> 
> Let me think I got some credit for that after my heads-up ;-)

Oh definitely, you did.

That was part of my internal monologue / uneasiness with the Type-2
patches, something like => "why is CXL sending 0 sized resources to
resource_contains()... oh, no, this whole partition tracking
implementation needs a re-think".