* Re: [PATCH] drm/virtio: Remove incorrect kfree()
From: Gerd Hoffmann @ 2019-01-08 10:40 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: David Airlie, Daniel Vetter, dri-devel, virtualization
In-Reply-To: <20190102175507.4653-1-ezequiel@collabora.com>
On Wed, Jan 02, 2019 at 02:55:06PM -0300, Ezequiel Garcia wrote:
> The virtio_gpu_output is a member of struct virtio_gpu_device
> and is not a dynamically-allocated chunk, so it's wrong to kfree() it.
> Removing it fixes a memory corruption BUG() that can be triggered
> when the virtio-gpu driver is removed.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Both patches are queued up.
thanks,
Gerd
^ permalink raw reply
* Re: [RFC PATCH V3 0/5] Hi:
From: Jason Wang @ 2019-01-08 10:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, davem, linux-kernel, kvm, virtualization
In-Reply-To: <20190107094516-mutt-send-email-mst@kernel.org>
On 2019/1/7 下午10:47, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 02:58:08PM +0800, Jason Wang wrote:
>> On 2019/1/5 上午5:41, Michael S. Tsirkin wrote:
>>> On Sat, Dec 29, 2018 at 08:46:51PM +0800, Jason Wang wrote:
>>>> This series tries to access virtqueue metadata through kernel virtual
>>>> address instead of copy_user() friends since they had too much
>>>> overheads like checks, spec barriers or even hardware feature
>>>> toggling.
>>> I think it's a reasonable approach.
>>> However I need to look at whether and which mmu notifiers are invoked before
>>> writeback. Do you know?
>>
>> I don't know but just looking at the MMU notifier ops definition, there's no
>> such callback if my understanding is correct.
>>
>> Thanks
> In that case how are you making sure used ring updates are written back?
> If they aren't guest will crash ...
I think this is the writeback issue you mentioned early. I don't do a
followup on the pointer but it looks to me some work is ongoing to fix
the issue.
I can investigate it more, but it's not something new, consider the case
of VFIO.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 1/2] virtio-net: bql support
From: Jason Wang @ 2019-01-08 10:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, virtualization, maxime.coquelin, wexu,
David S. Miller
In-Reply-To: <20190107091334-mutt-send-email-mst@kernel.org>
On 2019/1/7 下午10:19, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 02:31:47PM +0800, Jason Wang wrote:
>> On 2019/1/7 下午12:01, Michael S. Tsirkin wrote:
>>> On Mon, Jan 07, 2019 at 11:51:55AM +0800, Jason Wang wrote:
>>>> On 2019/1/7 上午11:17, Michael S. Tsirkin wrote:
>>>>> On Mon, Jan 07, 2019 at 10:14:37AM +0800, Jason Wang wrote:
>>>>>> On 2019/1/2 下午9:59, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Jan 02, 2019 at 11:28:43AM +0800, Jason Wang wrote:
>>>>>>>> On 2018/12/31 上午2:45, Michael S. Tsirkin wrote:
>>>>>>>>> On Thu, Dec 27, 2018 at 06:00:36PM +0800, Jason Wang wrote:
>>>>>>>>>> On 2018/12/26 下午11:19, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Thu, Dec 06, 2018 at 04:17:36PM +0800, Jason Wang wrote:
>>>>>>>>>>>> On 2018/12/6 上午6:54, Michael S. Tsirkin wrote:
>>>>>>>>>>>>> When use_napi is set, let's enable BQLs. Note: some of the issues are
>>>>>>>>>>>>> similar to wifi. It's worth considering whether something similar to
>>>>>>>>>>>>> commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be
>>>>>>>>>>>>> benefitial.
>>>>>>>>>>>> I've played a similar patch several days before. The tricky part is the mode
>>>>>>>>>>>> switching between napi and no napi. We should make sure when the packet is
>>>>>>>>>>>> sent and trakced by BQL, it should be consumed by BQL as well. I did it by
>>>>>>>>>>>> tracking it through skb->cb. And deal with the freeze by reset the BQL
>>>>>>>>>>>> status. Patch attached.
>>>>>>>>>>>>
>>>>>>>>>>>> But when testing with vhost-net, I don't very a stable performance,
>>>>>>>>>>> So how about increasing TSQ pacing shift then?
>>>>>>>>>> I can test this. But changing default TCP value is much more than a
>>>>>>>>>> virtio-net specific thing.
>>>>>>>>> Well same logic as wifi applies. Unpredictable latencies related
>>>>>>>>> to radio in one case, to host scheduler in the other.
>>>>>>>>>
>>>>>>>>>>>> it was
>>>>>>>>>>>> probably because we batch the used ring updating so tx interrupt may come
>>>>>>>>>>>> randomly. We probably need to implement time bounded coalescing mechanism
>>>>>>>>>>>> which could be configured from userspace.
>>>>>>>>>>> I don't think it's reasonable to expect userspace to be that smart ...
>>>>>>>>>>> Why do we need time bounded? used ring is always updated when ring
>>>>>>>>>>> becomes empty.
>>>>>>>>>> We don't add used when means BQL may not see the consumed packet in time.
>>>>>>>>>> And the delay varies based on the workload since we count packets not bytes
>>>>>>>>>> or time before doing the batched updating.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>> Sorry I still don't get it.
>>>>>>>>> When nothing is outstanding then we do update the used.
>>>>>>>>> So if BQL stops userspace from sending packets then
>>>>>>>>> we get an interrupt and packets start flowing again.
>>>>>>>> Yes, but how about the cases of multiple flows. That's where I see unstable
>>>>>>>> results.
>>>>>>>>
>>>>>>>>
>>>>>>>>> It might be suboptimal, we might need to tune it but I doubt running
>>>>>>>>> timers is a solution, timer interrupts cause VM exits.
>>>>>>>> Probably not a timer but a time counter (or event byte counter) in vhost to
>>>>>>>> add used and signal guest if it exceeds a value instead of waiting the
>>>>>>>> number of packets.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>> Well we already have VHOST_NET_WEIGHT - is it too big then?
>>>>>> I'm not sure, it might be too big.
>>>>>>
>>>>>>
>>>>>>> And maybe we should expose the "MORE" flag in the descriptor -
>>>>>>> do you think that will help?
>>>>>>>
>>>>>> I don't know. But how a "more" flag can help here?
>>>>>>
>>>>>> Thanks
>>>>> It sounds like we should be a bit more aggressive in updating used ring.
>>>>> But if we just do it naively we will harm performance for sure as that
>>>>> is how we are doing batching right now.
>>>> I agree but the problem is to balance the PPS and throughput. More batching
>>>> helps for PPS but may damage TCP throughput.
>>> That is what more flag is supposed to be I think - it is only set if
>>> there's a socket that actually needs the skb freed in order to go on.
>>
>> I'm not quite sure I get, but is this something similar to what you want?
>>
>> https://lists.linuxfoundation.org/pipermail/virtualization/2014-October/027667.html
>>
>> Which enables tx interrupt for TCP packets, and you want to add used more
>> aggressively for those sockets?
>>
>>
>> Thanks
> That's the idea.
> But then you said we can just play with event index
> instead. I think the answer to why not do that is that it's tricky to do
> without races.
We don't do batched used ring update at that time. We can check whether
or not guest asking for a interrupt and add used immediately. Actually,
I've played a patch to do this. It helps a little but damage the PPS.
This is probably because we need more userspace memory accesses.
>
>
> We need to think about the exact semantics: e.g. I think it is better to
> keep interrupts on and then saying "I promise sending more buffers even
> if you do not use any buffers so using this one is not urgent" rather
> than as your patches do keeping them off and then saying "this one is
> urgent".
>
> The reason being is that "I promise to send more" is
> more informative and can allow better batching for the
> host.
Just to make sure I understand, you mean set batch flag for e.g non TCP
socket?
Thanks
>
>>>>> Instead we could make guest
>>>>> control batching using the more flag - if that's not set we write out
>>>>> the used ring.
>>>> It's under the control of guest, so I'm afraid we still need some more guard
>>>> (e.g time/bytes counters) on host.
>>>>
>>>> Thanks
>>> Point is if guest does not care about the skb being freed, then there is no
>>> rush host side to mark buffer used.
>>>
>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V3 0/5] Hi:
From: Jason Wang @ 2019-01-08 10:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, Dan Williams, davem
In-Reply-To: <20190107091947-mutt-send-email-mst@kernel.org>
On 2019/1/7 下午10:37, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 02:50:17PM +0800, Jason Wang wrote:
>> On 2019/1/7 下午12:17, Michael S. Tsirkin wrote:
>>> On Mon, Jan 07, 2019 at 11:53:41AM +0800, Jason Wang wrote:
>>>> On 2019/1/7 上午11:28, Michael S. Tsirkin wrote:
>>>>> On Mon, Jan 07, 2019 at 10:19:03AM +0800, Jason Wang wrote:
>>>>>> On 2019/1/3 上午4:47, Michael S. Tsirkin wrote:
>>>>>>> On Sat, Dec 29, 2018 at 08:46:51PM +0800, Jason Wang wrote:
>>>>>>>> This series tries to access virtqueue metadata through kernel virtual
>>>>>>>> address instead of copy_user() friends since they had too much
>>>>>>>> overheads like checks, spec barriers or even hardware feature
>>>>>>>> toggling.
>>>>>>> Will review, thanks!
>>>>>>> One questions that comes to mind is whether it's all about bypassing
>>>>>>> stac/clac. Could you please include a performance comparison with
>>>>>>> nosmap?
>>>>>>>
>>>>>> On machine without SMAP (Sandy Bridge):
>>>>>>
>>>>>> Before: 4.8Mpps
>>>>>>
>>>>>> After: 5.2Mpps
>>>>> OK so would you say it's really unsafe versus safe accesses?
>>>>> Or would you say it's just a better written code?
>>>> It's the effect of removing speculation barrier.
>>> You mean __uaccess_begin_nospec introduced by
>>> commit 304ec1b050310548db33063e567123fae8fd0301
>>> ?
>> Yes.
>>
>>
>>> So fundamentally we do access_ok checks when supplying
>>> the memory table to the kernel thread, and we should
>>> do the spec barrier there.
>>>
>>> Then we can just create and use a variant of uaccess macros that does
>>> not include the barrier?
>>
>> The unsafe ones?
> Fundamentally yes.
>
>
>>> Or, how about moving the barrier into access_ok?
>>> This way repeated accesses with a single access_ok get a bit faster.
>>> CC Dan Williams on this idea.
>>
>> The problem is, e.g for vhost control path. During mem table validation, we
>> don't even want to access them there. So the spec barrier is not needed.
> Again spec barrier is not needed as such at all. It's defence in depth.
> And mem table init is slow path. So we can stick a barrier there and it
> won't be a problem for anyone.
Consider it's a generic helper. For a deep defense we should keep it
around the places we do the real userspace memory access.
>
>>>
>>>>>> On machine with SMAP (Broadwell):
>>>>>>
>>>>>> Before: 5.0Mpps
>>>>>>
>>>>>> After: 6.1Mpps
>>>>>>
>>>>>> No smap: 7.5Mpps
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>> no smap being before or after?
>>>>>
>>>> Let me clarify:
>>>>
>>>>
>>>> Before (SMAP on): 5.0Mpps
>>>>
>>>> Before (SMAP off): 7.5Mpps
>>>>
>>>> After (SMAP on): 6.1Mpps
>>>>
>>>>
>>>> Thanks
>>> How about after + smap off?
>>
>> After (SMAP off): 8.0Mpps
>>
>>> And maybe we want a module option just for the vhost thread to keep smap
>>> off generally since almost all it does is copy stuff from userspace into
>>> kernel anyway. Because what above numbers should is that we really
>>> really want a solution that isn't limited to just meta-data access,
>>> and I really do not see how any such solution can not also be
>>> used to make meta-data access fast.
>>
>> As we've discussed in another thread of previous version. This requires lots
>> of changes, the main issues is SMAP state was not saved/restored on explicit
>> schedule().
> I wonder how expensive can reading eflags be?
> If it's cheap we can just check EFLAGS.AC and rerun stac if needed.
Probably not expensive, but consider vhost is probably the only user, is
it really worth to do this? If we do vmap + batched copy, most part of
the code were still under protection of SMAP but the performance is
almost the same. Isn't this a much better solution?
>
>> Even if it did, since vhost will call lots of net/block codes,
>> any kind of uaccess in those codes needs understand this special request
>> from vhost e.g you provably need to invent a new kinds of iov iterator that
>> does not touch SMAP at all. And I'm not sure this is the only thing we need
>> to deal with.
>
> Well we wanted to move packet processing from tun into vhost anyway right?
Yes, but how about other devices? And we should deal with zerocopy path.
It not a small amount of refactoring and work.
>
>> So I still prefer to:
>>
>> 1) speedup the metadata access through vmap + MMU notifier
>>
>> 2) speedup the datacopy with batched copy (unsafe ones or other new
>> interfaces)
>>
>> Thanks
> I just guess once you do (2) you will want to rework (1) to use
> the new interfaces.
Do you mean batching? So batched copy is much more easier, just few
codes if unsafe variants if ready or we can invent new safe variants.
But it would still be slower than vmap. And what's more, vmap does not
conflict with batching.
> So all the effort you are now investing in (1)
> will be wasted. Just my $.02.
>
Speeding up metadata access is much easier and vmap was the fastest
method. So we can benefit from it soon. Speeding up data copy requires
much more work to do. And in the future if kernel or vhost is ready for
some new API and perf numbers prove its advantage, it doesn't harm to
switch.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation
From: Christian Borntraeger @ 2019-01-08 8:46 UTC (permalink / raw)
To: Wei Wang, virtio-dev, linux-kernel, virtualization, kvm, mst,
cohuck
Cc: pasic, pbonzini, dgilbert
In-Reply-To: <5C343688.2060609@intel.com>
On 08.01.2019 06:35, Wei Wang wrote:
> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>>
>> On 07.01.2019 08:01, Wei Wang wrote:
>>> virtio-ccw has deadlock issues with reading the config space inside the
>>> interrupt context, so we tweak the virtballoon_changed implementation
>>> by moving the config read operations into the related workqueue contexts.
>>> The config_read_bitmap is used as a flag to the workqueue callbacks
>>> about the related config fields that need to be read.
>>>
>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>>> the value should be obtained via virtio_balloon_cmd_id_received.
>>>
>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>> Together with
>> virtio_pci: use queue idx instead of array idx to set up the vq
>> virtio: don't allocate vqs when names[i] = NULL
>>
>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> OK. I don't plan to send a new version of the above patches as no changes needed so far.
>
> Michael, if the above two patches look good to you, please help add the related tested-by
> and reviewed-by tags. Thanks.
Can we also make sure that
virtio_pci: use queue idx instead of array idx to set up the vq
virtio: don't allocate vqs when names[i] = NULL
also land in stable?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v4 3/3] virtio_balloon: remove the unnecessary 0-initialization
From: Greg KH @ 2019-01-08 7:10 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, mst, cohuck, linux-kernel, stable,
virtualization, pasic, pbonzini, dgilbert
In-Reply-To: <1546923005-40517-4-git-send-email-wei.w.wang@intel.com>
On Tue, Jan 08, 2019 at 12:50:05PM +0800, Wei Wang wrote:
> We've changed to kzalloc the vb struct, so no need to 0-initialize
> this field one more time.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index d48c12c..048959a 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -925,7 +925,6 @@ static int virtballoon_probe(struct virtio_device *vdev)
> VIRTIO_BALLOON_CMD_ID_STOP);
> vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> VIRTIO_BALLOON_CMD_ID_STOP);
> - vb->num_free_page_blocks = 0;
> spin_lock_init(&vb->free_page_list_lock);
> INIT_LIST_HEAD(&vb->free_page_list);
> if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> --
> 2.7.4
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply
* Re: [PATCH v4 2/3] virtio-balloon: improve update_balloon_size_func
From: Greg KH @ 2019-01-08 7:10 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, mst, cohuck, linux-kernel, stable,
virtualization, pasic, pbonzini, dgilbert
In-Reply-To: <1546923005-40517-3-git-send-email-wei.w.wang@intel.com>
On Tue, Jan 08, 2019 at 12:50:04PM +0800, Wei Wang wrote:
> There is no need to update the balloon actual register when there is no
> ballooning request. This patch avoids update_balloon_size when diff is 0.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/virtio/virtio_balloon.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 45d32f5..d48c12c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -457,9 +457,12 @@ static void update_balloon_size_func(struct work_struct *work)
> update_balloon_size_work);
> diff = towards_target(vb);
>
> + if (!diff)
> + return;
> +
> if (diff > 0)
> diff -= fill_balloon(vb, diff);
> - else if (diff < 0)
> + else
> diff += leak_balloon(vb, -diff);
> update_balloon_size(vb);
>
> --
> 2.7.4
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply
* Re: [PATCH v4 1/3] virtio-balloon: tweak config_changed implementation
From: Greg KH @ 2019-01-08 7:10 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, mst, cohuck, linux-kernel, stable,
virtualization, pasic, pbonzini, dgilbert
In-Reply-To: <1546923005-40517-2-git-send-email-wei.w.wang@intel.com>
On Tue, Jan 08, 2019 at 12:50:03PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> The cmd_id_received is also renamed to cmd_id_received_cache, and
> the value should be obtained via virtio_balloon_cmd_id_received.
>
> Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
> 1 file changed, 65 insertions(+), 33 deletions(-)
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply
* Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation
From: Wei Wang @ 2019-01-08 5:35 UTC (permalink / raw)
To: Christian Borntraeger, virtio-dev, linux-kernel, virtualization,
kvm, mst, cohuck
Cc: pasic, pbonzini, dgilbert
In-Reply-To: <4ea022d4-6b89-b481-5747-871e154dd21a@de.ibm.com>
On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>
> On 07.01.2019 08:01, Wei Wang wrote:
>> virtio-ccw has deadlock issues with reading the config space inside the
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
>> The config_read_bitmap is used as a flag to the workqueue callbacks
>> about the related config fields that need to be read.
>>
>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>> the value should be obtained via virtio_balloon_cmd_id_received.
>>
>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> Together with
> virtio_pci: use queue idx instead of array idx to set up the vq
> virtio: don't allocate vqs when names[i] = NULL
>
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
OK. I don't plan to send a new version of the above patches as no
changes needed so far.
Michael, if the above two patches look good to you, please help add the
related tested-by
and reviewed-by tags. Thanks.
Best,
Wei
^ permalink raw reply
* [PATCH v4 3/3] virtio_balloon: remove the unnecessary 0-initialization
From: Wei Wang @ 2019-01-08 4:50 UTC (permalink / raw)
To: virtio-dev, linux-kernel, stable, virtualization, kvm, mst,
cohuck
Cc: pasic, dgilbert, pbonzini
In-Reply-To: <1546923005-40517-1-git-send-email-wei.w.wang@intel.com>
We've changed to kzalloc the vb struct, so no need to 0-initialize
this field one more time.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/virtio/virtio_balloon.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index d48c12c..048959a 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -925,7 +925,6 @@ static int virtballoon_probe(struct virtio_device *vdev)
VIRTIO_BALLOON_CMD_ID_STOP);
vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
VIRTIO_BALLOON_CMD_ID_STOP);
- vb->num_free_page_blocks = 0;
spin_lock_init(&vb->free_page_list_lock);
INIT_LIST_HEAD(&vb->free_page_list);
if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
--
2.7.4
^ permalink raw reply related
* [PATCH v4 2/3] virtio-balloon: improve update_balloon_size_func
From: Wei Wang @ 2019-01-08 4:50 UTC (permalink / raw)
To: virtio-dev, linux-kernel, stable, virtualization, kvm, mst,
cohuck
Cc: pasic, dgilbert, pbonzini
In-Reply-To: <1546923005-40517-1-git-send-email-wei.w.wang@intel.com>
There is no need to update the balloon actual register when there is no
ballooning request. This patch avoids update_balloon_size when diff is 0.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
drivers/virtio/virtio_balloon.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 45d32f5..d48c12c 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -457,9 +457,12 @@ static void update_balloon_size_func(struct work_struct *work)
update_balloon_size_work);
diff = towards_target(vb);
+ if (!diff)
+ return;
+
if (diff > 0)
diff -= fill_balloon(vb, diff);
- else if (diff < 0)
+ else
diff += leak_balloon(vb, -diff);
update_balloon_size(vb);
--
2.7.4
^ permalink raw reply related
* [PATCH v4 1/3] virtio-balloon: tweak config_changed implementation
From: Wei Wang @ 2019-01-08 4:50 UTC (permalink / raw)
To: virtio-dev, linux-kernel, stable, virtualization, kvm, mst,
cohuck
Cc: pasic, dgilbert, pbonzini
In-Reply-To: <1546923005-40517-1-git-send-email-wei.w.wang@intel.com>
virtio-ccw has deadlock issues with reading the config space inside the
interrupt context, so we tweak the virtballoon_changed implementation
by moving the config read operations into the related workqueue contexts.
The config_read_bitmap is used as a flag to the workqueue callbacks
about the related config fields that need to be read.
The cmd_id_received is also renamed to cmd_id_received_cache, and
the value should be obtained via virtio_balloon_cmd_id_received.
Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
1 file changed, 65 insertions(+), 33 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..45d32f5 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -61,6 +61,10 @@ enum virtio_balloon_vq {
VIRTIO_BALLOON_VQ_MAX
};
+enum virtio_balloon_config_read {
+ VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
+};
+
struct virtio_balloon {
struct virtio_device *vdev;
struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
@@ -77,14 +81,20 @@ struct virtio_balloon {
/* Prevent updating balloon when it is being canceled. */
spinlock_t stop_update_lock;
bool stop_update;
+ /* Bitmap to indicate if reading the related config fields are needed */
+ unsigned long config_read_bitmap;
/* The list of allocated free pages, waiting to be given back to mm */
struct list_head free_page_list;
spinlock_t free_page_list_lock;
/* The number of free page blocks on the above list */
unsigned long num_free_page_blocks;
- /* The cmd id received from host */
- u32 cmd_id_received;
+ /*
+ * The cmd id received from host.
+ * Read it via virtio_balloon_cmd_id_received to get the latest value
+ * sent from host.
+ */
+ u32 cmd_id_received_cache;
/* The cmd id that is actively in use */
__virtio32 cmd_id_active;
/* Buffer to store the stop sign */
@@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
return num_returned;
}
+static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
+{
+ if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
+ return;
+
+ /* No need to queue the work if the bit was already set. */
+ if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+ &vb->config_read_bitmap))
+ return;
+
+ queue_work(vb->balloon_wq, &vb->report_free_page_work);
+}
+
static void virtballoon_changed(struct virtio_device *vdev)
{
struct virtio_balloon *vb = vdev->priv;
unsigned long flags;
- s64 diff = towards_target(vb);
-
- if (diff) {
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update)
- queue_work(system_freezable_wq,
- &vb->update_balloon_size_work);
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
- }
- if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
- virtio_cread(vdev, struct virtio_balloon_config,
- free_page_report_cmd_id, &vb->cmd_id_received);
- if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
- /* Pass ULONG_MAX to give back all the free pages */
- return_free_pages_to_mm(vb, ULONG_MAX);
- } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
- vb->cmd_id_received !=
- virtio32_to_cpu(vdev, vb->cmd_id_active)) {
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update) {
- queue_work(vb->balloon_wq,
- &vb->report_free_page_work);
- }
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
- }
+ spin_lock_irqsave(&vb->stop_update_lock, flags);
+ if (!vb->stop_update) {
+ queue_work(system_freezable_wq,
+ &vb->update_balloon_size_work);
+ virtio_balloon_queue_free_page_work(vb);
}
+ spin_unlock_irqrestore(&vb->stop_update_lock, flags);
}
static void update_balloon_size(struct virtio_balloon *vb)
@@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
return 0;
}
+static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
+{
+ if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+ &vb->config_read_bitmap))
+ virtio_cread(vb->vdev, struct virtio_balloon_config,
+ free_page_report_cmd_id,
+ &vb->cmd_id_received_cache);
+
+ return vb->cmd_id_received_cache;
+}
+
static int send_cmd_id_start(struct virtio_balloon *vb)
{
struct scatterlist sg;
@@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
while (virtqueue_get_buf(vq, &unused))
;
- vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
+ vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
+ virtio_balloon_cmd_id_received(vb));
sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
if (!err)
@@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
* stop the reporting.
*/
cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
- if (cmd_id_active != vb->cmd_id_received)
+ if (unlikely(cmd_id_active !=
+ virtio_balloon_cmd_id_received(vb)))
break;
/*
@@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
return 0;
}
-static void report_free_page_func(struct work_struct *work)
+static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
{
int err;
- struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
- report_free_page_work);
struct device *dev = &vb->vdev->dev;
/* Start by sending the received cmd id to host with an outbuf. */
@@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
dev_err(dev, "Failed to send a stop id, err = %d\n", err);
}
+static void report_free_page_func(struct work_struct *work)
+{
+ struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
+ report_free_page_work);
+ u32 cmd_id_received;
+
+ cmd_id_received = virtio_balloon_cmd_id_received(vb);
+ if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
+ /* Pass ULONG_MAX to give back all the free pages */
+ return_free_pages_to_mm(vb, ULONG_MAX);
+ } else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
+ cmd_id_received !=
+ virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
+ virtio_balloon_report_free_page(vb);
+ }
+}
+
#ifdef CONFIG_BALLOON_COMPACTION
/*
* virtballoon_migratepage - perform the balloon page migration on behalf of
@@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
goto out_del_vqs;
}
INIT_WORK(&vb->report_free_page_work, report_free_page_func);
- vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
+ vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
VIRTIO_BALLOON_CMD_ID_STOP);
vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
--
2.7.4
^ permalink raw reply related
* [PATCH v4 0/3] virtio-balloon: tweak config_changed
From: Wei Wang @ 2019-01-08 4:50 UTC (permalink / raw)
To: virtio-dev, linux-kernel, stable, virtualization, kvm, mst,
cohuck
Cc: pasic, dgilbert, pbonzini
Since virtio-ccw doesn't work with accessing to the config space
inside an interrupt context, this patch series avoids that issue by
moving the config register accesses to the related workqueue contexts.
v3->v4 ChangeLog:
- change virtio32_to_cpu to cpu_to_virtio_32 in send_cmd_id_start;
v2->v3 ChangeLog:
- rename cmd_id_received to cmd_id_received_cache, and have call sites
read the latest value via virtio_balloon_cmd_id_received. (Still
kept Cornelia and Halil's reviewed-by as it's a minor change)
- remove zeroing vb->num_free_page_blocks in probe since vb is
allocated via kzalloc.
v1->v2 ChangeLog:
- add config_read_bitmap to indicate to the workqueue callbacks about
the necessity of reading the related config fields.
Wei Wang (3):
virtio-balloon: tweak config_changed implementation
virtio-balloon: improve update_balloon_size_func
virtio_balloon: remove the unnecessary 0-initialization
drivers/virtio/virtio_balloon.c | 104 ++++++++++++++++++++++++++--------------
1 file changed, 69 insertions(+), 35 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH v15 23/26] sched: early boot clock
From: Dominique Martinet @ 2019-01-08 1:09 UTC (permalink / raw)
To: Pavel Tatashin
Cc: gnomes, feng.tang, kvm, Petr Mladek, Peter Zijlstra,
heiko.carstens, qemu-devel, virtualization, Steven Sistare, hpa,
boris.ostrovsky, prarit, linux-s390, x86, linux, Daniel Jordan,
mingo, Pavel Tatashin, John Stultz, Thomas Gleixner,
Salvatore Bonaccorso, jgross, douly.fnst, sboyd, LKML,
schwidefsky, pbonzini
In-Reply-To: <CA+CK2bD3kbcNvkVf2kiKQYs+srcw7BdWngqt_wM7Z-QsBJyH6w@mail.gmail.com>
Pavel Tatashin wrote on Mon, Jan 07, 2019:
> I did exactly the same sequence on Kaby Lake CPU and could not
> reproduce it. What is your host CPU?
skylake consumer laptop CPU: Intel(R) Core(TM) i7-6500U CPU @ 2.50GHz
I don't have any kaby lake around; I have access to older servers though...
--
Dominique
^ permalink raw reply
* Re: [PATCH v15 23/26] sched: early boot clock
From: Dominique Martinet @ 2019-01-07 23:48 UTC (permalink / raw)
To: Pavel Tatashin
Cc: gnomes, feng.tang, kvm, Petr Mladek, Peter Zijlstra,
heiko.carstens, qemu-devel, virtualization, Steven Sistare, hpa,
boris.ostrovsky, prarit, linux-s390, x86, linux, Daniel Jordan,
mingo, Pavel Tatashin, John Stultz, Thomas Gleixner,
Salvatore Bonaccorso, jgross, douly.fnst, sboyd, LKML,
schwidefsky, pbonzini
In-Reply-To: <CA+CK2bDj1f=VpT+k-e8kSSaoLJ3W9-JPPq9udFdDwd6ijXi28g@mail.gmail.com>
Pavel Tatashin wrote on Mon, Jan 07, 2019:
> I could not reproduce the problem. Did you suspend to memory between
> wake ups? Does this time jump happen every time, even if your laptop
> sleeps for a minute?
I'm not sure I understand "suspend to memory between the wake ups".
The full sequence is:
- start a VM (just in case, I let it boot till the end)
- suspend to memory (aka systemctl suspend) the host
- after resuming the host, soft reboot the VM (login through
serial/ssh/whatever and reboot or in the qemu console 'system_reset')
I've just slept exactly one minute and reproduced again with the fedora
stock kernel now (4.19.13-300.fc29.x86_64) in the VM.
Interestingly I'm not getting the same offset between multiple reboots
now despite not suspending again; but if I don't suspend I cannot seem
to get it to give an offset at all (only tried for a few minutes; this
might not be true) ; OTOH I pushed my luck further and even with a five
seconds sleep I'm getting a noticeable offset on first VM reboot after
resume:
[ 0.000000] Hypervisor detected: KVM
[ 0.000000] kvm-clock: Using msrs 4b564d01 and 4b564d00
[ 179.362163] kvm-clock: cpu 0, msr 13c01001, primary cpu clock
[ 179.362163] clocksource: kvm-clock: mask: 0xffffffffffffffff max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns
Honestly not sure what more information I could give, I'll try on some
other hardware than my laptop (if I can get a server to resume after
suspend through ipmi or wake on lan); but I don't have anything I could
install ubuntu on to try their qemu's version... although I really don't
want to believe that's the difference...
Thanks,
--
Dominique
^ permalink raw reply
* Re: [RFC PATCH V3 0/5] Hi:
From: Dan Williams @ 2019-01-07 22:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: KVM list, Netdev, Linux Kernel Mailing List, virtualization,
David Miller
In-Reply-To: <20190107172517-mutt-send-email-mst@kernel.org>
On Mon, Jan 7, 2019 at 2:25 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jan 07, 2019 at 01:39:15PM -0800, Dan Williams wrote:
> > On Mon, Jan 7, 2019 at 6:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sun, Jan 06, 2019 at 11:15:20PM -0800, Dan Williams wrote:
> > > > On Sun, Jan 6, 2019 at 8:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Mon, Jan 07, 2019 at 11:53:41AM +0800, Jason Wang wrote:
> > > > > >
> > > > > > On 2019/1/7 上午11:28, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Jan 07, 2019 at 10:19:03AM +0800, Jason Wang wrote:
> > > > > > > > On 2019/1/3 上午4:47, Michael S. Tsirkin wrote:
> > > > > > > > > On Sat, Dec 29, 2018 at 08:46:51PM +0800, Jason Wang wrote:
> > > > > > > > > > This series tries to access virtqueue metadata through kernel virtual
> > > > > > > > > > address instead of copy_user() friends since they had too much
> > > > > > > > > > overheads like checks, spec barriers or even hardware feature
> > > > > > > > > > toggling.
> > > > > > > > > Will review, thanks!
> > > > > > > > > One questions that comes to mind is whether it's all about bypassing
> > > > > > > > > stac/clac. Could you please include a performance comparison with
> > > > > > > > > nosmap?
> > > > > > > > >
> > > > > > > > On machine without SMAP (Sandy Bridge):
> > > > > > > >
> > > > > > > > Before: 4.8Mpps
> > > > > > > >
> > > > > > > > After: 5.2Mpps
> > > > > > > OK so would you say it's really unsafe versus safe accesses?
> > > > > > > Or would you say it's just a better written code?
> > > > > >
> > > > > >
> > > > > > It's the effect of removing speculation barrier.
> > > > >
> > > > >
> > > > > You mean __uaccess_begin_nospec introduced by
> > > > > commit 304ec1b050310548db33063e567123fae8fd0301
> > > > > ?
> > > > >
> > > > > So fundamentally we do access_ok checks when supplying
> > > > > the memory table to the kernel thread, and we should
> > > > > do the spec barrier there.
> > > > >
> > > > > Then we can just create and use a variant of uaccess macros that does
> > > > > not include the barrier?
> > > > >
> > > > > Or, how about moving the barrier into access_ok?
> > > > > This way repeated accesses with a single access_ok get a bit faster.
> > > > > CC Dan Williams on this idea.
> > > >
> > > > It would be interesting to see how expensive re-doing the address
> > > > limit check is compared to the speculation barrier. I.e. just switch
> > > > vhost_get_user() to use get_user() rather than __get_user(). That will
> > > > sanitize the pointer in the speculative path without a barrier.
> > >
> > > Hmm it's way cheaper even though IIRC it's measureable.
> > > Jason, would you like to try?
> > > Although frankly __get_user being slower than get_user feels very wrong.
> > > Not yet sure what to do exactly but would you agree?
> >
> > Agree. __get_user() being faster than get_user() defeats the whole
> > point of converting code paths to the access_ok() + __get_user()
> > pattern.
>
> Did you mean the reverse?
Hmm, no... I'll rephrase: __get_user() should have lower overhead than
get_user().
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V3 0/5] Hi:
From: Michael S. Tsirkin @ 2019-01-07 22:25 UTC (permalink / raw)
To: Dan Williams
Cc: KVM list, Netdev, Linux Kernel Mailing List, virtualization,
David Miller
In-Reply-To: <CAPcyv4jf-4=2oGJrdvMBeRiAJnQoKbYhwZDD+5ZEH+xgDaOzRQ@mail.gmail.com>
On Mon, Jan 07, 2019 at 01:39:15PM -0800, Dan Williams wrote:
> On Mon, Jan 7, 2019 at 6:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Jan 06, 2019 at 11:15:20PM -0800, Dan Williams wrote:
> > > On Sun, Jan 6, 2019 at 8:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, Jan 07, 2019 at 11:53:41AM +0800, Jason Wang wrote:
> > > > >
> > > > > On 2019/1/7 上午11:28, Michael S. Tsirkin wrote:
> > > > > > On Mon, Jan 07, 2019 at 10:19:03AM +0800, Jason Wang wrote:
> > > > > > > On 2019/1/3 上午4:47, Michael S. Tsirkin wrote:
> > > > > > > > On Sat, Dec 29, 2018 at 08:46:51PM +0800, Jason Wang wrote:
> > > > > > > > > This series tries to access virtqueue metadata through kernel virtual
> > > > > > > > > address instead of copy_user() friends since they had too much
> > > > > > > > > overheads like checks, spec barriers or even hardware feature
> > > > > > > > > toggling.
> > > > > > > > Will review, thanks!
> > > > > > > > One questions that comes to mind is whether it's all about bypassing
> > > > > > > > stac/clac. Could you please include a performance comparison with
> > > > > > > > nosmap?
> > > > > > > >
> > > > > > > On machine without SMAP (Sandy Bridge):
> > > > > > >
> > > > > > > Before: 4.8Mpps
> > > > > > >
> > > > > > > After: 5.2Mpps
> > > > > > OK so would you say it's really unsafe versus safe accesses?
> > > > > > Or would you say it's just a better written code?
> > > > >
> > > > >
> > > > > It's the effect of removing speculation barrier.
> > > >
> > > >
> > > > You mean __uaccess_begin_nospec introduced by
> > > > commit 304ec1b050310548db33063e567123fae8fd0301
> > > > ?
> > > >
> > > > So fundamentally we do access_ok checks when supplying
> > > > the memory table to the kernel thread, and we should
> > > > do the spec barrier there.
> > > >
> > > > Then we can just create and use a variant of uaccess macros that does
> > > > not include the barrier?
> > > >
> > > > Or, how about moving the barrier into access_ok?
> > > > This way repeated accesses with a single access_ok get a bit faster.
> > > > CC Dan Williams on this idea.
> > >
> > > It would be interesting to see how expensive re-doing the address
> > > limit check is compared to the speculation barrier. I.e. just switch
> > > vhost_get_user() to use get_user() rather than __get_user(). That will
> > > sanitize the pointer in the speculative path without a barrier.
> >
> > Hmm it's way cheaper even though IIRC it's measureable.
> > Jason, would you like to try?
> > Although frankly __get_user being slower than get_user feels very wrong.
> > Not yet sure what to do exactly but would you agree?
>
> Agree. __get_user() being faster than get_user() defeats the whole
> point of converting code paths to the access_ok() + __get_user()
> pattern.
Did you mean the reverse?
> >
> >
> > > I recall we had a convert access_ok() discussion with this result here:
> > >
> > > https://lkml.org/lkml/2018/1/17/929
> >
> > Sorry let me try to clarify. IIUC speculating access_ok once
> > is harmless. As Linus said the problem is with "_subsequent_
> > accesses that can then be used to perturb the cache".
> >
> > Thus:
> >
> > 1. if (!access_ok)
> > 2. return
> > 3. get_user
> > 4. if (!access_ok)
> > 5. return
> > 6. get_user
> >
> > Your proposal that Linus nacked was to effectively add a barrier after
> > lines 2 and 5 (also using the array_index_nospec trick for speed),
> > right? Unfortunately that needs a big API change.
> >
> > I am asking about adding barrier_nospec within access_ok.
> > Thus effectively before lines 1 and 4.
> > access_ok will be slower but after all the point of access_ok is
> > to then access the same memory multiple times.
>
> If the barrier is before lines 1 and 4 then it offers no real
> protection as far I can see. It will start speculating again on the
> user controlled pointer value after the barrier resolves.
>
> > So we should be making __get_user faster and access_ok slower ...
>
> I agree, but then the barrier always needs to be after the access_ok()
> check unconditionally called in the return path from access_ok(). At
> that point it's back to the implementation that Linus nak'd, or I'm
> missing some other detail.
>
> ...but maybe if it is limited to a new access_ok_nospec() then the
> concern is addressed? Then rename current __get_user() to
> __get_user_nospec() and make a new __get_user() that is back to being
> optimal.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V3 0/5] Hi:
From: Dan Williams @ 2019-01-07 21:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: KVM list, Netdev, Linux Kernel Mailing List, virtualization,
David Miller
In-Reply-To: <20190107084853-mutt-send-email-mst@kernel.org>
On Mon, Jan 7, 2019 at 6:11 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Jan 06, 2019 at 11:15:20PM -0800, Dan Williams wrote:
> > On Sun, Jan 6, 2019 at 8:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jan 07, 2019 at 11:53:41AM +0800, Jason Wang wrote:
> > > >
> > > > On 2019/1/7 上午11:28, Michael S. Tsirkin wrote:
> > > > > On Mon, Jan 07, 2019 at 10:19:03AM +0800, Jason Wang wrote:
> > > > > > On 2019/1/3 上午4:47, Michael S. Tsirkin wrote:
> > > > > > > On Sat, Dec 29, 2018 at 08:46:51PM +0800, Jason Wang wrote:
> > > > > > > > This series tries to access virtqueue metadata through kernel virtual
> > > > > > > > address instead of copy_user() friends since they had too much
> > > > > > > > overheads like checks, spec barriers or even hardware feature
> > > > > > > > toggling.
> > > > > > > Will review, thanks!
> > > > > > > One questions that comes to mind is whether it's all about bypassing
> > > > > > > stac/clac. Could you please include a performance comparison with
> > > > > > > nosmap?
> > > > > > >
> > > > > > On machine without SMAP (Sandy Bridge):
> > > > > >
> > > > > > Before: 4.8Mpps
> > > > > >
> > > > > > After: 5.2Mpps
> > > > > OK so would you say it's really unsafe versus safe accesses?
> > > > > Or would you say it's just a better written code?
> > > >
> > > >
> > > > It's the effect of removing speculation barrier.
> > >
> > >
> > > You mean __uaccess_begin_nospec introduced by
> > > commit 304ec1b050310548db33063e567123fae8fd0301
> > > ?
> > >
> > > So fundamentally we do access_ok checks when supplying
> > > the memory table to the kernel thread, and we should
> > > do the spec barrier there.
> > >
> > > Then we can just create and use a variant of uaccess macros that does
> > > not include the barrier?
> > >
> > > Or, how about moving the barrier into access_ok?
> > > This way repeated accesses with a single access_ok get a bit faster.
> > > CC Dan Williams on this idea.
> >
> > It would be interesting to see how expensive re-doing the address
> > limit check is compared to the speculation barrier. I.e. just switch
> > vhost_get_user() to use get_user() rather than __get_user(). That will
> > sanitize the pointer in the speculative path without a barrier.
>
> Hmm it's way cheaper even though IIRC it's measureable.
> Jason, would you like to try?
> Although frankly __get_user being slower than get_user feels very wrong.
> Not yet sure what to do exactly but would you agree?
Agree. __get_user() being faster than get_user() defeats the whole
point of converting code paths to the access_ok() + __get_user()
pattern.
>
>
> > I recall we had a convert access_ok() discussion with this result here:
> >
> > https://lkml.org/lkml/2018/1/17/929
>
> Sorry let me try to clarify. IIUC speculating access_ok once
> is harmless. As Linus said the problem is with "_subsequent_
> accesses that can then be used to perturb the cache".
>
> Thus:
>
> 1. if (!access_ok)
> 2. return
> 3. get_user
> 4. if (!access_ok)
> 5. return
> 6. get_user
>
> Your proposal that Linus nacked was to effectively add a barrier after
> lines 2 and 5 (also using the array_index_nospec trick for speed),
> right? Unfortunately that needs a big API change.
>
> I am asking about adding barrier_nospec within access_ok.
> Thus effectively before lines 1 and 4.
> access_ok will be slower but after all the point of access_ok is
> to then access the same memory multiple times.
If the barrier is before lines 1 and 4 then it offers no real
protection as far I can see. It will start speculating again on the
user controlled pointer value after the barrier resolves.
> So we should be making __get_user faster and access_ok slower ...
I agree, but then the barrier always needs to be after the access_ok()
check unconditionally called in the return path from access_ok(). At
that point it's back to the implementation that Linus nak'd, or I'm
missing some other detail.
...but maybe if it is limited to a new access_ok_nospec() then the
concern is addressed? Then rename current __get_user() to
__get_user_nospec() and make a new __get_user() that is back to being
optimal.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency
From: Michael S. Tsirkin @ 2019-01-07 19:13 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Andrea Parri, linux-doc, Peter Zijlstra, Akira Yokosawa,
Will Deacon, virtualization, David Howells, linux-arch,
Jonathan Corbet, linux-sparse, Alan Stern, Matt Turner,
Jade Alglave, Boqun Feng, Arnd Bergmann, Daniel Lustig,
Nicholas Piggin, Ivan Kokshaysky, Luc Maranget, Richard Henderson,
netdev, linux-kernel, linux-alpha, Luc Van Oostenryck
In-Reply-To: <20190107190236.GF1215@linux.ibm.com>
On Mon, Jan 07, 2019 at 11:02:36AM -0800, Paul E. McKenney wrote:
> On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> > > On Sun, Jan 06, 2019 at 11:23:07PM -0500, Michael S. Tsirkin wrote:
> > > > On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> > > > > On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> > >
> > > > > > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > > > > > + !defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > > > > > +
> > > > > > +#define dependent_ptr_mb(ptr, val) ({ \
> > > > > > + long dependent_ptr_mb_val = (long)(val); \
> > > > > > + long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val; \
> > > > > > + \
> > > > > > + BUILD_BUG_ON(sizeof(val) > sizeof(long)); \
> > > > > > + OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val); \
> > > > > > + (typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val); \
> > > > > > +})
> > > > > > +
> > > > > > +#else
> > > > > > +
> > > > > > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> > > > >
> > > > >
> > > > > So for the example of patch 4, we'd better fall back to rmb() or need a
> > > > > dependent_ptr_rmb()?
> > > > >
> > > > > Thanks
> > > >
> > > > You mean for strongly ordered architectures like Intel?
> > > > Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> > > > dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
> > > >
> > > > mb variant is unused right now so I'll remove it.
> > >
> > > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > > implications at all. The address dependency is strictly weaker than an
> > > rmb in that it will only order the two loads in qestion and not, like
> > > rmb, any prior to any later load.
> >
> > So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> >
> > 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> > the madatory one can't, so assuming we do not want
> > it to be stronger than rmp then either we want
> > smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> > or we just will specify that dependent_ptr() works for
> > both DMA and SMP.
> >
> > 2. Down the road, someone might want to order a store after a load.
> > Address dependency does that for us too. Assuming we make
> > dependent_ptr a NOP on x86, we will want an mb variant
> > which isn't a NOP on x86. Will we want to rename
> > dependent_ptr to dependent_ptr_rmb at that point?
>
> But x86 preserves store-after-load orderings anyway, and even Alpha
> respects ordering from loads to dependent stores. So what am I missing
> here?
>
> Thanx, Paul
Oh you are right. Stores are not reordered with older loads on x86.
So point 2 is moot. Sorry about the noise.
I guess at this point the only sticking point is the ECC compiler.
I'm inclined to stick an mb() there, seeing as it doesn't even
have spectre protection enabled. Slow but safe.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 2/4] include/linux/compiler.h: allow memory operands
From: Michael S. Tsirkin @ 2019-01-07 18:16 UTC (permalink / raw)
To: Will Deacon
Cc: Andrea Parri, linux-arch, Paul E. McKenney, Peter Zijlstra,
Daniel Lustig, Akira Yokosawa, linux-kernel, Nicholas Piggin,
virtualization, David Howells, linux-sparse, Alan Stern, netdev,
Luc Maranget, Jade Alglave, Boqun Feng, Luc Van Oostenryck
In-Reply-To: <20190107175427.GB22911@fuggles.cambridge.arm.com>
On Mon, Jan 07, 2019 at 05:54:27PM +0000, Will Deacon wrote:
> On Wed, Jan 02, 2019 at 03:57:54PM -0500, Michael S. Tsirkin wrote:
> > We don't really care whether the variable is in-register
> > or in-memory. Relax the constraint accordingly.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > include/linux/compiler.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 1ad367b4cd8d..6601d39e8c48 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -154,7 +154,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> > #ifndef OPTIMIZER_HIDE_VAR
> > /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> > #define OPTIMIZER_HIDE_VAR(var) \
> > - __asm__ ("" : "=r" (var) : "0" (var))
> > + __asm__ ("" : "=rm" (var) : "0" (var))
> > #endif
>
> I think this can break for architectures with write-back addressing modes
> such as arm, where the "m" constraint is assumed to be evaluated precisely
> once in the asm block.
>
> Will
Thanks, I'll drop this patch.
^ permalink raw reply
* Re: [PATCH RFC 2/4] include/linux/compiler.h: allow memory operands
From: Will Deacon @ 2019-01-07 17:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Parri, linux-arch, Paul E. McKenney, Peter Zijlstra,
Daniel Lustig, Akira Yokosawa, linux-kernel, Nicholas Piggin,
virtualization, David Howells, linux-sparse, Alan Stern, netdev,
Luc Maranget, Jade Alglave, Boqun Feng, Luc Van Oostenryck
In-Reply-To: <20190102205715.14054-3-mst@redhat.com>
On Wed, Jan 02, 2019 at 03:57:54PM -0500, Michael S. Tsirkin wrote:
> We don't really care whether the variable is in-register
> or in-memory. Relax the constraint accordingly.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/linux/compiler.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 1ad367b4cd8d..6601d39e8c48 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -154,7 +154,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> #ifndef OPTIMIZER_HIDE_VAR
> /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> #define OPTIMIZER_HIDE_VAR(var) \
> - __asm__ ("" : "=r" (var) : "0" (var))
> + __asm__ ("" : "=rm" (var) : "0" (var))
> #endif
I think this can break for architectures with write-back addressing modes
such as arm, where the "m" constraint is assumed to be evaluated precisely
once in the asm block.
Will
^ permalink raw reply
* Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency
From: Michael S. Tsirkin @ 2019-01-07 16:22 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andrea Parri, linux-doc, Akira Yokosawa, Will Deacon,
virtualization, David Howells, linux-arch, Jonathan Corbet,
linux-sparse, Alan Stern, Matt Turner, Paul E. McKenney,
Boqun Feng, Arnd Bergmann, Daniel Lustig, Nicholas Piggin,
Ivan Kokshaysky, Luc Maranget, Richard Henderson, Jade Alglave,
netdev, linux-kernel, linux-alpha, Luc Van Oostenryck
In-Reply-To: <20190107155423.GM2218@hirez.programming.kicks-ass.net>
On Mon, Jan 07, 2019 at 04:54:23PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
>
> > > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > > implications at all. The address dependency is strictly weaker than an
> > > rmb in that it will only order the two loads in qestion and not, like
> > > rmb, any prior to any later load.
> >
> > So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> >
> > 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> > the madatory one can't, so assuming we do not want
> > it to be stronger than rmp then either we want
> > smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> > or we just will specify that dependent_ptr() works for
> > both DMA and SMP.
>
> The latter; the construct simply generates dependent loads. It is up to
> the CPU as to what all that works for.
But not on intel right? On intel loads are ordered so it can be a nop.
> > 2. Down the road, someone might want to order a store after a load.
> > Address dependency does that for us too. Assuming we make
> > dependent_ptr a NOP on x86, we will want an mb variant
> > which isn't a NOP on x86. Will we want to rename
> > dependent_ptr to dependent_ptr_rmb at that point?
>
> Not sure; what is the actual overhead of the construct on x86 vs the
> NOP?
I'll have to check. There's a pipeline stall almost for sure - that's
why we put it there after all :).
--
MST
^ permalink raw reply
* Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency
From: Peter Zijlstra @ 2019-01-07 15:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Parri, linux-doc, Akira Yokosawa, Will Deacon,
virtualization, David Howells, linux-arch, Jonathan Corbet,
linux-sparse, Alan Stern, Matt Turner, Paul E. McKenney,
Boqun Feng, Arnd Bergmann, Daniel Lustig, Nicholas Piggin,
Ivan Kokshaysky, Luc Maranget, Richard Henderson, Jade Alglave,
netdev, linux-kernel, linux-alpha, Luc Van Oostenryck
In-Reply-To: <20190107082223-mutt-send-email-mst@kernel.org>
On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > implications at all. The address dependency is strictly weaker than an
> > rmb in that it will only order the two loads in qestion and not, like
> > rmb, any prior to any later load.
>
> So I'm fine with this as it's enough for virtio, but I would like to point out two things:
>
> 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> the madatory one can't, so assuming we do not want
> it to be stronger than rmp then either we want
> smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> or we just will specify that dependent_ptr() works for
> both DMA and SMP.
The latter; the construct simply generates dependent loads. It is up to
the CPU as to what all that works for.
> 2. Down the road, someone might want to order a store after a load.
> Address dependency does that for us too. Assuming we make
> dependent_ptr a NOP on x86, we will want an mb variant
> which isn't a NOP on x86. Will we want to rename
> dependent_ptr to dependent_ptr_rmb at that point?
Not sure; what is the actual overhead of the construct on x86 vs the
NOP?
^ permalink raw reply
* Re: [PATCH 1/1] s390/virtio: handle find on invalid queue gracefully
From: Cornelia Huck @ 2019-01-07 15:06 UTC (permalink / raw)
To: Halil Pasic; +Cc: linux-s390, kvm, virtualization
In-Reply-To: <20190107123147.97038-1-pasic@linux.ibm.com>
On Mon, 7 Jan 2019 13:31:46 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:
> A queue with a capacity of zero is clearly not a valid virtio queue.
> Some emulators report zero queue size if queried with an invalid queue
> index. Instead of crashing in this case let us just return -EINVAL. To
s/-EINVAL/-ENOENT/
> make that work properly, let us fix the notifier cleanup logic as well.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> This patch is motivated by commit 86a5597 "virtio-balloon:
> VIRTIO_BALLOON_F_FREE_PAGE_HINT" (Wei Wang, 2018-08-27) which triggered
> the described scenario. The emulator in question is the current QEMU.
> The problem we run into is the underflow in the following loop
> in __vring_new_virtqueue():
> for (i = 0; i < vring.num-1; i++)
> vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1)
> Namely vring.num is an unsigned int.
>
> RFC --> v1:
> * Change error code from -EINVAL to -ENOENT, so we are in line with the
> other transports.
> * Push down the detection of the error into virtio_ccw_read_vq_conf().
> ---
> drivers/s390/virtio/virtio_ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Thanks, applied.
^ permalink raw reply
* Re: [RFC PATCH V3 1/5] vhost: generalize adding used elem
From: Michael S. Tsirkin @ 2019-01-07 14:50 UTC (permalink / raw)
To: Jason Wang
Cc: kvm, netdev, linux-kernel, Sean Christopherson, virtualization,
davem
In-Reply-To: <9111a7a2-8396-d866-449e-11ee4008f988@redhat.com>
On Mon, Jan 07, 2019 at 03:00:17PM +0800, Jason Wang wrote:
>
> On 2019/1/5 上午8:33, Sean Christopherson wrote:
> > On Fri, Jan 04, 2019 at 04:29:34PM -0500, Michael S. Tsirkin wrote:
> > > On Sat, Dec 29, 2018 at 08:46:52PM +0800, Jason Wang wrote:
> > > > Use one generic vhost_copy_to_user() instead of two dedicated
> > > > accessor. This will simplify the conversion to fine grain
> > > > accessors. About 2% improvement of PPS were seen during vitio-user
> > > > txonly test.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > I don't hve a problem with this patch but do you have
> > > any idea how come removing what's supposed to be
> > > an optimization speeds things up?
> > With SMAP, the 2x vhost_put_user() will also mean an extra STAC/CLAC pair,
> > which is probably slower than the overhead of CALL+RET to whatever flavor
> > of copy_user_generic() gets used. CALL+RET is really the only overhead
> > since all variants of copy_user_generic() unroll accesses smaller than
> > 64 bytes, e.g. on a 64-bit system, __copy_to_user() will write all 8
> > bytes in a single MOV.
> >
> > Removing the special casing also eliminates a few hundred bytes of code
> > as well as the need for hardware to predict count==1 vs. count>1.
> >
>
> Yes, I don't measure, but STAC/CALC is pretty expensive when we are do very
> small copies based on the result of nosmap PPS.
>
> Thanks
Yes all this really looks like a poster child for uaccess_begin/end
plus unsafe accesses. And if these APIs don't do the job for us
then maybe better ones are needed ...
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ 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