[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling |
Date: |
Wed, 22 Jul 2015 18:24:48 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Wed, 22 Jul 2015, Jan Beulich wrote:
> >>> On 22.07.15 at 16:50, <address@hidden> wrote:
> > On Wed, 22 Jul 2015, Jan Beulich wrote:
> >> >> --- a/xen-hvm.c
> >> >> +++ b/xen-hvm.c
> >> >> @@ -981,19 +981,30 @@ static void handle_ioreq(XenIOState *sta
> >> >>
> >> >> static int handle_buffered_iopage(XenIOState *state)
> >> >> {
> >> >> + buffered_iopage_t *buf_page = state->buffered_io_page;
> >> >> buf_ioreq_t *buf_req = NULL;
> >> >> ioreq_t req;
> >> >> int qw;
> >> >>
> >> >> - if (!state->buffered_io_page) {
> >> >> + if (!buf_page) {
> >> >> return 0;
> >> >> }
> >> >>
> >> >> memset(&req, 0x00, sizeof(req));
> >> >>
> >> >> - while (state->buffered_io_page->read_pointer !=
> >> >> state->buffered_io_page->write_pointer) {
> >> >> - buf_req = &state->buffered_io_page->buf_ioreq[
> >> >> - state->buffered_io_page->read_pointer %
> >> >> IOREQ_BUFFER_SLOT_NUM];
> >> >> + for (;;) {
> >> >> + uint32_t rdptr = buf_page->read_pointer, wrptr;
> >> >> +
> >> >> + xen_rmb();
> >> >
> >> > We don't need this barrier.
> >>
> >> How would we not? We need to make sure we read in this order
> >> read_pointer, write_pointer, and read_pointer again (in the
> >> comparison). Only that way we can be certain to hold a matching
> >> pair in hands at the end.
> >
> > See below
> >
> >
> >> >> + wrptr = buf_page->write_pointer;
> >> >> + xen_rmb();
> >> >> + if (rdptr != buf_page->read_pointer) {
> >> >
> >> > I think you have to use atomic_read to be sure that the second read to
> >> > buf_page->read_pointer is up to date and not optimized away.
> >>
> >> No, suppressing such an optimization is an intended (side) effect
> >> of the barriers used.
> >
> > I understand what you are saying but I am not sure if your assumption
> > is correct. Can the compiler optimize the second read anyway?
>
> No, it can't, due to the barrier.
OK
> >> > But if I think that it would be best to simply use atomic_read to read
> >> > both pointers at once using uint64_t as type, so you are sure to get a
> >> > consistent view and there is no need for this check.
> >>
> >> But I'm specifically trying to avoid e.g. a locked cmpxchg8b here on
> >> ix86.
> >
> > OK, but we don't need cmpxchg8b, just:
> >
> > #define atomic_read(ptr) (*(__typeof__(*ptr) volatile*) (ptr))
>
> This only gives the impression of being atomic when the type is wider
> than a machine word. There's no ix86 (i.e. 32-bit) instruction other
> than LOCK CMPXCHG8B (and possibly MMX/SSE/AVX ones) allowing
> to atomically read a 64-bit quantity.
Damn!
> > something like:
> >
> > for (;;) {
> > uint64_t ptrs;
> > uint32_t rdptr, wrptr;
> >
> > ptrs = atomic_read((uint64_t*)&state->buffered_io_page->read_pointer);
> > rdptr = (uint32_t)ptrs;
> > wrptr = *(((uint32_t*)&ptrs) + 1);
> >
> > if (rdptr == wrptr) {
> > continue;
> > }
> >
> > [work]
> >
> > atomic_add(&buf_page->read_pointer, qw + 1);
> > }
> >
> > it would work, wouldn't it?
>
> Looks like so, but the amount of casts alone makes me wish for
> no-one to consider this (but I agree that the casts could be
> taken care of). Still I think (as btw done elsewhere) the lock
> free access is preferable.
Actually I think it is conceptually easier to understand, but the
current implementation of atomic_read not working with uint64_t on
x86_32 is a real bummer. In that case I am OK with the lock free loop
too. Thanks for the explanation.
I'll queue this change up for the next QEMU release cycle.
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Stefano Stabellini, 2015/07/21
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Jan Beulich, 2015/07/22
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Stefano Stabellini, 2015/07/22
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Jan Beulich, 2015/07/22
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling,
Stefano Stabellini <=
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Stefano Stabellini, 2015/07/22
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Jan Beulich, 2015/07/23
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Stefano Stabellini, 2015/07/23
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Stefano Stabellini, 2015/07/23
- Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling, Jan Beulich, 2015/07/23