Re: [PATCH] system/qdev-monitor: move drain_call_rcu call under if (!dev) in qmp_device_add()

[Marc Hartmayer]

On Fri, Apr 26, 2024 at 11:57 AM +0300, Dmitrii Gavrilov 
<ds-gavr@yandex-team.ru> wrote:
> 26.04.2024, 11:17, "Marc Hartmayer" <mhartmay@linux.ibm.com>:
>
>  On Fri, Nov 03, 2023 at 01:56 PM +0300, Dmitrii Gavrilov 
> <ds-gavr@yandex-team.ru> wrote:
>
>   Original goal of addition of drain_call_rcu to qmp_device_add was to cover
>   the failure case of qdev_device_add. It seems call of drain_call_rcu was
>   misplaced in 7bed89958bfbf40df what led to waiting for pending RCU callbacks
>   under happy path too. What led to overall performance degradation of
>   qmp_device_add.
>
>   In this patch call of drain_call_rcu moved under handling of failure of
>   qdev_device_add.
>
>   Signed-off-by: Dmitrii Gavrilov <ds-gavr@yandex-team.ru>
>
>  I don't know the exact reason, but this commit caused udev events to
>  show up much slower than before (~3s vs. ~23s) when a virtio-scsi device
>  is hotplugged (I’ve tested this only on s390x). Importantly, this only
>  happens when asynchronous SCSI scanning is disabled in the *guest*
>  kernel (scsi_mod.scan=sync or CONFIG_SCSI_SCAN_ASYNC=n).
>
>  The `udevadm monitor` output captured while hotplugging the device
>  (using QEMU 012b170173bc ("system/qdev-monitor: move drain_call_rcu call
>  under if (!dev) in qmp_device_add()")):
>

[…snip…]

>  Any ideas?
>
>  Thanks in advance.
>
>  Kind regards,
>   Marc
>
> Hello!
>  
> Thank you for mentioning this.
>  
> At first glance it seems that using scsi in synchonous mode caues the global
> QEMU mutex lock until the scanning phase is complete. Prior to 012b170173bc
> ("system/qdev-monitor: move drain_call_rcu call under if (!dev) in
> qmp_device_add()") on each device adition the lock would be forcibly removed
> allowing callbacks (including UDEV ones) to be processed after a new device
> is added.
>  
> I`ll try to investigate this furter. But currently it appears to me like
> performance or observability dilemma.

I tried the test on my local laptop (x86_64) and there seems to be no
issue (I used the kernel cmdline option scsi_mod.scan=sync for the
guest) - guest and host kernel == 6.8.7. But please double check.

>  
> Is behaviour you mentioning consistant?

Yep, at least for more than 50 iterations (I stopped the test then).

>  
> Best regards,
> Dmitrii
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294