[PATCH v7 4/9] nvme-multipath: add support for latency I/O policy

[John Garry]

>>> +}
>>> +
>>> +/*
>>> + * Formula to calculate the EWMA (Exponentially Weighted Moving Average):
>>> + * ewma = (old_ewma * (EWMA_SHIFT - 1) + (EWMA_SHIFT)) / EWMA_SHIFT
>>> + * For instance, with EWMA_SHIFT = 3, this assigns 7/8 (~87.5 %) weight to
>>> + * the existing/old ewma and 1/8 (~12.5%) weight to the new sample.
>>> + */
>>> +static inline u64 calc_ewma_update(u64 old, u64 new)
>>> +{
>>> +    return (old * ((1 << NVME_DEFAULT_LATENCY_EWMA_SHIFT) - 1)
>>> +            + new) >> NVME_DEFAULT_LATENCY_EWMA_SHIFT;

side note: I have to admit that I did not check all the mathematics of 
these ewma calculations ...

>>> +}
>>> +
>>> +static void nvme_mpath_add_sample(struct request *rq, struct nvme_ns *ns)

Could the context analysis annotation be added here eventually to 
declare that the srcu read lock is held?

>>> +{
>>> +    int cpu;
>>> +    unsigned int op_type;
>>> +    struct nvme_path_lat *path_lat;
>>> +    struct nvme_path_lat_stat *stat;
>>> +    u64 now, latency, slat_ns, avg_lat_ns;
>>> +    struct nvme_ns_head *head = ns->head;
>>> +
>>> +    if (list_is_singular(&head->list))
>>> +        return;
>>> +
>>> +    now = ktime_get_ns();
>>> +    latency = now >= rq->io_start_time_ns ? now - rq->io_start_time_ns : 0;
>>> +    if (!latency)
>>> +        return;
>>> +
>>> +    /*
>>> +     * As completion code path is serialized(i.e. no same completion queue
>>> +     * update code could run simultaneously on multiple cpu) we can safely
>>> +     * access per cpu nvme path stat here from another cpu (in case the
>>> +     * completion cpu is different from submission cpu).
>>> +     * The only field which could be accessed simultaneously here is the
>>> +     * path ->weight which may be accessed by this function as well as I/O
>>> +     * submission path during path selection logic and we protect ->weight
>>> +     * using READ_ONCE/WRITE_ONCE. Yes this may not be 100% accurate but
>>> +     * we also don't need to be so accurate here as the path credit would
>>> +     * be anyways refilled, based on path weight, once path consumes all
>>> +     * its credits. And we limit path weight/credit max up to 64. Please
>>> +     * also refer nvme_latency_path().
>>> +     */

...

>>>    void nvme_mpath_end_request(struct request *rq)
>>>    {
>>>        struct nvme_ns *ns = rq->q->queuedata;
>>> @@ -206,6 +407,15 @@ void nvme_mpath_end_request(struct request *rq)
>>>        if (nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE)
>>>            atomic_dec_if_positive(&ns->ctrl->nr_active);
>>> +    if (test_bit(NVME_NS_PATH_STAT, &ns->flags)) {
>>> +        int srcu_idx;
>>> +
>>> +        srcu_idx = srcu_read_lock(&ns->head->srcu);
>>> +        if (test_bit(NVME_NS_PATH_STAT, &ns->flags))
>> 
>> Some may ask why check NVME_NS_PATH_STAT twice.
> 
> The first check is a fast-path optimization to avoid taking the SRCU read
> lock when latency sampling is disabled. The second check is needed because
> NVME_NS_PATH_STAT could be cleared after the first test but before acquiring
> the SRCU lock, so we revalidate it after entering the protected section.

It is probably worth a brief comment on that. A similar trick is done in 
__blk_mq_tag_busy() and every so often someone asks about it. Or maybe 
it is another function. I don't remember.

>> 
>>> +    blk_stat_enable_accounting(ns->queue);
>>> +    return true;
>>> +}
>>> +
>>> +static bool nvme_disable_ns_latency_sampling(struct nvme_ns *ns)
>>> +{
>>> +    int cpu;
>>> +    struct nvme_ns_head *head = ns->head;
>>> +    bool changed = false;
>>> +
>>> +    if (!test_and_clear_bit(NVME_NS_PATH_STAT, &ns->flags))
>>> +        return false;
>>> +
>>> +    for_each_possible_cpu(cpu) {
>>> +        if (ns == READ_ONCE(*per_cpu_ptr(head->latency_path, cpu))) {
>>> +            WRITE_ONCE(*per_cpu_ptr(head->latency_path, cpu), NULL);
>>> +            changed = true;
>>> +        }
>>> +    }
>>> +
>>> +    blk_stat_disable_accounting(ns->queue);
>>> +    blk_queue_flag_clear(QUEUE_FLAG_SAME_FORCE, ns->queue);
>> 
>> eh, what if QUEUE_FLAG_SAME_FORCE was already enabled before nvme_enable_ns_latency_sampling()?
>> 
> Good catch! It looks like we need a nested reference count for
> QUEUE_FLAG_SAME_FORCE, similar to QUEUE_FLAG_STATS and
> QUEUE_FLAG_QUIESCED.

Furthermore, I think that userspace can change this via sysfs, no? I 
think that the file is rq_affinity. If so, could that break things (if 
userspace did change this flag)?

>>>    }
>>> @@ -268,6 +554,45 @@ void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
>>>        srcu_read_unlock(&ctrl->srcu, srcu_idx);
>>>    }
>>> +int nvme_alloc_ns_stat(struct nvme_ns *ns)
>> 
>> Surely "mpath" should be in the name, no? It seems that every other public API in multpath.c has "mpath" in the name.
> 
> Not all APIs have "mpath" in its name, such as nvme_failover_req(),
> nvme_kick_requeue_lists() etc, but most other have. So I would
> rename it to nvme_mpath_alloc_ns_stat().

nvme_failover_req() would obviously be a multipath function from the 
name. Anyway, "mpath" in the name just seem better.

>> 
>>> +{
>>> +    int i, cpu;
>>> +    struct nvme_path_lat_work *work;
>>> +    gfp_t gfp = GFP_KERNEL | __GFP_ZERO;
>>> +
>>> +    if (!ns->head->disk)
>>> +        return 0;
>>> +
>>> +    ns->path_lat = __alloc_percpu_gfp(NVME_NUM_STAT_GROUPS *
>>> +                sizeof(struct nvme_path_lat),
>>> +                __alignof__(struct nvme_path_lat), gfp);
>>> +    if (!ns->path_lat)
>>> +        return -ENOMEM;
>>> +
>>> +    for_each_possible_cpu(cpu) {
>>> +        for (i = 0; i < NVME_NUM_STAT_GROUPS; i++) {
>>> +            work = &per_cpu_ptr(ns->path_lat, cpu)[i].work;
>>> +            work->ns = ns;
>>> +            work->op_type = i;
>>> +            INIT_WORK(&work->weight_work, nvme_mpath_weight_work);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void nvme_mpath_set_ctrl_paths(struct nvme_ctrl *ctrl)
>> 
>> what do you mean by "set" here?
> 
> It is intended as the counterpart of nvme_mpath_clear_ctrl_paths().
> The former clears/disables the I/O policy state for the controller
> namespaces, while this helper sets/enables it.

To me, clear paths meaning is obvious, in that any per-NUMA node paths 
are cleared for all the paths associated with the controller.

nvme_mpath_set_ctrl_paths() does not really do the opposite - it instead 
just enables the IO latency sampling per path.

Anyway, I don't feel too strongly about this, but it just seems that the 
naming could be improved.

>> 
>>> +{
>>> +    struct nvme_ns *ns;
>>> +    int srcu_idx;
>>> +
>>> +    srcu_idx = srcu_read_lock(&ctrl->srcu);
>>> +    list_for_each_entry_srcu(ns, &ctrl->namespaces, list,
>>> +                srcu_read_lock_held(&ctrl->srcu))
>>> +        nvme_enable_ns_latency_sampling(ns);
>>> +    srcu_read_unlock(&ctrl->srcu, srcu_idx);
>>> +}
>>> +
>>>    void nvme_mpath_revalidate_paths(struct nvme_ns_head *head)
>>>    {
>>>        sector_t capacity = get_capacity(head->disk);
>>> @@ -280,6 +605,8 @@ void nvme_mpath_revalidate_paths(struct nvme_ns_head *head)
>>>                     srcu_read_lock_held(&head->srcu)) {
>>>            if (capacity != get_capacity(ns->disk))
>>>                clear_bit(NVME_NS_READY, &ns->flags);
>>> +
>>> +        nvme_reset_ns_latency_stat(ns);
>>>        }
>>>        srcu_read_unlock(&head->srcu, srcu_idx);
>>> @@ -404,6 +731,92 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
>>>        return found;
>>>    }
>>> +static inline bool nvme_state_is_live(enum nvme_ana_state state)
...

>>>        }
>>>        mutex_unlock(&head->lock);
>>> +    mutex_lock(&nvme_subsystems_lock);
>> 
>> I am curious - why use the nvme_subsystems_lock?
>> 
> nvme_subsys_iopolicy_update() and nvme_mpath_set_live() can run concurrently.
> nvme_subsystems_lock serializes these paths so that latency sampling is
> enabled consistently with the subsystem I/O policy.

ok, maybe then please consider a comment. It can be useful.


>>> @@ -527,6 +530,30 @@ enum nvme_stat_group {
>>>        NVME_NUM_STAT_GROUPS
>>>    };
>>> +struct nvme_path_lat_stat {
>>> +    u64 nr_samples;        /* total num of samples processed */
>> 
>> why u64 and not unsigned long long?
>> 
> I used u64 intentionally because this is a monotonically increasing
> sample counter, and I wanted a fixed-width 64-bit type. I didn't see
> any particular advantage in using unsigned long long here. If there's
> a reason to prefer it in this context, I'm happy to change it.

hmmm... I thought that in general we only should use a fixed width type 
when it is required, e.g. reading from a 32b register, then use u32.