Message-ID: <Z3a7T8L6NpFprbcy@e133380.arm.com>
Date: Thu, 2 Jan 2025 16:14:07 +0000
From: Dave Martin <Dave.Martin@....com>
To: "Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	Zeng Heng <zengheng4@...wei.com>, James Morse <james.morse@....com>
Subject: Re: [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID
 translation

Hi,

On Fri, Dec 20, 2024 at 05:01:30AM +0000, Shaopeng Tan (Fujitsu) wrote:
> Hello Dave,

[...]

> > diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > b/drivers/platform/arm64/mpam/mpam_resctrl.c
> > index 30f2caec11d7..0473286ec65a 100644
> > --- a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > +++ b/drivers/platform/arm64/mpam/mpam_resctrl.c

[...]

> > @@ -163,35 +175,71 @@ static bool mpam_resctrl_hide_cdp(enum
> > resctrl_res_level rid)
> >   */
> >  u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)  {
> > -	return mpam_partid_max + 1;
> > +	u32 res = (mpam_partid_max + 1) / partid_per_closid;
> 
> If there is a remainder, are you going to ignore the remainder PARTIDs?
> It might be better to notify the user with a warning message.

You are right: the remainder is just thrown away, because the remaining
PARTIDs would not be sufficient for a whole resctrl control group.

It would be a good idea to print a warning somewhere though; I will try
to add that.

[...]

> I think it is better to check whether PARTID narrowing feature is
> supported or not before using this function. 
> If it is checked here, since this function is called multiple times,
> the message should be printed only once by WARN_ON_ONCE(). 
> 
> Also, if the partid_per_closid is greater than (mpam_partid_max + 1),
> returning "0" will not work properly.
> To switch to default behavior (PARTID narrowing disabled), should it
> return mapm_patid_max+ 1)?

The idea is that partid_per_closid is set by some configuration logic
that is not implemented yet.  This will be a bit more complicated than
just exposing this variable directly as a kernel parameter.

Currently I just implemented a basic range check to prevent the kernel
parameter being set to stupid values, but I didn't try to make it
completely robust.


In any case, since partid_per_closid defaults to 1, the behavior of
this function should just match the non-PARTID-Narrowing behaviour
unless the user requested a different value.

Bearing that in mind, I think that a "bad" value of partid_per_closid
should not be seen here, when the rest of the implementation is
complete.

Does this sound sensible?

It could make sense to have a WARN_ON_ONCE() here anyway, though,
since this code is not on a fast path.


> > -u32 resctrl_arch_system_num_rmid_idx(void)
> > +static void mpam_resctrl_partid_range(u32 closid, enum resctrl_conf_type
> > type,
> > +				      const struct rdt_resource *r,
> > +				      u16 *min_partid, u16 *max_partid)
> >  {
> > -	u8 closid_shift = fls(mpam_pmg_max);
> > -	u32 num_partid = resctrl_arch_get_num_closid(NULL);
> > +	u16 base_partid = closid;
> > +	u16 span = 1;
> > +
> > +	if (cdp_enabled) {
> > +		base_partid *= 2;
> > +		if (mpam_resctrl_hide_cdp(r->rid) ||
> > +		    type == CDP_NONE)
> > +			span *= 2;
> > +	}
> > 
> > -	return num_partid << closid_shift;
> > +	*min_partid = base_partid * partid_per_closid;
> > +	if (max_partid)
> > +		*max_partid = *min_partid + (span * partid_per_closid - 1);
> >  }
> 
> I think the min_partid/max_partid is different depending on the type
> (CDP_DATA or CDP_CODE).

You're right!  Looking at my fixup patches from the end of December,
it looks like I already encountered this problem and wrote a
workaround, but I didn't post it at the time.

[...]

> > @@ -1190,22 +1219,13 @@ int resctrl_arch_update_one(struct rdt_resource *r,
> > struct rdt_ctrl_domain *d,
> >  		return -EINVAL;
> >  	}
> > 
> > -	/*
> > -	 * When CDP is enabled, but the resource doesn't support it, we need
> > to
> > -	 * apply the same configuration to the other partid.
> > -	 */
> > -	if (mpam_resctrl_hide_cdp(r->rid)) {
> > -		partid = resctrl_get_config_index(closid, CDP_CODE);
> > +	for (partid = min_partid; partid <= max_partid; partid++) {
> >  		err = mpam_apply_config(dom->comp, partid, &cfg);
> >  		if (err)
> > -			return err;
> > -
> > -		partid = resctrl_get_config_index(closid, CDP_DATA);
> > -		return mpam_apply_config(dom->comp, partid, &cfg);
> > -
> > -	} else {
> > -		return mpam_apply_config(dom->comp, partid, &cfg);
> > +			break;
> >  	}
> > +
> > +	return err;
> >  }
> 
> For a mixture of MSCs system, MSCs that do not support PARTID
> narrowing and support Maximum Partition feature may not work
> properly.

This is true.

For this series, we just trust that the user understands what they are
doing, but the complete implementation will need additional checks.

My plan was to do the appropriate checks when the driver starts up,
to decide whether it is safe to set partid_per_closid to a value
different from 1.

Once we get to this code, the decision has already been taken and so
the value of partid_per_closid (and the resulting mapping of resctrl
groups onto PARTIDs) can be trusted as being appropriate for the
hardware.

Can you see any problems with this approach?


[...]

> I know this is not a complete implementation.
> Mapping relationship between intPARTIDs and reqPARTIDs need to be
> written to register MPAMCFG_INTPARTID/MPAMCFG_PART_SEL.
> 
> Best regards,
> Shaopeng TAN

Agreed.  I have some prototype code for that, but I need to split up
the patches and clean it up before posting it.

Thanks for the review.

Cheers
---Dave

Powered by blists - more mailing lists