On 08/27/2017 08:53 PM, Pranith Kumar wrote:
> Using heaptrack, I found that quite a few of our temporary allocations
> are coming from allocating work items. Instead of doing this
> continously, we can cache the allocated items and reuse them instead
> of freeing them.
>
> This reduces the number of allocations by 25% (200000 -> 150000 for
> ARM64 boot+shutdown test).
>
> Signed-off-by: Pranith Kumar <bobby.pr...@gmail.com>
> ---
> cpus-common.c | 85
> ++++++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 70 insertions(+), 15 deletions(-)
>
> diff --git a/cpus-common.c b/cpus-common.c
> index 59f751ecf9..a1c4c7d1a3 100644
> --- a/cpus-common.c
> +++ b/cpus-common.c
> @@ -24,6 +24,7 @@
> #include "sysemu/cpus.h"
>
> static QemuMutex qemu_cpu_list_lock;
> +static QemuMutex qemu_wi_pool_lock;
> static QemuCond exclusive_cond;
> static QemuCond exclusive_resume;
> static QemuCond qemu_work_cond;
> @@ -33,6 +34,58 @@ static QemuCond qemu_work_cond;
> */
> static int pending_cpus;
>
> +typedef struct qemu_work_item {
> + struct qemu_work_item *next;
> + run_on_cpu_func func;
> + run_on_cpu_data data;
> + bool free, exclusive, done;
> +} qemu_work_item;
> +
> +typedef struct qemu_wi_pool {
> + qemu_work_item *first, *last;
> +} qemu_wi_pool;
> +
> +qemu_wi_pool *wi_free_pool;
> +
> +static void qemu_init_workitem_pool(void)
> +{
> + wi_free_pool = g_malloc0(sizeof(qemu_wi_pool));
> + wi_free_pool->first = NULL;
> + wi_free_pool->last = NULL;
> +}
> +
> +static void qemu_wi_pool_insert(qemu_work_item *item)
> +{
> + qemu_mutex_lock(&qemu_wi_pool_lock);
> + if (wi_free_pool->last == NULL) {
> + wi_free_pool->first = item;
> + wi_free_pool->last = item;
> + } else {
> + wi_free_pool->last->next = item;
> + wi_free_pool->last = item;
> + }
> + qemu_mutex_unlock(&qemu_wi_pool_lock);
> +}
> +
> +static qemu_work_item* qemu_wi_pool_remove(void)
> +{
> + qemu_mutex_lock(&qemu_wi_pool_lock);
> + qemu_work_item *ret = wi_free_pool->first;
> +
> + if (ret == NULL)
> + goto out;
> +
> + wi_free_pool->first = ret->next;
> + if (wi_free_pool->last == ret) {
> + wi_free_pool->last = NULL;
> + }
Why does this list need to record a "last" element?
It would seem a simple lifo would be sufficient.
(You would also be able to manage the list via cmpxchg without a separate lock,
but perhaps the difference between the two isn't measurable.)
r~
