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

[Nick Piggin]

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