Re: [PATCH v2 8/8] drm/xe/gt_throttle: Avoid TOCTOU when monitoring reasons

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Oct 28, 2025 at 11:04:56AM -0500, Lucas De Marchi wrote:
> On Tue, Oct 28, 2025 at 10:02:45AM -0400, Rodrigo Vivi wrote:
> > On Sun, Oct 26, 2025 at 10:57:20PM -0700, Lucas De Marchi wrote:
> > > It's currently not possible to safely monitor if there's throttling
> > > happening and what are the reasons. The approach of reading the status
> > > and then reading the reasons is not reliable as by the time sysadmin
> > > reads the reason, the throttling could not be happening anymore.
> > > 
> > > Previous tentative to fix that[1] was breaking the ABI and potentially
> > > sysadmin's scripts. This takes a different approach of adding and
> > > documenting the additional attribute. It's still valuable, though
> > > redundant, to provide the simpler 0/1 interface.
> > > 
> > > In order to avoid userspace knowledge on the bitmask meaning and to be
> > > able to maintain the kernel side in sync with possible changes in
> > > future, just walk the attribute group and check what are the masks that
> > > match the value read.
> > > 
> > > [1] https://lore.kernel.org/intel-xe/20241025092238.167042-1-raag.jadav@intel.com/
> > > 
> > > Cc: Raag Jadav <raag.jadav@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_gt_throttle.c | 46 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 44 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_throttle.c b/drivers/gpu/drm/xe/xe_gt_throttle.c
> > > index fa7068aac3344..fd2988dacbbb6 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_throttle.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_throttle.c
> > > @@ -22,9 +22,15 @@
> > >   * Their availability depend on the platform and some may not be visible if that
> > >   * reason is not available.
> > >   *
> > > + * The ``status_reasons`` attribute can be used by sysadmin monitoring all
> > > + * possible reasons for throttling and reporting them. It's preferred over
> > > + * monitoring ``status`` and then reading the reason both for simplicity and to
> > > + * avoid TOCTOU.
> > 
> > Perhaps add something like: TOCTOU (time-of-check to time-of-use).
> > 
> 
> ok... just to be clear, this is not about any security issue that
> sometimes the TOCTOU abbreviation is associated with, it's just a
> "normal bug/race" in the interface.
> 
> > > + *
> > >   * The following attributes are available on Crescent Island platform:
> > >   *
> > > - * - ``status``: Overall throttle status
> > > + * - ``status``: Overall throttle status (0: no throttling, 1: throttling)
> > > + * - ``status_reasons``: All reasons causing throttling separated by newline.
> > >   * - ``reason_pl1``: package PL1
> > >   * - ``reason_pl2``: package PL2
> > >   * - ``reason_pl4``: package PL4
> > > @@ -43,7 +49,8 @@
> > >   *
> > >   * Other platforms support the following reasons:
> > >   *
> > > - * - ``status``: Overall status
> > > + * - ``status``: Overall throttle status (0: no throttling, 1: throttling)
> > > + * - ``status_reasons``: All reasons causing throttling separated by newline.
> > >   * - ``reason_pl1``: package PL1
> > >   * - ``reason_pl2``: package PL2
> > >   * - ``reason_pl4``: package PL4, Iccmax etc.
> > > @@ -111,12 +118,45 @@ static ssize_t reason_show(struct kobject *kobj,
> > >  	return sysfs_emit(buff, "%u\n", is_throttled_by(gt, ta->mask));
> > >  }
> > > 
> > > +static const struct attribute_group *get_platform_throttle_group(struct xe_device *xe);
> > > +
> > > +static ssize_t status_reasons_show(struct kobject *kobj,
> > > +				   struct kobj_attribute *attr, char *buff)
> > > +{
> > > +	struct xe_gt *gt = throttle_to_gt(kobj);
> > > +	struct xe_device *xe = gt_to_xe(gt);
> > > +	const struct attribute_group *group;
> > > +	struct attribute **pother;
> > > +	ssize_t ret = 0;
> > > +	u32 reasons;
> > > +
> > > +	reasons = xe_gt_throttle_get_limit_reasons(gt);
> > > +	group = get_platform_throttle_group(xe);
> > > +
> > > +	for (pother = group->attrs; *pother; pother++) {
> > > +		struct kobj_attribute *kattr = container_of(*pother, struct kobj_attribute, attr);
> > > +		struct throttle_attribute *other_ta = kobj_attribute_to_throttle(kattr);
> > > +
> > > +		if (other_ta->mask != U32_MAX && reasons & other_ta->mask)
> > > +			ret += sysfs_emit_at(buff, ret, "%s\n", (*pother)->name);
> > 
> > perhaps a space instead of the \n to keep only a single line?
> 
> I'm lazy and I don't like the additional space that the lazy approach
> would cause in the last entry :-/. For printing arrays to sysfs
> doesn't seem there's a uniform approach. Some use \n and some use
> space.
> 
> do we care about the extra space? if not, then it would be:
> 
> 	ret += sysfs_emit_at(buff, ret, "%s ", (*pother)->name);

For the simplicity (lazyness?!) I believe we should go like this
and then an extra \n after like done in
show_scaling_available_governors() at drivers/cpufreq/cpufreq.c

> 
> if we do, then:
> 
> 	ret += sysfs_emit_at(buff, ret, "%s%s", ret ? " " : "", (*pother)->name);
> or
> 	ret += sysfs_emit_at(buff, ret, "%.*s%s", !!ret, " ", (*pother)->name);
> 
> thanks
> Lucas De Marchi
> 
> > 
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  #define THROTTLE_ATTR_RO(name, _mask)				\
> > >  	struct throttle_attribute attr_##name =	{		\
> > >  		.attr = __ATTR(name, 0444, reason_show, NULL),	\
> > >  		.mask = _mask,					\
> > >  	}
> > > 
> > > +#define THROTTLE_ATTR_RO_FUNC(name, _mask, _show)		\
> > > +	struct throttle_attribute attr_##name =	{		\
> > > +		.attr = __ATTR(name, 0444, _show, NULL),	\
> > > +		.mask = _mask,					\
> > > +	}
> > > +
> > > +static THROTTLE_ATTR_RO_FUNC(status_reasons, 0, status_reasons_show);
> > >  static THROTTLE_ATTR_RO(status, U32_MAX);
> > >  static THROTTLE_ATTR_RO(reason_pl1, POWER_LIMIT_1_MASK);
> > >  static THROTTLE_ATTR_RO(reason_pl2, POWER_LIMIT_2_MASK);
> > > @@ -128,6 +168,7 @@ static THROTTLE_ATTR_RO(reason_vr_thermalert, VR_THERMALERT_MASK);
> > >  static THROTTLE_ATTR_RO(reason_vr_tdc, VR_TDC_MASK);
> > > 
> > >  static struct attribute *throttle_attrs[] = {
> > > +	&attr_status_reasons.attr.attr,
> > >  	&attr_status.attr.attr,
> > >  	&attr_reason_pl1.attr.attr,
> > >  	&attr_reason_pl2.attr.attr,
> > > @@ -153,6 +194,7 @@ static THROTTLE_ATTR_RO(reason_psys_crit, PSYS_CRIT_MASK);
> > > 
> > >  static struct attribute *cri_throttle_attrs[] = {
> > >  	/* Common */
> > > +	&attr_status_reasons.attr.attr,
> > >  	&attr_status.attr.attr,
> > >  	&attr_reason_pl1.attr.attr,
> > >  	&attr_reason_pl2.attr.attr,
> > > 
> > > --
> > > 2.51.0
> > >