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

[Raag Jadav <raag.jadav@intel.com>]

On Fri, Oct 31, 2025 at 01:24:08AM +0530, Vivi, Rodrigo wrote:
> On Thu, 2025-10-30 at 13:43 -0500, Lucas De Marchi wrote:
> > On Thu, Oct 30, 2025 at 05:06:46PM +0100, Raag Jadav wrote:
> > > On Thu, Oct 30, 2025 at 11:47:03AM -0400, Rodrigo Vivi wrote:
> > > > On Thu, Oct 30, 2025 at 09:55:30AM -0500, Lucas De Marchi wrote:
> > > > > On Thu, Oct 30, 2025 at 10:53:36AM +0100, Raag Jadav wrote:
> > > > > > On Wed, Oct 29, 2025 at 04:45:10PM -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/
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > > +static const struct attribute_group
> > > > > > > *get_platform_throttle_group(struct xe_device *xe);
> > > > > > > +
> > > > > > > +static ssize_t status_reasons_show(struct kobject *kobj,
> > > 
> > > Semantically there's much of a 'status' here, so this could simply
> > > be
> > > 'reasons' (and same for the attribute name).
> > > 
> > > > > > > +				   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 ", (*pother)->name);
> > > > > > 
> > > > > > Much better.
> > > > > > 
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	/* Drop extra space from last iteration above */
> > > > > > > +	if (ret)
> > > > > > > +		ret--;
> > > > > > > +
> > > > > > > +	ret += sysfs_emit_at(buff, ret, "\n");
> > > > > > 
> > > > > > I went through the documentation again and I couldn't find
> > > > > > any rules
> > > > > > related to empty files or whether it is allowed (just
> > > > > > thinking out
> > > > > > loud about no throttling cases).
> > > > > 
> > > > > do you mean if "empty" files are allowed in sysfs? I don't
> > > > > think there's
> > > > > any problem with that. It's also not empty, it has a newline
> > > > > there ;)
> > > > 
> > > > alternatively we could print the entire reg in hex format?
> > > > But I prefer the text line in this patch.
> > > > 
> > > > Nothing against the 'empty' file with or without the new-line,
> > > > but perhaps we could consider to track that in the loop
> > > > and if none is add we print
> > > > 
> > > > if (ret)
> > > > 	ret--;
> > > > else
> > > > 	sysfs_emit_at(buff, ret, "none");
> > > > 
> > > > and document that above...
> > > 
> > > +1.
> > 
> > collecting all the review commends from you 2, that would be with
> > this
> > diff squashed:
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt_throttle.c
> > b/drivers/gpu/drm/xe/xe_gt_throttle.c
> > index ca45aea8c17a6..f3476fda7f4f6 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_throttle.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_throttle.c
> > @@ -22,15 +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 (time-of-check to time-of-use).
> > + * The ``reasons`` attribute can be used by sysadmin to monitor all
> > possible
> > + * reasons for throttling and report them. It's preferred over
> > monitoring
> > + * ``status`` and then reading the reason from individual attributes
> > since that
> > + * is racy. If there's no throttling happening, "none" is returned.
> >    *
> >    * The following attributes are available on Crescent Island
> > platform:
> >    *
> >    * - ``status``: Overall throttle status (0: no throttling, 1:
> > throttling)
> > - * - ``status_reasons``: All reasons causing throttling separated by
> > newline.
> > + * - ``reasons``: All reasons causing throttling separated by space

"Array of" ...

> >    * - ``reason_pl1``: package PL1
> >    * - ``reason_pl2``: package PL2
> >    * - ``reason_pl4``: package PL4
> > @@ -50,7 +50,7 @@
> >    * Other platforms support the following reasons:
> >    *
> >    * - ``status``: Overall throttle status (0: no throttling, 1:
> > throttling)
> > - * - ``status_reasons``: All reasons causing throttling separated by
> > newline.
> > + * - ``reasons``: All reasons causing throttling separated by space

Ditto.

> >    * - ``reason_pl1``: package PL1
> >    * - ``reason_pl2``: package PL2
> >    * - ``reason_pl4``: package PL4, Iccmax etc.
> > @@ -120,8 +120,8 @@ static ssize_t reason_show(struct kobject *kobj,
> >   
> >   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)
> > +static ssize_t 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);
> > @@ -141,9 +141,11 @@ static ssize_t status_reasons_show(struct
> > kobject *kobj,
> >   			ret += sysfs_emit_at(buff, ret, "%s ",
> > (*pother)->name);
> >   	}
> >   
> > -	/* Drop extra space from last iteration above */
> >   	if (ret)
> > +		/* Drop extra space from last iteration above */
> >   		ret--;
> > +	else
> > +		ret += sysfs_emit_at(buff, ret, "none");
> >   
> >   	ret += sysfs_emit_at(buff, ret, "\n");
> >   
> > @@ -162,7 +164,7 @@ static ssize_t status_reasons_show(struct kobject
> > *kobj,
> >   		.mask = _mask,					\
> >   	}
> >   
> > -static THROTTLE_ATTR_RO_FUNC(status_reasons, 0,
> > status_reasons_show);
> > +static THROTTLE_ATTR_RO_FUNC(reasons, 0, 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);
> > @@ -174,7 +176,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_reasons.attr.attr,
> >   	&attr_status.attr.attr,
> >   	&attr_reason_pl1.attr.attr,
> >   	&attr_reason_pl2.attr.attr,
> > @@ -200,7 +202,7 @@ static THROTTLE_ATTR_RO(reason_psys_crit,
> > PSYS_CRIT_MASK);
> >   
> >   static struct attribute *cri_throttle_attrs[] = {
> >   	/* Common */
> > -	&attr_status_reasons.attr.attr,
> > +	&attr_reasons.attr.attr,
> >   	&attr_status.attr.attr,
> >   	&attr_reason_pl1.attr.attr,
> >   	&attr_reason_pl2.attr.attr,
> 
> works for me. count on my rv-b.
> 
> unless Raag has any further concern...

Works for me as well.

Raag