Received-SPF: pass (google.com: domain of linux-kernel+bounces-129062-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <1711680261-5789-1-git-send-email-zhiguo.niu@unisoc.com>
 <890bd06d-2a94-4138-9854-4a7ed74e0e51@acm.org> <CAHJ8P3K9OL6MHNrSrqmf0esbr2h1HJ3mVRmxDNVpf95ZMHQcqg@mail.gmail.com>
 <92e45c93-e2ff-4d34-b70f-7772f0596e68@acm.org> <CAHJ8P3KgU-tFDAgCNc5GcPbUBtDDyFmcfza2HsoD9TJ3h1DS=Q@mail.gmail.com>
 <c56a1d54-6d7d-4105-8109-d6a81bc1adbc@acm.org>
In-Reply-To: <c56a1d54-6d7d-4105-8109-d6a81bc1adbc@acm.org>
From: Zhiguo Niu <niuzhiguo84@gmail.com>
Date: Wed, 3 Apr 2024 13:41:24 +0800
Message-ID: <CAHJ8P3JYXA4VGfgXLMesuuUOUivc1EKwxGce4P5MWDHn1K675Q@mail.gmail.com>
Subject: Re: [PATCH] block/mq-deadline: Fix WARN when set async_depth by sysfs
To: Bart Van Assche <bvanassche@acm.org>
Cc: Zhiguo Niu <zhiguo.niu@unisoc.com>, axboe@kernel.dk, linux-kernel@vger.kernel.org, 
	linux-block@vger.kernel.org, ke.wang@unisoc.com, hongyu.jin@unisoc.com, 
	Damien Le Moal <dlemoal@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 3, 2024 at 7:20=E2=80=AFAM Bart Van Assche <bvanassche@acm.org>=
 wrote:
>
> On 4/1/24 10:44 PM, Zhiguo Niu wrote:
> > On Tue, Apr 2, 2024 at 5:23=E2=80=AFAM Bart Van Assche <bvanassche@acm.=
org> wrote:
> >> Oops, I misread your patch. After having taken another look, my
> >> conclusions are as follows:
> >> * sbitmap_queue_min_shallow_depth() is called. This causes
> >>     sbq->wake_batch to be modified but I don't think that it is a prop=
er
> >>     fix for dd_limit_depth().
> > yes, it will affect sbq->wake_batch,  But judging from the following co=
de:
> > [ ... ]
>
> If we want to allow small values of dd->async_depth, min_shallow_depth
> must be 1. The BFQ I/O scheduler also follows this approach.
>
> >> * dd_limit_depth() still assigns a number in the range 1..nr_requests =
to
> >>     data->shallow_depth while a number in the range 1..(1<<bt->sb.shif=
t)
> >>     should be assigned.
> > yes, In order to avoid the performance regression problem that Harshit
> > Mogalapalli reported, this patch will not directly modify
> > dd->async_depth,
> > but user can modify dd->async_depth from sysfs according to user's
> > request. which will modify data->shallow_depth after user modify it by
> > sysfs.
>
> It seems like there is no other option than keeping the current default
> depth limit for async requests ...
>
> > My personal opinion is to keep the current dd->aync_depth unchanged to
> > avoid causing performance regression,
> > but it should  allow users to set it by sysfs, and the WARN mentioned
> > best to be solved.
> > and just only change this part?
> >   -       sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd->async=
_depth);
> >   +       sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, 1);
> > thanks!
>
> The above change will suppress the kernel warning. I think the other
> changes from patch 2/2 are also necessary. Otherwise the unit of
> "async_depth" will depend on sbitmap word shift parameter. I don't think
> that users should have to worry about which shift value has been chosen
> by the sbitmap implementation.
Hi Bart Van Assche,
So will you help do an  official patch version about patch 1 and patch 2?

Besides, I am not sure  that  the following part will cause
performance regression or not?
+static int dd_to_word_depth(struct blk_mq_hw_ctx *hctx, unsigned int
qdepth)
+{
+       struct sbitmap_queue *bt =3D &hctx->sched_tags->bitmap_tags;
+       const unsigned int nrr =3D hctx->queue->nr_requests;
+
+       return max(((qdepth << bt->sb.shift) + nrr - 1) / nrr,
+                  bt->min_shallow_depth);
+}
+
which is somewhat similar to the previous commit d47f9717e5cf
("block/mq-deadline: use correct way to throttling write
requests"). just an example
hw conditions:  8 cpus,emmc flash, one hw queue, and queue_depth=3D64,
sched_tag/nr_request=3D128, init dd->async_depth=3D96
 bt->sb.shift=3D5,
so max(((qdepth << bt->sb.shift) + nrr - 1) / nrr,bt->min_shallow_depth);
will get 24 and commit d47f9717e5cf ("block/mq-deadline: use correct
way to throttling write
requests") also get 24, but your revert commit 256aab46e316 ("Revert
"block/mq-deadline: use
correct way to throttling write requests"") will get 96,
or am I missing something else?
thanks!
>
> Thanks,
>
> Bart.
>