* [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
@ 2022-09-01 2:27 Zhen Lei
2022-09-01 13:24 ` Joe Lawrence
2022-09-01 14:18 ` Petr Mladek
0 siblings, 2 replies; 7+ messages in thread
From: Zhen Lei @ 2022-09-01 2:27 UTC (permalink / raw)
To: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, live-patching, linux-kernel
Cc: Zhen Lei
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 <thunder.leizhen@huawei.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
2022-09-01 2:27 [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch() Zhen Lei
@ 2022-09-01 13:24 ` Joe Lawrence
2022-09-01 13:45 ` Leizhen (ThunderTown)
2022-09-01 14:18 ` Petr Mladek
1 sibling, 1 reply; 7+ messages in thread
From: Joe Lawrence @ 2022-09-01 13:24 UTC (permalink / raw)
To: Zhen Lei
Cc: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
linux-kernel, live-patching
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 <thunder.leizhen@huawei.com>
> ---
> 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 <joe.lawrence@redhat.com>
--
Joe
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
2022-09-01 13:24 ` Joe Lawrence
@ 2022-09-01 13:45 ` Leizhen (ThunderTown)
0 siblings, 0 replies; 7+ messages in thread
From: Leizhen (ThunderTown) @ 2022-09-01 13:45 UTC (permalink / raw)
To: Joe Lawrence
Cc: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
linux-kernel, live-patching
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@huawei.com>
>> ---
>> 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@redhat.com>
Thanks
>
--
Regards,
Zhen Lei
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
2022-09-01 2:27 [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch() Zhen Lei
2022-09-01 13:24 ` Joe Lawrence
@ 2022-09-01 14:18 ` Petr Mladek
2022-09-02 1:28 ` Leizhen (ThunderTown)
1 sibling, 1 reply; 7+ messages in thread
From: Petr Mladek @ 2022-09-01 14:18 UTC (permalink / raw)
To: Zhen Lei
Cc: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Joe Lawrence,
live-patching, linux-kernel
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 <thunder.leizhen@huawei.com>
> ---
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
2022-09-01 14:18 ` Petr Mladek
@ 2022-09-02 1:28 ` Leizhen (ThunderTown)
2022-09-02 13:36 ` Petr Mladek
0 siblings, 1 reply; 7+ messages in thread
From: Leizhen (ThunderTown) @ 2022-09-02 1:28 UTC (permalink / raw)
To: Petr Mladek
Cc: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Joe Lawrence,
live-patching, linux-kernel
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 <thunder.leizhen@huawei.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
2022-09-02 1:28 ` Leizhen (ThunderTown)
@ 2022-09-02 13:36 ` Petr Mladek
2022-09-05 1:20 ` Leizhen (ThunderTown)
0 siblings, 1 reply; 7+ messages in thread
From: Petr Mladek @ 2022-09-02 13:36 UTC (permalink / raw)
To: Leizhen (ThunderTown)
Cc: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Joe Lawrence,
live-patching, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()
2022-09-02 13:36 ` Petr Mladek
@ 2022-09-05 1:20 ` Leizhen (ThunderTown)
0 siblings, 0 replies; 7+ messages in thread
From: Leizhen (ThunderTown) @ 2022-09-05 1:20 UTC (permalink / raw)
To: Petr Mladek
Cc: Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Joe Lawrence,
live-patching, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-05 1:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-01 2:27 [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch() Zhen Lei
2022-09-01 13:24 ` Joe Lawrence
2022-09-01 13:45 ` Leizhen (ThunderTown)
2022-09-01 14:18 ` Petr Mladek
2022-09-02 1:28 ` Leizhen (ThunderTown)
2022-09-02 13:36 ` Petr Mladek
2022-09-05 1:20 ` Leizhen (ThunderTown)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).