Re: [RFC 0/3] FW guard class

[Nirmoy Das <nirmoy.das@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8881 bytes --]


On 6/18/2024 10:26 PM, Rodrigo Vivi wrote:
> On Mon, Jun 17, 2024 at 07:54:41PM -0500, Lucas De Marchi wrote:
>> On Mon, Jun 17, 2024 at 11:30:41PM GMT, Matthew Brost wrote:
>>> On Mon, Jun 17, 2024 at 09:24:42PM +0200, Michal Wajdeczko wrote:
>>>> On 17.06.2024 20:00, Rodrigo Vivi wrote:
>>>>> On Mon, Jun 17, 2024 at 05:24:24PM +0000, Matthew Brost wrote:
>>>>>> On Mon, Jun 17, 2024 at 04:34:27PM +0200, Michal Wajdeczko wrote:
>>>>>>> There is support for 'classes' with constructor and destructor
>>>>>>> semantics that can be used for any scope-based resource management,
>>>>>>> like device force-wake management.
>>>>>>>
>>>>>>> Add necessary definitions explicitly, since existing macros from
>>>>>>> linux/cleanup.h can't deal with our specific requirements yet.
>>>>>>>
>>>>>>> This should allow us to use:
>>>>>>>
>>>>>>> 	scoped_guard(xe_fw, fw, XE_FW_GT)
>>>>>>> 		foo();
>>>>>>> or
>>>>>>> 	CLASS(xe_fw, var)(fw, XE_FW_GT);
>>>>>>>
>>>>>>> without any concern of leaking the force-wake references.
>>>>>>>
>>>>>>> Note: this is preliminary code as right now it's unclear how to
>>>>>>> correctly handle errors from the force-wake functions.
>>>>>>>
>>>>>> I'm personally don't like this at all. IMO it obfuscate the code with
>>>>>> little real benefit. This is just an opinion though, others opinions may
>>>>>> differ from mine.
>>>> except that is more robust than hand-crafted code that is error prone,
>>>> like this snippet from wedged_mode_set():
>>>>
>>>> 	xe_pm_runtime_get(xe);
>>>> 	for_each_gt(gt, xe, id) {
>>>> 		ret = xe_guc_ads(...);
>>>> 		if (ret) {
>>>> 			xe_gt_err(gt, "...");
>>>> 			return -EIO;
>>>> 		}
>>>> 	}
>>>> 	xe_pm_runtime_put(xe);
>>>>
>>>> and thanks to PM guard class we could avoid such mistakes for free:
>>>>
>>>> 	scoped_guard(xe_pm, xe) {
>>>> 		for_each_gt(gt, xe, id) {
>>>> 			ret = xe_guc_ads(...);
>>>> 			if (ret) {
>>>> 				xe_gt_err(gt, "...");
>>>> 				return -EIO;
>>> Just responding with a question here - haven't looked at the rest of the
>>> comments.
>>>
>>> How is this not still a bug? Looking at scoped_guard, it appears to be a
>>> magic macro for loop which acquires / releases a lock or in your
>>> purposed case a PM or FW ref. Doesn't the 'return -EIO' skip the release
>>> step? I see coding patterns like above in the kernel [1] so I do assume
>> with __attribute__((cleanup)), the compiler guarantees that
>> it's executed when the variable goes out of scope. What you are probably
>> missing is the use of CLASS() declaring a variable inside the for, which
>> uses attribute cleanup:
>>
>> 	for (CLASS(_name, scope)(args),
>> 	     ...
>>
>> GCC's doc:
>>
>> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html
>>
>> 	The cleanup attribute runs a function when the variable goes out
>> 	of scope. This attribute can only be applied to auto function
>> 	scope variables; it may not be applied to parameters or
>> 	variables with static storage duration. The function must take
>> 	one parameter, a pointer to a type compatible with the variable.
>> 	The return value of the function (if any) is ignored.
>>
>> 	When multiple variables in the same scope have cleanup
>> 	attributes, at exit from the scope their associated cleanup
>> 	functions are run in reverse order of definition (last defined,
>> 	first cleanup).
>>
>> 	If -fexceptions is enabled, then cleanup_function is run during
>> 	the stack unwinding that happens during the processing of the
>> 	exception. Note that the cleanup attribute does not allow the
>> 	exception to be caught, only to perform an action. It is
>> 	undefined what happens if cleanup_function does not return
>> 	normally.
>>
>> This was only possible with the recent change in the kernel raising
>> the minimum C std to gnu11 (uapi is still c90 for compatibility):
>>
>> 	commit e8c07082a810fbb9db303a2b66b66b8d7e588b53
>> 	Author: Arnd Bergmann<arnd@arndb.de>
>> 	Date:   Tue Mar 8 22:56:14 2022 +0100
>>
>> 	    Kbuild: move to -std=gnu11
>>
>> 	    During a patch discussion, Linus brought up the option of changing
>> 	    the C standard version from gnu89 to gnu99, which allows using variable
>> 	    declaration inside of a for() loop. While the C99, C11 and later standards
>> 	    introduce many other features, most of these are already available in
>> 	    gnu89 as GNU extensions as well.
>>
>>> this works, just confused how it works.
>>>
>>> With that, any code which isn't easily understandable IMO is a negative
>>> ROI as it just creates confusion in the long / makes problems harder to
>>> understand. Again this is just my opinion.
>> I think that is mainly about getting used to the pattern. I think we
>> just have to be careful not to overshoot on trying to use everywhere.
>> For example, I don't know why there's already a second use in a separate
>> thread when we are still discussing it on this one.
>>
>> A very positive thing is that this is not xe's own invention and comes
>> from core kernel, maybe from the hottest path that is the scheduling and
>> locking. So I very much disagree with arguments raised here about
>> a) this is an alien thing and b) performance will be severely impacted
> just for the record:
> a) the alien thing is i915's with_runtime_pm... this is part of core kernel, so
> it is not an alien thing. I still don't like C++isms, but that is just a preference
> not a blocker.
>
> b) it is an overhead, but I really doubt that this would impact performance.
> Only data would show.
>
>> I've used __attribute__((cleanup)) in several userspace projects in the
>> past and it does help avoiding problems on the error path that is
>> usually not very well tested (and xe's track record on error path is not
>> very good either: those were the main issues being submitted in drm-xe-fixes
>> for the last release). So if we have a way to improve (and that I've already seen
>> being used successfully), I prefer failing on trying than on repeating
>> the same mistakes.
> Pretty much agreeing here! Specially because this is a Linux core kernel
> infra available. Let's try.
>
> Cc Nirmoy Das<nirmoy.das@intel.com>
>
> who is looking at the forcewake stuff and to solve the flow.
> Specially to get his eyes here and see if this would cover all the needed
> cases for the forcewake.
Sorry for late reply. This looks fine for me. We are still missing a 
corner case where if

xe_force_wake_get() fails this is not be calling xe_force_wake_put().
xe_force_wake_get() should revert the counter on failure.

Regards, Nirmoy

> If this series were suggesting another with_runtime_pm macro, then I would
> push back hard.
>
>>   In kmod my only regret is that I didn't start it
>> earlier, during the bootstrap of the project.
>>
>>
>> Lucas De Marchi
>>
>>
>>> Matt
>>>
>>> [1]https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/bmi323/bmi323_core.c#L1544
>>>
>>>> 			}
>>>> 		}
>>>> 	}
>>>>
>>>>> Well, on the positive side, it is not adding a driver only thing like
>>>>> i915's with_runtime_pm() macro.
>>>>>
>>>>> But I'm also not sure if I like the overall idea anyway:
>>>>>
>>>>> - I don't like adding C++isms in a pure C code. Specially something not
>>>>> so standard and common that will decrease the ramp-up time for newcomers.
>>>> does it mean that the use of other guard patterns seen elsewhere in the
>>>> tree is now prohibited on the Xe driver ? like:
>>>>
>>>> 	scoped_guard(mutex, &lock)
>>>> 		foo();
>>>>
>>>> 	scoped_guard(spinlock, &lock)
>>>> 		foo();
>>>> 	...
>>>>
>>>>> - It looks like and extra overhead on the object creation destruction.
>>>> from cleanup.h doc is sounds there is none:
>>>>
>>>>   "And through the magic of value-propagation and dead-code-elimination,
>>>> it eliminates the actual cleanup call and compiles into:"
>>>>
>>>>
>>>>> - It looks not flexible for handling different cases... like forcewake for
>>>>> instance where we might want to ignore the ack timeout in some cases.
>>>> there is scoped_cond_guard() that likely will be able to deal with it,
>>>> but I guess we first need to cleanup existing force_wake api as expected
>>>> flow is not clear and there are different approaches in the driver how
>>>> to deal with errors
>>>>
>>>>>> Matt
>>>>>>
>>>>>>> Cc: Rodrigo Vivi<rodrigo.vivi@intel.com>
>>>>>>> Cc: Lucas De Marchi<lucas.demarchi@intel.com>
>>>>>>>
>>>>>>> Michal Wajdeczko (3):
>>>>>>>    drm/xe: Introduce force-wake guard class
>>>>>>>    drm/xe: Use new FW guard in xe_mocs.c
>>>>>>>    drm/xe: Use new FW guard in xe_pat.c
>>>>>>>
>>>>>>>   drivers/gpu/drm/xe/xe_force_wake.h       | 48 +++++++++++++++++++
>>>>>>>   drivers/gpu/drm/xe/xe_force_wake_types.h | 12 +++++
>>>>>>>   drivers/gpu/drm/xe/xe_mocs.c             | 12 +----
>>>>>>>   drivers/gpu/drm/xe/xe_pat.c              | 60 ++++++++----------------
>>>>>>>   4 files changed, 82 insertions(+), 50 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> 2.43.0
>>>>>>>

[-- Attachment #2: Type: text/html, Size: 12119 bytes --]