[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9f18e699-4819-4d2f-a932-fc5e399e8abd@linux.ibm.com>
Date: Thu, 8 May 2025 11:23:17 +0530
From: Sourabh Jain <sourabhjain@...ux.ibm.com>
To: Shrikanth Hegde <sshegde@...ux.ibm.com>, maddy@...ux.ibm.com,
linuxppc-dev@...ts.ozlabs.org
Cc: npiggin@...il.com, christophe.leroy@...roup.eu, mpe@...erman.id.au,
peterz@...radead.org, ajd@...ux.ibm.com, mahesh@...ux.ibm.com,
hbathini@...ux.ibm.com, linux-kernel@...r.kernel.org,
Srikar Dronamraju <srikar@...ux.ibm.com>
Subject: Re: [PATCH v3 3/6] powerpc: fadump: use lock guard for mutex
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@...ux.ibm.com>
> Signed-off-by: Shrikanth Hegde <sshegde@...ux.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.
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.
So changes looks good to me:
Reviewed-by: Sourabh Jain <sourabhjain@...ux.ibm.com>
Powered by blists - more mailing lists