From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
qemu-devel@nongnu.org,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH] xen/HVM: atomically access pointers in bufioreq handling
Date: Wed, 22 Jul 2015 18:24:48 +0100 [thread overview]
Message-ID: <alpine.DEB.2.02.1507221649060.17378__47122.6970119917$1437586040$gmane$org@kaball.uk.xensource.com> (raw)
In-Reply-To: <55AFD416020000780009426D@prv-mh.provo.novell.com>
On Wed, 22 Jul 2015, Jan Beulich wrote:
> >>> On 22.07.15 at 16:50, <stefano.stabellini@eu.citrix.com> 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.
next prev parent reply other threads:[~2015-07-22 17:26 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-18 13:18 [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling Jan Beulich
2015-07-21 13:54 ` Stefano Stabellini
2015-07-22 14:03 ` Jan Beulich
2015-07-22 14:03 ` [Qemu-devel] " Jan Beulich
2015-07-22 14:50 ` Stefano Stabellini
2015-07-22 15:34 ` Jan Beulich
2015-07-22 15:34 ` [Qemu-devel] " Jan Beulich
2015-07-22 17:24 ` Stefano Stabellini [this message]
2015-07-22 17:24 ` Stefano Stabellini
2015-07-22 17:26 ` Stefano Stabellini
2015-07-22 17:26 ` [Qemu-devel] " Stefano Stabellini
2015-07-23 7:02 ` Jan Beulich
2015-07-23 7:02 ` [Qemu-devel] " Jan Beulich
2015-07-23 10:04 ` Stefano Stabellini
2015-07-23 10:09 ` Stefano Stabellini
2015-07-23 10:09 ` [Qemu-devel] " Stefano Stabellini
2015-07-23 11:20 ` Jan Beulich
2015-07-23 11:20 ` Jan Beulich
2015-07-23 10:04 ` Stefano Stabellini
2015-07-22 14:50 ` Stefano Stabellini
2015-07-21 13:54 ` Stefano Stabellini
-- strict thread matches above, loose matches on Subject: below --
2015-06-18 13:18 Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='alpine.DEB.2.02.1507221649060.17378__47122.6970119917$1437586040$gmane$org@kaball.uk.xensource.com' \
--to=stefano.stabellini@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=qemu-devel@nongnu.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.