[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq
From: |
Jan Beulich |
Subject: |
Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling |
Date: |
Wed, 22 Jul 2015 09:34:14 -0600 |
>>> 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.
>> > 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.
> 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.
Jan
- 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 <=
- 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, 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