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 >> 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 > > 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 >>>>>>> Cc: Lucas De Marchi >>>>>>> >>>>>>> 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 >>>>>>>