[PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[Shang XiaoJing]

Instead of invoking a synchronize_rcu() to free a pointer after a grace
period, we can directly make use of a new API that does the same but in
a more efficient way.

Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
---
Changelog:
v3: the first version of the PATCH
v1: v3 resent as v1
v2: use kfree_rcu() instead of kvfree_rcu() for clarity
v4: resend v2 as v4 to avoid versioning confusion
---
 drivers/staging/fwserial/fwserial.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 81b06d88ed0d..8d2b4ed1f39e 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -2117,8 +2117,7 @@ static void fwserial_remove_peer(struct fwtty_peer *peer)
 	if (port)
 		fwserial_release_port(port, true);
 
-	synchronize_rcu();
-	kfree(peer);
+	kfree_rcu(peer);
 }
 
 /**
-- 
2.17.1

Re: [PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[Greg KH]

On Mon, Sep 19, 2022 at 05:10:56PM +0800, Shang XiaoJing wrote:
> Instead of invoking a synchronize_rcu() to free a pointer after a grace
> period, we can directly make use of a new API that does the same but in
> a more efficient way.
> 
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> ---
> Changelog:
> v3: the first version of the PATCH
> v1: v3 resent as v1
> v2: use kfree_rcu() instead of kvfree_rcu() for clarity
> v4: resend v2 as v4 to avoid versioning confusion
> ---
>  drivers/staging/fwserial/fwserial.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
> index 81b06d88ed0d..8d2b4ed1f39e 100644
> --- a/drivers/staging/fwserial/fwserial.c
> +++ b/drivers/staging/fwserial/fwserial.c
> @@ -2117,8 +2117,7 @@ static void fwserial_remove_peer(struct fwtty_peer *peer)
>  	if (port)
>  		fwserial_release_port(port, true);
>  
> -	synchronize_rcu();
> -	kfree(peer);
> +	kfree_rcu(peer);

What is "more efficient" about this change?

And do you have the hardware for this device to test with?  It would be
good to finally get this out of staging, or to just remove it entirely
if no one uses it.

thanks,

greg k-h

Re: [PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[Dan Carpenter]

On Mon, Sep 19, 2022 at 05:10:56PM +0800, Shang XiaoJing wrote:
> Instead of invoking a synchronize_rcu() to free a pointer after a grace
> period, we can directly make use of a new API that does the same but in
> a more efficient way.
> 
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> ---
> Changelog:
> v3: the first version of the PATCH
> v1: v3 resent as v1
> v2: use kfree_rcu() instead of kvfree_rcu() for clarity
> v4: resend v2 as v4 to avoid versioning confusion

LOL...  I was still very confused about the versioning but I think the
patch is correct now.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[shangxiaojing]

On 2022/9/19 17:11, Greg KH wrote:
> On Mon, Sep 19, 2022 at 05:10:56PM +0800, Shang XiaoJing wrote:
>> Instead of invoking a synchronize_rcu() to free a pointer after a grace
>> period, we can directly make use of a new API that does the same but in
>> a more efficient way.
>>
>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
>> ---
>> Changelog:
>> v3: the first version of the PATCH
>> v1: v3 resent as v1
>> v2: use kfree_rcu() instead of kvfree_rcu() for clarity
>> v4: resend v2 as v4 to avoid versioning confusion
>> ---
>>   drivers/staging/fwserial/fwserial.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
>> index 81b06d88ed0d..8d2b4ed1f39e 100644
>> --- a/drivers/staging/fwserial/fwserial.c
>> +++ b/drivers/staging/fwserial/fwserial.c
>> @@ -2117,8 +2117,7 @@ static void fwserial_remove_peer(struct fwtty_peer *peer)
>>   	if (port)
>>   		fwserial_release_port(port, true);
>>   
>> -	synchronize_rcu();
>> -	kfree(peer);
>> +	kfree_rcu(peer);

The kfree_rcu(peer) should be kfree_rcu(peer, rcu), due to the rcu_head 
member named rcu in fwtty_peer.

This will be fixed in v5, sorry.

> What is "more efficient" about this change?

kfree_rcu() will call kvfree_call_rcu, due to the note in kernel/rcu/tree.c:

/Each kvfree_call_rcu() request is added to a batch. The batch will be 
drained every KFREE_DRAIN_JIFFIES number of jiffies. All the objects in 
the batch will be free'd in workqueue context. This allows us to: batch 
requests together to reduce the number of grace periods during heavy 
kfree_rcu()/kvfree_rcu() load. ///

which is the reason I use "more efficient".
>
> And do you have the hardware for this device to test with?  It would be
> good to finally get this out of staging, or to just remove it entirely
> if no one uses it.
Actually, not. I just found the code here may be optimized.
>
> thanks,
>
> greg k-h

Thanks,

Shang XiaoJing

Re: [PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[Dan Carpenter]

On Mon, Sep 19, 2022 at 05:43:33PM +0800, shangxiaojing wrote:
> 
> On 2022/9/19 17:11, Greg KH wrote:
> > On Mon, Sep 19, 2022 at 05:10:56PM +0800, Shang XiaoJing wrote:
> > > Instead of invoking a synchronize_rcu() to free a pointer after a grace
> > > period, we can directly make use of a new API that does the same but in
> > > a more efficient way.
> > > 
> > > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> > > ---
> > > Changelog:
> > > v3: the first version of the PATCH
> > > v1: v3 resent as v1
> > > v2: use kfree_rcu() instead of kvfree_rcu() for clarity
> > > v4: resend v2 as v4 to avoid versioning confusion
> > > ---
> > >   drivers/staging/fwserial/fwserial.c | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
> > > index 81b06d88ed0d..8d2b4ed1f39e 100644
> > > --- a/drivers/staging/fwserial/fwserial.c
> > > +++ b/drivers/staging/fwserial/fwserial.c
> > > @@ -2117,8 +2117,7 @@ static void fwserial_remove_peer(struct fwtty_peer *peer)
> > >   	if (port)
> > >   		fwserial_release_port(port, true);
> > > -	synchronize_rcu();
> > > -	kfree(peer);
> > > +	kfree_rcu(peer);
> 
> The kfree_rcu(peer) should be kfree_rcu(peer, rcu), due to the rcu_head
> member named rcu in fwtty_peer.

Oh, huh.  What happens if you don't pas the "rcu" parameter?  I see
there is a similar instance in ext4_apply_quota_options().

		kfree_rcu(qname);

regards,
dan carpenter

Re: [PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[shangxiaojing]

On 2022/9/19 18:27, Dan Carpenter wrote:
> On Mon, Sep 19, 2022 at 05:43:33PM +0800, shangxiaojing wrote:
>> On 2022/9/19 17:11, Greg KH wrote:
>>> On Mon, Sep 19, 2022 at 05:10:56PM +0800, Shang XiaoJing wrote:
>>>> Instead of invoking a synchronize_rcu() to free a pointer after a grace
>>>> period, we can directly make use of a new API that does the same but in
>>>> a more efficient way.
>>>>
>>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
>>>> ---
>>>> Changelog:
>>>> v3: the first version of the PATCH
>>>> v1: v3 resent as v1
>>>> v2: use kfree_rcu() instead of kvfree_rcu() for clarity
>>>> v4: resend v2 as v4 to avoid versioning confusion
>>>> ---
>>>>    drivers/staging/fwserial/fwserial.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
>>>> index 81b06d88ed0d..8d2b4ed1f39e 100644
>>>> --- a/drivers/staging/fwserial/fwserial.c
>>>> +++ b/drivers/staging/fwserial/fwserial.c
>>>> @@ -2117,8 +2117,7 @@ static void fwserial_remove_peer(struct fwtty_peer *peer)
>>>>    	if (port)
>>>>    		fwserial_release_port(port, true);
>>>> -	synchronize_rcu();
>>>> -	kfree(peer);
>>>> +	kfree_rcu(peer);
>> The kfree_rcu(peer) should be kfree_rcu(peer, rcu), due to the rcu_head
>> member named rcu in fwtty_peer.
> Oh, huh.  What happens if you don't pas the "rcu" parameter?  I see
> there is a similar instance in ext4_apply_quota_options().
>
> 		kfree_rcu(qname);

We assume for the condition that CONFIG_KASAN_GENERIC does not defined 
and system has no pressure (bacause kvfree_call_rcu maintains 3 path):

The input parameter "head" of kvfree_call_rcu will be NULL if we don't 
pass the rcu, which will run like:

                             might_sleep();
                             synchronize_rcu();
                             kvfree((void *) func);

instead of:

                             if (head) {
                                     call_rcu(head, func);
                                     return;
                             }

where the difference is synchronize_rcu vs call_rcu.

call_rcu() will not block, having a wider range of use compared with 
synchronize_rcu().

>
> regards,
> dan carpenter

Thanks,

Shang XiaoJing

Re: [PATCH -next v4] staging: fwserial: Switch to kfree_rcu() API

[shangxiaojing]

On 2022/9/19 18:27, Dan Carpenter wrote:
> On Mon, Sep 19, 2022 at 05:43:33PM +0800, shangxiaojing wrote:
>> On 2022/9/19 17:11, Greg KH wrote:
>>> On Mon, Sep 19, 2022 at 05:10:56PM +0800, Shang XiaoJing wrote:
>>>> Instead of invoking a synchronize_rcu() to free a pointer after a grace
>>>> period, we can directly make use of a new API that does the same but in
>>>> a more efficient way.
>>>>
>>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
>>>> ---
>>>> Changelog:
>>>> v3: the first version of the PATCH
>>>> v1: v3 resent as v1
>>>> v2: use kfree_rcu() instead of kvfree_rcu() for clarity
>>>> v4: resend v2 as v4 to avoid versioning confusion
>>>> ---
>>>>    drivers/staging/fwserial/fwserial.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
>>>> index 81b06d88ed0d..8d2b4ed1f39e 100644
>>>> --- a/drivers/staging/fwserial/fwserial.c
>>>> +++ b/drivers/staging/fwserial/fwserial.c
>>>> @@ -2117,8 +2117,7 @@ static void fwserial_remove_peer(struct fwtty_peer *peer)
>>>>    	if (port)
>>>>    		fwserial_release_port(port, true);
>>>> -	synchronize_rcu();
>>>> -	kfree(peer);
>>>> +	kfree_rcu(peer);
>> The kfree_rcu(peer) should be kfree_rcu(peer, rcu), due to the rcu_head
>> member named rcu in fwtty_peer.
> Oh, huh.  What happens if you don't pas the "rcu" parameter?  I see
> there is a similar instance in ext4_apply_quota_options().
>
> 		kfree_rcu(qname);

In order to keep the code logic unchanged, "kfree_rcu(peer);" is better 
than "kfree_rcu(peer, rcu);"

I'm sorry for this repeated revision.


Thanks,

Shang XiaoJing