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

On 8/11/26 3:33 PM, John Garry wrote:
> 
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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?
> 
Yes it will be added when I resend series based off nvme-7.3 as
support of clang context annotation is added in nvme-7.3.

>>>> +{
>>>> +    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.
> 
Yeah okay will add comment in the code.

>>>
>>>> +    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)?
> 
So that's where I suggested using a nested ref count. I'd
add an helper similar to blk_stat_{enable|dsiable}_accounting() and
that new helper would be then used in both sysfs path as well
latency policy enable/disable path.

>>>>    }
>>>> @@ -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.
> 
sure, will add one.
> 
>>>> @@ -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.
> 
The sizeof unsigned long long counter would be 8 bytes (or 64 bit) on both
32-bit and 64-but system, isn't it? So, it seems, using u64 makes the
intended width clearer than unsigned long long.

Thanks,
--Nilay