memory barrier in ll_rw_blk.c (was Re: [PATCH][5/?] count writeback pages in nr_scanned)

[Nick Piggin <nickpiggin@yahoo.com.au>]

Jens Axboe wrote:
> On Thu, Jan 06 2005, Nick Piggin wrote:

>>
>>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.
> 

No that's right... but between the prepare_to_wait and the io_schedule,
get_request takes the lock and checks nr_requests. I think we are safe?

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

That may be a good idea (I haven't really taken much notice of how other
code uses it).

I'm not worried about any possible performance advantages of removing it,
rather just having a memory barrier without comments can be perplexing.