Re: [Query] increased latency observed in cpu hotplug path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2016-09-21 23:06 GMT+09:00 Khan, Imran <kimran@xxxxxxxxxxxxxx>:

>>> commit 084ee793ec1ff4e2171b481642bfbdaa2e10664c
>>> Author: Imran Khan <kimran@xxxxxxxxxxxxxx>
>>> Date:   Thu Sep 15 16:44:02 2016 +0530
>>>
>>>     blk-mq: use static mapping
>>>
>>>     blk-mq layer performs a remapping between s/w and h/w contexts and also
>>>     between h/w contexts and CPUs, whenever a CPU hotplug event happens.
>>>     This remapping has to wait for queue freezing which may take tens of
>>>     miliseconds, resulting in a high latency in CPU hotplug path.
>>>     This patch makes the above mentioned mappings static so that we can
>>>     avoid remapping when CPU hotplug event happens and this results in
>>>     improved CPU hotplug latency.
>>>
>>>     Change-Id: Idf38cb6c4e78c91fda3c86608c6d0441f01ab435
>>>     Signed-off-by: Imran Khan <kimran@xxxxxxxxxxxxxx>
>>>
>>> diff --git a/block/blk-mq-cpumap.c b/block/blk-mq-cpumap.c
>>> index 8764c24..85fdb88 100644
>>> --- a/block/blk-mq-cpumap.c
>>> +++ b/block/blk-mq-cpumap.c
>>> @@ -52,18 +52,13 @@ int blk_mq_update_queue_map(unsigned int *map, unsigned int nr_queues,
>>>
>>>      queue = 0;
>>>      for_each_possible_cpu(i) {
>>> -            if (!cpumask_test_cpu(i, online_mask)) {
>>> -                    map[i] = 0;
>>> -                    continue;
>>> -            }
>>> -
>>>              /*
>>>               * Easy case - we have equal or more hardware queues. Or
>>>               * there are no thread siblings to take into account. Do
>>>               * 1:1 if enough, or sequential mapping if less.
>>>               */
>>> -            if (nr_queues >= nr_cpus || nr_cpus == nr_uniq_cpus) {
>>> -                    map[i] = cpu_to_queue_index(nr_cpus, nr_queues, queue);
>>> +            if (nr_queues >= nr_cpu_ids) {
>>> +                    map[i] = cpu_to_queue_index(nr_cpu_ids, nr_queues, queue);
>>>                      queue++;
>>>                      continue;
>>>              }
>>> @@ -75,7 +70,7 @@ int blk_mq_update_queue_map(unsigned int *map, unsigned int nr_queues,
>>>               */
>>>              first_sibling = get_first_sibling(i);
>>>              if (first_sibling == i) {
>>> -                    map[i] = cpu_to_queue_index(nr_uniq_cpus, nr_queues,
>>> +                    map[i] = cpu_to_queue_index(nr_cpu_ids, nr_queues,
>>>                                                      queue);
>>>                      queue++;
>>>              } else
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 6d6f8fe..0753fdf 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -1783,10 +1783,6 @@ static void blk_mq_init_cpu_queues(struct request_queue *q,
>>>              INIT_LIST_HEAD(&__ctx->rq_list);
>>>              __ctx->queue = q;
>>>
>>> -            /* If the cpu isn't online, the cpu is mapped to first hctx */
>>> -            if (!cpu_online(i))
>>> -                    continue;
>>> -
>>>              hctx = q->mq_ops->map_queue(q, i);
>>>
>>>              /*
>>> @@ -1820,12 +1816,9 @@ static void blk_mq_map_swqueue(struct request_queue *q,
>>>       * Map software to hardware queues
>>>       */
>>>      queue_for_each_ctx(q, ctx, i) {
>>> -            /* If the cpu isn't online, the cpu is mapped to first hctx */
>>> -            if (!cpumask_test_cpu(i, online_mask))
>>> -                    continue;
>>> -
>>>              hctx = q->mq_ops->map_queue(q, i);
>>> -            cpumask_set_cpu(i, hctx->cpumask);
>>> +            if (cpumask_test_cpu(i, online_mask))
>>> +                    cpumask_set_cpu(i, hctx->cpumask);
>>>              ctx->index_hw = hctx->nr_ctx;
>>>              hctx->ctxs[hctx->nr_ctx++] = ctx;
>>>      }
>>> @@ -1863,17 +1856,22 @@ static void blk_mq_map_swqueue(struct request_queue *q,
>>>
>>>              /*
>>>               * Initialize batch roundrobin counts
>>> +             * Set next_cpu for only those hctxs that have an online CPU
>>> +             * in their cpumask field. For hctxs that belong to few online
>>> +             * and few offline CPUs, this will always provide one CPU from
>>> +             * online ones. For hctxs belonging to all offline CPUs, their
>>> +             * cpumask will be updated in reinit_notify.
>>>               */
>>> -            hctx->next_cpu = cpumask_first(hctx->cpumask);
>>> -            hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
>>> +            if(cpumask_first(hctx->cpumask) < nr_cpu_ids) {
>>> +                    hctx->next_cpu = cpumask_first(hctx->cpumask);
>>> +                    hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
>>> +            }
>>>      }
>>>
>>>      queue_for_each_ctx(q, ctx, i) {
>>> -            if (!cpumask_test_cpu(i, online_mask))
>>> -                    continue;
>>> -
>>>              hctx = q->mq_ops->map_queue(q, i);
>>> -            cpumask_set_cpu(i, hctx->tags->cpumask);
>>> +            if (cpumask_test_cpu(i, online_mask))
>>> +                    cpumask_set_cpu(i, hctx->tags->cpumask);
>>>      }
>>>  }
>>>
>>> @@ -2101,31 +2099,12 @@ void blk_mq_free_queue(struct request_queue *q)
>>>      blk_mq_free_hw_queues(q, set);
>>>  }
>>>
>>> -/* Basically redo blk_mq_init_queue with queue frozen */
>>> -static void blk_mq_queue_reinit(struct request_queue *q,
>>> -                            const struct cpumask *online_mask)
>>> -{
>>> -    WARN_ON_ONCE(!atomic_read(&q->mq_freeze_depth));
>>> -
>>> -    blk_mq_sysfs_unregister(q);
>>> -
>>> -    blk_mq_update_queue_map(q->mq_map, q->nr_hw_queues, online_mask);

Now blk_mq_update_queue_map() is only used in block/blk-mq-cpumap.c,
so you can make blk_mq_update_queue_map() static function.

>>> -
>>> -    /*
>>> -     * redo blk_mq_init_cpu_queues and blk_mq_init_hw_queues. FIXME: maybe
>>> -     * we should change hctx numa_node according to new topology (this
>>> -     * involves free and re-allocate memory, worthy doing?)
>>> -     */
>>> -
>>> -    blk_mq_map_swqueue(q, online_mask);
>>> -
>>> -    blk_mq_sysfs_register(q);
>>> -}
>>> -
>>>  static int blk_mq_queue_reinit_notify(struct notifier_block *nb,
>>>                                    unsigned long action, void *hcpu)
>>>  {
>>>      struct request_queue *q;
>>> +    struct blk_mq_hw_ctx *hctx;
>>> +    int i;
>>>      int cpu = (unsigned long)hcpu;
>>>      /*
>>>       * New online cpumask which is going to be set in this hotplug event.
>>> @@ -2155,43 +2134,29 @@ static int blk_mq_queue_reinit_notify(struct notifier_block *nb,
>>>      case CPU_DEAD:
>>>      case CPU_UP_CANCELED:
>>>              cpumask_copy(&online_new, cpu_online_mask);
>>> +            list_for_each_entry(q, &all_q_list, all_q_node) {
>>> +                    queue_for_each_hw_ctx(q, hctx, i) {
>>> +                            cpumask_clear_cpu(cpu, hctx->cpumask);
>>> +                            cpumask_clear_cpu(cpu, hctx->tags->cpumask);
>>> +                    }
>>> +            }

Do we need to hold all_q_mutex while iterating through all_q_list?

>>>              break;
>>>      case CPU_UP_PREPARE:
>>>              cpumask_copy(&online_new, cpu_online_mask);
>>>              cpumask_set_cpu(cpu, &online_new);
>>> +            /* Update hctx->cpumask for newly onlined CPUs */
>>> +            list_for_each_entry(q, &all_q_list, all_q_node) {
>>> +                    queue_for_each_hw_ctx(q, hctx, i) {
>>> +                            cpumask_set_cpu(cpu, hctx->cpumask);
>>> +                            hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
>>> +                            cpumask_set_cpu(cpu, hctx->tags->cpumask);
>>> +                    }
>>> +            }
>>>              break;
>>>      default:
>>>              return NOTIFY_OK;
>>>      }
>>>
>>> -    mutex_lock(&all_q_mutex);
>>> -
>>> -    /*
>>> -     * We need to freeze and reinit all existing queues.  Freezing
>>> -     * involves synchronous wait for an RCU grace period and doing it
>>> -     * one by one may take a long time.  Start freezing all queues in
>>> -     * one swoop and then wait for the completions so that freezing can
>>> -     * take place in parallel.
>>> -     */
>>> -    list_for_each_entry(q, &all_q_list, all_q_node)
>>> -            blk_mq_freeze_queue_start(q);
>>> -    list_for_each_entry(q, &all_q_list, all_q_node) {
>>> -            blk_mq_freeze_queue_wait(q);
>>> -
>>> -            /*
>>> -             * timeout handler can't touch hw queue during the
>>> -             * reinitialization
>>> -             */
>>> -            del_timer_sync(&q->timeout);
>>> -    }
>>> -
>>> -    list_for_each_entry(q, &all_q_list, all_q_node)
>>> -            blk_mq_queue_reinit(q, &online_new);
>>> -
>>> -    list_for_each_entry(q, &all_q_list, all_q_node)
>>> -            blk_mq_unfreeze_queue(q);
>>> -
>>> -    mutex_unlock(&all_q_mutex);

This change makes 'online_new' cpumask useless in blk_mq_queue_reinit_notify(),
so you can remove it completely.

>>>      return NOTIFY_OK;
>>>  }
>>>
>>>
>>>
>>>
>> With the patch I have collected some numbers, running some android monkey tests along with
>> an app that does random hootplugging of CPUs. The numbers obtained are as below:
>>
>> CPU UP time(in micro seconds):
>>          default blk-mq: 58629 54207 61792 58458 59286
>> blk-mq with above patch: 2755  2588  3412  3239  3010
>>
>> CPU DOWN time(in micros seconds):
>>          default blk-mq: 95990  103379 103686 93214  128274
>> blk-mq with above patch: 67178  49876  70027  61261  59656
>>
> Could someone please review the patch and provide some feedback?
> Even if the feedback is about the approach we are trying here or some
> suggestions to move ahead in that direction.
>>>>> Did you tried Tejun's percpu_ref patchset that I said in the last email?
>>>>> (http://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg984823.html)
>>>>>
>>>>>
>>>> I have tried the suggested patch but did not see any improvements in cpu
>>>> hotplug latencies
>>>>
>>>
>>>
>>
>>
>
>
> --
> Imran Khan
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux