Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 07:38:04PM -0700, Richard Henderson wrote: >On 10/13/19 6:01 PM, Wei Yang wrote: >>> No, please. >>> >>> (1) The compiler does not know that qemu_*host_page_size is a power of 2, >>> and >>> will generate a real division at runtime. The same is true for >>> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. >>> >> >> Confused >> >> The definition of ROUND_UP is: >> >> #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) > >Ah, my bad, I did confuse this with QEMU_ALIGN_UP. > >Hmm. > > lea -1(n, size), t > neg size > and size, t > >vs > > mov mask, t > not t > add n, t > and mask, t > >which is what I proposed here > >>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > >I'm ok with your version. > >Reviewed-by: Richard Henderson > Thanks for your clarification. Have a nice day > >r~ > -- Wei Yang Help you, Help me
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On 10/13/19 6:01 PM, Wei Yang wrote: >> No, please. >> >> (1) The compiler does not know that qemu_*host_page_size is a power of 2, and >> will generate a real division at runtime. The same is true for >> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. >> > > Confused > > The definition of ROUND_UP is: > > #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) Ah, my bad, I did confuse this with QEMU_ALIGN_UP. Hmm. lea -1(n, size), t neg size and size, t vs mov mask, t not t add n, t and mask, t which is what I proposed here >> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html I'm ok with your version. Reviewed-by: Richard Henderson r~
Re: Occasional VM soft lockup when a remote cdrom is attached
Really appreciate your advice. Some comments below:
On 2019/10/12 3:06, John Snow wrote:
On 10/11/19 9:22 AM, Guoheyi wrote:
Hi folks,
We observed Linux on VM occasionally (at very low rate) got soft lockup
when a remote cdrom is attached. The guest hangs up at below call trace:
That's certainly a new one to me :)
[Tue Oct8 23:02:53 2019]ata_scsi_queuecmd+0xe0/0x2a0 [libata]
[Tue Oct8 23:02:53 2019]scsi_dispatch_cmd+0xec/0x288
[Tue Oct8 23:02:53 2019]scsi_queue_rq+0x5f4/0x6b8
[Tue Oct8 23:02:53 2019]blk_mq_dispatch_rq_list+0xb0/0x690
[Tue Oct8 23:02:53 2019]blk_mq_do_dispatch_sched+0x8c/0x130
[Tue Oct8 23:02:53 2019]blk_mq_sched_dispatch_requests+0x128/0x1f0
[Tue Oct8 23:02:53 2019]__blk_mq_run_hw_queue+0x9c/0x128
[Tue Oct8 23:02:53 2019]__blk_mq_delay_run_hw_queue+0x198/0x1d8
[Tue Oct8 23:02:53 2019]blk_mq_run_hw_queue+0x68/0x180
[Tue Oct8 23:02:53 2019]blk_mq_sched_insert_request+0xbc/0x210
[Tue Oct8 23:02:53 2019]blk_execute_rq_nowait+0x118/0x168
[Tue Oct8 23:02:53 2019]blk_execute_rq+0x74/0xd8
[Tue Oct8 23:02:53 2019]__scsi_execute+0xd8/0x1e0
[Tue Oct8 23:02:53 2019]sr_check_events+0xd4/0x2c8 [sr_mod]
[Tue Oct8 23:02:53 2019]cdrom_check_events+0x34/0x50 [cdrom]
[Tue Oct8 23:02:53 2019]sr_block_check_events+0xdc/0x108 [sr_mod]
[Tue Oct8 23:02:53 2019]disk_check_events+0x60/0x198
[Tue Oct8 23:02:53 2019]disk_events_workfn+0x24/0x30
[Tue Oct8 23:02:53 2019]process_one_work+0x1b4/0x3f8
[Tue Oct8 23:02:53 2019]worker_thread+0x54/0x470
[Tue Oct8 23:02:53 2019]kthread+0x134/0x138
[Tue Oct8 23:02:53 2019]ret_from_fork+0x10/0x18
We are running the whole stack on ARM64 platforms, using rcdrom on host
to connect a remote cdrom, which is appeared as "/dev/sr0" on the host.
Our Linux kernel version is 4.19.36 and qemu version is 2.8.1, which is
fairly old but I checked the mainline and found the work flow does not
change much. And KVM is enabled.
We provide the remote cdrom to guest as a block device, attached under
ICH SATA bus.
The work flow should be like this (please correct me if I was wrong):
1. There is a kworker thread in guest kernel which will check cdrom
status periodically.
2. The call of "ata_scsi_queuecmd" in guest will write AHCI port
register "PORT_CMD_ISSUE", so this VCPU thread is trapped out to qemu.
3. qemu will grab the BQL and then dispatch the access to
ahci_port_write().
4. For this is a "get event status notification" command, qemu finally
goes to cmd_get_event_status_notification() and then cdrom_is_inserted().
via
cmd_get_event_status_notification (SCSI 0x4A)
event_status_media
blk_is_inserted
5. In cdrom_is_inserted(), an ioctl to cdrom fd is issued.
Using the bdrv_host_cdrom BlockDriver, for the .bdrv_is_inserted callback.
However, in the last step, we found the ioctl() may have large latency,
for it is a virtual device of remote cdrom, when the remote server is
busy and of poor performance. We have observed more than 8 seconds
latency in half an hour test, and the latency might reach more than 20
seconds when guest soft lockup occurred.
I'm not sure what can be done here. the host_cdrom driver has a few
methods to query state (cdrom_is_inserted, cdrom_eject,
cdrom_lock_medium) and in general code is going to rely on
bdrv_is_inserted returning a truthful answer.
(I'm not sure we have callbacks established to tell when the backing
media we are ourselves relying on has gone away. Maybe it could be
added, but it's not there now. We could maybe cache the answer if we had
something reliable.)
You could always try using the host_device driver instead of the
host_cdrom one, which will just treat it as a "normal" block device
instead of a CDROM one, and doesn't use any cdrom specific ioctls. It
might avoid the costly call.
By following this work around, the cdrom device was represented to guest
as a normal disk ("/dev/sdb"). We are not sure if this will cause more
functional differences.
My question is, is there any way to get around of this issue? Does it
make sense for qemu to setup an IO thread to issue this ioctl() and let
the VCPU thread return to guest as soon as possible? Or it is kernel's
responsibility to break up the long time ioctl() and return to user space?
Yeah, I think you could probably try to make this change -- the code is
unfortunately very callback-hell-ish with poor abstraction boundaries,
but obviously the data transfer commands already defer to bottom halves.
(Ideally, I think every ATAPI command would just immediately defer to a
bottom half, but I don't know what effect that would have on callers of
ide_atapi_cmd. I'd have to audit it, and it's quite squiggly.)
"Patches welcome" as they say, but it's quite messy down there.
It sounds like a long term work, especially for we are not familiar with
SATA/AHCI devices...
Thanks,
HG
--js
Any comments or advice will be appreciated :)
HG
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote: >On 10/12/19 10:11 PM, Wei Yang wrote: >> Use ROUND_UP() to define, which is a little bit easy to read. >> >> Signed-off-by: Wei Yang >> --- >> include/exec/cpu-all.h | 7 +++ >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h >> index ad9ab85eb3..255bb186ac 100644 >> --- a/include/exec/cpu-all.h >> +++ b/include/exec/cpu-all.h >> @@ -220,7 +220,7 @@ extern int target_page_bits; >> >> #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) >> #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) >> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & >> TARGET_PAGE_MASK) >> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) >> >> /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even >> * when intptr_t is 32-bit and we are aligning a long long. >> @@ -228,9 +228,8 @@ extern int target_page_bits; >> extern uintptr_t qemu_host_page_size; >> extern intptr_t qemu_host_page_mask; >> >> -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & >> qemu_host_page_mask) >> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) >> & \ >> -qemu_real_host_page_mask) >> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) >> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), >> qemu_real_host_page_size) > > >No, please. > >(1) The compiler does not know that qemu_*host_page_size is a power of 2, and >will generate a real division at runtime. The same is true for >TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. > Confused The definition of ROUND_UP is: #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) Why it will do division? This will be expanded to the same form as the original code, if my understanding is correct. Would you mind telling me more? >(2) The first hunk conflicts with an in-flight patch of mine: > >https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > > >r~ -- Wei Yang Help you, Help me
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote: > On 10/12/19 10:11 PM, Wei Yang wrote: > > Use ROUND_UP() to define, which is a little bit easy to read. > > > > Signed-off-by: Wei Yang > > --- > > include/exec/cpu-all.h | 7 +++ > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > > index ad9ab85eb3..255bb186ac 100644 > > --- a/include/exec/cpu-all.h > > +++ b/include/exec/cpu-all.h > > @@ -220,7 +220,7 @@ extern int target_page_bits; > > > > #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) > > #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) > > -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & > > TARGET_PAGE_MASK) > > +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) > > > > /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even > > * when intptr_t is 32-bit and we are aligning a long long. > > @@ -228,9 +228,8 @@ extern int target_page_bits; > > extern uintptr_t qemu_host_page_size; > > extern intptr_t qemu_host_page_mask; > > > > -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & > > qemu_host_page_mask) > > -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - > > 1) & \ > > -qemu_real_host_page_mask) > > +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) > > +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), > > qemu_real_host_page_size) > > > No, please. > > (1) The compiler does not know that qemu_*host_page_size is a power of 2, and > will generate a real division at runtime. The same is true for > TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. Ouch, good point, I didn't think of that when I gave an R-b. > (2) The first hunk conflicts with an in-flight patch of mine: > > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > > > r~ > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PATCH] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK
The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
performed if it can be offloaded or otherwise performed efficiently.
However a misaligned write request requires a RMW so we should return
an error and let the caller decide how to proceed.
This hits an assertion since commit c8bb23cbdb if the required
alignment is larger than the cluster size:
qemu-img create -f qcow2 -o cluster_size=2k img.qcow2 4G
qemu-io -c "open -o driver=qcow2,file.align=4k blkdebug::img.qcow2" \
-c 'write 0 512'
qemu-io: block/io.c:1127: bdrv_driver_pwritev: Assertion `!(flags &
BDRV_REQ_NO_FALLBACK)' failed.
Aborted
Signed-off-by: Alberto Garcia
---
block/io.c | 6 +
tests/qemu-iotests/268 | 55 ++
tests/qemu-iotests/268.out | 7 +
tests/qemu-iotests/group | 1 +
4 files changed, 69 insertions(+)
create mode 100755 tests/qemu-iotests/268
create mode 100644 tests/qemu-iotests/268.out
diff --git a/block/io.c b/block/io.c
index 4f9ee97c2b..c5d4d029da 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2071,6 +2071,12 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
return ret;
}
+/* If the request is misaligned then we can't make it efficient */
+if (((offset & (align - 1)) || (bytes & (align - 1))) &&
+(flags & BDRV_REQ_NO_FALLBACK)) {
+return -ENOTSUP;
+}
+
bdrv_inc_in_flight(bs);
/*
* Align write if necessary by performing a read-modify-write cycle.
diff --git a/tests/qemu-iotests/268 b/tests/qemu-iotests/268
new file mode 100755
index 00..895f6e593f
--- /dev/null
+++ b/tests/qemu-iotests/268
@@ -0,0 +1,55 @@
+#!/usr/bin/env bash
+#
+# Test write request with required alignment larger than the cluster size
+#
+# Copyright (C) 2019 Igalia, S.L.
+# Author: Alberto Garcia
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On 10/12/19 10:11 PM, Wei Yang wrote: > There are three page size in qemu: > > real host page size > host page size > target page size > > All of them have dedicate variable to represent. For the last two, we > use the same form in the whole qemu project, while for the first one we > use two forms: qemu_real_host_page_size and getpagesize(). > > qemu_real_host_page_size is defined to be a replacement of > getpagesize(), so let it serve the role. > > [Note] Not fully tested for some arch or device. > > Signed-off-by: Wei Yang > --- Reviewed-by: Richard Henderson r~
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On 10/12/19 10:11 PM, Wei Yang wrote: > Use ROUND_UP() to define, which is a little bit easy to read. > > Signed-off-by: Wei Yang > --- > include/exec/cpu-all.h | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index ad9ab85eb3..255bb186ac 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -220,7 +220,7 @@ extern int target_page_bits; > > #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) > #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) > -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & > TARGET_PAGE_MASK) > +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) > > /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even > * when intptr_t is 32-bit and we are aligning a long long. > @@ -228,9 +228,8 @@ extern int target_page_bits; > extern uintptr_t qemu_host_page_size; > extern intptr_t qemu_host_page_mask; > > -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & > qemu_host_page_mask) > -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) > & \ > -qemu_real_host_page_mask) > +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) > +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size) No, please. (1) The compiler does not know that qemu_*host_page_size is a power of 2, and will generate a real division at runtime. The same is true for TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. (2) The first hunk conflicts with an in-flight patch of mine: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html r~
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 10:11:44AM +0800, Wei Yang wrote: > Use ROUND_UP() to define, which is a little bit easy to read. > > Signed-off-by: Wei Yang Reviewed-by: David Gibson > --- > include/exec/cpu-all.h | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index ad9ab85eb3..255bb186ac 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -220,7 +220,7 @@ extern int target_page_bits; > > #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) > #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) > -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & > TARGET_PAGE_MASK) > +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) > > /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even > * when intptr_t is 32-bit and we are aligning a long long. > @@ -228,9 +228,8 @@ extern int target_page_bits; > extern uintptr_t qemu_host_page_size; > extern intptr_t qemu_host_page_mask; > > -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & > qemu_host_page_mask) > -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) > & \ > -qemu_real_host_page_mask) > +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) > +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size) > > /* same as PROT_xxx */ > #define PAGE_READ 0x0001 -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Sun, Oct 13, 2019 at 10:11:45AM +0800, Wei Yang wrote: > There are three page size in qemu: > > real host page size > host page size > target page size > > All of them have dedicate variable to represent. For the last two, we > use the same form in the whole qemu project, while for the first one we > use two forms: qemu_real_host_page_size and getpagesize(). > > qemu_real_host_page_size is defined to be a replacement of > getpagesize(), so let it serve the role. > > [Note] Not fully tested for some arch or device. > > Signed-off-by: Wei Yang Reviewed-by: David Gibson Although the chances of someone messing this up again are almost 100%. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
