Looking at this function in ll_rw_blk.c:
static void blk_queue_congestion_threshold(struct request_queue *q)
{
int nr;
nr = q->nr_requests - (q->nr_requests / 8) + 1;
if (nr > q->nr_requests)
nr = q->nr_requests;
q->nr_congestion_on = nr;
nr = q->nr_requests - (q->nr_requests / 8) - 1;
if (nr < 1)
nr = 1;
q->nr_congestion_off = nr;
}
Why are the "on" and "off" thresholds the same, i.e. shouldn't there be some
hysteresis? Con Kolivas posted a patch that changed the "off" threshold to
"nr_requests - nr_requests/8 - nr_requests/16" and it was said to be better,
but it never made it into mainline (it also changed get_request_wait() and that
was never merged either):
--- patches/linux-2.6.9-rc4-ck1/drivers/block/ll_rw_blk.c 2004-10-12 12:25:09.798003278 +0200
+++ linux-2.6.9-rc4-ck1/drivers/block/ll_rw_blk.c 2004-10-12 12:25:42.959479479 +0200
@@ -100,7 +100,7 @@
nr = q->nr_requests;
q->nr_congestion_on = nr;
- nr = q->nr_requests - (q->nr_requests / 8) - 1;
+ nr = q->nr_requests - (q->nr_requests / 8) - (q->nr_requests/16)- 1;
if (nr < 1)
nr = 1;
q->nr_congestion_off = nr;
@@ -1758,8 +1758,10 @@
{
DEFINE_WAIT(wait);
struct request *rq;
+ struct io_context *ioc;
generic_unplug_device(q);
+ ioc = get_io_context(GFP_NOIO);
do {
struct request_list *rl = &q->rq;
@@ -1769,7 +1771,6 @@
rq = get_request(q, rw, GFP_NOIO);
if (!rq) {
- struct io_context *ioc;
io_schedule();
@@ -1779,12 +1780,11 @@
* up to a big batch of them for a small period time.
* See ioc_batching, ioc_set_batching
*/
- ioc = get_io_context(GFP_NOIO);
ioc_set_batching(q, ioc);
- put_io_context(ioc);
}
finish_wait(&rl->wait[rw], &wait);
} while (!rq);
+ put_io_context(ioc);
return rq;
}
--Chuck Ebbert 03-Nov-04 19:58:53
Chuck Ebbert wrote:
> Looking at this function in ll_rw_blk.c:
>
>
> static void blk_queue_congestion_threshold(struct request_queue *q)
> {
> int nr;
>
> nr = q->nr_requests - (q->nr_requests / 8) + 1;
> if (nr > q->nr_requests)
> nr = q->nr_requests;
> q->nr_congestion_on = nr;
>
> nr = q->nr_requests - (q->nr_requests / 8) - 1;
> if (nr < 1)
> nr = 1;
> q->nr_congestion_off = nr;
> }
>
>
> Why are the "on" and "off" thresholds the same, i.e. shouldn't there be some
They aren't the same, there is some hysteresis.
> hysteresis? Con Kolivas posted a patch that changed the "off" threshold to
> "nr_requests - nr_requests/8 - nr_requests/16" and it was said to be better,
> but it never made it into mainline (it also changed get_request_wait() and that
> was never merged either):
>
Patch was from Arjan. IIRC everyone agreed it looked good, and from
all the feedback I have seen it has worked well. Jens just may not
have had time to get it merged, or forgotten about it.
It can probably at least go to -mm for now.
>
> --- patches/linux-2.6.9-rc4-ck1/drivers/block/ll_rw_blk.c 2004-10-12 12:25:09.798003278 +0200
> +++ linux-2.6.9-rc4-ck1/drivers/block/ll_rw_blk.c 2004-10-12 12:25:42.959479479 +0200
> @@ -100,7 +100,7 @@
> nr = q->nr_requests;
> q->nr_congestion_on = nr;
>
> - nr = q->nr_requests - (q->nr_requests / 8) - 1;
> + nr = q->nr_requests - (q->nr_requests / 8) - (q->nr_requests/16)- 1;
> if (nr < 1)
> nr = 1;
> q->nr_congestion_off = nr;
The stuff below this hunk is a different thing altogether, and should
not be merged.
> @@ -1758,8 +1758,10 @@
> {
> DEFINE_WAIT(wait);
> struct request *rq;
> + struct io_context *ioc;
>
> generic_unplug_device(q);
> + ioc = get_io_context(GFP_NOIO);
> do {
> struct request_list *rl = &q->rq;
>
> @@ -1769,7 +1771,6 @@
> rq = get_request(q, rw, GFP_NOIO);
>
> if (!rq) {
> - struct io_context *ioc;
>
> io_schedule();
>
> @@ -1779,12 +1780,11 @@
> * up to a big batch of them for a small period time.
> * See ioc_batching, ioc_set_batching
> */
> - ioc = get_io_context(GFP_NOIO);
> ioc_set_batching(q, ioc);
> - put_io_context(ioc);
> }
> finish_wait(&rl->wait[rw], &wait);
> } while (!rq);
> + put_io_context(ioc);
>
> return rq;
> }
>
>
On Thu, Nov 04 2004, Nick Piggin wrote:
> Chuck Ebbert wrote:
> > Looking at this function in ll_rw_blk.c:
> >
> >
> >static void blk_queue_congestion_threshold(struct request_queue *q)
> >{
> > int nr;
> >
> > nr = q->nr_requests - (q->nr_requests / 8) + 1;
> > if (nr > q->nr_requests)
> > nr = q->nr_requests;
> > q->nr_congestion_on = nr;
> >
> > nr = q->nr_requests - (q->nr_requests / 8) - 1;
> > if (nr < 1)
> > nr = 1;
> > q->nr_congestion_off = nr;
> >}
> >
> >
> > Why are the "on" and "off" thresholds the same, i.e. shouldn't there be
> > some
>
> They aren't the same, there is some hysteresis.
>
> >hysteresis? Con Kolivas posted a patch that changed the "off" threshold to
> >"nr_requests - nr_requests/8 - nr_requests/16" and it was said to be
> >better,
> >but it never made it into mainline (it also changed get_request_wait() and
> >that
> >was never merged either):
> >
>
> Patch was from Arjan. IIRC everyone agreed it looked good, and from
> all the feedback I have seen it has worked well. Jens just may not
> have had time to get it merged, or forgotten about it.
>
> It can probably at least go to -mm for now.
It should just go to Linus, imho. It just got lost, I'll send it out
today.
> >--- patches/linux-2.6.9-rc4-ck1/drivers/block/ll_rw_blk.c 2004-10-12
> >12:25:09.798003278 +0200
> >+++ linux-2.6.9-rc4-ck1/drivers/block/ll_rw_blk.c 2004-10-12
> >12:25:42.959479479 +0200
> >@@ -100,7 +100,7 @@
> > nr = q->nr_requests;
> > q->nr_congestion_on = nr;
> >
> >- nr = q->nr_requests - (q->nr_requests / 8) - 1;
> >+ nr = q->nr_requests - (q->nr_requests / 8) - (q->nr_requests/16)-
> >1;
> > if (nr < 1)
> > nr = 1;
> > q->nr_congestion_off = nr;
>
> The stuff below this hunk is a different thing altogether, and should
> not be merged.
>
> >@@ -1758,8 +1758,10 @@
> > {
> > DEFINE_WAIT(wait);
> > struct request *rq;
> >+ struct io_context *ioc;
> >
> > generic_unplug_device(q);
> >+ ioc = get_io_context(GFP_NOIO);
> > do {
> > struct request_list *rl = &q->rq;
> >
> >@@ -1769,7 +1771,6 @@
> > rq = get_request(q, rw, GFP_NOIO);
> >
> > if (!rq) {
> >- struct io_context *ioc;
> >
> > io_schedule();
> >
> >@@ -1779,12 +1780,11 @@
> > * up to a big batch of them for a small period
> > time.
> > * See ioc_batching, ioc_set_batching
> > */
> >- ioc = get_io_context(GFP_NOIO);
> > ioc_set_batching(q, ioc);
> >- put_io_context(ioc);
> > }
> > finish_wait(&rl->wait[rw], &wait);
> > } while (!rq);
> >+ put_io_context(ioc);
> >
> > return rq;
> > }
Yes this isn't valid, as discussed several times on linux-kernel.
--
Jens Axboe