[PATCH 2/3] nvme: multipath: only update ctrl->nr_active when using queue-depth iopolicy

[Ewan Milne]

I did 2/3 as a separate patch because Tomas wrote the first one
and I thought it was a little more clear to have the optimization
separated out, but I can combine them if we like.

-Ewan

On Tue, Nov 7, 2023 at 4:53 PM Keith Busch <kbusch at kernel.org> wrote:
>
> On Tue, Nov 07, 2023 at 04:23:30PM -0500, Ewan D. Milne wrote:
> > The atomic updates of ctrl->nr_active are unnecessary when using
> > numa or round-robin iopolicy, so avoid that cost on a per-request basis.
> > Clear nr_active when changing iopolicy and do not decrement below zero.
> > (This handles changing the iopolicy while requests are in flight.)
>
> Oh, here's restricting it to that policy. Any reason not to fold it in
> the first one?
>
> > -     atomic_inc(&ns->ctrl->nr_active);
> > +     if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD)
> > +             atomic_inc(&ns->ctrl->nr_active);
>
> This all looks racy with the ability to change policies in flight. I
> think we need to introduce a new flag and do something like this:
>
>         if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD) {
>                 atomic_inc(&ns->ctrl->nr_active);
>                 nvme_req(rq)->flags |= NVME_MPATH_COUNT_ACTIVE;
>         }
>
> ...
>
> > @@ -147,7 +148,8 @@ void nvme_mpath_end_request(struct request *rq)
> >       if (!(nvme_req(rq)->flags & NVME_MPATH_IO_STATS))
> >               return;
> >
> > -     atomic_dec(&ns->ctrl->nr_active);
> > +     if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD)
> > +             atomic_dec_if_positive(&ns->ctrl->nr_active);
>
> And then the above becomes:
>
>         if (nvme_req(rq)->flags & NVME_MPATH_COUNT_ACTIVE)
>                 atomic_dec_if_positive(&ns->ctrl->nr_active);
>
> ...
>
> > +void nvme_subsys_iopolicy_update(struct nvme_subsystem *subsys, int iopolicy)
> > +{
> > +     struct nvme_ctrl *ctrl;
> > +
> > +     WRITE_ONCE(subsys->iopolicy, iopolicy);
> > +
> > +     mutex_lock(&nvme_subsystems_lock);
> > +     list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> > +             atomic_set(&ctrl->nr_active, 0);
> > +     }
> > +     mutex_unlock(&nvme_subsystems_lock);
>
> And then you don't need to do the above since atomic_dec's are always
> pairs with atomic_inc's. What do you think?
>