RE: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > struct virtio_balloon {
> > struct virtio_device *vdev;
> > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > /* Prevent updating balloon when it is being canceled. */
> > spinlock_t stop_update_lock;
> > bool stop_update;
> > + /* Bitmap to indicate if reading the related config fields are needed
> */
> > + unsigned long config_read_bitmap;
> >
> > /* The list of allocated free pages, waiting to be given back to mm */
> > struct list_head free_page_list;
>
> It seems that you never initialize this bitmap. Probably harmless here but
> generally using uninitialized memory isn't good.
We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
> > +
> > static int send_free_pages(struct virtio_balloon *vb) {
> > int err;
> > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > * stop the reporting.
> > */
> > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> >cmd_id_active);
> > + virtio_balloon_read_cmd_id_received(vb);
[1]
> > if (cmd_id_active != vb->cmd_id_received)
> > break;
> >
> > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
>
> This will not achieve what you are trying to do, which is cancel reporting if
> it's
> in progress.
>
> You need to re-read each time you compare to cmd_id_active.
Yes, already did, please see [1] above
> An API similar to
> u32 virtio_balloon_cmd_id_received(vb)
> seems to be called for, and I would rename cmd_id_received to
> cmd_id_received_cache to make sure we caught all users.
>
I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
that the driver just received from the device. There is one called
"cmd_id_active" which
is the one that the driver is actively using.
So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
Best,
Wei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH V3 0/5] Hi:
On Sat, Dec 29, 2018 at 08:46:51PM +0800, Jason Wang wrote: > This series tries to access virtqueue metadata through kernel virtual > address instead of copy_user() friends since they had too much > overheads like checks, spec barriers or even hardware feature > toggling. I think it's a reasonable approach. However I need to look at whether and which mmu notifiers are invoked before writeback. Do you know? > Test shows about 24% improvement on TX PPS. It should benefit other > cases as well. > > Changes from V2: > - fix buggy range overlapping check > - tear down MMU notifier during vhost ioctl to make sure invalidation > request can read metadata userspace address and vq size without > holding vq mutex. > Changes from V1: > - instead of pinning pages, use MMU notifier to invalidate vmaps and > remap duing metadata prefetch > - fix build warning on MIPS > > Jason Wang (5): > vhost: generalize adding used elem > vhost: fine grain userspace memory accessors > vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch() > vhost: introduce helpers to get the size of metadata area > vhost: access vq metadata through kernel virtual address > > drivers/vhost/net.c | 4 +- > drivers/vhost/vhost.c | 416 +- > drivers/vhost/vhost.h | 15 +- > 3 files changed, 384 insertions(+), 51 deletions(-) > > -- > 2.17.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] s390/virtio: handle find on invalid queue gracefully
On Thu, 3 Jan 2019 14:00:10 +0100
Halil Pasic wrote:
> On Wed, 2 Jan 2019 19:25:49 +0100
> Cornelia Huck wrote:
>
> > On Wed, 2 Jan 2019 18:50:20 +0100
> > Halil Pasic wrote:
> >
> > > A queue with a capacity of zero is clearly not a valid virtio queue.
> > > Some emulators report zero queue size if queried with an invalid queue
> > > index. Instead of crashing in this case let us just return -EINVAL. To
> > > make that work properly, let us fix the notifier cleanup logic as well.
> > >
> > > Signed-off-by: Halil Pasic
> > > ---
> > >
> > > This patch is motivated by commit 86a5597 "virtio-balloon:
> > > VIRTIO_BALLOON_F_FREE_PAGE_HINT" (Wei Wang, 2018-08-27) which triggered
> > > the described scenario. The emulator in question is the current QEMU.
> > > The problem we run into is the underflow in the following loop
> > > in __vring_new_virtqueue():
> > > for (i = 0; i < vring.num-1; i++)
> > > vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1)
> > > Namely vring.num is an unsigned int.
> > >
> > > RFC because I'm not sure about -EINVAL being a good choice, and about
> > > us caring about what happens if a virtio driver misbehaves like
> > > described.
> >
> > For virtio-pci, the spec says that a zero queue size means that the
> > queue is unavailable. I don't think we have specified that explicitly
> > for virtio-ccw, but it does make sense.
> >
> > virtio-pci returns -ENOENT in that case, which might be a good choice
> > here as well.
>
> virtio-mmio does the same. I will change it to -ENOENT. Maybe also do
> something like
> int virtio_ccw_read_vq_conf() {
> [..]
> return vcdev->config_block->num ?: -ENOENT;
> }
>
> instead of the extra if statement, or?
Yes, why not.
>
> >
> > >
> > > ---
> > > drivers/s390/virtio/virtio_ccw.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/s390/virtio/virtio_ccw.c
> > > b/drivers/s390/virtio/virtio_ccw.c
> > > index fc9dbad476c0..147927ed4fca 100644
> > > --- a/drivers/s390/virtio/virtio_ccw.c
> > > +++ b/drivers/s390/virtio/virtio_ccw.c
> > > @@ -272,6 +272,8 @@ static void virtio_ccw_drop_indicators(struct
> > > virtio_ccw_device *vcdev)
> > > {
> > > struct virtio_ccw_vq_info *info;
> > >
> > > + if (!vcdev->airq_info)
> > > + return;
> >
> > Which case is this guarding against? names[i] was NULL for every index?
> >
>
> Consider:
> static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>
>struct virtqueue *vqs[],
>
>vq_callback_t *callbacks[],
>
>const char * const names[],
>
>const bool *ctx,
>
>struct irq_affinity *desc)
>
> {
>
> struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>
> unsigned long *indicatorp = NULL;
>
> int ret, i;
>
> struct ccw1 *ccw;
>
>
>
> ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
>
> if (!ccw)
>
> return -ENOMEM;
>
>
>
> for (i = 0; i < nvqs; ++i) {
>
> vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i], names[i],
>
> ctx ? ctx[i] : false, ccw);
>
> if (IS_ERR(vqs[i])) {
>
> ret = PTR_ERR(vqs[i]);
>
> vqs[i] = NULL;
>
> goto out;
>
> }
>
> }
> [..]
> if (vcdev->is_thinint) {
>
> ret = virtio_ccw_register_adapter_ind(vcdev, vqs, nvqs, ccw);
>
> if (ret)
>
> /* no error, just fall back to legacy interrupts */
>
> vcdev->is_thinint = false;
>
> }
> [..]
> out:
Re: [RFC PATCH V3 5/5] vhost: access vq metadata through kernel virtual address
On Sat, Dec 29, 2018 at 08:46:56PM +0800, Jason Wang wrote:
> It was noticed that the copy_user() friends that was used to access
> virtqueue metdata tends to be very expensive for dataplane
> implementation like vhost since it involves lots of software checks,
> speculation barrier, hardware feature toggling (e.g SMAP). The
> extra cost will be more obvious when transferring small packets since
> the time spent on metadata accessing become significant..
>
> This patch tries to eliminate those overhead by accessing them through
> kernel virtual address by vmap(). To make the pages can be migrated,
> instead of pinning them through GUP, we use mmu notifiers to
> invalidate vmaps and re-establish vmaps during each round of metadata
> prefetching in necessary. For devices that doesn't use metadata
> prefetching, the memory acessors fallback to normal copy_user()
> implementation gracefully. The invalidation was synchronized with
> datapath through vq mutex, and in order to avoid hold vq mutex during
> range checking, MMU notifier was teared down when trying to modify vq
> metadata.
>
> Note that this was only done when device IOTLB is not enabled. We
> could use similar method to optimize it in the future.
>
> Tests shows about ~24% improvement on TX PPS when using virtio-user +
> vhost_net + xdp1 on TAP:
>
> Before: ~5.0Mpps
> After: ~6.1Mpps
>
> Signed-off-by: Jason Wang
> ---
> drivers/vhost/vhost.c | 263 +-
> drivers/vhost/vhost.h | 13 +++
> 2 files changed, 274 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 54b43feef8d9..e1ecb8acf8a3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -440,6 +440,9 @@ void vhost_dev_init(struct vhost_dev *dev,
> vq->indirect = NULL;
> vq->heads = NULL;
> vq->dev = dev;
> + memset(>avail_ring, 0, sizeof(vq->avail_ring));
> + memset(>used_ring, 0, sizeof(vq->used_ring));
> + memset(>desc_ring, 0, sizeof(vq->desc_ring));
> mutex_init(>mutex);
> vhost_vq_reset(dev, vq);
> if (vq->handle_kick)
> @@ -510,6 +513,73 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue
> *vq, int num)
> return sizeof(*vq->desc) * num;
> }
>
> +static void vhost_uninit_vmap(struct vhost_vmap *map)
> +{
> + if (map->addr)
> + vunmap(map->unmap_addr);
> +
> + map->addr = NULL;
> + map->unmap_addr = NULL;
> +}
> +
> +static int vhost_invalidate_vmap(struct vhost_virtqueue *vq,
> + struct vhost_vmap *map,
> + unsigned long ustart,
> + size_t size,
> + unsigned long start,
> + unsigned long end,
> + bool blockable)
> +{
> + if (end < ustart || start > ustart - 1 + size)
> + return 0;
> +
> + if (!blockable)
> + return -EAGAIN;
> +
> + mutex_lock(>mutex);
> + vhost_uninit_vmap(map);
> + mutex_unlock(>mutex);
> +
> + return 0;
> +}
> +
> +static int vhost_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> + struct mm_struct *mm,
> + unsigned long start,
> + unsigned long end,
> + bool blockable)
> +{
> + struct vhost_dev *dev = container_of(mn, struct vhost_dev,
> + mmu_notifier);
> + int i;
> +
> + for (i = 0; i < dev->nvqs; i++) {
> + struct vhost_virtqueue *vq = dev->vqs[i];
> +
> + if (vhost_invalidate_vmap(vq, >avail_ring,
> + (unsigned long)vq->avail,
> + vhost_get_avail_size(vq, vq->num),
> + start, end, blockable))
> + return -EAGAIN;
> + if (vhost_invalidate_vmap(vq, >desc_ring,
> + (unsigned long)vq->desc,
> + vhost_get_desc_size(vq, vq->num),
> + start, end, blockable))
> + return -EAGAIN;
> + if (vhost_invalidate_vmap(vq, >used_ring,
> + (unsigned long)vq->used,
> + vhost_get_used_size(vq, vq->num),
> + start, end, blockable))
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}
> +
> +static const struct mmu_notifier_ops vhost_mmu_notifier_ops = {
> + .invalidate_range_start = vhost_mmu_notifier_invalidate_range_start,
> +};
> +
> /* Caller should have device mutex */
> long
Re: [RFC PATCH V3 1/5] vhost: generalize adding used elem
On Sat, Dec 29, 2018 at 08:46:52PM +0800, Jason Wang wrote:
> Use one generic vhost_copy_to_user() instead of two dedicated
> accessor. This will simplify the conversion to fine grain
> accessors. About 2% improvement of PPS were seen during vitio-user
> txonly test.
>
> Signed-off-by: Jason Wang
I don't hve a problem with this patch but do you have
any idea how come removing what's supposed to be
an optimization speeds things up?
> ---
> drivers/vhost/vhost.c | 11 +--
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 55e5aa662ad5..f179b5ee14c4 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2174,16 +2174,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue
> *vq,
>
> start = vq->last_used_idx & (vq->num - 1);
> used = vq->used->ring + start;
> - if (count == 1) {
> - if (vhost_put_user(vq, heads[0].id, >id)) {
> - vq_err(vq, "Failed to write used id");
> - return -EFAULT;
> - }
> - if (vhost_put_user(vq, heads[0].len, >len)) {
> - vq_err(vq, "Failed to write used len");
> - return -EFAULT;
> - }
> - } else if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
> + if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
> vq_err(vq, "Failed to write used");
> return -EFAULT;
> }
> --
> 2.17.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/2] virtio-balloon: improve update_balloon_size_func
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 Reviewed-by: Cornelia Huck Reviewed-by: Halil Pasic --- drivers/virtio/virtio_balloon.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 35ee762..af1e6d9 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -453,9 +453,12 @@ static void update_balloon_size_func(struct work_struct *work) update_balloon_size_work); diff = towards_target(vb); + if (!diff) + return; + if (diff > 0) diff -= fill_balloon(vb, diff); - else if (diff < 0) + else diff += leak_balloon(vb, -diff); update_balloon_size(vb); -- 2.7.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
virtio-ccw has deadlock issues with reading the config space inside the
interrupt context, so we tweak the virtballoon_changed implementation
by moving the config read operations into the related workqueue contexts.
The config_read_bitmap is used as a flag to the workqueue callbacks
about the related config fields that need to be read.
Reported-by: Christian Borntraeger
Signed-off-by: Wei Wang
---
drivers/virtio/virtio_balloon.c | 81 +++--
1 file changed, 53 insertions(+), 28 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..35ee762 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -61,6 +61,10 @@ enum virtio_balloon_vq {
VIRTIO_BALLOON_VQ_MAX
};
+enum virtio_balloon_config_read {
+ VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
+};
+
struct virtio_balloon {
struct virtio_device *vdev;
struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
@@ -77,6 +81,8 @@ struct virtio_balloon {
/* Prevent updating balloon when it is being canceled. */
spinlock_t stop_update_lock;
bool stop_update;
+ /* Bitmap to indicate if reading the related config fields are needed */
+ unsigned long config_read_bitmap;
/* The list of allocated free pages, waiting to be given back to mm */
struct list_head free_page_list;
@@ -390,37 +396,31 @@ static unsigned long return_free_pages_to_mm(struct
virtio_balloon *vb,
return num_returned;
}
+static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
+{
+ if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
+ return;
+
+ /* No need to queue the work if the bit was already set. */
+ if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+>config_read_bitmap))
+ return;
+
+ queue_work(vb->balloon_wq, >report_free_page_work);
+}
+
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);
- 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);
+ virtio_balloon_queue_free_page_work(vb);
}
+ spin_unlock_irqrestore(>stop_update_lock, flags);
}
static void update_balloon_size(struct virtio_balloon *vb)
@@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon
*vb)
return 0;
}
+static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
+{
+ if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+ >config_read_bitmap))
+ return;
+
+ virtio_cread(vb->vdev, struct virtio_balloon_config,
+free_page_report_cmd_id, >cmd_id_received);
+}
+
static int send_free_pages(struct virtio_balloon *vb)
{
int err;
@@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
* stop the reporting.
*/
cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
+ virtio_balloon_read_cmd_id_received(vb);
if (cmd_id_active != vb->cmd_id_received)
break;
@@ -637,11 +648,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
Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
On Fri, Jan 04, 2019 at 03:11:52PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger
> Signed-off-by: Wei Wang
> ---
> drivers/virtio/virtio_balloon.c | 81
> +++--
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..35ee762 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
> VIRTIO_BALLOON_VQ_MAX
> };
>
> +enum virtio_balloon_config_read {
> + VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
It seems that you never initialize this bitmap. Probably harmless here
but generally using uninitialized memory isn't good.
> @@ -390,37 +396,31 @@ static unsigned long return_free_pages_to_mm(struct
> virtio_balloon *vb,
> return num_returned;
> }
>
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + return;
> +
> + /* No need to queue the work if the bit was already set. */
> + if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + >config_read_bitmap))
> + return;
> +
> + queue_work(vb->balloon_wq, >report_free_page_work);
> +}
> +
> 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);
> - 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);
> + virtio_balloon_queue_free_page_work(vb);
> }
> + spin_unlock_irqrestore(>stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon
> *vb)
> return 0;
> }
>
> +static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
> +{
> + if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + >config_read_bitmap))
> + return;
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, >cmd_id_received);
> +}
> +
> static int send_free_pages(struct virtio_balloon *vb)
> {
> int err;
> @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
>* stop the reporting.
>*/
> cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> + virtio_balloon_read_cmd_id_received(vb);
>
Re: [PATCH 2/2] virtio: document virtio_config_ops restrictions
On Thu, 3 Jan 2019 18:28:49 +0100 Halil Pasic wrote: > On Thu, 3 Jan 2019 17:08:04 +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. > > I think the only exception is @bus_name (and maybe @generation, I don't > know) because it does not have to 'speak' with the hypervisor. If a > transport operation has to 'speak' with the hypervisor, we do it by > making it interpret a channel program. That means not safe to be called > form atomic context. Or am I missing something? I explicitly singled out the listed callbacks because they read/write a value and there might be more to them than meets the eye. I would assume that nobody expects calling e.g. find_vqs (allocating queues) from an atomic context to be a good idea :) Maybe I should do s/Generally speaking/In particular/ ? That said, it's only a hint; we should add might_sleep as well to interfaces where it makes sense. ___ 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, 3 Jan 2019 11:28:28 -0500 "Michael S. Tsirkin" wrote: > 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. Yes, it makes sense to go over the code and add might_sleep to functions where it makes sense after the balloon changes have been merged. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger
> Signed-off-by: Wei Wang
> ---
> drivers/virtio/virtio_balloon.c | 81
> +++--
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
(...)
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
s/are/is/
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
(...)
Bitmap handling looks sane to me.
Reviewed-by: Cornelia Huck
___
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, 3 Jan 2019 11:51:45 -0500 "Michael S. Tsirkin" wrote: > 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. Thanks for letting me know; I presume you can simply pick up the patch as I sent it along in the pull request as well. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
