livepatch: Move error print out of lock protection in klp_enable_patch()

[PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Zhen Lei 3 years, 7 months ago

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

Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Petr Mladek 3 years, 7 months ago

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

Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Leizhen (ThunderTown) 3 years, 7 months ago


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

Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Petr Mladek 3 years, 7 months ago

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

Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Leizhen (ThunderTown) 3 years, 7 months ago


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

Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Joe Lawrence 3 years, 7 months ago

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

Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable_patch()

Posted by Leizhen (ThunderTown) 3 years, 7 months ago


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