From: "Jan Beulich" <JBeulich@suse.com>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] xen/HVM: atomically access pointers in bufioreq handling
Date: Wed, 22 Jul 2015 08:03:40 -0600 [thread overview]
Message-ID: <55AFBEDC020000780009417A@prv-mh.provo.novell.com> (raw)
In-Reply-To: <alpine.DEB.2.02.1507201723370.17378@kaball.uk.xensource.com>
>>> On 21.07.15 at 15:54, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 18 Jun 2015, Jan Beulich wrote:
>> The number of slots per page being 511 (i.e. not a power of two) means
>> that the (32-bit) read and write indexes going beyond 2^32 will likely
>> disturb operation. The hypervisor side gets I/O req server creation
>> extended so we can indicate that we're using suitable atomic accesses
>> where needed (not all accesses to the two pointers really need to be
>> atomic), allowing it to atomically canonicalize both pointers when both
>> have gone through at least one cycle.
>
> The description is a bit terse: which accesses don't really need to be
> atomic?
Perhaps I should drop this part - I more or less copied the hypervisor
side's commit message, and the above really applies to e.g.
if ( (pg->ptrs.write_pointer - pg->ptrs.read_pointer) >=
(IOREQ_BUFFER_SLOT_NUM - qw) )
in hypervisor code.
>> --- 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.
>> + 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.
> 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.
>> handle_ioreq(state, &req);
>>
>> - xen_mb();
>> - state->buffered_io_page->read_pointer += qw ? 2 : 1;
>> + atomic_add(&buf_page->read_pointer, qw + 1);
>
> I couldn't get specific info on the type of barrier implemented by
> __sync_fetch_and_add, so I cannot tell for sure whether removing
> xen_mb() is appropriate. Do you have a link? I suspect that given the
> strong guarantees of the x86 architecture we'll be fine. I would be less
> confident if this code was used on other archs.
gcc.pdf, in the section covering them, says "In most cases, these
built-in functions are considered a full barrier. [...] Further,
instructions are issued as necessary to prevent the processor from
speculating loads across the operation and from queuing stores
after the operation." Details on individual builtins subsequently
tell the exceptions from this general rule, but the one used here is
not among the exceptions.
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -370,7 +370,8 @@ static inline void xen_unmap_pcidev(XenX
>> static inline int xen_create_ioreq_server(XenXC xc, domid_t dom,
>> ioservid_t *ioservid)
>> {
>> - int rc = xc_hvm_create_ioreq_server(xc, dom, 1, ioservid);
>> + int rc = xc_hvm_create_ioreq_server(xc, dom, HVM_IOREQSRV_BUFIOREQ_ATOMIC,
>> + ioservid);
>
> I am concerned that passing 2 instead of 1 could break older
> hypervisors. However handle_bufioreq was never defined as a true
> boolean, so maybe it is OK?
Indeed I'm building on it only having done == 0 or != 0 checks.
> The alternative would be to create a xen_xc_hvm_create_ioreq_server
> versioned wrapper in include/hw/xen/xen_common.h.
Which is what I aimed at avoiding.
Jan
next prev parent reply other threads:[~2015-07-22 14:03 UTC|newest]
Thread overview: 21+ 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-21 13:54 ` [Qemu-devel] " Stefano Stabellini
2015-07-22 14:03 ` Jan Beulich [this message]
2015-07-22 14:50 ` Stefano Stabellini
2015-07-22 14:50 ` [Qemu-devel] " 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
2015-07-22 17:24 ` [Qemu-devel] " 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 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-23 7:02 ` Jan Beulich
2015-07-22 14:03 ` 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=55AFBEDC020000780009417A@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=qemu-devel@nongnu.org \
--cc=stefano.stabellini@eu.citrix.com \
--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.