* Re: [PATCH] drm: qxl: ratelimit pr_info message, reduce log spamming
From: Dan Carpenter @ 2017-09-12 14:09 UTC (permalink / raw)
To: Emil Velikov
Cc: David Airlie, kernel-janitors, Linux-Kernel@Vger. Kernel. Org,
ML dri-devel, open list:VIRTIO GPU DRIVER, Dave Airlie,
Colin King
In-Reply-To: <CACvgo53vbGbvejkEnQuuB-ZuVfbzvSmAsx3Ovj-wMWMQE8ihXw@mail.gmail.com>
On Tue, Sep 12, 2017 at 03:02:04PM +0100, Emil Velikov wrote:
> That said, I'm not sure how useful the information is - perhaps it's
> better to drop it all together?
Or a WARN_ONCE().
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] drm: qxl: ratelimit pr_info message, reduce log spamming
From: Emil Velikov @ 2017-09-12 14:02 UTC (permalink / raw)
To: Colin King
Cc: David Airlie, kernel-janitors, Linux-Kernel@Vger. Kernel. Org,
ML dri-devel, open list:VIRTIO GPU DRIVER, Dave Airlie
In-Reply-To: <20170912094548.30603-1-colin.king@canonical.com>
Hi Colin,
On 12 September 2017 at 10:45, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Simply mmap'ing /dev/dri/card0 repeatedly will spam the kernel
> log with qxl_mmap information messages. The following example code
> illustrates this:
>
> int main(void)
> {
> int fd = open("/dev/dri/card0", O_RDONLY);
> if (fd == -1)
> err(1, "open failed");
>
> for (;;) {
> void *m = mmap(NULL, 4096, PROT_READ,
> MAP_SHARED, fd, 0);
> if (m != MAP_FAILED)
> munmap(m, 4096);
> }
> }
>
> Reduce the spamming by ratelimiting the pr_info messages.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/gpu/drm/qxl/qxl_ttm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index 7ecf8a4b9fe6..6502e699f462 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -124,7 +124,7 @@ int qxl_mmap(struct file *filp, struct vm_area_struct *vma)
> int r;
>
> if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) {
> - pr_info("%s: vma->vm_pgoff (%ld) < DRM_FILE_PAGE_OFFSET\n",
> + pr_info_ratelimited("%s: vma->vm_pgoff (%ld) < DRM_FILE_PAGE_OFFSET\n",
Quick grep suggests that only qxl and vmwgfx produce any output in
this case, likely for the reasons you mentioned.
That said, I'm not sure how useful the information is - perhaps it's
better to drop it all together? Regardless of the route taken, vmwgfx
could use a similar fix.
HTH
Emil
^ permalink raw reply
* Re: [PATCH v15 1/5] lib/xbitmap: Introduce xbitmap
From: Wei Wang @ 2017-09-12 13:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mhocko, mgorman
In-Reply-To: <20170911125455.GA32538@bombadil.infradead.org>
On 09/11/2017 08:54 PM, Matthew Wilcox wrote:
> On Mon, Aug 28, 2017 at 06:08:29PM +0800, Wei Wang wrote:
>> From: Matthew Wilcox <mawilcox@microsoft.com>
>>
>> The eXtensible Bitmap is a sparse bitmap representation which is
>> efficient for set bits which tend to cluster. It supports up to
>> 'unsigned long' worth of bits, and this commit adds the bare bones --
>> xb_set_bit(), xb_clear_bit() and xb_test_bit().
>>
>> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
> This is quite naughty of you. You've modified the xbitmap implementation
> without any indication in the changelog that you did so.
This was changed in the previous version and included in that
v13->v14 ChangeLog: https://lkml.org/lkml/2017/8/16/923
> I don't
> think the modifications you made are an improvement, but without any
> argumentation from you I don't know why you think they're an improvement.
Probably it shouldn't be modified when the discussion is incomplete:
https://lkml.org/lkml/2017/8/10/36
Sorry about that. Hope we could get more feedback from you on the
changes later.
If you want, we can continue this part from the the v13 patch, which might
be closer to the implementation that you like:
https://lkml.org/lkml/2017/8/3/60
>> diff --git a/lib/xbitmap.c b/lib/xbitmap.c
>> new file mode 100644
>> index 0000000..8c55296
>> --- /dev/null
>> +++ b/lib/xbitmap.c
>> @@ -0,0 +1,176 @@
>> +#include <linux/slab.h>
>> +#include <linux/xbitmap.h>
>> +
>> +/*
>> + * The xbitmap implementation supports up to ULONG_MAX bits, and it is
>> + * implemented based on ida bitmaps. So, given an unsigned long index,
>> + * the high order XB_INDEX_BITS bits of the index is used to find the
>> + * corresponding item (i.e. ida bitmap) from the radix tree, and the low
>> + * order (i.e. ilog2(IDA_BITMAP_BITS)) bits of the index are indexed into
>> + * the ida bitmap to find the bit.
>> + */
>> +#define XB_INDEX_BITS (BITS_PER_LONG - ilog2(IDA_BITMAP_BITS))
>> +#define XB_MAX_PATH (DIV_ROUND_UP(XB_INDEX_BITS, \
>> + RADIX_TREE_MAP_SHIFT))
>> +#define XB_PRELOAD_SIZE (XB_MAX_PATH * 2 - 1)
> I don't understand why you moved the xb_preload code here from the
> radix tree. I want all the code which touches the preload implementation
> together in one place, which is the radix tree.
Based on the previous comments (put all the code to lib/xbitmap.c) and your
comment here, I will move xb_preload() and the above Macro to radix-tree.c,
while leaving the rest in xbitmap.c.
Would this be something you expected? Or would you like to move all back
to radix-tree.c like that in v13?
Best,
Wei
^ permalink raw reply
* [PATCH] drm: qxl: ratelimit pr_info message, reduce log spamming
From: Colin King @ 2017-09-12 9:45 UTC (permalink / raw)
To: Dave Airlie, Gerd Hoffmann, David Airlie, virtualization,
dri-devel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Simply mmap'ing /dev/dri/card0 repeatedly will spam the kernel
log with qxl_mmap information messages. The following example code
illustrates this:
int main(void)
{
int fd = open("/dev/dri/card0", O_RDONLY);
if (fd == -1)
err(1, "open failed");
for (;;) {
void *m = mmap(NULL, 4096, PROT_READ,
MAP_SHARED, fd, 0);
if (m != MAP_FAILED)
munmap(m, 4096);
}
}
Reduce the spamming by ratelimiting the pr_info messages.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/gpu/drm/qxl/qxl_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 7ecf8a4b9fe6..6502e699f462 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -124,7 +124,7 @@ int qxl_mmap(struct file *filp, struct vm_area_struct *vma)
int r;
if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) {
- pr_info("%s: vma->vm_pgoff (%ld) < DRM_FILE_PAGE_OFFSET\n",
+ pr_info_ratelimited("%s: vma->vm_pgoff (%ld) < DRM_FILE_PAGE_OFFSET\n",
__func__, vma->vm_pgoff);
return -EINVAL;
}
--
2.14.1
^ permalink raw reply related
* Re: [PATCH v15 2/5] lib/xbitmap: add xb_find_next_bit() and xb_zero()
From: Matthew Wilcox @ 2017-09-11 13:27 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mhocko, mgorman
In-Reply-To: <1503914913-28893-3-git-send-email-wei.w.wang@intel.com>
On Mon, Aug 28, 2017 at 06:08:30PM +0800, Wei Wang wrote:
> +/**
> + * xb_zero - zero a range of bits in the xbitmap
> + * @xb: the xbitmap that the bits reside in
> + * @start: the start of the range, inclusive
> + * @end: the end of the range, inclusive
> + */
> +void xb_zero(struct xb *xb, unsigned long start, unsigned long end)
> +{
> + unsigned long i;
> +
> + for (i = start; i <= end; i++)
> + xb_clear_bit(xb, i);
> +}
> +EXPORT_SYMBOL(xb_zero);
Um. This is not exactly going to be quick if you're clearing a range of bits.
I think it needs to be more along the lines of this:
void xb_clear(struct xb *xb, unsigned long start, unsigned long end)
{
struct radix_tree_root *root = &xb->xbrt;
struct radix_tree_node *node;
void **slot;
struct ida_bitmap *bitmap;
for (; end < start; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
unsigned long index = start / IDA_BITMAP_BITS;
unsigned long bit = start % IDA_BITMAP_BITS;
bitmap = __radix_tree_lookup(root, index, &node, &slot);
if (radix_tree_exception(bitmap)) {
unsigned long ebit = bit + 2;
unsigned long tmp = (unsigned long)bitmap;
if (ebit >= BITS_PER_LONG)
continue;
tmp &= ... something ...;
if (tmp == RADIX_TREE_EXCEPTIONAL_ENTRY)
__radix_tree_delete(root, node, slot);
else
rcu_assign_pointer(*slot, (void *)tmp);
} else if (bitmap) {
unsigned int nbits = end - start + 1;
if (nbits + bit > IDA_BITMAP_BITS)
nbits = IDA_BITMAP_BITS - bit;
bitmap_clear(bitmap->bitmap, bit, nbits);
if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS)) {
kfree(bitmap);
__radix_tree_delete(root, node, slot);
}
}
}
}
Also note that this should be called xb_clear(), not xb_zero() to fit
in with bitmap_clear(). And this needs a thorough test suite testing
all values for 'start' and 'end' between 0 and at least 1024; probably
much higher. And a variable number of bits need to be set before calling
xb_clear() in the test suite.
Also, this implementation above is missing a few tricks. For example,
if 'bit' is 0 and 'nbits' == IDA_BITMAP_BITS, we can simply call kfree
without first zeroing out the bits and then checking if the whole thing
is zero.
Another missing optimisation above is that we always restart the radix
tree walk from the top instead of simply moving on to the next bitmap.
This is still a thousand times faster than the implementation you posted,
but I'd be keen to add that optimisation too.
> +/**
> + * xb_find_next_bit - find next 1 or 0 in the give range of bits
> + * @xb: the xbitmap that the bits reside in
> + * @start: the start of the range, inclusive
> + * @end: the end of the range, inclusive
> + * @set: the polarity (1 or 0) of the next bit to find
> + *
> + * Return the index of the found bit in the xbitmap. If the returned index
> + * exceeds @end, it indicates that no such bit is found in the given range.
> + */
> +unsigned long xb_find_next_bit(struct xb *xb, unsigned long start,
> + unsigned long end, bool set)
> +{
> + unsigned long i;
> +
> + for (i = start; i <= end; i++) {
> + if (xb_test_bit(xb, i) == set)
> + break;
> + }
> +
> + return i;
> +}
> +EXPORT_SYMBOL(xb_find_next_bit);
Similar comments ... this is going to be very slow. You can use the
tags in the tree to help you find set and clear bits so performance
doesn't fall apart in big trees.
I'd like to see this be two functions, xb_find_next_zero_bit() and
xb_find_next_set_bit().
^ permalink raw reply
* Re: [PATCH v15 1/5] lib/xbitmap: Introduce xbitmap
From: Matthew Wilcox @ 2017-09-11 12:54 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mhocko, mgorman
In-Reply-To: <1503914913-28893-2-git-send-email-wei.w.wang@intel.com>
On Mon, Aug 28, 2017 at 06:08:29PM +0800, Wei Wang wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> The eXtensible Bitmap is a sparse bitmap representation which is
> efficient for set bits which tend to cluster. It supports up to
> 'unsigned long' worth of bits, and this commit adds the bare bones --
> xb_set_bit(), xb_clear_bit() and xb_test_bit().
>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
This is quite naughty of you. You've modified the xbitmap implementation
without any indication in the changelog that you did so. I don't
think the modifications you made are an improvement, but without any
argumentation from you I don't know why you think they're an improvement.
> diff --git a/lib/radix-tree.c b/lib/radix-tree.c
> index 898e879..ee72e2c 100644
> --- a/lib/radix-tree.c
> +++ b/lib/radix-tree.c
> @@ -496,6 +496,7 @@ static int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
> out:
> return ret;
> }
> +EXPORT_SYMBOL(__radix_tree_preload);
>
> /*
> * Load up this CPU's radix_tree_node buffer with sufficient objects to
You exported this to modules for some reason. Why?
> @@ -2003,6 +2018,7 @@ static bool __radix_tree_delete(struct radix_tree_root *root,
> replace_slot(slot, NULL, node, -1, exceptional);
> return node && delete_node(root, node, NULL, NULL);
> }
> +EXPORT_SYMBOL(__radix_tree_delete);
>
> /**
> * radix_tree_iter_delete - delete the entry at this iterator position
Ditto?
> diff --git a/lib/xbitmap.c b/lib/xbitmap.c
> new file mode 100644
> index 0000000..8c55296
> --- /dev/null
> +++ b/lib/xbitmap.c
> @@ -0,0 +1,176 @@
> +#include <linux/slab.h>
> +#include <linux/xbitmap.h>
> +
> +/*
> + * The xbitmap implementation supports up to ULONG_MAX bits, and it is
> + * implemented based on ida bitmaps. So, given an unsigned long index,
> + * the high order XB_INDEX_BITS bits of the index is used to find the
> + * corresponding item (i.e. ida bitmap) from the radix tree, and the low
> + * order (i.e. ilog2(IDA_BITMAP_BITS)) bits of the index are indexed into
> + * the ida bitmap to find the bit.
> + */
> +#define XB_INDEX_BITS (BITS_PER_LONG - ilog2(IDA_BITMAP_BITS))
> +#define XB_MAX_PATH (DIV_ROUND_UP(XB_INDEX_BITS, \
> + RADIX_TREE_MAP_SHIFT))
> +#define XB_PRELOAD_SIZE (XB_MAX_PATH * 2 - 1)
I don't understand why you moved the xb_preload code here from the
radix tree. I want all the code which touches the preload implementation
together in one place, which is the radix tree.
> +enum xb_ops {
> + XB_SET,
> + XB_CLEAR,
> + XB_TEST
> +};
> +
> +static int xb_bit_ops(struct xb *xb, unsigned long bit, enum xb_ops ops)
> +{
> + int ret = 0;
> + unsigned long index = bit / IDA_BITMAP_BITS;
> + struct radix_tree_root *root = &xb->xbrt;
> + struct radix_tree_node *node;
> + void **slot;
> + struct ida_bitmap *bitmap;
> + unsigned long ebit, tmp;
> +
> + bit %= IDA_BITMAP_BITS;
> + ebit = bit + RADIX_TREE_EXCEPTIONAL_SHIFT;
> +
> + switch (ops) {
> + case XB_SET:
> + ret = __radix_tree_create(root, index, 0, &node, &slot);
> + if (ret)
> + return ret;
> + bitmap = rcu_dereference_raw(*slot);
> + if (radix_tree_exception(bitmap)) {
> + tmp = (unsigned long)bitmap;
> + if (ebit < BITS_PER_LONG) {
> + tmp |= 1UL << ebit;
> + rcu_assign_pointer(*slot, (void *)tmp);
> + return 0;
> + }
> + bitmap = this_cpu_xchg(ida_bitmap, NULL);
> + if (!bitmap)
> + return -EAGAIN;
> + memset(bitmap, 0, sizeof(*bitmap));
> + bitmap->bitmap[0] =
> + tmp >> RADIX_TREE_EXCEPTIONAL_SHIFT;
> + rcu_assign_pointer(*slot, bitmap);
> + }
> + if (!bitmap) {
> + if (ebit < BITS_PER_LONG) {
> + bitmap = (void *)((1UL << ebit) |
> + RADIX_TREE_EXCEPTIONAL_ENTRY);
> + __radix_tree_replace(root, node, slot, bitmap,
> + NULL, NULL);
> + return 0;
> + }
> + bitmap = this_cpu_xchg(ida_bitmap, NULL);
> + if (!bitmap)
> + return -EAGAIN;
> + memset(bitmap, 0, sizeof(*bitmap));
> + __radix_tree_replace(root, node, slot, bitmap, NULL,
> + NULL);
> + }
> + __set_bit(bit, bitmap->bitmap);
> + break;
> + case XB_CLEAR:
> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
> + if (radix_tree_exception(bitmap)) {
> + tmp = (unsigned long)bitmap;
> + if (ebit >= BITS_PER_LONG)
> + return 0;
> + tmp &= ~(1UL << ebit);
> + if (tmp == RADIX_TREE_EXCEPTIONAL_ENTRY)
> + __radix_tree_delete(root, node, slot);
> + else
> + rcu_assign_pointer(*slot, (void *)tmp);
> + return 0;
> + }
> + if (!bitmap)
> + return 0;
> + __clear_bit(bit, bitmap->bitmap);
> + if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS)) {
> + kfree(bitmap);
> + __radix_tree_delete(root, node, slot);
> + }
> + break;
> + case XB_TEST:
> + bitmap = radix_tree_lookup(root, index);
> + if (!bitmap)
> + return 0;
> + if (radix_tree_exception(bitmap)) {
> + if (ebit > BITS_PER_LONG)
> + return 0;
> + return (unsigned long)bitmap & (1UL << bit);
> + }
> + ret = test_bit(bit, bitmap->bitmap);
> + break;
> + default:
> + return -EINVAL;
> + }
> + return ret;
> +}
This is what I have the biggest problem with. You've spliced
three functions together into a single 86-line function. All that
they share is the first 11 lines of setup! Go back and read
Documentation/process/coding-style.rst section 6 again.
And you've just deleted the test suite. Test suites are incredibly
important! They keep us from regressing.
^ permalink raw reply
* mm, virtio: possible OOM lockup at virtballoon_oom_notify()
From: Tetsuo Handa @ 2017-09-11 10:27 UTC (permalink / raw)
To: mst, jasowang; +Cc: linux-mm, virtualization
Hello.
I noticed that virtio_balloon is using register_oom_notifier() and
leak_balloon() from virtballoon_oom_notify() might depend on
__GFP_DIRECT_RECLAIM memory allocation.
In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
serialize against fill_balloon(). But in fill_balloon(),
alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE] implies
__GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, this allocation attempt might
depend on somebody else's __GFP_DIRECT_RECLAIM | !__GFP_NORETRY memory
allocation. Such __GFP_DIRECT_RECLAIM | !__GFP_NORETRY allocation can reach
__alloc_pages_may_oom() and hold oom_lock mutex and call out_of_memory().
And leak_balloon() is called by virtballoon_oom_notify() via
blocking_notifier_call_chain() callback when vb->balloon_lock mutex is already
held by fill_balloon(). As a result, despite __GFP_NORETRY is specified,
fill_balloon() can indirectly get stuck waiting for vb->balloon_lock mutex
at leak_balloon().
Also, in leak_balloon(), virtqueue_add_outbuf(GFP_KERNEL) is called via
tell_host(). Reaching __alloc_pages_may_oom() from this virtqueue_add_outbuf()
request from leak_balloon() from virtballoon_oom_notify() from
blocking_notifier_call_chain() from out_of_memory() leads to OOM lockup
because oom_lock mutex is already held before calling out_of_memory().
OOM notifier callback should not (directly or indirectly) depend on
__GFP_DIRECT_RECLAIM memory allocation attempt. Can you fix this dependency?
^ permalink raw reply
* Re: packed ring layout proposal v3
From: Jason Wang @ 2017-09-11 7:47 UTC (permalink / raw)
To: Michael S. Tsirkin, virtio-dev; +Cc: virtualization
In-Reply-To: <20160915223915.qjlnlvf2w7u37bu3@redhat.com>
On 2017年09月10日 13:06, Michael S. Tsirkin wrote:
> This is an update from v2 version.
> Changes:
> - update event suppression mechanism
> - add wrap counter: DESC_WRAP flag in addition to
> DESC_DRIVER flag used for validity so device does not have to
> write out all used descriptors.
Do we have benchmark result to show the advantage of DESC_DRIVER over
e.g avail/used index?
> - more options especially helpful for hardware implementations
> - in-order processing option due to popular demand
> - list of TODO items to consider as a follow-up, only two are
> open questions we need to descide now, marked as blocker,
> others are future enhancement ideas.
>
> ---
>
> Performance analysis of this is in my kvm forum 2016 presentation.
> The idea is to have a r/w descriptor in a ring structure,
> replacing the used and available ring, index and descriptor
> buffer.
>
> Note: the following mode of operation will actually work
> without changes when descriptor rings do not overlap, with driver
> writing out available entries in a read-only driver descriptor ring,
> device writing used entries in a write-only device descriptor ring.
>
> TODO: does this have any value for some? E.g. as a security feature?
>
>
> * Descriptor ring:
>
> Driver writes descriptors with unique index values and DESC_DRIVER set in flags.
You probably mean DESC_HW here?
> Descriptors are written in a ring order: from start to end of ring,
> wrapping around to the beginning.
> Device writes used descriptors with correct len, index, and DESC_HW clear.
> Again descriptors are written in ring order. This might not be the same
> order of driver descriptors, and not all descriptors have to be written out.
>
> Driver and device are expected to maintain (internally) a wrap-around
> bit, starting at 0 and changing value each time they start writing out
> descriptors at the beginning of the ring. This bit is passed as
> DESC_WRAP bit in the flags field.
I'm not sure this is really needed, I think it could be done through
checking vq.num?
>
> Flags are always set/cleared last.
>
> Note that driver can write descriptors out in any order, but device
> will not execute descriptor X+1 until descriptor X has been
> read as valid.
>
> Driver operation:
>
> Driver makes descriptors available to device by writing out descriptors
> in the descriptor ring. Once ring is full, driver waits for device to
> use some descriptors before making more available.
>
> Descriptors can be used by device in any order, but must be read from
> ring in-order, and must be read completely before starting use. Thus,
> once a descriptor is used, driver can over-write both this descriptor
> and any descriptors which preceded it in the ring.
>
> Driver can detect use of descriptor either by device specific means
> (e.g. detect a buffer data change by device) or in a generic way
> by detecting that a used buffer has been written out by device.
>
>
> Driver writes out available scatter/gather descriptor entries in guest
> descriptor format:
>
>
> #define DESC_WRAP 0x0040
> #define DESC_DRIVER 0x0080
>
> struct desc {
> __le64 addr;
> __le32 len;
> __le16 index;
> __le16 flags;
> };
>
> Fields:
>
> addr - physical address of a s/g entry
> len - length of an entry
> index - unique index. The low $\lceil log(N) \rceil - 1$
> bits of the index is a driver-supplied value which can have any value
> (under driver control). The high bits are reserved and should be
> set to 0.
Drivers usually have their own information ring, so I'm not sure
exposing such flexibility is really needed. For completion the only
hardware meaningful information is the index of the descriptor. And
DESC_WRAP could be checked implicitly through index in desc < index of
this descriptor. (Though I'm still not quite sure DESC_WRAP is needed).
>
> flags - descriptor flags.
>
> Descriptors written by driver have DESC_DRIVER set.
>
> Writing out this field makes the descriptor available for the device to use,
> so all other fields must be valid when it is written.
>
> DESC_WRAP - device uses this field to detect descriptor change by driver.
This looks a little bit confused, device in fact can check this through
DESC_HW (or DESC_DRIVER) too?
>
> Driver can use 1 bit to set direction
> /* This marks a descriptor as write-only (otherwise read-only). */
> #define VRING_DESC_F_WRITE 2
>
>
> Device operation (using descriptors):
>
> Device is looking for descriptors in ring order. After detecting that
> the flags value has changed with DESC_DRIVER set and DESC_WRAP matching
> the wrap-around counter, it can start using the descriptors.
> Descriptors can be used in any order, but must be read from ring
> in-order. In other words device must not read descriptor X after it
> started using descriptor X+1. Further, all buffer descriptors must be
> read completely before device starts using the buffer.
>
> This because once a descriptor is used, driver can over-write both this
> descriptor and any preceeding descriptors in ring.
>
> To help driver detect use of descriptors and to pass extra meta-data
> to driver, device writes out used entries in device descriptor format:
>
>
> #define DESC_WRAP 0x0040
> #define DESC_DRIVER 0x0080
>
> struct desc {
> __le64 reserved;
> __le32 len;
> __le16 index;
> __le16 flags;
> };
>
> Fields:
>
> reserved - can be any value, ignored by driver
> len - length written by device. only valid if VRING_DESC_F_WRITE is set
> len bytes of data from beginning of buffer are assumed to have been updated
> index - unique index copied from the driver descriptor that has been used.
> flags - descriptor flags.
>
> Descriptors written by device have DESC_DRIVER clear.
>
> Writing out this field notifies the driver that it can re-use the
> descriptor id. It is also a signal that driver can over-write the
> relevant descriptor (with the supplied id), and any other
>
> DESC_WRAP - driver uses this field to detect descriptor change by device.
>
> If device has used a buffer containing a write descriptor, it sets this bit:
> #define VRING_DESC_F_WRITE 2
>
> * Driver scatter/gather support
>
> Driver can use 1 bit to chain s/g entries in a request, similar to virtio 1.0:
>
> /* This marks a buffer as continuing in the next ring entry. */
> #define VRING_DESC_F_NEXT 1
>
> When driver descriptors are chained in this way, multiple
> descriptors are treated as a part of a single transaction
> containing an optional write buffer followed by an optional read buffer.
> All descriptors in the chain must have the same ID.
>
> Unlike virtio 1.0, use of this flag will be an optional feature
> so both devices and drivers can opt out of it.
> If they do, they can either negotiate indirect descriptors or use
> single-descriptor entries exclusively for buffers.
>
> Device might detect a partial descriptor chain (VRING_DESC_F_NEXT
> set but next descriptor not valid). In that case it must not
> use any parts of the chain - it will later be completed by driver,
> but device is allowed to store the valid parts of the chain as
> driver is not allowed to change them anymore.
Does it mean e.g device need to busy wait for complete chain if it found
an incomplete one? Looks suboptimal.
>
> Two options are available:
>
> Device can write out the same number of descriptors for the chain,
> setting VRING_DESC_F_NEXT for all but the last descriptor.
> Driver will ignore all used descriptors with VRING_DESC_F_NEXT bit set.
>
> Device only writes out a single descriptor for the whole chain.
> However, to keep device and driver in sync, it then skips a number of
> descriptors corresponding to the length of the chain before writing out
> the next used descriptor.
> After detecting a used descriptor driver must find out the length of the
> chain that it built in order to know where to look for the next
> device descriptor.
>
> * Indirect buffers
>
> Indirect buffer descriptors is an optional feature.
> These are always written by driver, not the device.
> Indirect buffers have a special flag bit set - like in virtio 1.0:
>
> /* This means the buffer contains a table of buffer descriptors. */
> #define VRING_DESC_F_INDIRECT 4
>
> VRING_DESC_F_WRITE and VRING_DESC_F_NEXT are always clear.
>
> len specifies the length of the indirect descriptor buffer in bytes
> and must be a multiple of 16.
>
> Unlike virtio 1.0, the buffer pointed to is a table, not a list:
> struct indirect_descriptor_table {
> /* The actual descriptors (16 bytes each) */
> struct indirect_desc desc[len / 16];
> };
>
> The first descriptor is located at start of the indirect descriptor
> table, additional indirect descriptors come immediately afterwards.
>
> struct indirect_desc {
> __le64 addr;
> __le32 len;
> __le16 reserved;
> __le16 flags;
> };
>
>
> DESC_F_WRITE is the only valid flag for descriptors in the indirect
> table. Others should be set to 0 and are ignored. reserved field is
> also set to 0 and should be ignored.
>
> TODO (blocker): virtio 1.0 allows a s/g entry followed by
> an indirect descriptor. Is this useful?
>
> This support would be an optional feature, same as in virtio 1.0
>
> * Batching descriptors:
>
> virtio 1.0 allows passing a batch of descriptors in both directions, by
> incrementing the used/avail index by values > 1.
> At the moment only batching of used descriptors is used.
>
> We can support this by chaining a list of device descriptors through
> VRING_DESC_F_MORE flag. Device sets this bit to signal
> driver that this is part of a batch of used descriptors
> which are all part of a single transaction.
If this is a part of a single transaction, I don't see obvious different
with DESC_F_NEXT?). I thought for batching, each descriptor is
independent and should belong to several different transactions. (E.g
for net, each descriptor could be an independent packet).
>
> Driver might detect a partial descriptor chain (VRING_DESC_F_MORE
> set but next descriptor not valid). In that case it must not
> use any parts of the chain - it will later be completed by device,
> but driver is allowed to store the valid parts of the chain as
> device is not allowed to change them anymore.
>
> Descriptor should not have both VRING_DESC_F_MORE and
> VRING_DESC_F_NEXT set.
>
> * Using descriptors in order
>
> Some devices can guarantee that descriptors are used in
> the order in which they were made available.
> This allows driver optimizations and can be negotiated through
> a feature bit.
>
> * Per ring flags
>
> It is useful to support features for some rings but not others.
> E.g. it's reasonable to use single buffers for RX rings but
> sg or indirect for TX rings of the network device.
> Generic configuration space will be extended so features can
> be negotiated per ring.
>
> * Selective use of descriptors
>
> As described above, descriptors with NEXT bit set are part of a
> scatter/gather chain and so do not have to cause device to write a used
> descriptor out.
>
> Similarly, driver can set a flag VRING_DESC_F_MORE in the descriptor to
> signal to device that it does not have to write out the used descriptor
> as it is part of a batch of descriptors. Device has two options (similar
> to VRING_DESC_F_NEXT):
>
> Device can write out the same number of descriptors for the batch,
> setting VRING_DESC_F_MORE for all but the last descriptor.
> Driver will ignore all used descriptors with VRING_DESC_F_MORE bit set.
>
> Device only writes out a single descriptor for the whole batch.
> However, to keep device and driver in sync, it then skips a number of
> descriptors corresponding to the length of the batch before writing out
> the next used descriptor.
> After detecting a used descriptor driver must find out the length of the
> batch that it built in order to know where to look for the next
> device descriptor.
A silly question, how can driver find out the length of the batch
effectively? Looks like it can only scan the ring until one that has
DESC_HW cleared?
>
>
> TODO (blocker): skipping descriptors for selective and
> scatter/gather seem to be only requested with in-order right now. Let's
> require in-order for this skipping? This will simplify the accounting
> by driver.
>
>
> * Interrupt/event suppression
>
> virtio 1.0 has two mechanisms for suppression but only
> one can be used at a time. we pack them together
> in a structure - one for interrupts, one for notifications:
>
> struct event {
> __le16 idx;
> __le16 flags;
> }
>
> Both fields would be optional, with a feature bit:
> VIRTIO_F_EVENT_IDX
> VIRTIO_F_EVENT_FLAGS
>
> Flags can be used like in virtio 1.0, by storing a special
> value there:
>
> #define VRING_F_EVENT_ENABLE 0x0
>
> #define VRING_F_EVENT_DISABLE 0x1
>
> Event index includes the index of the descriptor
> which should trigger the event, and the wrap counter
> in the high bit.
Not specific to v3, but looks like with event index, we can't achieve
interruptless or exitless consider idx may wrap.
>
> In that case, interrupt triggers when descriptor is written at a given
> location in the ring (or skipped in case of NEXT/MORE).
>
> If both features are negotiated, a special flags value
> can be used to switch to event idx:
>
> #define VRING_F_EVENT_IDX 0x2
>
> * Available notification
>
> Driver currently writes out the queue number to device to
> kick off ring processing.
>
> As queue number is currently 16 bit, we can extend that
> to additionally include the offset within ring of the descriptor
> which triggered the kick event in bits 16 to 30,
> and the wrap counter in the high bit (31).
>
> Device is allowed to pre-fetch descriptors beyond the specified
> offset but is not required to do so.
With DESC_HW or other flag, prefetching may introduce extra overhead I
think since it need to keep scan descriptor until DESC_HW is not set?
>
>
>
> * TODO: interrupt coalescing
>
> Does it make sense just for networking or for all devices?
> If later should we make it a per ring or a global feature?
>
>
> * TODO: event index/flags in device memory?
>
> Should we move the event index/flags to device memory?
> Might be helpful for hardware configuration so they do not
> need to do DMA reads to check whether interrupt is needed.
> OTOH maybe interrupt coalescing is sufficient for this.
>
>
> * TODO: Device specific descriptor flags
>
> We have a lot of unused space in the descriptor. This can be put to
> good use by reserving some flag bits for device use.
> For example, network device can set a bit to request
> that header in the descriptor is suppressed
> (in case it's all 0s anyway). This reduces cache utilization.
>
> Note: this feature can be supported in virtio 1.0 as well,
> as we have unused bits in both descriptor and used ring there.
I think we need try at least packing virtio-net header in the descriptor
ring.
>
> * TODO: Descriptor length in device descriptors
>
> Some devices use identically-sized buffers in all descriptors.
> Ignoring length for driver descriptors there could be an option too.
>
> * TODO: Writing at an offset
>
> Some devices might want to write into some descriptors
> at an offset, the offset would be in reserved field in the descriptor,
> possibly a descriptor flag could indicate this:
>
> #define VRING_DESC_F_OFFSET 0x0020
>
> How exactly to use the offset would be device specific,
> for example it can be used to align beginning of packet
> in the 1st buffer for mergeable buffers to cache line boundary
> while also aligning rest of buffers.
May be even more e.g NET_SKB_PAD, then we could use build_skb() for
Linux drivers.
>
> * TODO: Non power-of-2 ring sizes
>
> As the ring simply wraps around, there's no reason to
> require ring size to be power of two.
> It can be made a separate feature though.
>
>
> TODO: limits on buffer alignment/size
>
> Seems to be useful for RX for networking.
> Is there a need to negotiate above in a generic way
> or is this a networking specific optimization?
>
> TODO: expose wrap counters to device for debugging
>
> TODO: expose last avail/used offsets to device/driver for debugging
>
> TODO: ability to reset individual rings
Any actual usage of this?
Thanks
>
> ---
>
> Note: should this proposal be accepted and approved, one or more
> claims disclosed to the TC admin and listed on the Virtio TC
> IPR page https://www.oasis-open.org/committees/virtio/ipr.php
> might become Essential Claims.
> Note: the page above is unfortunately out of date and out of
> my hands. I'm in the process of updating ipr disclosures
> in github instead. Will make sure all is in place before
> this proposal is put to vote. As usual this TC operates under the
> Non-Assertion Mode of the OASIS IPR Policy, which protects
> anyone implementing the virtio spec.
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* packed ring layout proposal v3
From: Michael S. Tsirkin @ 2017-09-10 5:06 UTC (permalink / raw)
To: virtio-dev; +Cc: virtualization
This is an update from v2 version.
Changes:
- update event suppression mechanism
- add wrap counter: DESC_WRAP flag in addition to
DESC_DRIVER flag used for validity so device does not have to
write out all used descriptors.
- more options especially helpful for hardware implementations
- in-order processing option due to popular demand
- list of TODO items to consider as a follow-up, only two are
open questions we need to descide now, marked as blocker,
others are future enhancement ideas.
---
Performance analysis of this is in my kvm forum 2016 presentation.
The idea is to have a r/w descriptor in a ring structure,
replacing the used and available ring, index and descriptor
buffer.
Note: the following mode of operation will actually work
without changes when descriptor rings do not overlap, with driver
writing out available entries in a read-only driver descriptor ring,
device writing used entries in a write-only device descriptor ring.
TODO: does this have any value for some? E.g. as a security feature?
* Descriptor ring:
Driver writes descriptors with unique index values and DESC_DRIVER set in flags.
Descriptors are written in a ring order: from start to end of ring,
wrapping around to the beginning.
Device writes used descriptors with correct len, index, and DESC_HW clear.
Again descriptors are written in ring order. This might not be the same
order of driver descriptors, and not all descriptors have to be written out.
Driver and device are expected to maintain (internally) a wrap-around
bit, starting at 0 and changing value each time they start writing out
descriptors at the beginning of the ring. This bit is passed as
DESC_WRAP bit in the flags field.
Flags are always set/cleared last.
Note that driver can write descriptors out in any order, but device
will not execute descriptor X+1 until descriptor X has been
read as valid.
Driver operation:
Driver makes descriptors available to device by writing out descriptors
in the descriptor ring. Once ring is full, driver waits for device to
use some descriptors before making more available.
Descriptors can be used by device in any order, but must be read from
ring in-order, and must be read completely before starting use. Thus,
once a descriptor is used, driver can over-write both this descriptor
and any descriptors which preceded it in the ring.
Driver can detect use of descriptor either by device specific means
(e.g. detect a buffer data change by device) or in a generic way
by detecting that a used buffer has been written out by device.
Driver writes out available scatter/gather descriptor entries in guest
descriptor format:
#define DESC_WRAP 0x0040
#define DESC_DRIVER 0x0080
struct desc {
__le64 addr;
__le32 len;
__le16 index;
__le16 flags;
};
Fields:
addr - physical address of a s/g entry
len - length of an entry
index - unique index. The low $\lceil log(N) \rceil - 1$
bits of the index is a driver-supplied value which can have any value
(under driver control). The high bits are reserved and should be
set to 0.
flags - descriptor flags.
Descriptors written by driver have DESC_DRIVER set.
Writing out this field makes the descriptor available for the device to use,
so all other fields must be valid when it is written.
DESC_WRAP - device uses this field to detect descriptor change by driver.
Driver can use 1 bit to set direction
/* This marks a descriptor as write-only (otherwise read-only). */
#define VRING_DESC_F_WRITE 2
Device operation (using descriptors):
Device is looking for descriptors in ring order. After detecting that
the flags value has changed with DESC_DRIVER set and DESC_WRAP matching
the wrap-around counter, it can start using the descriptors.
Descriptors can be used in any order, but must be read from ring
in-order. In other words device must not read descriptor X after it
started using descriptor X+1. Further, all buffer descriptors must be
read completely before device starts using the buffer.
This because once a descriptor is used, driver can over-write both this
descriptor and any preceeding descriptors in ring.
To help driver detect use of descriptors and to pass extra meta-data
to driver, device writes out used entries in device descriptor format:
#define DESC_WRAP 0x0040
#define DESC_DRIVER 0x0080
struct desc {
__le64 reserved;
__le32 len;
__le16 index;
__le16 flags;
};
Fields:
reserved - can be any value, ignored by driver
len - length written by device. only valid if VRING_DESC_F_WRITE is set
len bytes of data from beginning of buffer are assumed to have been updated
index - unique index copied from the driver descriptor that has been used.
flags - descriptor flags.
Descriptors written by device have DESC_DRIVER clear.
Writing out this field notifies the driver that it can re-use the
descriptor id. It is also a signal that driver can over-write the
relevant descriptor (with the supplied id), and any other
DESC_WRAP - driver uses this field to detect descriptor change by device.
If device has used a buffer containing a write descriptor, it sets this bit:
#define VRING_DESC_F_WRITE 2
* Driver scatter/gather support
Driver can use 1 bit to chain s/g entries in a request, similar to virtio 1.0:
/* This marks a buffer as continuing in the next ring entry. */
#define VRING_DESC_F_NEXT 1
When driver descriptors are chained in this way, multiple
descriptors are treated as a part of a single transaction
containing an optional write buffer followed by an optional read buffer.
All descriptors in the chain must have the same ID.
Unlike virtio 1.0, use of this flag will be an optional feature
so both devices and drivers can opt out of it.
If they do, they can either negotiate indirect descriptors or use
single-descriptor entries exclusively for buffers.
Device might detect a partial descriptor chain (VRING_DESC_F_NEXT
set but next descriptor not valid). In that case it must not
use any parts of the chain - it will later be completed by driver,
but device is allowed to store the valid parts of the chain as
driver is not allowed to change them anymore.
Two options are available:
Device can write out the same number of descriptors for the chain,
setting VRING_DESC_F_NEXT for all but the last descriptor.
Driver will ignore all used descriptors with VRING_DESC_F_NEXT bit set.
Device only writes out a single descriptor for the whole chain.
However, to keep device and driver in sync, it then skips a number of
descriptors corresponding to the length of the chain before writing out
the next used descriptor.
After detecting a used descriptor driver must find out the length of the
chain that it built in order to know where to look for the next
device descriptor.
* Indirect buffers
Indirect buffer descriptors is an optional feature.
These are always written by driver, not the device.
Indirect buffers have a special flag bit set - like in virtio 1.0:
/* This means the buffer contains a table of buffer descriptors. */
#define VRING_DESC_F_INDIRECT 4
VRING_DESC_F_WRITE and VRING_DESC_F_NEXT are always clear.
len specifies the length of the indirect descriptor buffer in bytes
and must be a multiple of 16.
Unlike virtio 1.0, the buffer pointed to is a table, not a list:
struct indirect_descriptor_table {
/* The actual descriptors (16 bytes each) */
struct indirect_desc desc[len / 16];
};
The first descriptor is located at start of the indirect descriptor
table, additional indirect descriptors come immediately afterwards.
struct indirect_desc {
__le64 addr;
__le32 len;
__le16 reserved;
__le16 flags;
};
DESC_F_WRITE is the only valid flag for descriptors in the indirect
table. Others should be set to 0 and are ignored. reserved field is
also set to 0 and should be ignored.
TODO (blocker): virtio 1.0 allows a s/g entry followed by
an indirect descriptor. Is this useful?
This support would be an optional feature, same as in virtio 1.0
* Batching descriptors:
virtio 1.0 allows passing a batch of descriptors in both directions, by
incrementing the used/avail index by values > 1.
At the moment only batching of used descriptors is used.
We can support this by chaining a list of device descriptors through
VRING_DESC_F_MORE flag. Device sets this bit to signal
driver that this is part of a batch of used descriptors
which are all part of a single transaction.
Driver might detect a partial descriptor chain (VRING_DESC_F_MORE
set but next descriptor not valid). In that case it must not
use any parts of the chain - it will later be completed by device,
but driver is allowed to store the valid parts of the chain as
device is not allowed to change them anymore.
Descriptor should not have both VRING_DESC_F_MORE and
VRING_DESC_F_NEXT set.
* Using descriptors in order
Some devices can guarantee that descriptors are used in
the order in which they were made available.
This allows driver optimizations and can be negotiated through
a feature bit.
* Per ring flags
It is useful to support features for some rings but not others.
E.g. it's reasonable to use single buffers for RX rings but
sg or indirect for TX rings of the network device.
Generic configuration space will be extended so features can
be negotiated per ring.
* Selective use of descriptors
As described above, descriptors with NEXT bit set are part of a
scatter/gather chain and so do not have to cause device to write a used
descriptor out.
Similarly, driver can set a flag VRING_DESC_F_MORE in the descriptor to
signal to device that it does not have to write out the used descriptor
as it is part of a batch of descriptors. Device has two options (similar
to VRING_DESC_F_NEXT):
Device can write out the same number of descriptors for the batch,
setting VRING_DESC_F_MORE for all but the last descriptor.
Driver will ignore all used descriptors with VRING_DESC_F_MORE bit set.
Device only writes out a single descriptor for the whole batch.
However, to keep device and driver in sync, it then skips a number of
descriptors corresponding to the length of the batch before writing out
the next used descriptor.
After detecting a used descriptor driver must find out the length of the
batch that it built in order to know where to look for the next
device descriptor.
TODO (blocker): skipping descriptors for selective and
scatter/gather seem to be only requested with in-order right now. Let's
require in-order for this skipping? This will simplify the accounting
by driver.
* Interrupt/event suppression
virtio 1.0 has two mechanisms for suppression but only
one can be used at a time. we pack them together
in a structure - one for interrupts, one for notifications:
struct event {
__le16 idx;
__le16 flags;
}
Both fields would be optional, with a feature bit:
VIRTIO_F_EVENT_IDX
VIRTIO_F_EVENT_FLAGS
Flags can be used like in virtio 1.0, by storing a special
value there:
#define VRING_F_EVENT_ENABLE 0x0
#define VRING_F_EVENT_DISABLE 0x1
Event index includes the index of the descriptor
which should trigger the event, and the wrap counter
in the high bit.
In that case, interrupt triggers when descriptor is written at a given
location in the ring (or skipped in case of NEXT/MORE).
If both features are negotiated, a special flags value
can be used to switch to event idx:
#define VRING_F_EVENT_IDX 0x2
* Available notification
Driver currently writes out the queue number to device to
kick off ring processing.
As queue number is currently 16 bit, we can extend that
to additionally include the offset within ring of the descriptor
which triggered the kick event in bits 16 to 30,
and the wrap counter in the high bit (31).
Device is allowed to pre-fetch descriptors beyond the specified
offset but is not required to do so.
* TODO: interrupt coalescing
Does it make sense just for networking or for all devices?
If later should we make it a per ring or a global feature?
* TODO: event index/flags in device memory?
Should we move the event index/flags to device memory?
Might be helpful for hardware configuration so they do not
need to do DMA reads to check whether interrupt is needed.
OTOH maybe interrupt coalescing is sufficient for this.
* TODO: Device specific descriptor flags
We have a lot of unused space in the descriptor. This can be put to
good use by reserving some flag bits for device use.
For example, network device can set a bit to request
that header in the descriptor is suppressed
(in case it's all 0s anyway). This reduces cache utilization.
Note: this feature can be supported in virtio 1.0 as well,
as we have unused bits in both descriptor and used ring there.
* TODO: Descriptor length in device descriptors
Some devices use identically-sized buffers in all descriptors.
Ignoring length for driver descriptors there could be an option too.
* TODO: Writing at an offset
Some devices might want to write into some descriptors
at an offset, the offset would be in reserved field in the descriptor,
possibly a descriptor flag could indicate this:
#define VRING_DESC_F_OFFSET 0x0020
How exactly to use the offset would be device specific,
for example it can be used to align beginning of packet
in the 1st buffer for mergeable buffers to cache line boundary
while also aligning rest of buffers.
* TODO: Non power-of-2 ring sizes
As the ring simply wraps around, there's no reason to
require ring size to be power of two.
It can be made a separate feature though.
TODO: limits on buffer alignment/size
Seems to be useful for RX for networking.
Is there a need to negotiate above in a generic way
or is this a networking specific optimization?
TODO: expose wrap counters to device for debugging
TODO: expose last avail/used offsets to device/driver for debugging
TODO: ability to reset individual rings
---
Note: should this proposal be accepted and approved, one or more
claims disclosed to the TC admin and listed on the Virtio TC
IPR page https://www.oasis-open.org/committees/virtio/ipr.php
might become Essential Claims.
Note: the page above is unfortunately out of date and out of
my hands. I'm in the process of updating ipr disclosures
in github instead. Will make sure all is in place before
this proposal is put to vote. As usual this TC operates under the
Non-Assertion Mode of the OASIS IPR Policy, which protects
anyone implementing the virtio spec.
--
MST
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v15 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2017-09-08 11:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <20170908062748-mutt-send-email-mst@kernel.org>
On 09/08/2017 11:36 AM, Michael S. Tsirkin wrote:
> On Tue, Aug 29, 2017 at 11:09:18AM +0800, Wei Wang wrote:
>> On 08/29/2017 02:03 AM, Michael S. Tsirkin wrote:
>>> On Mon, Aug 28, 2017 at 06:08:31PM +0800, Wei Wang wrote:
>>>> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
>>>> of balloon (i.e. inflated/deflated) pages using scatter-gather lists
>>>> to the host.
>>>>
>>>> The implementation of the previous virtio-balloon is not very
>>>> efficient, because the balloon pages are transferred to the
>>>> host one by one. Here is the breakdown of the time in percentage
>>>> spent on each step of the balloon inflating process (inflating
>>>> 7GB of an 8GB idle guest).
>>>>
>>>> 1) allocating pages (6.5%)
>>>> 2) sending PFNs to host (68.3%)
>>>> 3) address translation (6.1%)
>>>> 4) madvise (19%)
>>>>
>>>> It takes about 4126ms for the inflating process to complete.
>>>> The above profiling shows that the bottlenecks are stage 2)
>>>> and stage 4).
>>>>
>>>> This patch optimizes step 2) by transferring pages to the host in
>>>> sgs. An sg describes a chunk of guest physically continuous pages.
>>>> With this mechanism, step 4) can also be optimized by doing address
>>>> translation and madvise() in chunks rather than page by page.
>>>>
>>>> With this new feature, the above ballooning process takes ~597ms
>>>> resulting in an improvement of ~86%.
>>>>
>>>> TODO: optimize stage 1) by allocating/freeing a chunk of pages
>>>> instead of a single page each time.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>> Signed-off-by: Liang Li <liang.z.li@intel.com>
>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>> drivers/virtio/virtio_balloon.c | 171 ++++++++++++++++++++++++++++++++----
>>>> include/uapi/linux/virtio_balloon.h | 1 +
>>>> 2 files changed, 155 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>>>> index f0b3a0b..8ecc1d4 100644
>>>> --- a/drivers/virtio/virtio_balloon.c
>>>> +++ b/drivers/virtio/virtio_balloon.c
>>>> @@ -32,6 +32,8 @@
>>>> #include <linux/mm.h>
>>>> #include <linux/mount.h>
>>>> #include <linux/magic.h>
>>>> +#include <linux/xbitmap.h>
>>>> +#include <asm/page.h>
>>>> /*
>>>> * Balloon device works in 4K page units. So each page is pointed to by
>>>> @@ -79,6 +81,9 @@ struct virtio_balloon {
>>>> /* Synchronize access/update to this struct virtio_balloon elements */
>>>> struct mutex balloon_lock;
>>>> + /* The xbitmap used to record balloon pages */
>>>> + struct xb page_xb;
>>>> +
>>>> /* The array of pfns we tell the Host about. */
>>>> unsigned int num_pfns;
>>>> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
>>>> @@ -141,13 +146,111 @@ static void set_page_pfns(struct virtio_balloon *vb,
>>>> page_to_balloon_pfn(page) + i);
>>>> }
>>>> +static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
>>>> +{
>>>> + struct scatterlist sg;
>>>> +
>>>> + sg_init_one(&sg, addr, size);
>>>> + return virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
>>>> +}
>>>> +
>>>> +static void send_balloon_page_sg(struct virtio_balloon *vb,
>>>> + struct virtqueue *vq,
>>>> + void *addr,
>>>> + uint32_t size,
>>>> + bool batch)
>>>> +{
>>>> + unsigned int len;
>>>> + int err;
>>>> +
>>>> + err = add_one_sg(vq, addr, size);
>>>> + /* Sanity check: this can't really happen */
>>>> + WARN_ON(err);
>>> It might be cleaner to detect that add failed due to
>>> ring full and kick then. Just an idea, up to you
>>> whether to do it.
>>>
>>>> +
>>>> + /* If batching is in use, we batch the sgs till the vq is full. */
>>>> + if (!batch || !vq->num_free) {
>>>> + virtqueue_kick(vq);
>>>> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
>>>> + /* Release all the entries if there are */
>>> Meaning
>>> Account for all used entries if any
>>> ?
>>>
>>>> + while (virtqueue_get_buf(vq, &len))
>>>> + ;
>>> Above code is reused below. Add a function?
>>>
>>>> + }
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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.
>>>> + * The page xbitmap is searched for continuous "1" bits, which correspond
>>>> + * to continuous pages, to chunk into sgs.
>>>> + *
>>>> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
>>>> + * need to be searched.
>>>> + */
>>>> +static void tell_host_sgs(struct virtio_balloon *vb,
>>>> + struct virtqueue *vq,
>>>> + unsigned long page_xb_start,
>>>> + unsigned long page_xb_end)
>>>> +{
>>>> + unsigned long sg_pfn_start, sg_pfn_end;
>>>> + void *sg_addr;
>>>> + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
>>>> +
>>>> + sg_pfn_start = page_xb_start;
>>>> + while (sg_pfn_start < page_xb_end) {
>>>> + sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
>>>> + page_xb_end, 1);
>>>> + if (sg_pfn_start == page_xb_end + 1)
>>>> + break;
>>>> + sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
>>>> + page_xb_end, 0);
>>>> + sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
>>>> + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
>>>> + while (sg_len > sg_max_len) {
>>>> + send_balloon_page_sg(vb, vq, sg_addr, sg_max_len, 1);
>>> Last argument should be true, not 1.
>>>
>>>> + sg_addr += sg_max_len;
>>>> + sg_len -= sg_max_len;
>>>> + }
>>>> + send_balloon_page_sg(vb, vq, sg_addr, sg_len, 1);
>>>> + xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
>>>> + sg_pfn_start = sg_pfn_end + 1;
>>>> + }
>>>> +
>>>> + /*
>>>> + * The last few sgs may not reach the batch size, but need a kick to
>>>> + * notify the device to handle them.
>>>> + */
>>>> + if (vq->num_free != virtqueue_get_vring_size(vq)) {
>>>> + virtqueue_kick(vq);
>>>> + wait_event(vb->acked, virtqueue_get_buf(vq, &sg_len));
>>>> + while (virtqueue_get_buf(vq, &sg_len))
>>>> + ;
>>> Some entries can get used after a pause. Looks like they will leak then?
>>> One fix would be to convert above if to a while loop.
>>> I don't know whether to do it like this in send_balloon_page_sg too.
>>>
>> Thanks for the above comments. I've re-written this part of code.
>> Please have a check below if there is anything more we could improve:
>>
>> static void kick_and_wait(struct virtqueue *vq, wait_queue_head_t wq_head)
>> {
>> unsigned int len;
>>
>> virtqueue_kick(vq);
>> wait_event(wq_head, virtqueue_get_buf(vq, &len));
>> /* Detach all the used buffers from the vq */
>> while (virtqueue_get_buf(vq, &len))
>> ;
> I would move this last part to before add_buf. Increases chances
> it succeeds even in case of a bug.
>
>> }
>>
>> static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
>> {
>> struct scatterlist sg;
>> int ret;
>>
>> sg_init_one(&sg, addr, size);
>> ret = virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
>> if (unlikely(ret == -ENOSPC))
>> dev_warn(&vq->vdev->dev, "%s: failed due to ring full\n",
>> __func__);
> So if this ever triggers then kick and wait might fail, right?
> I think you should not special-case this one then.
OK, I will remove the check above, and take other suggestions as well.
Thanks.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v15 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-09-08 3:36 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <59A4DADE.5050303@intel.com>
On Tue, Aug 29, 2017 at 11:09:18AM +0800, Wei Wang wrote:
> On 08/29/2017 02:03 AM, Michael S. Tsirkin wrote:
> > On Mon, Aug 28, 2017 at 06:08:31PM +0800, Wei Wang wrote:
> > > Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
> > > of balloon (i.e. inflated/deflated) pages using scatter-gather lists
> > > to the host.
> > >
> > > The implementation of the previous virtio-balloon is not very
> > > efficient, because the balloon pages are transferred to the
> > > host one by one. Here is the breakdown of the time in percentage
> > > spent on each step of the balloon inflating process (inflating
> > > 7GB of an 8GB idle guest).
> > >
> > > 1) allocating pages (6.5%)
> > > 2) sending PFNs to host (68.3%)
> > > 3) address translation (6.1%)
> > > 4) madvise (19%)
> > >
> > > It takes about 4126ms for the inflating process to complete.
> > > The above profiling shows that the bottlenecks are stage 2)
> > > and stage 4).
> > >
> > > This patch optimizes step 2) by transferring pages to the host in
> > > sgs. An sg describes a chunk of guest physically continuous pages.
> > > With this mechanism, step 4) can also be optimized by doing address
> > > translation and madvise() in chunks rather than page by page.
> > >
> > > With this new feature, the above ballooning process takes ~597ms
> > > resulting in an improvement of ~86%.
> > >
> > > TODO: optimize stage 1) by allocating/freeing a chunk of pages
> > > instead of a single page each time.
> > >
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/virtio/virtio_balloon.c | 171 ++++++++++++++++++++++++++++++++----
> > > include/uapi/linux/virtio_balloon.h | 1 +
> > > 2 files changed, 155 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index f0b3a0b..8ecc1d4 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -32,6 +32,8 @@
> > > #include <linux/mm.h>
> > > #include <linux/mount.h>
> > > #include <linux/magic.h>
> > > +#include <linux/xbitmap.h>
> > > +#include <asm/page.h>
> > > /*
> > > * Balloon device works in 4K page units. So each page is pointed to by
> > > @@ -79,6 +81,9 @@ struct virtio_balloon {
> > > /* Synchronize access/update to this struct virtio_balloon elements */
> > > struct mutex balloon_lock;
> > > + /* The xbitmap used to record balloon pages */
> > > + struct xb page_xb;
> > > +
> > > /* The array of pfns we tell the Host about. */
> > > unsigned int num_pfns;
> > > __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> > > @@ -141,13 +146,111 @@ static void set_page_pfns(struct virtio_balloon *vb,
> > > page_to_balloon_pfn(page) + i);
> > > }
> > > +static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
> > > +{
> > > + struct scatterlist sg;
> > > +
> > > + sg_init_one(&sg, addr, size);
> > > + return virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
> > > +}
> > > +
> > > +static void send_balloon_page_sg(struct virtio_balloon *vb,
> > > + struct virtqueue *vq,
> > > + void *addr,
> > > + uint32_t size,
> > > + bool batch)
> > > +{
> > > + unsigned int len;
> > > + int err;
> > > +
> > > + err = add_one_sg(vq, addr, size);
> > > + /* Sanity check: this can't really happen */
> > > + WARN_ON(err);
> > It might be cleaner to detect that add failed due to
> > ring full and kick then. Just an idea, up to you
> > whether to do it.
> >
> > > +
> > > + /* If batching is in use, we batch the sgs till the vq is full. */
> > > + if (!batch || !vq->num_free) {
> > > + virtqueue_kick(vq);
> > > + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > + /* Release all the entries if there are */
> > Meaning
> > Account for all used entries if any
> > ?
> >
> > > + while (virtqueue_get_buf(vq, &len))
> > > + ;
> >
> > Above code is reused below. Add a function?
> >
> > > + }
> > > +}
> > > +
> > > +/*
> > > + * 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.
> > > + * The page xbitmap is searched for continuous "1" bits, which correspond
> > > + * to continuous pages, to chunk into sgs.
> > > + *
> > > + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> > > + * need to be searched.
> > > + */
> > > +static void tell_host_sgs(struct virtio_balloon *vb,
> > > + struct virtqueue *vq,
> > > + unsigned long page_xb_start,
> > > + unsigned long page_xb_end)
> > > +{
> > > + unsigned long sg_pfn_start, sg_pfn_end;
> > > + void *sg_addr;
> > > + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
> > > +
> > > + sg_pfn_start = page_xb_start;
> > > + while (sg_pfn_start < page_xb_end) {
> > > + sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
> > > + page_xb_end, 1);
> > > + if (sg_pfn_start == page_xb_end + 1)
> > > + break;
> > > + sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
> > > + page_xb_end, 0);
> > > + sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
> > > + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
> > > + while (sg_len > sg_max_len) {
> > > + send_balloon_page_sg(vb, vq, sg_addr, sg_max_len, 1);
> > Last argument should be true, not 1.
> >
> > > + sg_addr += sg_max_len;
> > > + sg_len -= sg_max_len;
> > > + }
> > > + send_balloon_page_sg(vb, vq, sg_addr, sg_len, 1);
> > > + xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
> > > + sg_pfn_start = sg_pfn_end + 1;
> > > + }
> > > +
> > > + /*
> > > + * The last few sgs may not reach the batch size, but need a kick to
> > > + * notify the device to handle them.
> > > + */
> > > + if (vq->num_free != virtqueue_get_vring_size(vq)) {
> > > + virtqueue_kick(vq);
> > > + wait_event(vb->acked, virtqueue_get_buf(vq, &sg_len));
> > > + while (virtqueue_get_buf(vq, &sg_len))
> > > + ;
> > Some entries can get used after a pause. Looks like they will leak then?
> > One fix would be to convert above if to a while loop.
> > I don't know whether to do it like this in send_balloon_page_sg too.
> >
>
> Thanks for the above comments. I've re-written this part of code.
> Please have a check below if there is anything more we could improve:
>
> static void kick_and_wait(struct virtqueue *vq, wait_queue_head_t wq_head)
> {
> unsigned int len;
>
> virtqueue_kick(vq);
> wait_event(wq_head, virtqueue_get_buf(vq, &len));
> /* Detach all the used buffers from the vq */
> while (virtqueue_get_buf(vq, &len))
> ;
I would move this last part to before add_buf. Increases chances
it succeeds even in case of a bug.
> }
>
> static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
> {
> struct scatterlist sg;
> int ret;
>
> sg_init_one(&sg, addr, size);
> ret = virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
> if (unlikely(ret == -ENOSPC))
> dev_warn(&vq->vdev->dev, "%s: failed due to ring full\n",
> __func__);
So if this ever triggers then kick and wait might fail, right?
I think you should not special-case this one then.
>
> return ret;
> }
>
> static void send_balloon_page_sg(struct virtio_balloon *vb,
> struct virtqueue *vq,
> void *addr,
> uint32_t size,
> bool batch)
> {
> int err;
>
> do {
> err = add_one_sg(vq, addr, size);
> if (err == -ENOSPC || !batch || !vq->num_free)
> kick_and_wait(vq, vb->acked);
> } while (err == -ENOSPC);
> }
>
>
> Best,
> Wei
I think this fixes the bug, yes. I would skip trying to handle ENOSPC,
it's more a less a bug if it triggers. Just bail out on any error.
--
MST
^ permalink raw reply
* [PATCH v3 2/2] paravirt,xen: correct xen_nopvspin case
From: Juergen Gross @ 2017-09-06 17:36 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: <20170906173625.18158-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 disable the virt_spin_lock_key.
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..e8ab80ad7a6f 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -10,6 +10,7 @@
#include <linux/slab.h>
#include <asm/paravirt.h>
+#include <asm/qspinlock.h>
#include <xen/interface/xen.h>
#include <xen/events.h>
@@ -129,6 +130,7 @@ void __init xen_init_spinlocks(void)
if (!xen_pvspin) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+ static_branch_disable(&virt_spin_lock_key);
return;
}
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
--
2.12.3
^ permalink raw reply related
* [PATCH v3 1/2] paravirt/locks: use new static key for controlling call of virt_spin_lock()
From: Juergen Gross @ 2017-09-06 17:36 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: <20170906173625.18158-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.
Add a static key controlling whether virt_spin_lock() should be
called or not. When running on bare metal set the new key to false.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/qspinlock.h | 11 ++++++++++-
arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
arch/x86/kernel/smpboot.c | 2 ++
kernel/locking/qspinlock.c | 4 ++++
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 48a706f641f2..308dfd0714c7 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -1,6 +1,7 @@
#ifndef _ASM_X86_QSPINLOCK_H
#define _ASM_X86_QSPINLOCK_H
+#include <linux/jump_label.h>
#include <asm/cpufeature.h>
#include <asm-generic/qspinlock_types.h>
#include <asm/paravirt.h>
@@ -46,10 +47,14 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
#endif
#ifdef CONFIG_PARAVIRT
+DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key);
+
+void native_pv_lock_init(void) __init;
+
#define virt_spin_lock virt_spin_lock
static inline bool virt_spin_lock(struct qspinlock *lock)
{
- if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ if (!static_branch_likely(&virt_spin_lock_key))
return false;
/*
@@ -65,6 +70,10 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
return true;
}
+#else
+static inline void native_pv_lock_init(void)
+{
+}
#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 8f2d1c9d43a8..2fc65ddea40d 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -42,3 +42,9 @@ struct pv_lock_ops pv_lock_ops = {
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
+
+void __init native_pv_lock_init(void)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ static_branch_disable(&virt_spin_lock_key);
+}
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)
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 294294c71ba4..838d235b87ef 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -76,6 +76,10 @@
#define MAX_NODES 4
#endif
+#ifdef CONFIG_PARAVIRT
+DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
+#endif
+
/*
* Per-CPU queue node structures; we can never have more than 4 nested
* contexts: task, softirq, hardirq, nmi.
--
2.12.3
^ permalink raw reply related
* [PATCH v3 0/2] guard virt_spin_lock() with a static key
From: Juergen Gross @ 2017-09-06 17:36 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 guarded by a static key 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.
V3:
- remove test for hypervisor environment from virt_spin_lock(9 as
suggested by Waiman Long
V2:
- use static key instead of making virt_spin_lock() a pvops function
Juergen Gross (2):
paravirt/locks: use new static key for controlling call of
virt_spin_lock()
paravirt,xen: correct xen_nopvspin case
arch/x86/include/asm/qspinlock.h | 11 ++++++++++-
arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
arch/x86/kernel/smpboot.c | 2 ++
arch/x86/xen/spinlock.c | 2 ++
kernel/locking/qspinlock.c | 4 ++++
5 files changed, 24 insertions(+), 1 deletion(-)
--
2.12.3
^ permalink raw reply
* Re: [PATCH v2 1/2] paravirt/locks: use new static key for controlling call of virt_spin_lock()
From: Juergen Gross @ 2017-09-06 17:33 UTC (permalink / raw)
To: Waiman Long, Peter Zijlstra
Cc: jeremy, x86, linux-kernel, virtualization, chrisw, mingo, tglx,
hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <73d39808-ad47-6d06-89be-034181099408@redhat.com>
On 06/09/17 18:13, Waiman Long wrote:
> On 09/06/2017 12:04 PM, Peter Zijlstra wrote:
>> On Wed, Sep 06, 2017 at 11:49:49AM -0400, Waiman Long wrote:
>>>> #define virt_spin_lock virt_spin_lock
>>>> static inline bool virt_spin_lock(struct qspinlock *lock)
>>>> {
>>>> + if (!static_branch_likely(&virt_spin_lock_key))
>>>> + return false;
>>>> if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>>> return false;
>>>>
>> Now native has two NOPs instead of one. Can't we merge these two static
>> branches?
>
>
> I guess we can remove the static_cpu_has() call. Just that any spin_lock
> calls before native_pv_lock_init() will use the virt_spin_lock(). That
> is still OK as the init call is done before SMP starts.
Hmm, right. I'll send V3 in some minutes.
Juergen
^ permalink raw reply
* Re: [PATCH v2 1/2] paravirt/locks: use new static key for controlling call of virt_spin_lock()
From: Waiman Long @ 2017-09-06 16:13 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: <20170906160415.crt2ajuf6553bit7@hirez.programming.kicks-ass.net>
On 09/06/2017 12:04 PM, Peter Zijlstra wrote:
> On Wed, Sep 06, 2017 at 11:49:49AM -0400, Waiman Long wrote:
>>> #define virt_spin_lock virt_spin_lock
>>> static inline bool virt_spin_lock(struct qspinlock *lock)
>>> {
>>> + if (!static_branch_likely(&virt_spin_lock_key))
>>> + return false;
>>> if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> return false;
>>>
> Now native has two NOPs instead of one. Can't we merge these two static
> branches?
I guess we can remove the static_cpu_has() call. Just that any spin_lock
calls before native_pv_lock_init() will use the virt_spin_lock(). That
is still OK as the init call is done before SMP starts.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v2 1/2] paravirt/locks: use new static key for controlling call of virt_spin_lock()
From: Peter Zijlstra @ 2017-09-06 16:04 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: <a0cf01cf-922a-dbc7-949e-0f87bcdd1ad9@redhat.com>
On Wed, Sep 06, 2017 at 11:49:49AM -0400, Waiman Long wrote:
> > #define virt_spin_lock virt_spin_lock
> > static inline bool virt_spin_lock(struct qspinlock *lock)
> > {
> > + if (!static_branch_likely(&virt_spin_lock_key))
> > + return false;
> > if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> > return false;
> >
Now native has two NOPs instead of one. Can't we merge these two static
branches?
^ permalink raw reply
* Re: [PATCH v2 2/2] paravirt,xen: correct xen_nopvspin case
From: Waiman Long @ 2017-09-06 15:50 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: <20170906152928.8658-3-jgross@suse.com>
On 09/06/2017 11:29 AM, Juergen Gross wrote:
> 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 disable the virt_spin_lock_key.
>
> 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..e8ab80ad7a6f 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -10,6 +10,7 @@
> #include <linux/slab.h>
>
> #include <asm/paravirt.h>
> +#include <asm/qspinlock.h>
>
> #include <xen/interface/xen.h>
> #include <xen/events.h>
> @@ -129,6 +130,7 @@ void __init xen_init_spinlocks(void)
>
> if (!xen_pvspin) {
> printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
> + static_branch_disable(&virt_spin_lock_key);
> return;
> }
> printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
Acked-by: Waiman Long <longman@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] paravirt/locks: use new static key for controlling call of virt_spin_lock()
From: Waiman Long @ 2017-09-06 15:49 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: <20170906152928.8658-2-jgross@suse.com>
On 09/06/2017 11:29 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.
>
> Add a static key controlling whether virt_spin_lock() should be
> called or not. When running on bare metal set the new key to false.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> arch/x86/include/asm/qspinlock.h | 11 +++++++++++
> arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
> arch/x86/kernel/smpboot.c | 2 ++
> kernel/locking/qspinlock.c | 4 ++++
> 4 files changed, 23 insertions(+)
>
> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
> index 48a706f641f2..fc39389f196b 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -1,6 +1,7 @@
> #ifndef _ASM_X86_QSPINLOCK_H
> #define _ASM_X86_QSPINLOCK_H
>
> +#include <linux/jump_label.h>
> #include <asm/cpufeature.h>
> #include <asm-generic/qspinlock_types.h>
> #include <asm/paravirt.h>
> @@ -46,9 +47,15 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
> #endif
>
> #ifdef CONFIG_PARAVIRT
> +DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key);
> +
> +void native_pv_lock_init(void) __init;
> +
> #define virt_spin_lock virt_spin_lock
> static inline bool virt_spin_lock(struct qspinlock *lock)
> {
> + if (!static_branch_likely(&virt_spin_lock_key))
> + return false;
> if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> return false;
>
> @@ -65,6 +72,10 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
>
> return true;
> }
> +#else
> +static inline void native_pv_lock_init(void)
> +{
> +}
> #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 8f2d1c9d43a8..2fc65ddea40d 100644
> --- a/arch/x86/kernel/paravirt-spinlocks.c
> +++ b/arch/x86/kernel/paravirt-spinlocks.c
> @@ -42,3 +42,9 @@ struct pv_lock_ops pv_lock_ops = {
> #endif /* SMP */
> };
> EXPORT_SYMBOL(pv_lock_ops);
> +
> +void __init native_pv_lock_init(void)
> +{
> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> + static_branch_disable(&virt_spin_lock_key);
> +}
> 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)
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index 294294c71ba4..838d235b87ef 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -76,6 +76,10 @@
> #define MAX_NODES 4
> #endif
>
> +#ifdef CONFIG_PARAVIRT
> +DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
> +#endif
> +
> /*
> * Per-CPU queue node structures; we can never have more than 4 nested
> * contexts: task, softirq, hardirq, nmi.
Acked-by: Waiman Long <longman@redhat.com>
^ permalink raw reply
* [PATCH v2 2/2] paravirt,xen: correct xen_nopvspin case
From: Juergen Gross @ 2017-09-06 15:29 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: <20170906152928.8658-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 disable the virt_spin_lock_key.
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..e8ab80ad7a6f 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -10,6 +10,7 @@
#include <linux/slab.h>
#include <asm/paravirt.h>
+#include <asm/qspinlock.h>
#include <xen/interface/xen.h>
#include <xen/events.h>
@@ -129,6 +130,7 @@ void __init xen_init_spinlocks(void)
if (!xen_pvspin) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+ static_branch_disable(&virt_spin_lock_key);
return;
}
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
--
2.12.3
^ permalink raw reply related
* [PATCH v2 1/2] paravirt/locks: use new static key for controlling call of virt_spin_lock()
From: Juergen Gross @ 2017-09-06 15:29 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: <20170906152928.8658-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.
Add a static key controlling whether virt_spin_lock() should be
called or not. When running on bare metal set the new key to false.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/qspinlock.h | 11 +++++++++++
arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
arch/x86/kernel/smpboot.c | 2 ++
kernel/locking/qspinlock.c | 4 ++++
4 files changed, 23 insertions(+)
diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 48a706f641f2..fc39389f196b 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -1,6 +1,7 @@
#ifndef _ASM_X86_QSPINLOCK_H
#define _ASM_X86_QSPINLOCK_H
+#include <linux/jump_label.h>
#include <asm/cpufeature.h>
#include <asm-generic/qspinlock_types.h>
#include <asm/paravirt.h>
@@ -46,9 +47,15 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
#endif
#ifdef CONFIG_PARAVIRT
+DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key);
+
+void native_pv_lock_init(void) __init;
+
#define virt_spin_lock virt_spin_lock
static inline bool virt_spin_lock(struct qspinlock *lock)
{
+ if (!static_branch_likely(&virt_spin_lock_key))
+ return false;
if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
return false;
@@ -65,6 +72,10 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
return true;
}
+#else
+static inline void native_pv_lock_init(void)
+{
+}
#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 8f2d1c9d43a8..2fc65ddea40d 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -42,3 +42,9 @@ struct pv_lock_ops pv_lock_ops = {
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
+
+void __init native_pv_lock_init(void)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ static_branch_disable(&virt_spin_lock_key);
+}
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)
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 294294c71ba4..838d235b87ef 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -76,6 +76,10 @@
#define MAX_NODES 4
#endif
+#ifdef CONFIG_PARAVIRT
+DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
+#endif
+
/*
* Per-CPU queue node structures; we can never have more than 4 nested
* contexts: task, softirq, hardirq, nmi.
--
2.12.3
^ permalink raw reply related
* [PATCH v2 0/2] guard virt_spin_lock() with a static key
From: Juergen Gross @ 2017-09-06 15:29 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 guarded by a static key 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.
V2:
- use static key instead of making virt_spin_lock() a pvops function
Juergen Gross (2):
paravirt/locks: use new static key for controlling call of
virt_spin_lock()
paravirt,xen: correct xen_nopvspin case
arch/x86/include/asm/qspinlock.h | 11 +++++++++++
arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
arch/x86/kernel/smpboot.c | 2 ++
arch/x86/xen/spinlock.c | 2 ++
kernel/locking/qspinlock.c | 4 ++++
5 files changed, 25 insertions(+)
--
2.12.3
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Waiman Long @ 2017-09-06 13:11 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: <20170906130600.2dke3ntcw2i5fss2@hirez.programming.kicks-ass.net>
On 09/06/2017 09:06 AM, Peter Zijlstra wrote:
> On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
>> 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.
> But only if we actually end up using the test-and-set thing, because if
> you have paravirt we end up using that.
I am talking about scenario like a kernel with paravirt spinlock running
in a VM managed by VMware, for example. We may not want to penalize them
while there are alternatives with less penalty.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
From: Peter Zijlstra @ 2017-09-06 13:06 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: <dd4511e9-3730-913b-9a80-59476661f3c8@redhat.com>
On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
> 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.
But only if we actually end up using the test-and-set thing, because if
you have paravirt we end up using that.
And the test-and-set thing sucks anyway. But yes, you're right, that
case gets worse.
^ 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