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
Signed-off-by: Zhen Lei <[email protected]>
---
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;
}
--
2.25.1
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 see some arches do a little better, some a little worse with gcc-9.3.0
cross compilers:
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.
> Signed-off-by: Zhen Lei <[email protected]>
> ---
> 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:
Acked-by: Joe Lawrence <[email protected]>
--
Joe
On Thu 2022-09-01 10:27:06, 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
Please, is this part of some longterm effort to reduce the size of
the kernel? Or is this just some random observation?
> Signed-off-by: Zhen Lei <[email protected]>
> ---
> 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);
I do not see how this change could reliably reduce the code size.
As Joe wrote, it looks like a random effect that is specific to a
particular compiler version, compilation options, and architecture.
I am against these kind of random microptimizations. It is just a call
for problems. If you move printk() outside of a lock, you need to make
sure that the information is not racy.
It might be safe in this particular case. But it is a bad practice.
It adds an extra work. It is error-prone with questionable gain.
I am sorry but I NACK this patch. There must be better ways to
reduce the kernel binary size.
Best Regards,
Petr
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 <[email protected]>
>> ---
>> 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 <[email protected]>
Thanks
>
--
Regards,
Zhen Lei
On 2022/9/1 22:18, Petr Mladek wrote:
> On Thu 2022-09-01 10:27:06, 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
>
> Please, is this part of some longterm effort to reduce the size of
> the kernel? Or is this just some random observation?
>
>
>> Signed-off-by: Zhen Lei <[email protected]>
>> ---
>> 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);
>
> I do not see how this change could reliably reduce the code size.
>
> As Joe wrote, it looks like a random effect that is specific to a
> particular compiler version, compilation options, and architecture.
>
> I am against these kind of random microptimizations. It is just a call
> for problems. If you move printk() outside of a lock, you need to make
> sure that the information is not racy.
OK.
mutex_lock(&klp_mutex);
if (!klp_is_patch_compatible(patch)) {
mutex_unlock(&klp_mutex);
<--------- Do you mean the incompatible patches maybe disabled at this point?
pr_err("Livepatch patch (%s) ...\n", patch->mod->name);
return -EINVAL;
}
>
> It might be safe in this particular case. But it is a bad practice.
> It adds an extra work. It is error-prone with questionable gain.
>
> I am sorry but I NACK this patch. There must be better ways to
OK
> reduce the kernel binary size.
>
> Best Regards,
> Petr
> .
>
--
Regards,
Zhen Lei
On Fri 2022-09-02 09:28:59, Leizhen (ThunderTown) wrote:
> >> 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);
> >
> > I do not see how this change could reliably reduce the code size.
> >
> > As Joe wrote, it looks like a random effect that is specific to a
> > particular compiler version, compilation options, and architecture.
> >
> > I am against these kind of random microptimizations. It is just a call
> > for problems. If you move printk() outside of a lock, you need to make
> > sure that the information is not racy.
>
> OK.
>
> mutex_lock(&klp_mutex);
> if (!klp_is_patch_compatible(patch)) {
> mutex_unlock(&klp_mutex);
> <--------- Do you mean the incompatible patches maybe disabled at this point?
This particular change is safe in the current design.
klp_enable_patch() is called from the module_init() callback
where patch->mod->name is defined. So it can't change.
The problem is that it is not obvious that it is safe. One has to
think about it. Also it might become dangerous when someone
tries to call klp_enable_livepatch() for another livepatch module.
> pr_err("Livepatch patch (%s) ...\n", patch->mod->name);
> return -EINVAL;
> }
>
> >
> > It might be safe in this particular case. But it is a bad practice.
> > It adds an extra work. It is error-prone with questionable gain.
> >
> > I am sorry but I NACK this patch. There must be better ways to
>
> OK
Thanks for understanding.
Best Regards,
Petr
On 2022/9/2 21:36, Petr Mladek wrote:
> On Fri 2022-09-02 09:28:59, Leizhen (ThunderTown) wrote:
>>>> 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);
>>>
>>> I do not see how this change could reliably reduce the code size.
>>>
>>> As Joe wrote, it looks like a random effect that is specific to a
>>> particular compiler version, compilation options, and architecture.
>>>
>>> I am against these kind of random microptimizations. It is just a call
>>> for problems. If you move printk() outside of a lock, you need to make
>>> sure that the information is not racy.
>>
>> OK.
>>
>> mutex_lock(&klp_mutex);
>> if (!klp_is_patch_compatible(patch)) {
>> mutex_unlock(&klp_mutex);
>> <--------- Do you mean the incompatible patches maybe disabled at this point?
>
> This particular change is safe in the current design.
> klp_enable_patch() is called from the module_init() callback
> where patch->mod->name is defined. So it can't change.
>
> The problem is that it is not obvious that it is safe. One has to
> think about it. Also it might become dangerous when someone
> tries to call klp_enable_livepatch() for another livepatch module.
OK, I got it, thanks.
>
>> pr_err("Livepatch patch (%s) ...\n", patch->mod->name);
>> return -EINVAL;
>> }
>>
>>>
>>> It might be safe in this particular case. But it is a bad practice.
>>> It adds an extra work. It is error-prone with questionable gain.
>>>
>>> I am sorry but I NACK this patch. There must be better ways to
>>
>> OK
>
> Thanks for understanding.
>
> Best Regards,
> Petr
> .
>
--
Regards,
Zhen Lei