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

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

On Mon, Oct 27, 2025 at 08:26:01AM -0500, Lucas De Marchi wrote:
> On Mon, Oct 27, 2025 at 12:50:43PM +0100, Raag Jadav 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/
> > 
> > Thanks for looking into this, but following the recent developments on [2]
> > I'm beginning to have my doubts about this.
> > 
> > I don't have any strong opinions but we can probably hold on to this one
> > until we reach some conclusion on the matter.
> > 
> > [2] https://lore.kernel.org/intel-xe/20251014053257.3417575-2-riana.tauro@intel.com/
> 
> what's the relation between that patch and this one? If it's about
> multiple values in a single attribute, then they are not really at the
> same level:
> 
> Compare this:
> 	pl1
> 	pl4
> 
> To that:
> 	Capability Info: 0x138320 - 0x2001ae06
> 	Postcode Info: 0x138324 - 0x0
> 	Overflow Info: 0x138328 - 0x0
> 	Auxiliary Info 0: 0x13832c - 0x0
> 
> And from Documentation/filesystems/sysfs.rst:
> 
> 	Attributes should be ASCII text files, preferably with only one value
> 	per file. It is noted that it may not be efficient to contain only one
> 	value per file, so it is socially acceptable to express an array of
> 	values of the same type.
> 
> 	Mixing types, expressing multiple lines of data, and doing fancy
> 	formatting of data is heavily frowned upon. Doing these things may get
> 	you publicly humiliated and your code rewritten without notice.
> 
> I'd say the array of "reasons" in this patch clearly matches the first
> paragraph: it's the same type, not fancy formatting and has a reason to
> exist: TOCTOU mentioned in the docs and commit message). Another thing
> that I consider important: not expose the raw number to userspace so it
> doesn't have to decode the bitmapp per platform in userspace (the
> platform abstraction belongs in the kernel) and is easy to maintain for
> new platforms (hence the walk over the attribute_group).
> 
> The capability stuff on the other patch is much closer to the second
> paragraph.

Let me discuss this with Anirban if there are any particular caveats
on consumer side.

Raag