2019-09-27 06:05:41

by Yunfeng Ye

Subject: [PATCH v2] async: Let kfree() out of the critical area of the lock

The async_lock is big global lock, and kfree() is not always cheap, it
will increase lock contention. it's better let kfree() outside the lock
to keep the critical area as short as possible.

Signed-off-by: Yunfeng Ye <[email protected]>
Reviewed-by: Alexander Duyck <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
---
v1 -> v2:
- update the description
- add "Reviewed-by"

kernel/async.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/async.c b/kernel/async.c
index 4f9c1d6..1de270d 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -135,12 +135,12 @@ static void async_run_entry_fn(struct work_struct *work)
list_del_init(&entry->domain_list);
list_del_init(&entry->global_list);

- /* 3) free the entry */
- kfree(entry);
atomic_dec(&entry_count);
-
spin_unlock_irqrestore(&async_lock, flags);

+ /* 3) free the entry */
+ kfree(entry);
+
/* 4) wake up any waiters */
wake_up(&async_done);
}
--
2.7.4

2019-10-11 08:13:11

by Yunfeng Ye

Subject: [PATCH v2] async: Let kfree() out of the critical area of the lock

The async_lock is big global lock, and kfree() is not always cheap, it
will increase lock contention. it's better let kfree() outside the lock
to keep the critical area as short as possible.

Signed-off-by: Yunfeng Ye <[email protected]>
Reviewed-by: Alexander Duyck <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
---
v1 -> v2:
- update the description
- add "Reviewed-by"

kernel/async.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/async.c b/kernel/async.c
index 4f9c1d6..1de270d 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -135,12 +135,12 @@ static void async_run_entry_fn(struct work_struct *work)
list_del_init(&entry->domain_list);
list_del_init(&entry->global_list);

- /* 3) free the entry */
- kfree(entry);
atomic_dec(&entry_count);
-
spin_unlock_irqrestore(&async_lock, flags);

+ /* 3) free the entry */
+ kfree(entry);
+
/* 4) wake up any waiters */
wake_up(&async_done);
}
--
2.7.4

2019-11-15 18:34:14

by Thomas Gleixner

Subject: Re: [PATCH v2] async: Let kfree() out of the critical area of the lock

On Fri, 11 Oct 2019, Yunfeng Ye wrote:

> The async_lock is big global lock, and kfree() is not always cheap, it
> will increase lock contention. it's better let kfree() outside the lock
> to keep the critical area as short as possible.
>
> Signed-off-by: Yunfeng Ye <[email protected]>
> Reviewed-by: Alexander Duyck <[email protected]>
> Reviewed-by: Bart Van Assche <[email protected]>
> ---
> v1 -> v2:
> - update the description
> - add "Reviewed-by"
>
> kernel/async.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/async.c b/kernel/async.c
> index 4f9c1d6..1de270d 100644
> --- a/kernel/async.c
> +++ b/kernel/async.c
> @@ -135,12 +135,12 @@ static void async_run_entry_fn(struct work_struct *work)
> list_del_init(&entry->domain_list);
> list_del_init(&entry->global_list);
>
> - /* 3) free the entry */
> - kfree(entry);
> atomic_dec(&entry_count);
> -
> spin_unlock_irqrestore(&async_lock, flags);
>
> + /* 3) free the entry */
> + kfree(entry);
> +
> /* 4) wake up any waiters */
> wake_up(&async_done);

The wakeup should happen before the kfree() for the very same reasons.

Thanks,

tglx

2019-11-19 03:07:22

by Yunfeng Ye

Subject: Re: [PATCH v2] async: Let kfree() out of the critical area of the lock

On 2019/11/16 2:32, Thomas Gleixner wrote:
>
>
> On Fri, 11 Oct 2019, Yunfeng Ye wrote:
>
>> The async_lock is big global lock, and kfree() is not always cheap, it
>> will increase lock contention. it's better let kfree() outside the lock
>> to keep the critical area as short as possible.
>>
>> Signed-off-by: Yunfeng Ye <[email protected]>
>> Reviewed-by: Alexander Duyck <[email protected]>
>> Reviewed-by: Bart Van Assche <[email protected]>
>> ---
>> v1 -> v2:
>> - update the description
>> - add "Reviewed-by"
>>
>> kernel/async.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/async.c b/kernel/async.c
>> index 4f9c1d6..1de270d 100644
>> --- a/kernel/async.c
>> +++ b/kernel/async.c
>> @@ -135,12 +135,12 @@ static void async_run_entry_fn(struct work_struct *work)
>> list_del_init(&entry->domain_list);
>> list_del_init(&entry->global_list);
>>
>> - /* 3) free the entry */
>> - kfree(entry);
>> atomic_dec(&entry_count);
>> -
>> spin_unlock_irqrestore(&async_lock, flags);
>>
>> + /* 3) free the entry */
>> + kfree(entry);
>> +
>> /* 4) wake up any waiters */
>> wake_up(&async_done);
>
> The wakeup should happen before the kfree() for the very same reasons.
>
ok, I will modify as your suggest, thanks.

> Thanks,
>
> tglx
>
> .
>