| Message ID | 20250505075333.184463-4-sshegde@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc: use lock guards for mutex Set 1 | expand |
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. 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@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.
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();