Re: [PATCH][5/?] count writeback pages in nr_scanned

[Jens Axboe]

On Thu, Jan 06 2005, Nick Piggin wrote:
> Andrew Morton wrote:
> >Nick Piggin <nickpiggin@xxxxxxxxxxxx> wrote:
> >
> >>> If the queue is not congested, blk_congestion_wait() will still sleep.  
> >>See
> >>> freed_request().
> >>> 
> >>
> >>Hmm... doesn't look like it to me:
> >>
> >>         if (rl->count[rw] < queue_congestion_off_threshold(q))
> >>                 clear_queue_congested(q, rw);
> >>
> >>And clear_queue_congested does an unconditional wakeup (if there
> >>is someone sleeping on the congestion queue).
> >
> >
> >That's my point.  blk_congestion_wait() will always sleep, regardless of
> >the queue's congestion state.
> >
> 
> Oh yes, but it will return as soon as a single request is finished.
> Which is probably a couple of milliseconds, rather than the 100 we
> had hoped for. So the allocators will wake up again and go around
> the loop and still make no progress.
> 
> However, if you had a plain io_schedule_timeout there, at least you
> would sleep for the full extend of the specified timeout.
> 
> BTW. Jens, now that I look at freed_request, is the memory barrier
> required? If so, what is it protecting?
> 

> 
> 
> This memory barrier is not needed because the waitqueue will only get
> waiters on it in the following situations:
> 
> rq->count has exceeded the threshold - however all manipulations of ->count
> are performed under the runqueue lock, and so we will correctly pick up any
> waiter.
> 
> Memory allocation for the request fails. In this case, there is no additional
> help provided by the memory barrier. We are guaranteed to eventually wake
> up waiters because the request allocation mempool guarantees that if the mem
> allocation for a request fails, there must be some requests in flight. They
> will wake up waiters when they are retired.

Not sure I agree completely. Yes it will work, but only because it tests
<= q->nr_requests and I don't think that 'eventually' is good enough :-)

The actual waitqueue manipulation doesn't happen under the queue lock,
so the memory barrier is needed to pickup the change on SMP. So I'd like
to keep the barrier.

I'd prefer to add smp_mb() to waitqueue_active() actually!

-- 
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/