On 2022/9/1 21:24, Joe Lawrence wrote:
> On 8/31/22 10:27 PM, Zhen Lei wrote:
>> The patch->mod is not a protected object of mutex_lock(&klp_mutex). Since
>> it's in the error handling branch, it might not be helpful to reduce lock
>> conflicts, but it can reduce some code size.
>>
>> Before:
>> text data bss dec hex filename
>> 10330 464 8 10802 2a32 kernel/livepatch/core.o
>>
>> After:
>> text data bss dec hex filename
>> 10307 464 8 10779 2a1b kernel/livepatch/core.o
>>
>
> Is a size change expected, or is it just compiler fall out from
> shuffling the code around a little bit?
I thought it was because mutex_lock()/mutex_unlock() was close enough to
reduce a "&klp_mutex" operation. Now, I was wrong.
>
> I see some arches do a little better, some a little worse with gcc-9.3.0
> cross compilers:
Sorry. This is what I should have done. I built it on x86_64 with gcc-8.4.0
>
> Before
> ------
> text data bss dec hex filename
> 8490 600 8 9098 238a arm64/kernel/livepatch/core.o
> 9424 680 8 10112 2780 s390/kernel/livepatch/core.o
> 9802 228 4 10034 2732 ppc32/kernel/livepatch/core.o
> 13746 456 8 14210 3782 ppc64le/kernel/livepatch/core.o
> 10443 464 8 10915 2aa3 x86_64/kernel/livepatch/core.o
>
>
> After
> -----
> text data bss dec hex filename
> 8514 600 8 9122 23a2 arm64/kernel/livepatch/core.o
> 9424 680 8 10112 2780 s390/kernel/livepatch/core.o
> 9818 228 4 10050 2742 ppc32/kernel/livepatch/core.o
> 13762 456 8 14226 3792 ppc64le/kernel/livepatch/core.o
> 10446 464 8 10918 2aa6 x86_64/kernel/livepatch/core.o
>
> In which case, I'd just omit the size savings from the commit msg.
OK. Should I send v2 to update commit message?
>
>> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
>> ---
>> kernel/livepatch/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 42f7e716d56bf72..cb7abc821a50584 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -1041,9 +1041,9 @@ int klp_enable_patch(struct klp_patch *patch)
>> mutex_lock(&klp_mutex);
>>
>> if (!klp_is_patch_compatible(patch)) {
>> + mutex_unlock(&klp_mutex);
>> pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
>> patch->mod->name);
>> - mutex_unlock(&klp_mutex);
>> return -EINVAL;
>> }
>>
>>
>
> That said, I don't see anything obviously wrong about the change (we
> don't need to sync our error msgs, right?) so:
Yes
>
> Acked-by: Joe Lawrence <joe.lawrence@xxxxxxxxxx>
Thanks
>
--
Regards,
Zhen Lei
