Re: [PATCH v3 3/6] powerpc: fadump: use lock guard for mutex

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 5/8/25 11:23, Sourabh Jain wrote:
> 

Hi Sourabh.

> On 05/05/25 13:23, Shrikanth Hegde wrote:
>> use scoped_guard for scope based resource management of mutex.
>> This would make the code simpler and easier to maintain.
>>
>> More details on lock guards can be found at
>> https://lore.kernel.org/all/20230612093537.614161713@infradead.org/T/#u
>>
>> Reviewed-by: Srikar Dronamraju <srikar@linux.ibm.com>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/fadump.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>> index df16c7f547ab..b8c7993c5bb1 100644
>> --- a/arch/powerpc/kernel/fadump.c
>> +++ b/arch/powerpc/kernel/fadump.c
>> @@ -1375,15 +1375,12 @@ static void fadump_free_elfcorehdr_buf(void)
>>   static void fadump_invalidate_release_mem(void)
>>   {
>> -    mutex_lock(&fadump_mutex);
>> -    if (!fw_dump.dump_active) {
>> -        mutex_unlock(&fadump_mutex);
>> -        return;
>> +    scoped_guard(mutex, &fadump_mutex) {
>> +        if (!fw_dump.dump_active)
>> +            return;
>> +        fadump_cleanup();
>>       }
>> -    fadump_cleanup();
>> -    mutex_unlock(&fadump_mutex);
>> -
>>       fadump_free_elfcorehdr_buf();
>>       fadump_release_memory(fw_dump.boot_mem_top, 
>> memblock_end_of_DRAM());
>>       fadump_free_cpu_notes_buf();
> 
> I tried to understand how scoped_guard gets unwrapped and what changes
> it brings to the assembly of the update function. However, with GCC version
> 11.5.0 20240719 (Red Hat 11.5.0-5), identical assembly was generated for 
> the
> fadump_invalidate_release_mem function with or without this patch.
> 
> Which was a surprise to me because there are lot macros and compiler
> magic involved here to call destructor ( for example https:// 
> clang.llvm.org/docs/AttributeReference.html#cleanup)
> when a variable goes out of scope.

that is nice to see.

> 
> c000000000053978 <fadump_invalidate_release_mem.part.0>:
> c000000000053978:       ae 01 4c 3c     addis   r2,r12,430
> c00000000005397c:       88 47 42 38     addi    r2,r2,18312
> c000000000053980:       a6 02 08 7c     mflr    r0
> c000000000053984:       11 57 02 48     bl      c000000000079094 <_mcount>
> c000000000053988:       a6 02 08 7c     mflr    r0
> c00000000005398c:       f8 ff e1 fb     std     r31,-8(r1)
> c000000000053990:       f0 ff c1 fb     std     r30,-16(r1)
> c000000000053994:       1f 01 e2 3f     addis   r31,r2,287
> c000000000053998:       30 ea ff 3b     addi    r31,r31,-5584
> c00000000005399c:       10 00 01 f8     std     r0,16(r1)
> c0000000000539a0:       81 ff 21 f8     stdu    r1,-128(r1)
> c0000000000539a4:       18 00 41 f8     std     r2,24(r1)
> c0000000000539a8:       ad fe ff 4b     bl      c000000000053854 
> <fadump_cleanup+0x8>
> c0000000000539ac:       c2 00 62 3c     addis   r3,r2,194
> c0000000000539b0:       98 c3 63 38     addi    r3,r3,-15464
> c0000000000539b4:       c9 1d 06 49     bl      c0000000010b577c 
> <mutex_unlock+0x8>
> c0000000000539b8:       00 00 00 60     nop
> c0000000000539bc:       1f 01 22 3d     addis   r9,r2,287
> snip...
> 
> 
> Also, fadump_invalidate_release_mem() is only called in the fadump 
> kernel in two scenarios
> to release the reserved memory:
> 
> 1. After dump collection
> 2. When fadump fails to process the dump
> 
> So even if the compiler messes up something here, there is no impact on 
> dump collection as such.

If there is a compiler mess up we will have a much bigger issue, since 
these are quite widely used in core areas such as scheduler, timers etc.

> 
> So changes looks good to me:
> Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>

Thanks for taking a look and reviewing it.