[PATCH v2 0/2] virtio-balloon: tweak config_changed
Since virtio-ccw doesn't work with accessing to the config space inside an interrupt context, this patch series avoids that issue by moving the config register accesses to the related workqueue contexts. v1->v2 ChangeLog: - add config_read_bitmap to indicate to the workqueue callbacks about the necessity of reading the related config fields. Wei Wang (2): virtio-balloon: tweak config_changed implementation virtio-balloon: improve update_balloon_size_func drivers/virtio/virtio_balloon.c | 86 +++-- 1 file changed, 57 insertions(+), 29 deletions(-) -- 2.7.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
On 01/04/2019 12:42 AM, Michael S. Tsirkin wrote: On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote: virtio-ccw has deadlock issues with reading config registers inside the interrupt context, so we tweak the virtballoon_changed implementation by moving the config read operations into the related workqueue contexts. Signed-off-by: Wei Wang --- drivers/virtio/virtio_balloon.c | 54 - 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 728ecd1..9a82a11 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev) { struct virtio_balloon *vb = vdev->priv; unsigned long flags; - s64 diff = towards_target(vb); - - if (diff) { - spin_lock_irqsave(>stop_update_lock, flags); - if (!vb->stop_update) - queue_work(system_freezable_wq, - >update_balloon_size_work); - spin_unlock_irqrestore(>stop_update_lock, flags); - } - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { - virtio_cread(vdev, struct virtio_balloon_config, -free_page_report_cmd_id, >cmd_id_received); There's one problem with this approach: previously updating the cmd_id_received here would immediately stop the report in send_free_pages. With this approach we are waiting for the wq to schedule, which might be blocked waiting for report to complete. So host can no longer quickly stop the report in progress. A simple work-around would be to set some kind of flag whenever there is a change interrupt, then have send_free_pages test it and re-read cmd_id_received. Needs to be an atomic I guess ... Yes, sounds better..will update the patch. Best, Wei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v15 23/26] sched: early boot clock
Pavel Tatashin wrote on Thu, Jan 03, 2019: > Could you please send the config file and qemu arguments that were > used to reproduce this problem. Running qemu by hand, nothing fancy e.g. this works: # qemu-system-x86_64 -m 1G -smp 4 -drive file=/root/kvm-wrapper/disks/f2.img,if=virtio -serial mon:stdio --enable-kvm -cpu Haswell -device virtio-rng-pci -nographic (used a specific cpu just in case but normally runnning with cpu host on a skylake machine; can probably go older) qemu is fedora 29 blend as is: $ qemu-system-x86_64 --version QEMU emulator version 3.0.0 (qemu-3.0.0-3.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers compressed .config attached to the mail, this can likely be trimmed down some as well but that takes more time for me.. I didn't rebuild the kernel so not 100% sure (comes from /proc/config.gz) but it should work on a 4.20-rc2 kernel as written in the first few lines; 857baa87b64 I referred to in another mail was merged in 4.19-rc1 so anything past that is probably OK to reproduce... Re-checked today with these exact options (fresh VM start; then suspend laptop for a bit, then reboot VM): [0.00] Hypervisor detected: KVM [0.00] kvm-clock: Using msrs 4b564d01 and 4b564d00 [ 2477.907447] kvm-clock: cpu 0, msr 153a4001, primary cpu clock [ 2477.907448] clocksource: kvm-clock: mask: 0x max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns [ 2477.907450] tsc: Detected 2592.000 MHz processor As offered previously, happy to help in any way. Thanks, -- Dominique config.xz Description: Binary data ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PULL 0/1] virtio-ccw: one more patch for 4.21
On Thu, Jan 03, 2019 at 11:50:22AM -0500, Michael S. Tsirkin wrote: > On Thu, Jan 03, 2019 at 09:55:07AM +0100, Cornelia Huck wrote: > > On Wed, 19 Dec 2018 11:14:13 +0100 > > Cornelia Huck wrote: > > > > > The following changes since commit > > > 37d1246af2d530710cf5822d2845774f01c03b22: > > > > > > virtio_net: bulk free tx skbs (2018-12-06 14:28:39 -0500) > > > > > > are available in the Git repository at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git > > > tags/virtio-ccw-20181219 > > > > > > for you to fetch changes up to 6fb95ef30707f1b2fcaea8d6dc873055e0460b1a: > > > > > > virtio-ccw: diag 500 may return a negative cookie (2018-12-19 11:01:57 > > > +0100) > > > > > > > > > One small documentation fix for 4.21. > > > > > > > > > > > > Cornelia Huck (1): > > > virtio-ccw: diag 500 may return a negative cookie > > > > > > Documentation/virtual/kvm/s390-diag.txt | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > ping? > > Sorry holidays :) > > I'll pick this up for the next pull. On a related note I'm not yet set up to handle pull requests, I'd prefer just patches for now. > -- > MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PULL 0/1] virtio-ccw: one more patch for 4.21
On Thu, Jan 03, 2019 at 09:55:07AM +0100, Cornelia Huck wrote: > On Wed, 19 Dec 2018 11:14:13 +0100 > Cornelia Huck wrote: > > > The following changes since commit 37d1246af2d530710cf5822d2845774f01c03b22: > > > > virtio_net: bulk free tx skbs (2018-12-06 14:28:39 -0500) > > > > are available in the Git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git > > tags/virtio-ccw-20181219 > > > > for you to fetch changes up to 6fb95ef30707f1b2fcaea8d6dc873055e0460b1a: > > > > virtio-ccw: diag 500 may return a negative cookie (2018-12-19 11:01:57 > > +0100) > > > > > > One small documentation fix for 4.21. > > > > > > > > Cornelia Huck (1): > > virtio-ccw: diag 500 may return a negative cookie > > > > Documentation/virtual/kvm/s390-diag.txt | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > ping? Sorry holidays :) I'll pick this up for the next pull. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio: document virtio_config_ops restrictions
On Thu, Jan 03, 2019 at 05:08:04PM +0100, Cornelia Huck wrote: > Some transports (e.g. virtio-ccw) implement virtio operations that > seem to be a simple read/write as something more involved that > cannot be done from an atomic context. > > Give at least a hint about that. > > Signed-off-by: Cornelia Huck > --- > include/linux/virtio_config.h | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > index 7087ef946ba7..987b6491b946 100644 > --- a/include/linux/virtio_config.h > +++ b/include/linux/virtio_config.h > @@ -12,6 +12,11 @@ struct irq_affinity; > > /** > * virtio_config_ops - operations for configuring a virtio device > + * Note: Do not assume that a transport implements all of the operations > + * getting/setting a value as a simple read/write! Generally speaking, > + * any of @get/@set, @get_status/@set_status, or @get_features/ > + * @finalize_features are NOT safe to be called from an atomic > + * context. > * @get: read the value of a configuration field > * vdev: the virtio_device > * offset: the offset of the configuration field Then might_sleep in virtio_cread/virtio_cwrite and friends would be appropriate? I guess we'll need to fix balloon first. > -- > 2.17.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote: > virtio-ccw has deadlock issues with reading config registers inside the > interrupt context, so we tweak the virtballoon_changed implementation > by moving the config read operations into the related workqueue contexts. > > Signed-off-by: Wei Wang > --- > drivers/virtio/virtio_balloon.c | 54 > - > 1 file changed, 26 insertions(+), 28 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 728ecd1..9a82a11 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device > *vdev) > { > struct virtio_balloon *vb = vdev->priv; > unsigned long flags; > - s64 diff = towards_target(vb); > - > - if (diff) { > - spin_lock_irqsave(>stop_update_lock, flags); > - if (!vb->stop_update) > - queue_work(system_freezable_wq, > ->update_balloon_size_work); > - spin_unlock_irqrestore(>stop_update_lock, flags); > - } > > - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > - virtio_cread(vdev, struct virtio_balloon_config, > - free_page_report_cmd_id, >cmd_id_received); There's one problem with this approach: previously updating the cmd_id_received here would immediately stop the report in send_free_pages. With this approach we are waiting for the wq to schedule, which might be blocked waiting for report to complete. So host can no longer quickly stop the report in progress. A simple work-around would be to set some kind of flag whenever there is a change interrupt, then have send_free_pages test it and re-read cmd_id_received. Needs to be an atomic I guess ... > - if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) { > - /* Pass ULONG_MAX to give back all the free pages */ > - return_free_pages_to_mm(vb, ULONG_MAX); > - } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP && > -vb->cmd_id_received != > -virtio32_to_cpu(vdev, vb->cmd_id_active)) { > - spin_lock_irqsave(>stop_update_lock, flags); > - if (!vb->stop_update) { > - queue_work(vb->balloon_wq, > ->report_free_page_work); > - } > - spin_unlock_irqrestore(>stop_update_lock, flags); > - } > + spin_lock_irqsave(>stop_update_lock, flags); > + if (!vb->stop_update) { > + queue_work(system_freezable_wq, > +>update_balloon_size_work); > + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) > + queue_work(vb->balloon_wq, >report_free_page_work); > } > + spin_unlock_irqrestore(>stop_update_lock, flags); > } > > static void update_balloon_size(struct virtio_balloon *vb) > @@ -637,11 +619,9 @@ static int send_free_pages(struct virtio_balloon *vb) > return 0; > } > > -static void report_free_page_func(struct work_struct *work) > +static void virtio_balloon_report_free_page(struct virtio_balloon *vb) > { > int err; > - struct virtio_balloon *vb = container_of(work, struct virtio_balloon, > - report_free_page_work); > struct device *dev = >vdev->dev; > > /* Start by sending the received cmd id to host with an outbuf. */ > @@ -659,6 +639,24 @@ static void report_free_page_func(struct work_struct > *work) > dev_err(dev, "Failed to send a stop id, err = %d\n", err); > } > > +static void report_free_page_func(struct work_struct *work) > +{ > + struct virtio_balloon *vb = container_of(work, struct virtio_balloon, > + report_free_page_work); > + > + virtio_cread(vb->vdev, struct virtio_balloon_config, > + free_page_report_cmd_id, >cmd_id_received); > + > + if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) { > + /* Pass ULONG_MAX to give back all the free pages */ > + return_free_pages_to_mm(vb, ULONG_MAX); > + } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP && > +vb->cmd_id_received != > +virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) { > + virtio_balloon_report_free_page(vb); > + } > +} > + > #ifdef CONFIG_BALLOON_COMPACTION > /* > * virtballoon_migratepage - perform the balloon page migration on behalf of > -- > 2.7.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] virtio: document virtio_config_ops restrictions
Some transports (e.g. virtio-ccw) implement virtio operations that seem to be a simple read/write as something more involved that cannot be done from an atomic context. Give at least a hint about that. Signed-off-by: Cornelia Huck --- include/linux/virtio_config.h | 5 + 1 file changed, 5 insertions(+) diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 7087ef946ba7..987b6491b946 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -12,6 +12,11 @@ struct irq_affinity; /** * virtio_config_ops - operations for configuring a virtio device + * Note: Do not assume that a transport implements all of the operations + * getting/setting a value as a simple read/write! Generally speaking, + * any of @get/@set, @get_status/@set_status, or @get_features/ + * @finalize_features are NOT safe to be called from an atomic + * context. * @get: read the value of a configuration field * vdev: the virtio_device * offset: the offset of the configuration field -- 2.17.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] virtio: fix virtio_config_ops description
- get_features has returned 64 bits since commit d025477368792 ("virtio: add support for 64 bit features.") - properly mark all optional callbacks Signed-off-by: Cornelia Huck --- include/linux/virtio_config.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 32baf8e26735..7087ef946ba7 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -22,7 +22,7 @@ struct irq_affinity; * offset: the offset of the configuration field * buf: the buffer to read the field value from. * len: the length of the buffer - * @generation: config generation counter + * @generation: config generation counter (optional) * vdev: the virtio_device * Returns the config generation counter * @get_status: read the status byte @@ -48,17 +48,17 @@ struct irq_affinity; * @del_vqs: free virtqueues found by find_vqs(). * @get_features: get the array of feature bits for this device. * vdev: the virtio_device - * Returns the first 32 feature bits (all we currently need). + * Returns the first 64 feature bits (all we currently need). * @finalize_features: confirm what device features we'll be using. * vdev: the virtio_device * This gives the final feature bits for the device: it can change * the dev->feature bits if it wants. * Returns 0 on success or error status - * @bus_name: return the bus name associated with the device + * @bus_name: return the bus name associated with the device (optional) * vdev: the virtio_device * This returns a pointer to the bus name a la pci_name from which * the caller can then copy. - * @set_vq_affinity: set the affinity for a virtqueue. + * @set_vq_affinity: set the affinity for a virtqueue (optional). * @get_vq_affinity: get the affinity for a virtqueue (optional). */ typedef void vq_callback_t(struct virtqueue *); -- 2.17.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/2] virtio: virtio_config_ops documentation
After the latest virtio-balloon changes, it became clear that it is not obvious that some of the virtio operations (e.g. reading or writing the config space) cannot be done from an atomic context for all transports. At least try to document that, and also fix some inconsistencies I noticed along the way. Cornelia Huck (2): virtio: fix virtio_config_ops description virtio: document virtio_config_ops restrictions include/linux/virtio_config.h | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) -- 2.17.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-ccw: wire up ->bus_name callback
Return the bus id of the ccw proxy device. This makes 'ethtool -i' show a more useful value than 'virtio' in the bus-info field. Signed-off-by: Cornelia Huck --- drivers/s390/virtio/virtio_ccw.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index fc9dbad476c0..689aec54bfcf 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -967,6 +967,13 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) kfree(ccw); } +static const char *virtio_ccw_bus_name(struct virtio_device *vdev) +{ + struct virtio_ccw_device *vcdev = to_vc_device(vdev); + + return dev_name(>cdev->dev); +} + static const struct virtio_config_ops virtio_ccw_config_ops = { .get_features = virtio_ccw_get_features, .finalize_features = virtio_ccw_finalize_features, @@ -977,6 +984,7 @@ static const struct virtio_config_ops virtio_ccw_config_ops = { .reset = virtio_ccw_reset, .find_vqs = virtio_ccw_find_vqs, .del_vqs = virtio_ccw_del_vqs, + .bus_name = virtio_ccw_bus_name, }; -- 2.17.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
On 01/03/2019 05:40 PM, Cornelia Huck wrote: On Thu, 3 Jan 2019 13:31:01 +0800 Wei Wang wrote: virtio-ccw has deadlock issues with reading config registers inside the s/config registers/the config space/ ? Sounds good. interrupt context, so we tweak the virtballoon_changed implementation by moving the config read operations into the related workqueue contexts. Also credit Christian with a Reported-by:? Yes, definitely. Sorry for missing that. Best, Wei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 2/2] virtio: don't allocate vqs when names[i] = NULL
On Fri, 28 Dec 2018 10:26:26 +0800 Wei Wang wrote: > Some vqs may not need to be allocated when their related feature bits > are disabled. So callers may pass in such vqs with "names = NULL". s/=/==/ (here and in the subject) > Then we skip such vq allocations. > > Signed-off-by: Wei Wang > --- > drivers/misc/mic/vop/vop_main.c| 9 +++-- > drivers/remoteproc/remoteproc_virtio.c | 9 +++-- > drivers/s390/virtio/virtio_ccw.c | 12 +--- > drivers/virtio/virtio_mmio.c | 9 +++-- > 4 files changed, 30 insertions(+), 9 deletions(-) Reviewed-by: Cornelia Huck ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 1/2] virtio_pci: use queue idx instead of array idx to set up the vq
On Fri, 28 Dec 2018 10:26:25 +0800 Wei Wang wrote: > When find_vqs, there will be no vq[i] allocation if its corresponding > names[i] is NULL. For example, the caller may pass in names[i] (i=4) > with names[2] being NULL because the related feature bit is turned off, > so technically there are 3 queues on the device, and name[4] should > correspond to the 3rd queue on the device. > > So we use queue_idx as the queue index, which is increased only when the > queue exists. > > Signed-off-by: Wei Wang > --- > drivers/virtio/virtio_pci_common.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Cornelia Huck ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 2/2] virtio-balloon: improve update_balloon_size_func
On Thu, 3 Jan 2019 13:31:02 +0800 Wei Wang wrote: > There is no need to update the balloon actual register when there is no > ballooning request. This patch avoids update_balloon_size when diff is 0. > > Signed-off-by: Wei Wang > --- > drivers/virtio/virtio_balloon.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) Reviewed-by: Cornelia Huck ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
On Thu, 3 Jan 2019 13:31:01 +0800 Wei Wang wrote: > virtio-ccw has deadlock issues with reading config registers inside the s/config registers/the config space/ ? > interrupt context, so we tweak the virtballoon_changed implementation > by moving the config read operations into the related workqueue contexts. Also credit Christian with a Reported-by:? > > Signed-off-by: Wei Wang > --- > drivers/virtio/virtio_balloon.c | 54 > - > 1 file changed, 26 insertions(+), 28 deletions(-) Reviewed-by: Cornelia Huck ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 0/2] virtio-balloon: tweak config_changed
On Thu, 3 Jan 2019 13:31:00 +0800 Wei Wang wrote: > Since virtio-ccw doesn't work with accessing to the config registers > inside an interrupt context, this patch series avoids that issue by > moving the config register accesses to the related workqueue contexts. > > Wei Wang (2): > virtio-balloon: tweak config_changed implementation > virtio-balloon: improve update_balloon_size_func > > drivers/virtio/virtio_balloon.c | 59 > + > 1 file changed, 30 insertions(+), 29 deletions(-) > A quick test (various 'balloon ' in the QEMU monitor) with a virtio-balloon-ccw device seems to work fine here. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PULL 0/1] virtio-ccw: one more patch for 4.21
On Wed, 19 Dec 2018 11:14:13 +0100 Cornelia Huck wrote: > The following changes since commit 37d1246af2d530710cf5822d2845774f01c03b22: > > virtio_net: bulk free tx skbs (2018-12-06 14:28:39 -0500) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git > tags/virtio-ccw-20181219 > > for you to fetch changes up to 6fb95ef30707f1b2fcaea8d6dc873055e0460b1a: > > virtio-ccw: diag 500 may return a negative cookie (2018-12-19 11:01:57 > +0100) > > > One small documentation fix for 4.21. > > > > Cornelia Huck (1): > virtio-ccw: diag 500 may return a negative cookie > > Documentation/virtual/kvm/s390-diag.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > ping? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization