* [PATCH net V2] vhost_net: correctly check tx avail during rx busy polling
From: Jason Wang @ 2017-09-05 1:22 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: linux-kernel, kvm, virtualization
We check tx avail through vhost_enable_notify() in the past which is
wrong since it only checks whether or not guest has filled more
available buffer since last avail idx synchronization which was just
done by vhost_vq_avail_empty() before. What we really want is checking
pending buffers in the avail ring. Fix this by calling
vhost_vq_avail_empty() instead.
This issue could be noticed by doing netperf TCP_RR benchmark as
client from guest (but not host). With this fix, TCP_RR from guest to
localhost restores from 1375.91 trans per sec to 55235.28 trans per
sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
Fixes: 030881372460 ("vhost_net: basic polling support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
- The patch is needed for -stable
- Changes from V1: enable vq notification when needed
---
drivers/vhost/net.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 06d0448..1c75572 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -634,8 +634,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
preempt_enable();
- if (vhost_enable_notify(&net->dev, vq))
+ if (!vhost_vq_avail_empty(&net->dev, vq))
vhost_poll_queue(&vq->poll);
+ else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+ vhost_disable_notify(&net->dev, vq);
+ vhost_poll_queue(&vq->poll);
+ }
+
mutex_unlock(&vq->mutex);
len = peek_head_len(rvq, sk);
--
2.7.4
^ permalink raw reply related
* RE: [PATCH v15 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
From: Wang, Wei W @ 2017-09-05 11:57 UTC (permalink / raw)
To: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
kvm@vger.kernel.org, linux-mm@kvack.org, mst@redhat.com,
mhocko@kernel.org, akpm@linux-foundation.org,
mawilcox@microsoft.com
Cc: aarcange@redhat.com, yang.zhang.wz@gmail.com,
liliang.opensource@gmail.com, willy@infradead.org,
amit.shah@redhat.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
mgorman@techsingularity.net
In-Reply-To: <1503914913-28893-6-git-send-email-wei.w.wang@intel.com>
Ping for comments if possible. Thanks.
On Monday, August 28, 2017 6:09 PM, Wang, Wei W wrote:
> [PATCH v15 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
>
> Add a new vq, ctrl_vq, to handle commands between the host and guest.
> With this feature, we will be able to have the control plane and data plane
> separated. In other words, the control related data of each feature will be sent
> via the ctrl_vq cmds, meanwhile each feature may have its own data plane vq.
>
> Free page report is the the first new feature controlled via ctrl_vq, and a new
> cmd class, VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE, is added.
> Currently, this feature has two cmds:
> VIRTIO_BALLOON_FREE_PAGE_F_START: This cmd is sent from host to guest to
> start the free page reporting work.
> VIRTIO_BALLOON_FREE_PAGE_F_STOP: This cmd is used bidirectionally. The
> guest would send the cmd to the host to indicate the reporting work is done.
> The host would send the cmd to the guest to actively request the stop of the
> reporting work.
>
> The free_page_vq is used to transmit the guest free page blocks to the host.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 247 +++++++++++++++++++++++++++++++++-
> --
> include/uapi/linux/virtio_balloon.h | 15 +++
> 2 files changed, 242 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index
> 8ecc1d4..1d384a4 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -55,7 +55,13 @@ static struct vfsmount *balloon_mnt;
>
> struct virtio_balloon {
> struct virtio_device *vdev;
> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *ctrl_vq,
> + *free_page_vq;
> +
> + /* Balloon's own wq for cpu-intensive work items */
> + struct workqueue_struct *balloon_wq;
> + /* The work items submitted to the balloon wq are listed here */
> + struct work_struct report_free_page_work;
>
> /* The balloon servicing is delegated to a freezable workqueue. */
> struct work_struct update_balloon_stats_work; @@ -65,6 +71,9 @@
> struct virtio_balloon {
> spinlock_t stop_update_lock;
> bool stop_update;
>
> + /* Stop reporting free pages */
> + bool report_free_page_stop;
> +
> /* Waiting for host to ack the pages we released. */
> wait_queue_head_t acked;
>
> @@ -93,6 +102,11 @@ struct virtio_balloon {
>
> /* To register callback in oom notifier call chain */
> struct notifier_block nb;
> +
> + /* Host to guest ctrlq cmd buf for free page report */
> + struct virtio_balloon_ctrlq_cmd free_page_cmd_in;
> + /* Guest to Host ctrlq cmd buf for free page report */
> + struct virtio_balloon_ctrlq_cmd free_page_cmd_out;
> };
>
> static struct virtio_device_id id_table[] = { @@ -177,6 +191,26 @@ static void
> send_balloon_page_sg(struct virtio_balloon *vb,
> }
> }
>
> +static void send_free_page_sg(struct virtqueue *vq, void *addr,
> +uint32_t size) {
> + unsigned int len;
> + int err = -ENOSPC;
> +
> + do {
> + if (vq->num_free) {
> + err = add_one_sg(vq, addr, size);
> + /* Sanity check: this can't really happen */
> + WARN_ON(err);
> + if (!err)
> + virtqueue_kick(vq);
> + }
> +
> + /* Release entries if there are */
> + while (virtqueue_get_buf(vq, &len))
> + ;
> + } while (err == -ENOSPC && vq->num_free); }
> +
> /*
> * Send balloon pages in sgs to host. The balloon pages are recorded in the
> * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> @@ -525,42 +559,206 @@ static void update_balloon_size_func(struct
> work_struct *work)
> queue_work(system_freezable_wq, work); }
>
> -static int init_vqs(struct virtio_balloon *vb)
> +static bool virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
> + unsigned long nr_pages)
> +{
> + struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> + void *addr = (void *)pfn_to_kaddr(pfn);
> + uint32_t len = nr_pages << PAGE_SHIFT;
> +
> + if (vb->report_free_page_stop)
> + return 1;
> +
> + send_free_page_sg(vb->free_page_vq, addr, len);
> +
> + return 0;
> +}
> +
> +static void ctrlq_add_cmd(struct virtqueue *vq,
> + struct virtio_balloon_ctrlq_cmd *cmd,
> + bool inbuf)
> {
> - struct virtqueue *vqs[3];
> - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
> - static const char * const names[] = { "inflate", "deflate", "stats" };
> - int err, nvqs;
> + struct scatterlist sg;
> + int err;
> +
> + sg_init_one(&sg, cmd, sizeof(struct virtio_balloon_ctrlq_cmd));
> + if (inbuf)
> + err = virtqueue_add_inbuf(vq, &sg, 1, cmd, GFP_KERNEL);
> + else
> + err = virtqueue_add_outbuf(vq, &sg, 1, cmd, GFP_KERNEL);
> +
> + /* Sanity check: this can't really happen */
> + WARN_ON(err);
> +}
>
> +static void ctrlq_send_cmd(struct virtio_balloon *vb,
> + struct virtio_balloon_ctrlq_cmd *cmd,
> + bool inbuf)
> +{
> + struct virtqueue *vq = vb->ctrl_vq;
> +
> + ctrlq_add_cmd(vq, cmd, inbuf);
> + if (!inbuf) {
> + /*
> + * All the input cmd buffers are replenished here.
> + * This is necessary because the input cmd buffers are lost
> + * after live migration. The device needs to rewind all of
> + * them from the ctrl_vq.
> + */
> + ctrlq_add_cmd(vq, &vb->free_page_cmd_in, true);
> + }
> + virtqueue_kick(vq);
> +}
> +
> +static void report_free_page_end(struct virtio_balloon *vb) {
> /*
> - * We expect two virtqueues: inflate and deflate, and
> - * optionally stat.
> + * The host may have already requested to stop the reporting before we
> + * finish, so no need to notify the host in this case.
> */
> - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ?
> 3 : 2;
> - err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
> + if (vb->report_free_page_stop)
> + return;
> +
> + vb->free_page_cmd_out.class =
> VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> + vb->free_page_cmd_out.cmd = VIRTIO_BALLOON_FREE_PAGE_F_STOP;
> + ctrlq_send_cmd(vb, &vb->free_page_cmd_out, false);
> + vb->report_free_page_stop = true;
> +}
> +
> +static void report_free_page(struct work_struct *work) {
> + struct virtio_balloon *vb;
> +
> + vb = container_of(work, struct virtio_balloon, report_free_page_work);
> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> + report_free_page_end(vb);
> +}
> +
> +static void ctrlq_handle(struct virtqueue *vq) {
> + struct virtio_balloon *vb = vq->vdev->priv;
> + struct virtio_balloon_ctrlq_cmd *cmd;
> + unsigned int len;
> +
> + cmd = (struct virtio_balloon_ctrlq_cmd *)virtqueue_get_buf(vq, &len);
> +
> + if (unlikely(!cmd))
> + return;
> +
> + /* The outbuf is sent by the host for recycling, so just return. */
> + if (cmd == &vb->free_page_cmd_out)
> + return;
> +
> + switch (cmd->class) {
> + case VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE:
> + if (cmd->cmd == VIRTIO_BALLOON_FREE_PAGE_F_STOP) {
> + vb->report_free_page_stop = true;
> + } else if (cmd->cmd == VIRTIO_BALLOON_FREE_PAGE_F_START)
> {
> + vb->report_free_page_stop = false;
> + queue_work(vb->balloon_wq, &vb-
> >report_free_page_work);
> + }
> + vb->free_page_cmd_in.class =
> +
> VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> + ctrlq_send_cmd(vb, &vb->free_page_cmd_in, true);
> + break;
> + default:
> + dev_warn(&vb->vdev->dev, "%s: cmd class not supported\n",
> + __func__);
> + }
> +}
> +
> +static int init_vqs(struct virtio_balloon *vb) {
> + struct virtqueue **vqs;
> + vq_callback_t **callbacks;
> + const char **names;
> + struct scatterlist sg;
> + int i, nvqs, err = -ENOMEM;
> +
> + /* Inflateq and deflateq are used unconditionally */
> + nvqs = 2;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
> + nvqs++;
> + /* If ctrlq is enabled, the free page vq will also be created */
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CTRL_VQ))
> + nvqs += 2;
> +
> + /* Allocate space for find_vqs parameters */
> + vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
> + if (!vqs)
> + goto err_vq;
> + callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
> + if (!callbacks)
> + goto err_callback;
> + names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
> + if (!names)
> + goto err_names;
> +
> + callbacks[0] = balloon_ack;
> + names[0] = "inflate";
> + callbacks[1] = balloon_ack;
> + names[1] = "deflate";
> +
> + i = 2;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> + callbacks[i] = stats_request;
> + names[i] = "stats";
> + i++;
> + }
> +
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CTRL_VQ)) {
> + callbacks[i] = ctrlq_handle;
> + names[i++] = "ctrlq";
> + callbacks[i] = NULL;
> + names[i] = "free_page_vq";
> + }
> +
> + err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
> + NULL, NULL);
> if (err)
> - return err;
> + goto err_find;
>
> vb->inflate_vq = vqs[0];
> vb->deflate_vq = vqs[1];
> + i = 2;
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> - struct scatterlist sg;
> - unsigned int num_stats;
> - vb->stats_vq = vqs[2];
> -
> + vb->stats_vq = vqs[i++];
> /*
> * Prime this virtqueue with one buffer so the hypervisor can
> * use it to signal us later (it can't be broken yet!).
> */
> - num_stats = update_balloon_stats(vb);
> -
> - sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
> - < 0)
> - BUG();
> + < 0) {
> + dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
> + __func__);
> + goto err_find;
> + }
> virtqueue_kick(vb->stats_vq);
> }
> +
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CTRL_VQ)) {
> + vb->ctrl_vq = vqs[i++];
> + vb->free_page_vq = vqs[i];
> + /* Prime the ctrlq with an inbuf for the host to send a cmd */
> + vb->free_page_cmd_in.class =
> +
> VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> + ctrlq_send_cmd(vb, &vb->free_page_cmd_in, true);
> + }
> +
> + kfree(names);
> + kfree(callbacks);
> + kfree(vqs);
> return 0;
> +
> +err_find:
> + kfree(names);
> +err_names:
> + kfree(callbacks);
> +err_callback:
> + kfree(vqs);
> +err_vq:
> + return err;
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> @@ -689,6 +887,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> xb_init(&vb->page_xb);
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_CTRL_VQ)) {
> + vb->balloon_wq = alloc_workqueue("balloon-wq",
> + WQ_FREEZABLE |
> WQ_CPU_INTENSIVE, 0);
> + INIT_WORK(&vb->report_free_page_work, report_free_page);
> + vb->report_free_page_stop = true;
> + }
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -753,6 +958,7 @@ static void virtballoon_remove(struct virtio_device
> *vdev)
> spin_unlock_irq(&vb->stop_update_lock);
> cancel_work_sync(&vb->update_balloon_size_work);
> cancel_work_sync(&vb->update_balloon_stats_work);
> + cancel_work_sync(&vb->report_free_page_work);
>
> remove_common(vb);
> #ifdef CONFIG_BALLOON_COMPACTION
> @@ -806,6 +1012,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> VIRTIO_BALLOON_F_SG,
> + VIRTIO_BALLOON_F_CTRL_VQ,
> };
>
> static struct virtio_driver virtio_balloon_driver = { diff --git
> a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 37780a7..dbf0616 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -35,6 +35,7 @@
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon
> on OOM */
> #define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists
> */
> +#define VIRTIO_BALLOON_F_CTRL_VQ 4 /* Control Virtqueue */
>
> /* Size of a PFN in the balloon interface. */ #define
> VIRTIO_BALLOON_PFN_SHIFT 12 @@ -83,4 +84,18 @@ struct
> virtio_balloon_stat {
> __virtio64 val;
> } __attribute__((packed));
>
> +enum {
> + VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE = 0,
> + VIRTIO_BALLOON_CTRLQ_CLASS_MAX,
> +};
> +
> +struct virtio_balloon_ctrlq_cmd {
> + __virtio32 class;
> + __virtio32 cmd;
> +};
> +
> +/* Ctrlq commands related to VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE */
> +#define VIRTIO_BALLOON_FREE_PAGE_F_STOP 0
> +#define VIRTIO_BALLOON_FREE_PAGE_F_START 1
> +
> #endif /* _LINUX_VIRTIO_BALLOON_H */
> --
> 2.7.4
^ permalink raw reply
* [PATCH 0/4] make virt_spin_lock() a pvops function
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
With virt_spin_lock() being a pvops function the bare metal case can be
optimized by patching the call away completely. In case a kernel running
as a guest it can decide whether to use paravitualized spinlocks, the
current fallback to the unfair test-and-set scheme, or to mimic the
bare metal behavior.
Juergen Gross (4):
paravirt: add generic _paravirt_false() function
paravirt: switch vcpu_is_preempted to use _paravirt_false() on bare
metal
paravirt: add virt_spin_lock pvops function
paravirt,xen: correct xen_nopvspin case
arch/x86/include/asm/paravirt.h | 5 ++++
arch/x86/include/asm/paravirt_types.h | 3 +++
arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
arch/x86/kernel/paravirt-spinlocks.c | 22 ++++++++--------
arch/x86/kernel/paravirt.c | 7 +++++
arch/x86/kernel/paravirt_patch_32.c | 18 ++++++-------
arch/x86/kernel/paravirt_patch_64.c | 17 +++++--------
arch/x86/kernel/smpboot.c | 2 ++
arch/x86/xen/spinlock.c | 2 ++
9 files changed, 79 insertions(+), 45 deletions(-)
--
2.12.3
^ permalink raw reply
* [PATCH 1/4] paravirt: add generic _paravirt_false() function
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
In-Reply-To: <20170905132444.7163-1-jgross@suse.com>
Add a _paravirt_false() default function returning always false which
can be used for cases where a boolean pvops replacement should just
say "no".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt_types.h | 2 ++
arch/x86/kernel/paravirt.c | 7 +++++++
arch/x86/kernel/paravirt_patch_32.c | 8 ++++++++
arch/x86/kernel/paravirt_patch_64.c | 7 +++++++
4 files changed, 24 insertions(+)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 6b64fc6367f2..19efefc0e27e 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -377,6 +377,7 @@ extern struct pv_lock_ops pv_lock_ops;
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len);
unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
+unsigned paravirt_patch_false(void *insnbuf, unsigned len);
unsigned paravirt_patch_call(void *insnbuf,
const void *target, u16 tgt_clobbers,
unsigned long addr, u16 site_clobbers,
@@ -682,6 +683,7 @@ void paravirt_flush_lazy_mmu(void);
void _paravirt_nop(void);
u32 _paravirt_ident_32(u32);
u64 _paravirt_ident_64(u64);
+bool _paravirt_false(void);
#define paravirt_nop ((void *)_paravirt_nop)
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index a14df9eecfed..94105773c00c 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -66,6 +66,11 @@ u64 notrace _paravirt_ident_64(u64 x)
return x;
}
+bool notrace _paravirt_false(void)
+{
+ return false;
+}
+
void __init default_banner(void)
{
printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
@@ -149,6 +154,8 @@ unsigned paravirt_patch_default(u8 type, u16 clobbers, void *insnbuf,
ret = paravirt_patch_ident_32(insnbuf, len);
else if (opfunc == _paravirt_ident_64)
ret = paravirt_patch_ident_64(insnbuf, len);
+ else if (opfunc == _paravirt_false)
+ ret = paravirt_patch_false(insnbuf, len);
else if (type == PARAVIRT_PATCH(pv_cpu_ops.iret) ||
type == PARAVIRT_PATCH(pv_cpu_ops.usergs_sysret64))
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 553acbbb4d32..287c7b9735de 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -9,6 +9,8 @@ DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax");
DEF_NATIVE(pv_mmu_ops, write_cr3, "mov %eax, %cr3");
DEF_NATIVE(pv_mmu_ops, read_cr3, "mov %cr3, %eax");
+DEF_NATIVE(, xor, "xor %eax, %eax");
+
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
@@ -26,6 +28,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
return 0;
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 11aaf1eaa0e4..8ab4379ceea9 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -17,6 +17,7 @@ DEF_NATIVE(pv_cpu_ops, swapgs, "swapgs");
DEF_NATIVE(, mov32, "mov %edi, %eax");
DEF_NATIVE(, mov64, "mov %rdi, %rax");
+DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
@@ -35,6 +36,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
start__mov64, end__mov64);
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
--
2.12.3
^ permalink raw reply related
* [PATCH 2/4] paravirt: switch vcpu_is_preempted to use _paravirt_false() on bare metal
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
In-Reply-To: <20170905132444.7163-1-jgross@suse.com>
Instead of special casing pv_lock_ops.vcpu_is_preempted when patching
use _paravirt_false() on bare metal.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/kernel/paravirt-spinlocks.c | 14 +-------------
arch/x86/kernel/paravirt_patch_32.c | 10 ----------
arch/x86/kernel/paravirt_patch_64.c | 10 ----------
3 files changed, 1 insertion(+), 33 deletions(-)
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 8f2d1c9d43a8..26e4bd92f309 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,25 +20,13 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
-__visible bool __native_vcpu_is_preempted(long cpu)
-{
- return false;
-}
-PV_CALLEE_SAVE_REGS_THUNK(__native_vcpu_is_preempted);
-
-bool pv_is_native_vcpu_is_preempted(void)
-{
- return pv_lock_ops.vcpu_is_preempted.func ==
- __raw_callee_save___native_vcpu_is_preempted;
-}
-
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
.queued_spin_unlock = PV_CALLEE_SAVE(__native_queued_spin_unlock),
.wait = paravirt_nop,
.kick = paravirt_nop,
- .vcpu_is_preempted = PV_CALLEE_SAVE(__native_vcpu_is_preempted),
+ .vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 287c7b9735de..ea311a3563e3 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -13,7 +13,6 @@ DEF_NATIVE(, xor, "xor %eax, %eax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -35,7 +34,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -65,14 +63,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 8ab4379ceea9..64dffe4499b4 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -21,7 +21,6 @@ DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %rax, %rax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -43,7 +42,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -76,14 +74,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
--
2.12.3
^ permalink raw reply related
* [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
In-Reply-To: <20170905132444.7163-1-jgross@suse.com>
There are cases where a guest tries to switch spinlocks to bare metal
behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
has the downside of falling back to unfair test and set scheme for
qspinlocks due to virt_spin_lock() detecting the virtualized
environment.
Make virt_spin_lock() a paravirt operation in order to enable users
to select an explicit behavior like bare metal.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt.h | 5 ++++
arch/x86/include/asm/paravirt_types.h | 1 +
arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
arch/x86/kernel/smpboot.c | 2 ++
5 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index c25dd22f7c70..d9e954fb37df 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
}
+static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
+{
+ return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
+}
+
#endif /* SMP && PARAVIRT_SPINLOCKS */
#ifdef CONFIG_X86_32
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 19efefc0e27e..928f5e7953a7 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -319,6 +319,7 @@ struct pv_lock_ops {
void (*kick)(int cpu);
struct paravirt_callee_save vcpu_is_preempted;
+ struct paravirt_callee_save virt_spin_lock;
} __no_randomize_layout;
/* This contains all the paravirt structures: we get a convenient
diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 48a706f641f2..fbd98896385c 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
smp_store_release((u8 *)lock, 0);
}
+static inline bool native_virt_spin_lock(struct qspinlock *lock)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ return false;
+
+ /*
+ * On hypervisors without PARAVIRT_SPINLOCKS support we fall
+ * back to a Test-and-Set spinlock, because fair locks have
+ * horrible lock 'holder' preemption issues.
+ */
+
+ do {
+ while (atomic_read(&lock->val) != 0)
+ cpu_relax();
+ } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
+
+ return true;
+}
+
#ifdef CONFIG_PARAVIRT_SPINLOCKS
extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
extern void __pv_init_lock_hash(void);
@@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu)
{
return pv_vcpu_is_preempted(cpu);
}
+
+void native_pv_lock_init(void) __init;
#else
static inline void queued_spin_unlock(struct qspinlock *lock)
{
native_queued_spin_unlock(lock);
}
+
+static inline void native_pv_lock_init(void)
+{
+}
#endif
#ifdef CONFIG_PARAVIRT
#define virt_spin_lock virt_spin_lock
+#ifdef CONFIG_PARAVIRT_SPINLOCKS
static inline bool virt_spin_lock(struct qspinlock *lock)
{
- if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
- return false;
-
- /*
- * On hypervisors without PARAVIRT_SPINLOCKS support we fall
- * back to a Test-and-Set spinlock, because fair locks have
- * horrible lock 'holder' preemption issues.
- */
-
- do {
- while (atomic_read(&lock->val) != 0)
- cpu_relax();
- } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
-
- return true;
+ return pv_virt_spin_lock(lock);
+}
+#else
+static inline bool virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
}
+#endif /* CONFIG_PARAVIRT_SPINLOCKS */
#endif /* CONFIG_PARAVIRT */
#include <asm-generic/qspinlock.h>
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 26e4bd92f309..1be187ef8a38 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
+__visible bool __native_virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
+}
+PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
+
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
@@ -27,6 +33,14 @@ struct pv_lock_ops pv_lock_ops = {
.wait = paravirt_nop,
.kick = paravirt_nop,
.vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
+ .virt_spin_lock = PV_CALLEE_SAVE(__native_virt_spin_lock),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
+
+void __init native_pv_lock_init(void)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
+}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 54b9e89d4d6b..21500d3ba359 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,7 @@
#include <asm/i8259.h>
#include <asm/realmode.h>
#include <asm/misc.h>
+#include <asm/qspinlock.h>
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
/* already set me in cpu_online_mask in boot_cpu_init() */
cpumask_set_cpu(me, cpu_callout_mask);
cpu_set_state_online(me);
+ native_pv_lock_init();
}
void __init native_smp_cpus_done(unsigned int max_cpus)
--
2.12.3
^ permalink raw reply related
* [PATCH 4/4] paravirt,xen: correct xen_nopvspin case
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
In-Reply-To: <20170905132444.7163-1-jgross@suse.com>
With the boot parameter "xen_nopvspin" specified a Xen guest should not
make use of paravirt spinlocks, but behave as if running on bare
metal. This is not true, however, as the qspinlock code will fall back
to a test-and-set scheme when it is detecting a hypervisor.
In order to avoid this set the virt_spin_lock pvops function to
_paravirt_false() in case xen_nopvspin has been specified.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/xen/spinlock.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 25a7c4302ce7..c01cedfa745d 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -129,6 +129,8 @@ void __init xen_init_spinlocks(void)
if (!xen_pvspin) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
return;
}
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
--
2.12.3
^ permalink raw reply related
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Peter Zijlstra @ 2017-09-05 13:55 UTC (permalink / raw)
To: Juergen Gross
Cc: jeremy, x86, linux-kernel, virtualization, chrisw, mingo, tglx,
longman, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <20170905132444.7163-4-jgross@suse.com>
On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
> index 48a706f641f2..fbd98896385c 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
> smp_store_release((u8 *)lock, 0);
> }
>
Should this not have:
#ifdef CONFIG_PARAVIRT
?
> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> +{
> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> + return false;
> +
> + /*
> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall
> + * back to a Test-and-Set spinlock, because fair locks have
> + * horrible lock 'holder' preemption issues.
> + */
> +
> + do {
> + while (atomic_read(&lock->val) != 0)
> + cpu_relax();
> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> +
> + return true;
> +}
#endif
> +
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> extern void __pv_init_lock_hash(void);
> #ifdef CONFIG_PARAVIRT
> #define virt_spin_lock virt_spin_lock
> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
> static inline bool virt_spin_lock(struct qspinlock *lock)
> {
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
Because I think the above only ever uses native_virt_spin_lock() when
PARAVIRT.
> @@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
> /* already set me in cpu_online_mask in boot_cpu_init() */
> cpumask_set_cpu(me, cpu_callout_mask);
> cpu_set_state_online(me);
> + native_pv_lock_init();
> }
Aah, this is where that goes.. OK that works too.
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Juergen Gross @ 2017-09-05 14:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: jeremy, x86, linux-kernel, virtualization, chrisw, mingo, tglx,
longman, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <20170905135506.55drjahteljbatak@hirez.programming.kicks-ass.net>
On 05/09/17 15:55, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
>> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
>> index 48a706f641f2..fbd98896385c 100644
>> --- a/arch/x86/include/asm/qspinlock.h
>> +++ b/arch/x86/include/asm/qspinlock.h
>> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
>> smp_store_release((u8 *)lock, 0);
>> }
>>
>
>
> Should this not have:
>
> #ifdef CONFIG_PARAVIRT
>
> ?
I can add it if you want.
Juergen
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-05 14:02 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
In-Reply-To: <20170905132444.7163-4-jgross@suse.com>
On 09/05/2017 09:24 AM, Juergen Gross wrote:
> There are cases where a guest tries to switch spinlocks to bare metal
> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
> has the downside of falling back to unfair test and set scheme for
> qspinlocks due to virt_spin_lock() detecting the virtualized
> environment.
>
> Make virt_spin_lock() a paravirt operation in order to enable users
> to select an explicit behavior like bare metal.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> arch/x86/include/asm/paravirt.h | 5 ++++
> arch/x86/include/asm/paravirt_types.h | 1 +
> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
> arch/x86/kernel/smpboot.c | 2 ++
> 5 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index c25dd22f7c70..d9e954fb37df 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
> }
>
> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
> +{
> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
> +}
> +
> #endif /* SMP && PARAVIRT_SPINLOCKS */
>
> #ifdef CONFIG_X86_32
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index 19efefc0e27e..928f5e7953a7 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -319,6 +319,7 @@ struct pv_lock_ops {
> void (*kick)(int cpu);
>
> struct paravirt_callee_save vcpu_is_preempted;
> + struct paravirt_callee_save virt_spin_lock;
> } __no_randomize_layout;
>
> /* This contains all the paravirt structures: we get a convenient
> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
> index 48a706f641f2..fbd98896385c 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
> smp_store_release((u8 *)lock, 0);
> }
>
> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> +{
> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> + return false;
> +
I think you can take the above if statement out as you has done test in
native_pv_lock_init(). So the test will also be false here.
As this patch series is x86 specific, you should probably add "x86/" in
front of paravirt in the patch titles.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Peter Zijlstra @ 2017-09-05 14:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <3eee978b-e570-5675-7840-c448375b02fc@redhat.com>
On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
> On 09/05/2017 09:24 AM, Juergen Gross wrote:
> > +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> > +{
> > + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> > + return false;
> > +
>
> I think you can take the above if statement out as you has done test in
> native_pv_lock_init(). So the test will also be false here.
That does mean we'll run a test-and-set spinlock until paravirt patching
happens though. I prefer to not do that.
One important point.. we must not be holding any locks when we switch
over between the two locks. Back then I spend some time making sure that
didn't happen with the X86 feature flag muck.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-09-05 14:09 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <96819b6a-6d44-fd7e-37af-5a0db81b3840@redhat.com>
On Mon, Sep 4, 2017 at 5:03 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年09月02日 00:17, Willem de Bruijn wrote:
>>>>>
>>>>> This is not a 50/50 split, which impliesTw that some packets from the
>>>>> large
>>>>> packet flow are still converted to copying. Without the change the rate
>>>>> without queue was 80k zerocopy vs 80k copy, so this choice of
>>>>> (vq->num >> 2) appears too conservative.
>>>>>
>>>>> However, testing with (vq->num >> 1) was not as effective at mitigating
>>>>> stalls. I did not save that data, unfortunately. Can run more tests on
>>>>> fine
>>>>> tuning this variable, if the idea sounds good.
>>>>
>>>>
>>>> Looks like there're still two cases were left:
>>>
>>> To be clear, this patch is not intended to fix all issues. It is a small
>>> improvement to avoid HoL blocking due to queued zerocopy skbs.
>
>
> Right, just want to see if there's anything left.
>
>>>
>>> The trade-off is that reverting to copying in these cases increases
>>> cycle cost. I think that that is a trade-off worth making compared to
>>> the alternative drop in throughput. It probably would be good to be
>>> able to measure this without kernel instrumentation: export
>>> counters similar to net->tx_zcopy_err and net->tx_packets (though
>>> without reset to zero, as in vhost_net_tx_packet).
>
>
> I think it's acceptable if extra cycles were spent if we detect HOL anyhow.
>
>>>
>>>> 1) sndbuf is not INT_MAX
>>>
>>> You mean the case where the device stalls, later zerocopy notifications
>>> are queued, but these are never cleaned in free_old_xmit_skbs,
>>> because it requires a start_xmit and by now the (only) socket is out of
>>> descriptors?
>>
>> Typo, sorry. I meant out of sndbuf.
>
>
> I mean e.g for tun. If its sndbuf is smaller than e.g (vq->num >> 1) *
> $pkt_size and if all packet were held by some modules, limitation like
> vq->num >> 1 won't work since we hit sudbuf before it.
Good point.
>>
>>> A watchdog would help somewhat. With tx-napi, this case cannot occur,
>>> either, as free_old_xmit_skbs no longer depends on a call to start_xmit.
>>>
>>>> 2) tx napi is used for virtio-net
>>>
>>> I am not aware of any issue specific to the use of tx-napi?
>
>
> Might not be clear here, I mean e.g virtio_net (tx-napi) in guest +
> vhost_net (zerocopy) in host. In this case, even if we switch to datacopy if
> ubuf counts exceeds vq->num >> 1, we still complete tx buffers in order, tx
> interrupt could be delayed for indefinite time.
Copied buffers are completed immediately in handle_tx.
Do you mean when a process sends fewer packets than vq->num >> 1,
so that all are queued? Yes, then the latency is indeed that of the last
element leaving the qdisc.
>>>
>>>> 1) could be a corner case, and for 2) what your suggest here may not
>>>> solve
>>>> the issue since it still do in order completion.
>>>
>>> Somewhat tangential, but it might also help to break the in-order
>>> completion processing in vhost_zerocopy_signal_used. Complete
>>> all descriptors between done_idx and upend_idx. done_idx should
>>> then only be forward to the oldest still not-completed descriptor.
>>>
>>> In the test I ran, where the oldest descriptors are held in a queue and
>>> all newer ones are tail-dropped,
>
>
> Do you mean the descriptors were tail-dropped by vhost?
Tail-dropped by netem. The dropped items are completed out of
order by vhost before the held items.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-05 14:10 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
In-Reply-To: <20170905132444.7163-4-jgross@suse.com>
On 09/05/2017 09:24 AM, Juergen Gross wrote:
> There are cases where a guest tries to switch spinlocks to bare metal
> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
> has the downside of falling back to unfair test and set scheme for
> qspinlocks due to virt_spin_lock() detecting the virtualized
> environment.
>
> Make virt_spin_lock() a paravirt operation in order to enable users
> to select an explicit behavior like bare metal.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> arch/x86/include/asm/paravirt.h | 5 ++++
> arch/x86/include/asm/paravirt_types.h | 1 +
> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
> arch/x86/kernel/smpboot.c | 2 ++
> 5 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index c25dd22f7c70..d9e954fb37df 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
> }
>
> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
> +{
> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
> +}
> +
> #endif /* SMP && PARAVIRT_SPINLOCKS */
>
> #ifdef CONFIG_X86_32
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index 19efefc0e27e..928f5e7953a7 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -319,6 +319,7 @@ struct pv_lock_ops {
> void (*kick)(int cpu);
>
> struct paravirt_callee_save vcpu_is_preempted;
> + struct paravirt_callee_save virt_spin_lock;
> } __no_randomize_layout;
>
> /* This contains all the paravirt structures: we get a convenient
> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
> index 48a706f641f2..fbd98896385c 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
> smp_store_release((u8 *)lock, 0);
> }
>
> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> +{
> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> + return false;
> +
> + /*
> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall
> + * back to a Test-and-Set spinlock, because fair locks have
> + * horrible lock 'holder' preemption issues.
> + */
> +
> + do {
> + while (atomic_read(&lock->val) != 0)
> + cpu_relax();
> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> +
> + return true;
> +}
> +
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> extern void __pv_init_lock_hash(void);
> @@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu)
> {
> return pv_vcpu_is_preempted(cpu);
> }
> +
> +void native_pv_lock_init(void) __init;
> #else
> static inline void queued_spin_unlock(struct qspinlock *lock)
> {
> native_queued_spin_unlock(lock);
> }
> +
> +static inline void native_pv_lock_init(void)
> +{
> +}
> #endif
>
> #ifdef CONFIG_PARAVIRT
> #define virt_spin_lock virt_spin_lock
> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
> static inline bool virt_spin_lock(struct qspinlock *lock)
> {
> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> - return false;
Have you consider just add one more jump label here to skip
virt_spin_lock when KVM or Xen want to do so?
> -
> - /*
> - * On hypervisors without PARAVIRT_SPINLOCKS support we fall
> - * back to a Test-and-Set spinlock, because fair locks have
> - * horrible lock 'holder' preemption issues.
> - */
> -
> - do {
> - while (atomic_read(&lock->val) != 0)
> - cpu_relax();
> - } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> -
> - return true;
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
>
> #include <asm-generic/qspinlock.h>
> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
> index 26e4bd92f309..1be187ef8a38 100644
> --- a/arch/x86/kernel/paravirt-spinlocks.c
> +++ b/arch/x86/kernel/paravirt-spinlocks.c
> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
> __raw_callee_save___native_queued_spin_unlock;
> }
>
> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> +}
> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
I have some concern about the overhead of register saving/restoring have
on spin lock performance in case the kernel is under a non-KVM/Xen
hypervisor.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Juergen Gross @ 2017-09-05 14:18 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
In-Reply-To: <c62a47cd-c46d-bfbf-77f2-ec27c86c0f7c@redhat.com>
On 05/09/17 16:10, Waiman Long wrote:
> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>> There are cases where a guest tries to switch spinlocks to bare metal
>> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
>> has the downside of falling back to unfair test and set scheme for
>> qspinlocks due to virt_spin_lock() detecting the virtualized
>> environment.
>>
>> Make virt_spin_lock() a paravirt operation in order to enable users
>> to select an explicit behavior like bare metal.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> arch/x86/include/asm/paravirt.h | 5 ++++
>> arch/x86/include/asm/paravirt_types.h | 1 +
>> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
>> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
>> arch/x86/kernel/smpboot.c | 2 ++
>> 5 files changed, 55 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>> index c25dd22f7c70..d9e954fb37df 100644
>> --- a/arch/x86/include/asm/paravirt.h
>> +++ b/arch/x86/include/asm/paravirt.h
>> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
>> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
>> }
>>
>> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
>> +}
>> +
>> #endif /* SMP && PARAVIRT_SPINLOCKS */
>>
>> #ifdef CONFIG_X86_32
>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>> index 19efefc0e27e..928f5e7953a7 100644
>> --- a/arch/x86/include/asm/paravirt_types.h
>> +++ b/arch/x86/include/asm/paravirt_types.h
>> @@ -319,6 +319,7 @@ struct pv_lock_ops {
>> void (*kick)(int cpu);
>>
>> struct paravirt_callee_save vcpu_is_preempted;
>> + struct paravirt_callee_save virt_spin_lock;
>> } __no_randomize_layout;
>>
>> /* This contains all the paravirt structures: we get a convenient
>> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
>> index 48a706f641f2..fbd98896385c 100644
>> --- a/arch/x86/include/asm/qspinlock.h
>> +++ b/arch/x86/include/asm/qspinlock.h
>> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
>> smp_store_release((u8 *)lock, 0);
>> }
>>
>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>> +{
>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>> + return false;
>> +
>> + /*
>> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>> + * back to a Test-and-Set spinlock, because fair locks have
>> + * horrible lock 'holder' preemption issues.
>> + */
>> +
>> + do {
>> + while (atomic_read(&lock->val) != 0)
>> + cpu_relax();
>> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
>> +
>> + return true;
>> +}
>> +
>> #ifdef CONFIG_PARAVIRT_SPINLOCKS
>> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>> extern void __pv_init_lock_hash(void);
>> @@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu)
>> {
>> return pv_vcpu_is_preempted(cpu);
>> }
>> +
>> +void native_pv_lock_init(void) __init;
>> #else
>> static inline void queued_spin_unlock(struct qspinlock *lock)
>> {
>> native_queued_spin_unlock(lock);
>> }
>> +
>> +static inline void native_pv_lock_init(void)
>> +{
>> +}
>> #endif
>>
>> #ifdef CONFIG_PARAVIRT
>> #define virt_spin_lock virt_spin_lock
>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>> static inline bool virt_spin_lock(struct qspinlock *lock)
>> {
>> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>> - return false;
>
> Have you consider just add one more jump label here to skip
> virt_spin_lock when KVM or Xen want to do so?
Why? Did you look at patch 4? This is the way to do it...
>
>> -
>> - /*
>> - * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>> - * back to a Test-and-Set spinlock, because fair locks have
>> - * horrible lock 'holder' preemption issues.
>> - */
>> -
>> - do {
>> - while (atomic_read(&lock->val) != 0)
>> - cpu_relax();
>> - } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
>> -
>> - return true;
>> + return pv_virt_spin_lock(lock);
>> +}
>> +#else
>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> }
>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>> #endif /* CONFIG_PARAVIRT */
>>
>> #include <asm-generic/qspinlock.h>
>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>> index 26e4bd92f309..1be187ef8a38 100644
>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>> __raw_callee_save___native_queued_spin_unlock;
>> }
>>
>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> +}
>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>
> I have some concern about the overhead of register saving/restoring have
> on spin lock performance in case the kernel is under a non-KVM/Xen
> hypervisor.
We are on the slow path already.
Juergen
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-05 14:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <20170905140813.e455trgmcq3ahzry@hirez.programming.kicks-ass.net>
On 09/05/2017 10:08 AM, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + return false;
>>> +
>> I think you can take the above if statement out as you has done test in
>> native_pv_lock_init(). So the test will also be false here.
> That does mean we'll run a test-and-set spinlock until paravirt patching
> happens though. I prefer to not do that.
>
> One important point.. we must not be holding any locks when we switch
> over between the two locks. Back then I spend some time making sure that
> didn't happen with the X86 feature flag muck.
AFAICT, native_pv_lock_init() is called before SMP init. So it shouldn't
matter.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-05 14:24 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
In-Reply-To: <42918500-4487-f0d8-e6fd-99e4d0d7f1bf@suse.com>
On 09/05/2017 10:18 AM, Juergen Gross wrote:
> On 05/09/17 16:10, Waiman Long wrote:
>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>> There are cases where a guest tries to switch spinlocks to bare metal
>>> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
>>> has the downside of falling back to unfair test and set scheme for
>>> qspinlocks due to virt_spin_lock() detecting the virtualized
>>> environment.
>>>
>>> Make virt_spin_lock() a paravirt operation in order to enable users
>>> to select an explicit behavior like bare metal.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> arch/x86/include/asm/paravirt.h | 5 ++++
>>> arch/x86/include/asm/paravirt_types.h | 1 +
>>> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
>>> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
>>> arch/x86/kernel/smpboot.c | 2 ++
>>> 5 files changed, 55 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>> index c25dd22f7c70..d9e954fb37df 100644
>>> --- a/arch/x86/include/asm/paravirt.h
>>> +++ b/arch/x86/include/asm/paravirt.h
>>> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
>>> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
>>> }
>>>
>>> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
>>> +}
>>> +
>>> #endif /* SMP && PARAVIRT_SPINLOCKS */
>>>
>>> #ifdef CONFIG_X86_32
>>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>>> index 19efefc0e27e..928f5e7953a7 100644
>>> --- a/arch/x86/include/asm/paravirt_types.h
>>> +++ b/arch/x86/include/asm/paravirt_types.h
>>> @@ -319,6 +319,7 @@ struct pv_lock_ops {
>>> void (*kick)(int cpu);
>>>
>>> struct paravirt_callee_save vcpu_is_preempted;
>>> + struct paravirt_callee_save virt_spin_lock;
>>> } __no_randomize_layout;
>>>
>>> /* This contains all the paravirt structures: we get a convenient
>>> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
>>> index 48a706f641f2..fbd98896385c 100644
>>> --- a/arch/x86/include/asm/qspinlock.h
>>> +++ b/arch/x86/include/asm/qspinlock.h
>>> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
>>> smp_store_release((u8 *)lock, 0);
>>> }
>>>
>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + return false;
>>> +
>>> + /*
>>> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>>> + * back to a Test-and-Set spinlock, because fair locks have
>>> + * horrible lock 'holder' preemption issues.
>>> + */
>>> +
>>> + do {
>>> + while (atomic_read(&lock->val) != 0)
>>> + cpu_relax();
>>> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
>>> +
>>> + return true;
>>> +}
>>> +
>>> #ifdef CONFIG_PARAVIRT_SPINLOCKS
>>> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>>> extern void __pv_init_lock_hash(void);
>>> @@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu)
>>> {
>>> return pv_vcpu_is_preempted(cpu);
>>> }
>>> +
>>> +void native_pv_lock_init(void) __init;
>>> #else
>>> static inline void queued_spin_unlock(struct qspinlock *lock)
>>> {
>>> native_queued_spin_unlock(lock);
>>> }
>>> +
>>> +static inline void native_pv_lock_init(void)
>>> +{
>>> +}
>>> #endif
>>>
>>> #ifdef CONFIG_PARAVIRT
>>> #define virt_spin_lock virt_spin_lock
>>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>>> static inline bool virt_spin_lock(struct qspinlock *lock)
>>> {
>>> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> - return false;
>> Have you consider just add one more jump label here to skip
>> virt_spin_lock when KVM or Xen want to do so?
> Why? Did you look at patch 4? This is the way to do it...
I asked this because of my performance concern as stated later in the email.
>>> -
>>> - /*
>>> - * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>>> - * back to a Test-and-Set spinlock, because fair locks have
>>> - * horrible lock 'holder' preemption issues.
>>> - */
>>> -
>>> - do {
>>> - while (atomic_read(&lock->val) != 0)
>>> - cpu_relax();
>>> - } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
>>> -
>>> - return true;
>>> + return pv_virt_spin_lock(lock);
>>> +}
>>> +#else
>>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> }
>>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>>> #endif /* CONFIG_PARAVIRT */
>>>
>>> #include <asm-generic/qspinlock.h>
>>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>>> index 26e4bd92f309..1be187ef8a38 100644
>>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>>> __raw_callee_save___native_queued_spin_unlock;
>>> }
>>>
>>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> +}
>>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>> I have some concern about the overhead of register saving/restoring have
>> on spin lock performance in case the kernel is under a non-KVM/Xen
>> hypervisor.
> We are on the slow path already.
That is true, but I still still believe there will be performance impact
on lock contention behavior where the slowpath will be used.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-05 14:31 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
In-Reply-To: <3a2eefe7-5c0b-9768-2bb8-03c947133b06@redhat.com>
On 09/05/2017 10:24 AM, Waiman Long wrote:
> On 09/05/2017 10:18 AM, Juergen Gross wrote:
>> On 05/09/17 16:10, Waiman Long wrote:
>>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>>> There are cases where a guest tries to switch spinlocks to bare metal
>>>> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
>>>> has the downside of falling back to unfair test and set scheme for
>>>> qspinlocks due to virt_spin_lock() detecting the virtualized
>>>> environment.
>>>>
>>>> Make virt_spin_lock() a paravirt operation in order to enable users
>>>> to select an explicit behavior like bare metal.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> arch/x86/include/asm/paravirt.h | 5 ++++
>>>> arch/x86/include/asm/paravirt_types.h | 1 +
>>>> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
>>>> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
>>>> arch/x86/kernel/smpboot.c | 2 ++
>>>> 5 files changed, 55 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>>> index c25dd22f7c70..d9e954fb37df 100644
>>>> --- a/arch/x86/include/asm/paravirt.h
>>>> +++ b/arch/x86/include/asm/paravirt.h
>>>> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
>>>> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
>>>> }
>>>>
>>>> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
>>>> +{
>>>> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
>>>> +}
>>>> +
>>>> #endif /* SMP && PARAVIRT_SPINLOCKS */
>>>>
>>>> #ifdef CONFIG_X86_32
>>>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>>>> index 19efefc0e27e..928f5e7953a7 100644
>>>> --- a/arch/x86/include/asm/paravirt_types.h
>>>> +++ b/arch/x86/include/asm/paravirt_types.h
>>>> @@ -319,6 +319,7 @@ struct pv_lock_ops {
>>>> void (*kick)(int cpu);
>>>>
>>>> struct paravirt_callee_save vcpu_is_preempted;
>>>> + struct paravirt_callee_save virt_spin_lock;
>>>> } __no_randomize_layout;
>>>>
>>>> /* This contains all the paravirt structures: we get a convenient
>>>> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
>>>> index 48a706f641f2..fbd98896385c 100644
>>>> --- a/arch/x86/include/asm/qspinlock.h
>>>> +++ b/arch/x86/include/asm/qspinlock.h
>>>> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
>>>> smp_store_release((u8 *)lock, 0);
>>>> }
>>>>
>>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>>> +{
>>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>>> + return false;
>>>> +
>>>> + /*
>>>> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>>>> + * back to a Test-and-Set spinlock, because fair locks have
>>>> + * horrible lock 'holder' preemption issues.
>>>> + */
>>>> +
>>>> + do {
>>>> + while (atomic_read(&lock->val) != 0)
>>>> + cpu_relax();
>>>> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> #ifdef CONFIG_PARAVIRT_SPINLOCKS
>>>> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>>>> extern void __pv_init_lock_hash(void);
>>>> @@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu)
>>>> {
>>>> return pv_vcpu_is_preempted(cpu);
>>>> }
>>>> +
>>>> +void native_pv_lock_init(void) __init;
>>>> #else
>>>> static inline void queued_spin_unlock(struct qspinlock *lock)
>>>> {
>>>> native_queued_spin_unlock(lock);
>>>> }
>>>> +
>>>> +static inline void native_pv_lock_init(void)
>>>> +{
>>>> +}
>>>> #endif
>>>>
>>>> #ifdef CONFIG_PARAVIRT
>>>> #define virt_spin_lock virt_spin_lock
>>>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>>>> static inline bool virt_spin_lock(struct qspinlock *lock)
>>>> {
>>>> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>>> - return false;
>>> Have you consider just add one more jump label here to skip
>>> virt_spin_lock when KVM or Xen want to do so?
>> Why? Did you look at patch 4? This is the way to do it...
> I asked this because of my performance concern as stated later in the email.
For clarification, I was actually asking if you consider just adding one
more jump label to skip it for Xen/KVM instead of making
virt_spin_lock() a pv-op.
Cheers,
Longman
^ permalink raw reply
* [PATCH] paravirt: switch maintainer
From: Juergen Gross @ 2017-09-05 14:34 UTC (permalink / raw)
To: linux-kernel, x86, virtualization
Cc: Juergen Gross, jeremy, chrisw, mingo, hpa, tglx, akataria
Jeremy Fitzhardinge is stepping down as a paravirt maintainer. I'll
replace him.
While at it, update the file list to the actual pattern.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
MAINTAINERS | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8ef4694af6e8..d7565683aab3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9956,7 +9956,7 @@ F: include/uapi/linux/ppdev.h
F: Documentation/parport*.txt
PARAVIRT_OPS INTERFACE
-M: Jeremy Fitzhardinge <jeremy@goop.org>
+M: Juergen Gross <jgross@suse.com>
M: Chris Wright <chrisw@sous-sol.org>
M: Alok Kataria <akataria@vmware.com>
M: Rusty Russell <rusty@rustcorp.com.au>
@@ -9964,7 +9964,7 @@ L: virtualization@lists.linux-foundation.org
S: Supported
F: Documentation/virtual/paravirt_ops.txt
F: arch/*/kernel/paravirt*
-F: arch/*/include/asm/paravirt.h
+F: arch/*/include/asm/paravirt*.h
F: include/linux/hypervisor.h
PARIDE DRIVERS FOR PARALLEL PORT IDE DEVICES
--
2.12.3
^ permalink raw reply related
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Juergen Gross @ 2017-09-05 14:42 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
In-Reply-To: <a921465a-3c4f-9223-b4af-cd250df62f90@redhat.com>
On 05/09/17 16:31, Waiman Long wrote:
> On 09/05/2017 10:24 AM, Waiman Long wrote:
>> On 09/05/2017 10:18 AM, Juergen Gross wrote:
>>> On 05/09/17 16:10, Waiman Long wrote:
>>>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>>>> There are cases where a guest tries to switch spinlocks to bare metal
>>>>> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this
>>>>> has the downside of falling back to unfair test and set scheme for
>>>>> qspinlocks due to virt_spin_lock() detecting the virtualized
>>>>> environment.
>>>>>
>>>>> Make virt_spin_lock() a paravirt operation in order to enable users
>>>>> to select an explicit behavior like bare metal.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>> arch/x86/include/asm/paravirt.h | 5 ++++
>>>>> arch/x86/include/asm/paravirt_types.h | 1 +
>>>>> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
>>>>> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++
>>>>> arch/x86/kernel/smpboot.c | 2 ++
>>>>> 5 files changed, 55 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>>>> index c25dd22f7c70..d9e954fb37df 100644
>>>>> --- a/arch/x86/include/asm/paravirt.h
>>>>> +++ b/arch/x86/include/asm/paravirt.h
>>>>> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
>>>>> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu);
>>>>> }
>>>>>
>>>>> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock)
>>>>> +{
>>>>> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock);
>>>>> +}
>>>>> +
>>>>> #endif /* SMP && PARAVIRT_SPINLOCKS */
>>>>>
>>>>> #ifdef CONFIG_X86_32
>>>>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>>>>> index 19efefc0e27e..928f5e7953a7 100644
>>>>> --- a/arch/x86/include/asm/paravirt_types.h
>>>>> +++ b/arch/x86/include/asm/paravirt_types.h
>>>>> @@ -319,6 +319,7 @@ struct pv_lock_ops {
>>>>> void (*kick)(int cpu);
>>>>>
>>>>> struct paravirt_callee_save vcpu_is_preempted;
>>>>> + struct paravirt_callee_save virt_spin_lock;
>>>>> } __no_randomize_layout;
>>>>>
>>>>> /* This contains all the paravirt structures: we get a convenient
>>>>> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
>>>>> index 48a706f641f2..fbd98896385c 100644
>>>>> --- a/arch/x86/include/asm/qspinlock.h
>>>>> +++ b/arch/x86/include/asm/qspinlock.h
>>>>> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock)
>>>>> smp_store_release((u8 *)lock, 0);
>>>>> }
>>>>>
>>>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>>>> +{
>>>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>>>> + return false;
>>>>> +
>>>>> + /*
>>>>> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>>>>> + * back to a Test-and-Set spinlock, because fair locks have
>>>>> + * horrible lock 'holder' preemption issues.
>>>>> + */
>>>>> +
>>>>> + do {
>>>>> + while (atomic_read(&lock->val) != 0)
>>>>> + cpu_relax();
>>>>> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
>>>>> +
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> #ifdef CONFIG_PARAVIRT_SPINLOCKS
>>>>> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>>>>> extern void __pv_init_lock_hash(void);
>>>>> @@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu)
>>>>> {
>>>>> return pv_vcpu_is_preempted(cpu);
>>>>> }
>>>>> +
>>>>> +void native_pv_lock_init(void) __init;
>>>>> #else
>>>>> static inline void queued_spin_unlock(struct qspinlock *lock)
>>>>> {
>>>>> native_queued_spin_unlock(lock);
>>>>> }
>>>>> +
>>>>> +static inline void native_pv_lock_init(void)
>>>>> +{
>>>>> +}
>>>>> #endif
>>>>>
>>>>> #ifdef CONFIG_PARAVIRT
>>>>> #define virt_spin_lock virt_spin_lock
>>>>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>>>>> static inline bool virt_spin_lock(struct qspinlock *lock)
>>>>> {
>>>>> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>>>> - return false;
>>>> Have you consider just add one more jump label here to skip
>>>> virt_spin_lock when KVM or Xen want to do so?
>>> Why? Did you look at patch 4? This is the way to do it...
>> I asked this because of my performance concern as stated later in the email.
>
> For clarification, I was actually asking if you consider just adding one
> more jump label to skip it for Xen/KVM instead of making
> virt_spin_lock() a pv-op.
Aah, okay.
Bare metal could make use of this jump label, too.
I'll have a try how it looks like.
Juergen
^ permalink raw reply
* Re: [PATCH net V2] vhost_net: correctly check tx avail during rx busy polling
From: David Miller @ 2017-09-05 21:48 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1504574525-4711-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 5 Sep 2017 09:22:05 +0800
> We check tx avail through vhost_enable_notify() in the past which is
> wrong since it only checks whether or not guest has filled more
> available buffer since last avail idx synchronization which was just
> done by vhost_vq_avail_empty() before. What we really want is checking
> pending buffers in the avail ring. Fix this by calling
> vhost_vq_avail_empty() instead.
>
> This issue could be noticed by doing netperf TCP_RR benchmark as
> client from guest (but not host). With this fix, TCP_RR from guest to
> localhost restores from 1375.91 trans per sec to 55235.28 trans per
> sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
>
> Fixes: 030881372460 ("vhost_net: basic polling support")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Michael S. Tsirkin @ 2017-09-06 3:27 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-JQ5vL8fhLwtz9yY9B4NZR3Tf0kmZHAi-9ht_-Bz0n4Sw@mail.gmail.com>
On Tue, Sep 05, 2017 at 04:09:19PM +0200, Willem de Bruijn wrote:
> On Mon, Sep 4, 2017 at 5:03 AM, Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > On 2017年09月02日 00:17, Willem de Bruijn wrote:
> >>>>>
> >>>>> This is not a 50/50 split, which impliesTw that some packets from the
> >>>>> large
> >>>>> packet flow are still converted to copying. Without the change the rate
> >>>>> without queue was 80k zerocopy vs 80k copy, so this choice of
> >>>>> (vq->num >> 2) appears too conservative.
> >>>>>
> >>>>> However, testing with (vq->num >> 1) was not as effective at mitigating
> >>>>> stalls. I did not save that data, unfortunately. Can run more tests on
> >>>>> fine
> >>>>> tuning this variable, if the idea sounds good.
> >>>>
> >>>>
> >>>> Looks like there're still two cases were left:
> >>>
> >>> To be clear, this patch is not intended to fix all issues. It is a small
> >>> improvement to avoid HoL blocking due to queued zerocopy skbs.
> >
> >
> > Right, just want to see if there's anything left.
> >
> >>>
> >>> The trade-off is that reverting to copying in these cases increases
> >>> cycle cost. I think that that is a trade-off worth making compared to
> >>> the alternative drop in throughput. It probably would be good to be
> >>> able to measure this without kernel instrumentation: export
> >>> counters similar to net->tx_zcopy_err and net->tx_packets (though
> >>> without reset to zero, as in vhost_net_tx_packet).
> >
> >
> > I think it's acceptable if extra cycles were spent if we detect HOL anyhow.
> >
> >>>
> >>>> 1) sndbuf is not INT_MAX
> >>>
> >>> You mean the case where the device stalls, later zerocopy notifications
> >>> are queued, but these are never cleaned in free_old_xmit_skbs,
> >>> because it requires a start_xmit and by now the (only) socket is out of
> >>> descriptors?
> >>
> >> Typo, sorry. I meant out of sndbuf.
> >
> >
> > I mean e.g for tun. If its sndbuf is smaller than e.g (vq->num >> 1) *
> > $pkt_size and if all packet were held by some modules, limitation like
> > vq->num >> 1 won't work since we hit sudbuf before it.
>
> Good point.
I agree however anyone using SNDBUF < infinity already hits HOQ blocking
in some scenarios.
> >>
> >>> A watchdog would help somewhat. With tx-napi, this case cannot occur,
> >>> either, as free_old_xmit_skbs no longer depends on a call to start_xmit.
> >>>
> >>>> 2) tx napi is used for virtio-net
> >>>
> >>> I am not aware of any issue specific to the use of tx-napi?
> >
> >
> > Might not be clear here, I mean e.g virtio_net (tx-napi) in guest +
> > vhost_net (zerocopy) in host. In this case, even if we switch to datacopy if
> > ubuf counts exceeds vq->num >> 1, we still complete tx buffers in order, tx
> > interrupt could be delayed for indefinite time.
>
> Copied buffers are completed immediately in handle_tx.
>
> Do you mean when a process sends fewer packets than vq->num >> 1,
> so that all are queued? Yes, then the latency is indeed that of the last
> element leaving the qdisc.
>
> >>>
> >>>> 1) could be a corner case, and for 2) what your suggest here may not
> >>>> solve
> >>>> the issue since it still do in order completion.
> >>>
> >>> Somewhat tangential, but it might also help to break the in-order
> >>> completion processing in vhost_zerocopy_signal_used. Complete
> >>> all descriptors between done_idx and upend_idx. done_idx should
> >>> then only be forward to the oldest still not-completed descriptor.
> >>>
> >>> In the test I ran, where the oldest descriptors are held in a queue and
> >>> all newer ones are tail-dropped,
> >
> >
> > Do you mean the descriptors were tail-dropped by vhost?
>
> Tail-dropped by netem. The dropped items are completed out of
> order by vhost before the held items.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Peter Zijlstra @ 2017-09-06 7:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <a921465a-3c4f-9223-b4af-cd250df62f90@redhat.com>
Guys, please trim email.
On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> For clarification, I was actually asking if you consider just adding one
> more jump label to skip it for Xen/KVM instead of making
> virt_spin_lock() a pv-op.
I don't understand. What performance are you worried about. Native will
now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
^ permalink raw reply
* Re: [virtio-dev] [RFC] virtio-iommu version 0.4
From: Jean-Philippe Brucker @ 2017-09-06 11:48 UTC (permalink / raw)
To: Auger Eric, iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, robin.murphy,
eric.auger.pro
In-Reply-To: <f9b02ce1-057f-df4f-90d7-52616ad60b88@redhat.com>
Hi Eric,
On 23/08/17 14:55, Auger Eric wrote:
> Please find some comments/questions below:
Thanks a lot for this. Sorry for the delay, I was on holiday and it took
me a while to sort out the details.
> 2.6.7:1
> I do not understand the footnode #6 sentence: 'Without a specific
> definition of the ACK requirements for a given property type, it simply
> means that the driver read all fields of that property."
That doesn't serve any purpose, I'll remove the footnote.
> 2.6.7:2
> what if the driver has not provided a big enough buffer and the device
> cannot report all supported properties?
Good point, how about adding the following to 2.6.7.2 - Device
Requirements: PROBE Request
If the properties array is smaller than \field{probe_size}, then the
device SHOULD NOT write any property and SHOULD set the request
\field{status} to VIRTIO_IOMMU_S_INVAL.
> 2.6.8.2:
> Couldn't we use the same terminology as iommu_resv_type in iommu.h?
> the negative form is not the easiest to understand for me.
> Why F_MSI?
It also needs to be understandable by someone not familiar with the Linux
APIs. It's not entirely obvious to me what the iommu.h regions are without
looking at the implementation, maybe using names from the device point of
view is more clear. I don't mind simplifying though, as I don't see a
reason for the driver to know about BYPASS regions other than
HW MSIs.
How about I remove ABORT and BYPASS, make the only subtype RESV_MEM_T_MSI
(1)? We'd use subtype 0 as "don't care, just don't map this". Can call it
RESV_MEM_T_RESERVED. Flags won't be used for these subtypes.
Another subtype will be RESV_MEM_T_IDENTITY (see my reply to Kevin).
IDENTITY regions must be identity-mapped by the guest and, as I understand
it, are virtual addresses used for DMA by firmware. The flags field will
then contain read/write protection flags.
> 2.6.8.2.1
> Multiple overlapping RESV_MEM properties are merged together. Device
> requirement? if same types I assume?
Combination rules apply to device and driver. When the driver puts
multiple endpoints in the same domain, combination rules apply. They will
become important once the guest attempts to do things like combining
endpoints with different PASID capabilities in the same domain. I'm
considering these endpoints to be behind different physical IOMMUs.
For reserved regions, we specify what the driver should do and what the
device should enforce with regard to mapping IOVAs of overlapping regions.
For example, if a device has some virtual address RESV_T_MSI and an other
device has the same virtual address RESV_T_IDENTITY, what should the
driver do?
I think it should apply the RESV_T_IDENTITY. RESV_T_MSI is just a special
case of RESV_T_RESERVED, it's a hint for the IRQ subsystem and doesn't
have a meaning within a domain. From DMA mappings point of view, it is
effectively the same as RESV_T_RESERVED. When merging RESV_T_RESERVED and
RESV_T_IDENTITY, we should make it RESV_T_IDENTITY. Because it is required
for one endpoint to work (the one with IDENTITY) and is not accessed by
the other (the driver must not allocate this IOVA range for DMA).
More generally, I'd like to add the following combination table to the
spec, that shows the resulting reserved region type after merging regions
from two endpoints. N: no reserved region, R: RESV_T_RESERVED, M:
RESV_T_MSI, I: RESV_T_IDENTITY. It is symmetric so I didn't fill the
bottom half.
| N | R | M | I
------------------
N | N | R | M | ?
------------------
R | | R | R | I
------------------
M | | | M | I
------------------
I | | | | I
The 'I' column is problematic. If one endpoint has an IDENTITY region and
the other doesn't have anything at that virtual address, then making that
region an identity mapping will result in the second endpoint being able
to access firmware memory. If using nested translation, stage-2 can help
us here. But in general we should allow the device to reject an attach
that would result in a N+I combination if the host considers it too dodgy.
I think the other combinations are safe enough.
> what if the device is a physical assigned device. does the device report
> the host doorbell or the guest doorbell, may be worth to clarify.
Indeed. For a physical assigned device it is the guest doorbell as it
corresponds to the virtual ITS or APIC (though with the APIC it's the same
region).
> 3.1.1 last paragraph
> you talk about device ID but this is a terminology only known by ARM I
> think. Actually it is introduced later in 3.1.2.
Indeed, I'll move the explanation up.
> 3.1.2
> About ACPI integration. Isn't IORT ARM specific?
IORT is currently edited by ARM, as is virtio-iommu. Their contents are
not ARM specific however, they describe software interfaces that solve
mostly generic problems, and aim to be open.
IORT describes the relation between endpoints and their translation
agents. This problem has already been solved by IVRS, DMAR and IORT. From
a technical point of view the latter is more flexible and can host a new
translation agent. It should be relatively easy to use for virtio-iommu in
the kernel, and therefore I don't think we should introduce a fourth type
of table just for virtio-iommu.
> Will other architectures use that table?
Yes, in particular the IORT node would be used on virtual x86 platforms.
On ARM platforms, Linux guests use existing device-tree bindings.
Operating systems that don't implement DT could use IORT as well.
> In IORT we also have Named Component node
> which look pretty the same. Could you elaborate of the difference?
It's the same mechanism, except that the named component node is supposed
to be a leaf in the topology, not a translation agent. Using the same node
type could make it difficult for the driver to understand the table. But
see below.
> Device Object name: isn't it the path to the LNR0005 in the namespace?
It is. However, I wrote the example table to give a rough idea, but didn't
investigate the feasibility. I now think it would be easier to not present
the virtio-iommu as a LNRO0005 to the guest, but solely as the IORT node.
Instead of a namespace path, the node would directly describe the _CRS
content, which should be enough for the kernel to instantiate the device.
I'll prototype something to understand the problem better and find which
is easier to do. I'm busy preparing for conferences, so it might be two or
three weeks.
Unrelated remarks:
* When renaming "devices" to "endpoints" in v0.3 (to avoid confusion wrt
"The device", which is virtio-iommu dev), I didn't update the
attach/detach/probe requests, they still have 'device' fields. Since I'm
already butchering the API by renaming address_space to domain, should I
also rename 'device' to 'endpoint' in the structure? It doesn't matter as
much, since the difference here is mostly stylistic. But I think I will
get it out of the way for v0.5, to make things more clear overall.
* Regarding my question in v0.4 about adding a 'size' field to all
requests. I don't think we should do that. The problem was for parsing
interleaved RO/WO/RO buffers, but we might never hit the problem if we
design virtio-iommu properly. Framing should be left to the transport
protocol and it'd be mostly redundant in virtio-iommu requests.
Thanks,
Jean
^ permalink raw reply
* Re: [RFC] virtio-iommu version 0.4
From: Jean-Philippe Brucker @ 2017-09-06 11:54 UTC (permalink / raw)
To: Tian, Kevin, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: lorenzo.pieralisi@arm.com, mst@redhat.com, marc.zyngier@arm.com,
will.deacon@arm.com, eric.auger@redhat.com, robin.murphy@arm.com,
eric.auger.pro@gmail.com
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D190D941E3@SHSMSX101.ccr.corp.intel.com>
Hi Kevin,
On 28/08/17 08:39, Tian, Kevin wrote:
> Here comes some comments:
>
> 1.1 Motivation
>
> You describe I/O page faults handling as future work. Seems you considered
> only recoverable fault (since "aka. PCI PRI" being used). What about other
> unrecoverable faults e.g. what to do if a virtual DMA request doesn't find
> a valid mapping? Even when there is no PRI support, we need some basic
> form of fault reporting mechanism to indicate such errors to guest.
I am considering recoverable faults as the end goal, but reporting
unrecoverable faults should use the same queue, with slightly different
fields and no need for the driver to reply to the device.
> 2.6.8.2 Property RESV_MEM
>
> I'm not immediately clear when VIRTIO_IOMMU_PROBE_RESV_MEM_T_ABORT
> should be explicitly reported. Is there any real example on bare metal IOMMU?
> usually reserved memory is reported to CPU through other method (e.g. e820
> on x86 platform). Of course MSI is a special case which is covered by BYPASS
> and MSI flag... If yes, maybe you can also include an example in implementation
> notes.
The RESV_MEM regions only describes IOVA space for the moment, not
guest-physical, so I guess it provides different information than e820.
I think a useful example is the PCI bridge windows reported by the Linux
host to userspace using RESV_RESERVED regions (see
iommu_dma_get_resv_regions). If I understand correctly, they represent DMA
addresses that shouldn't be accessed by endpoints because they won't reach
the IOMMU. These are specific to the physical topology: a device will have
different reserved regions depending on the PCI slot it occupies.
When handled properly, PCI bridge windows quickly become a nuisance. With
kvmtool we observed that carving out their addresses globally removes a
lot of useful GPA space from the guest. Without a virtual IOMMU we can
either ignore them and hope everything will be fine, or remove all
reserved regions from the GPA space (which currently means editing by hand
the static guest-physical map...)
That's where RESV_MEM_T_ABORT comes handy with virtio-iommu. It describes
reserved IOVAs for a specific endpoint, and therefore removes the need to
carve the window out of the whole guest.
> Another thing I want to ask your opinion, about whether there is value of
> adding another subtype (MEM_T_IDENTITY), asking for identity mapping
> in the address space. It's similar to Reserved Memory Region Reporting
> (RMRR) structure defined in VT-d, to indicate BIOS allocated reserved
> memory ranges which may be DMA target and has to be identity mapped
> when DMA remapping is enabled. I'm not sure whether ARM has similar
> capability and whether there might be a general usage beyond VT-d. For
> now the only usage in my mind is to assign a device with RMRR associated
> on VT-d (Intel GPU, or some USB controllers) where the RMRR info needs
> propagated to the guest (since identity mapping also means reservation
> of virtual address space).
Yes I think adding MEM_T_IDENTITY will be necessary. I can see they are
used for both iGPU and USB controllers on my x86 machines. Do you know
more precisely what they are used for by the firmware?
It's not necessary with the base virtio-iommu device though (v0.4),
because the device can create the identity mappings itself and report them
to the guest as MEM_T_BYPASS. However, when we start handing page table
control over to the guest, the host won't be in control of IOVA->GPA
mappings and will need to gracefully ask the guest to do it.
I'm not aware of any firmware description resembling Intel RMRR or AMD
IVMD on ARM platforms. I do think ARM platforms could need MEM_T_IDENTITY
for requesting the guest to map MSI windows when page-table handover is in
use (MSI addresses are translated by the physical SMMU, so a IOVA->GPA
mapping must be installed by the guest). But since a vSMMU would need a
solution as well, I think I'll try to implement something more generic.
> 2.6.8.2.3 Device Requirements: Property RESV_MEM
>
> --citation start--
> If an endpoint is attached to an address space, the device SHOULD leave
> any access targeting one of its VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS
> regions pass through untranslated. In other words, the device SHOULD
> handle such a region as if it was identity-mapped (virtual address equal to
> physical address). If the endpoint is not attached to any address space,
> then the device MAY abort the transaction.
> --citation end
>
> I have a question for the last sentence. From definition of BYPASS, it's
> orthogonal to whether there is an address space attached, then should
> we still allow "May abort" behavior?
The behavior is left as an implementation choice, and I'm not sure it's
worth enforcing in the architecture. If the endpoint isn't attached to any
domain then (unless VIRTIO_IOMMU_F_BYPASS is negotiated), it isn't
necessarily able to do DMA at all. The virtio-iommu device may setup DMA
mastering lazily, in which case any DMA transaction would abort, or have
setup DMA already, in which case the endpoint can access MEM_T_BYPASS regions.
Thanks!
Jean
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-06 12:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <20170906070816.s6rtgtqpljnbudux@hirez.programming.kicks-ass.net>
On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> Guys, please trim email.
>
> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>> For clarification, I was actually asking if you consider just adding one
>> more jump label to skip it for Xen/KVM instead of making
>> virt_spin_lock() a pv-op.
> I don't understand. What performance are you worried about. Native will
> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
It is not native that I am talking about. I am worry about VM with
non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
Now that function will become a callee-saved function call instead of
being inlined into the native slowpath function.
Cheers,
Longman
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox