* Re: [PATCH v3 0/4] implement vcpu preempted check
From: Peter Zijlstra @ 2016-09-29 10:31 UTC (permalink / raw)
To: Christian Borntraeger
Cc: kernellwp, linux-s390, benh, jgross, kvm, Pan Xinhui, will.deacon,
linux-kernel, Heiko Carstens, virtualization, mingo, paulus, mpe,
xen-devel-request, pbonzini, paulmck, linuxppc-dev
In-Reply-To: <166f3bad-f700-4624-6c1c-996f90ad609c@de.ibm.com>
On Thu, Sep 29, 2016 at 12:23:19PM +0200, Christian Borntraeger wrote:
> On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
> > On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
> >> change from v2:
> >> no code change, fix typos, update some comments
> >>
> >> change from v1:
> >> a simplier definition of default vcpu_is_preempted
> >> skip mahcine type check on ppc, and add config. remove dedicated macro.
> >> add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner.
> >> add more comments
> >> thanks boqun and Peter's suggestion.
> >>
> >> This patch set aims to fix lock holder preemption issues.
> >
> > So I really like the concept, but I would also really like to see
> > support for more hypervisors included before we can move forward with
> > this.
> >
> > Please consider s390 and (x86/arm) KVM. Once we have a few, more can
> > follow later, but I think its important to not only have PPC support for
> > this.
>
> Actually the s390 preemted check via sigp sense running is available for
> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
> longer buy s390 systems without LPAR.
>
> As Heiko already pointed out we could simply use a small inline function
> that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)
Sure, and I had vague memories of Heiko's email. This patch set however
completely fails to do that trivial hooking up.
^ permalink raw reply
* Re: [PATCH v3 0/4] implement vcpu preempted check
From: Christian Borntraeger @ 2016-09-29 10:23 UTC (permalink / raw)
To: Peter Zijlstra, Pan Xinhui
Cc: kernellwp, linux-s390, jgross, kvm, xen-devel-request,
will.deacon, linux-kernel, Heiko Carstens, virtualization, mingo,
paulus, mpe, benh, pbonzini, paulmck, linuxppc-dev
In-Reply-To: <20160929101040.GV5016@twins.programming.kicks-ass.net>
On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
> On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
>> change from v2:
>> no code change, fix typos, update some comments
>>
>> change from v1:
>> a simplier definition of default vcpu_is_preempted
>> skip mahcine type check on ppc, and add config. remove dedicated macro.
>> add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner.
>> add more comments
>> thanks boqun and Peter's suggestion.
>>
>> This patch set aims to fix lock holder preemption issues.
>
> So I really like the concept, but I would also really like to see
> support for more hypervisors included before we can move forward with
> this.
>
> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
> follow later, but I think its important to not only have PPC support for
> this.
Actually the s390 preemted check via sigp sense running is available for
all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
longer buy s390 systems without LPAR.
As Heiko already pointed out we could simply use a small inline function
that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)
Christian
^ permalink raw reply
* [PATCH v2 1/1] virtio: update balloon size in balloon "probe"
From: Denis V. Lunev @ 2016-09-29 10:17 UTC (permalink / raw)
To: virtualization, linux-kernel; +Cc: den, Konstantin Neumoin, Michael S . Tsirkin
From: Konstantin Neumoin <kneumoin@virtuozzo.com>
The following commit 'fad7b7b27b6a (virtio_balloon: Use a workqueue
instead of "vballoon" kthread)' has added a regression. Original code with
kthread starts the thread inside probe and checks the necessity to update
balloon inside the thread immediately.
Nowadays the code behaves differently. Work is queued only on the first
command from the host after the negotiation. Thus there is a window
especially at the guest startup or the module reloading when the balloon
size is not updated until the notification from the host.
This patch adds balloon size check at the end of the probe to match
original behaviour.
Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_balloon.c | 2 ++
1 file changed, 2 insertions(+)
Changes from v1:
- fixed description
- removed update_balloon_size() call
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e7003d..181793f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -577,6 +577,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
virtio_device_ready(vdev);
+ if (towards_target(vb))
+ virtballoon_changed(vdev);
return 0;
out_del_vqs:
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 0/4] implement vcpu preempted check
From: Peter Zijlstra @ 2016-09-29 10:10 UTC (permalink / raw)
To: Pan Xinhui
Cc: kernellwp, linux-s390, jgross, kvm, xen-devel-request,
will.deacon, linux-kernel, virtualization, mingo, paulus, mpe,
benh, pbonzini, paulmck, linuxppc-dev
In-Reply-To: <1469101514-49475-1-git-send-email-xinhui.pan@linux.vnet.ibm.com>
On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
> change from v2:
> no code change, fix typos, update some comments
>
> change from v1:
> a simplier definition of default vcpu_is_preempted
> skip mahcine type check on ppc, and add config. remove dedicated macro.
> add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner.
> add more comments
> thanks boqun and Peter's suggestion.
>
> This patch set aims to fix lock holder preemption issues.
So I really like the concept, but I would also really like to see
support for more hypervisors included before we can move forward with
this.
Please consider s390 and (x86/arm) KVM. Once we have a few, more can
follow later, but I think its important to not only have PPC support for
this.
^ permalink raw reply
* Re: [PATCH 1/1] update balloon size in balloon "probe"
From: Konstantin Neumoin @ 2016-09-28 14:03 UTC (permalink / raw)
To: Michael S. Tsirkin, Denis V. Lunev; +Cc: linux-kernel, virtualization
In-Reply-To: <c41cf1cf-35e5-1f31-0d2c-9864e6012ac9@virtuozzo.com>
On 09/26/2016 10:16 AM, Konstantin Neumoin wrote:
> On 09/23/2016 09:58 PM, Michael S. Tsirkin wrote:
>> On Fri, Sep 23, 2016 at 04:47:57PM +0300, Denis V. Lunev wrote:
>>> From: Konstantin Neumoin <kneumoin@virtuozzo.com>
>>>
>>> Patch
>>> Commit 3d2a3774c1b046f548ebea0391a602fd5685a307
>>> Author: Michael S. Tsirkin <mst@redhat.com>
>>> Date: Tue Mar 10 11:55:08 2015 +1030
>>> virtio-balloon: do not call blocking ops when !TASK_RUNNING
>>> has added a regression. Original code with wait_event_interruptible
>>> checked the condition before start waiting and started balloon
>>> operations
>>> if necessary.
>> I don't get it, sorry.
>>
>> + add_wait_queue(&vb->config_change, &wait);
>> + for (;;) {
>> + if ((diff = towards_target(vb)) != 0 ||
>> + vb->need_stats_update ||
>> + kthread_should_stop() ||
>> + freezing(current))
>> + break;
>> + wait_woken(&wait, TASK_INTERRUPTIBLE,
>> MAX_SCHEDULE_TIMEOUT);
>> + }
>> + remove_wait_queue(&vb->config_change, &wait);
>>
>> Seems to check the condition before waiting.
>>
>> The issue is more likely with this patch:
>>
>> commit fad7b7b27b6a168ca8ebc84482043886f837b89d
>> Author: Petr Mladek <pmladek@suse.com>
>> Date: Mon Jan 25 17:38:05 2016 +0100
>>
>> virtio_balloon: Use a workqueue instead of "vballoon" kthread
> yes, you are right, sorry, commit message is incorrect.
>>
>>
>>> Right now balloon is not inflated if ballon target is set before the
>>> driver is loaded.
>>>
>>> Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: "Michael S. Tsirkin" <mst@redhat.com>
>>> ---
>>> drivers/virtio/virtio_balloon.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_balloon.c
>>> b/drivers/virtio/virtio_balloon.c
>>> index 4e7003d..0a6c10f 100644
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -577,6 +577,10 @@ static int virtballoon_probe(struct
>>> virtio_device *vdev)
>>> virtio_device_ready(vdev);
>>> + if (towards_target(vb))
>>> + virtballoon_changed(vdev);
>>> + update_balloon_size(vb);
>>> +
>>> return 0;
>>> out_del_vqs:
>>
>> I know we have same thing on restore, but it seems bogus
>> there as well:
>> if (towards_target(vb))
>> virtballoon_changed(vdev);
>> update_balloon_size(vb);
>>
>> makes no sense because virtballoon_changed merely queues
>> the work.
> in virtballoon_probe we just init work items, but if we have a target
> - num_pages != 0
> we will catch it only after virtballoon_changed will be called, isn't it?
>
> So, I believe, on probe, we have to do the same thing as on restore:
> check towards_target(vb)
> and call virtballoon_changed(vdev) if necessary.
>>
Also we could try inflate the balloon once(by fill_balloon()) on probe
and it leads to config change,
but we will not receive this notification because of config change
pending will be enabled after
drv->prob() in virtio_dev_probe.
>>> --
>>> 2.7.4
>
>
^ permalink raw reply
* Re: [PATCH] virtio/s390: add missing \n to end of dev_err message
From: Christian Borntraeger @ 2016-09-28 6:38 UTC (permalink / raw)
To: Colin King, Cornelia Huck, Martin Schwidefsky, Heiko Carstens,
linux-s390, virtualization, kvm
Cc: linux-kernel
In-Reply-To: <20160927200844.16008-1-colin.king@canonical.com>
On 09/27/2016 10:08 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Trival fix, dev_err message is missing a \n, so add it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 8688ad4..9d36dc4 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -462,7 +462,7 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw)
> * This may happen on device detach.
> */
> if (ret && (ret != -ENODEV))
> - dev_warn(&vq->vdev->dev, "Error %d while deleting queue %d",
> + dev_warn(&vq->vdev->dev, "Error %d while deleting queue %d\n",
> ret, index);
>
> vring_del_virtqueue(vq);
>
Thanks, applied.
^ permalink raw reply
* [PATCH] virtio/s390: add missing \n to end of dev_err message
From: Colin King @ 2016-09-27 20:08 UTC (permalink / raw)
To: Christian Borntraeger, Cornelia Huck, Martin Schwidefsky,
Heiko Carstens, linux-s390, virtualization, kvm
Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trival fix, dev_err message is missing a \n, so add it.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/s390/virtio/virtio_ccw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 8688ad4..9d36dc4 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -462,7 +462,7 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw)
* This may happen on device detach.
*/
if (ret && (ret != -ENODEV))
- dev_warn(&vq->vdev->dev, "Error %d while deleting queue %d",
+ dev_warn(&vq->vdev->dev, "Error %d while deleting queue %d\n",
ret, index);
vring_del_virtqueue(vq);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v9 17/19] drm/virtio: kconfig: Fix recursive dependency issue.
From: Bjorn Andersson @ 2016-09-27 17:01 UTC (permalink / raw)
To: Emil Velikov
Cc: devicetree, kernel, Arnd Bergmann, vinod.koul, linux-remoteproc,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel, patrice.chotard,
Peter Griffin, David Airlie, dmaengine, dan.j.williams,
open list:VIRTIO GPU DRIVER, lee.jones, LAKML
In-Reply-To: <CACvgo53fuoPVjBAkZbWFPVxosObjvxoDgFdR_BkSTgevqyGg9g@mail.gmail.com>
On Wed 21 Sep 05:09 PDT 2016, Emil Velikov wrote:
> On 20 September 2016 at 09:32, Peter Griffin <peter.griffin@linaro.org> wrote:
> > Hi Emil,
> >
> > On Tue, 20 Sep 2016, Emil Velikov wrote:
> >
> >> On 5 September 2016 at 14:16, Peter Griffin <peter.griffin@linaro.org> wrote:
> >> > ST_SLIM_REMOTEPROC must select REMOTEPROC, which exposes the following
> >> > recursive dependency.
> >
> >
> >> >
> >> From a humble skim through remoteproc, drm and a few other subsystems
> >> I think the above is wrong. All the drivers (outside of remoteproc),
> >> that I've seen, depend on the core component, they don't select it.
> >
> > I will let Bjorn comment on the remoteproc subsystem Kconfig design, and
> > why it is like it is.
> >
> > For this particular SLIM_RPROC I have added it to Kconfig in keeping with all
> > the other drivers in the remoteproc subsystem which has exposed this recursive
> > dependency issue.
> >
> > For this particular kconfig symbol a quick grep reveals more drivers in
> > the kernel using 'select', than 'depend on'
> >
> > git grep "select VIRTIO" | wc -l
> > 14
> >
> > git grep "depends on VIRTIO" | wc -l
> > 10
> >
> Might be worth taking a closer look into these at some point.
>
The general idea here is that VIRTIO provides the "framework" and as
such drivers implementing VIRTIO do select and drivers using virtio use
depends.
This is found in several places around the kernel.
> >
> >> Furthermore most places explicitly hide the drivers from the menu if
> >> the core component isn't enabled.
> >
> > Remoteproc subsystem takes a different approach, the core code is only enabled
> > if a driver which relies on it is enabled. This IMHO makes sense, as
> > remoteproc is not widely used (only a few particular ARM SoC's).
> >
> > It is true that for subsystems which rely on the core component being
> > explicitly enabled, they often tend to hide drivers which depend on it
> > from the menu unless it is. This also makes sense.
> >
> >>
> >> Is there something that requires such a different/unusual behaviour in
> >> remoteproc ?
> >>
There's nothing unusual in remoteproc that forces us to stay with this
model; however the parts related to the REMOTEPROC config is useless by
themselves.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH] VSOCK: Don't dec ack backlog twice for rejected connections
From: David Miller @ 2016-09-27 11:59 UTC (permalink / raw)
To: jhansen; +Cc: pv-drivers, netdev, linux-kernel, virtualization, stefanha,
gregkh
In-Reply-To: <1474959593-15311-1-git-send-email-jhansen@vmware.com>
From: Jorgen Hansen <jhansen@vmware.com>
Date: Mon, 26 Sep 2016 23:59:53 -0700
> If a pending socket is marked as rejected, we will decrease the
> sk_ack_backlog twice. So don't decrement it for rejected sockets
> in vsock_pending_work().
>
> Testing of the rejected socket path was done through code
> modifications.
>
> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Jorgen Hansen <jhansen@vmware.com>
> Reviewed-by: Adit Ranadive <aditr@vmware.com>
> Reviewed-by: Aditya Sarwade <asarwade@vmware.com>
Applied, thanks.
^ permalink raw reply
* VIRTIO_F_IOMMU_PLATFORM
From: Will Deacon @ 2016-09-27 8:57 UTC (permalink / raw)
To: mst; +Cc: virtualization
Hi Michael,
In commit 1a937693993f ("virtio: new feature to detect IOMMU device quirk"),
you added a new feature bit (VIRTIO_F_IOMMU_PLATFORM) to describe whether
or not a given virtio device requires physical address or bus addresses.
Is there a plan to get this incorporated into the virtio spec [1]? At the
moment, I can't see a working draft that includes the new bit. Having it
in the spec would help convince implementors that it's not just a Linux
thing.
Thanks,
Will
[1] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html
^ permalink raw reply
* [PATCH] VSOCK: Don't dec ack backlog twice for rejected connections
From: Jorgen Hansen @ 2016-09-27 6:59 UTC (permalink / raw)
To: netdev, linux-kernel, virtualization
Cc: pv-drivers, gregkh, Jorgen Hansen, davem, stefanha
If a pending socket is marked as rejected, we will decrease the
sk_ack_backlog twice. So don't decrement it for rejected sockets
in vsock_pending_work().
Testing of the rejected socket path was done through code
modifications.
Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jorgen Hansen <jhansen@vmware.com>
Reviewed-by: Adit Ranadive <aditr@vmware.com>
Reviewed-by: Aditya Sarwade <asarwade@vmware.com>
---
net/vmw_vsock/af_vsock.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 17dbbe6..8a398b3 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -465,6 +465,8 @@ void vsock_pending_work(struct work_struct *work)
if (vsock_is_pending(sk)) {
vsock_remove_pending(listener, sk);
+
+ listener->sk_ack_backlog--;
} else if (!vsk->rejected) {
/* We are not on the pending list and accept() did not reject
* us, so we must have been accepted by our user process. We
@@ -475,8 +477,6 @@ void vsock_pending_work(struct work_struct *work)
goto out;
}
- listener->sk_ack_backlog--;
-
/* We need to remove ourself from the global connected sockets list so
* incoming packets can't find this socket, and to reduce the reference
* count.
@@ -2010,5 +2010,5 @@ EXPORT_SYMBOL_GPL(vsock_core_get_transport);
MODULE_AUTHOR("VMware, Inc.");
MODULE_DESCRIPTION("VMware Virtual Socket Family");
-MODULE_VERSION("1.0.1.0-k");
+MODULE_VERSION("1.0.2.0-k");
MODULE_LICENSE("GPL v2");
--
1.7.0
^ permalink raw reply related
* [PATCH] vhost: mark symbols static in vhost.c
From: Baoyou Xie @ 2016-09-26 11:45 UTC (permalink / raw)
To: mst
Cc: kvm, arnd, netdev, tang.qiang007, xie.baoyou, linux-kernel,
virtualization, han.fei, baoyou.xie
We get 4 warnings when building kernel with W=1:
drivers/vhost/vhost.c:52:23: warning: no previous prototype for 'vhost_umem_interval_tree_insert' [-Wmissing-prototypes]
drivers/vhost/vhost.c:52:23: warning: no previous prototype for 'vhost_umem_interval_tree_remove' [-Wmissing-prototypes]
drivers/vhost/vhost.c:52:23: warning: no previous prototype for 'vhost_umem_interval_tree_iter_first' [-Wmissing-prototypes]
drivers/vhost/vhost.c:52:23: warning: no previous prototype for 'vhost_umem_interval_tree_iter_next' [-Wmissing-prototypes]
In fact, these functions are defined in the
macro 'INTERVAL_TREE_DEFINE' in include/linux/interval_tree_generic.h
and don't need a declaration, but can be made static.
so this patch marks these functions with 'static' by modifying
the macro.
Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
---
drivers/vhost/vhost.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c6f2d89..99c6d0d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -48,8 +48,8 @@ enum {
#define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
INTERVAL_TREE_DEFINE(struct vhost_umem_node,
- rb, __u64, __subtree_last,
- START, LAST, , vhost_umem_interval_tree);
+ rb, __u64, __subtree_last, START, LAST,
+ static, vhost_umem_interval_tree);
#ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
static void vhost_disable_cross_endian(struct vhost_virtqueue *vq)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 1/1] update balloon size in balloon "probe"
From: Konstantin Neumoin @ 2016-09-26 7:16 UTC (permalink / raw)
To: Michael S. Tsirkin, Denis V. Lunev; +Cc: linux-kernel, virtualization
In-Reply-To: <20160923215334-mutt-send-email-mst@kernel.org>
On 09/23/2016 09:58 PM, Michael S. Tsirkin wrote:
> On Fri, Sep 23, 2016 at 04:47:57PM +0300, Denis V. Lunev wrote:
>> From: Konstantin Neumoin <kneumoin@virtuozzo.com>
>>
>> Patch
>> Commit 3d2a3774c1b046f548ebea0391a602fd5685a307
>> Author: Michael S. Tsirkin <mst@redhat.com>
>> Date: Tue Mar 10 11:55:08 2015 +1030
>> virtio-balloon: do not call blocking ops when !TASK_RUNNING
>> has added a regression. Original code with wait_event_interruptible
>> checked the condition before start waiting and started balloon operations
>> if necessary.
> I don't get it, sorry.
>
> + add_wait_queue(&vb->config_change, &wait);
> + for (;;) {
> + if ((diff = towards_target(vb)) != 0 ||
> + vb->need_stats_update ||
> + kthread_should_stop() ||
> + freezing(current))
> + break;
> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> + }
> + remove_wait_queue(&vb->config_change, &wait);
>
> Seems to check the condition before waiting.
>
> The issue is more likely with this patch:
>
> commit fad7b7b27b6a168ca8ebc84482043886f837b89d
> Author: Petr Mladek <pmladek@suse.com>
> Date: Mon Jan 25 17:38:05 2016 +0100
>
> virtio_balloon: Use a workqueue instead of "vballoon" kthread
yes, you are right, sorry, commit message is incorrect.
>
>
>
>> Right now balloon is not inflated if ballon target is set before the
>> driver is loaded.
>>
>> Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: "Michael S. Tsirkin" <mst@redhat.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 4e7003d..0a6c10f 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -577,6 +577,10 @@ static int virtballoon_probe(struct virtio_device *vdev)
>>
>> virtio_device_ready(vdev);
>>
>> + if (towards_target(vb))
>> + virtballoon_changed(vdev);
>> + update_balloon_size(vb);
>> +
>> return 0;
>>
>> out_del_vqs:
>
> I know we have same thing on restore, but it seems bogus
> there as well:
> if (towards_target(vb))
> virtballoon_changed(vdev);
> update_balloon_size(vb);
>
> makes no sense because virtballoon_changed merely queues
> the work.
in virtballoon_probe we just init work items, but if we have a target
- num_pages != 0
we will catch it only after virtballoon_changed will be called, isn't it?
So, I believe, on probe, we have to do the same thing as on restore:
check towards_target(vb)
and call virtballoon_changed(vdev) if necessary.
>
>> --
>> 2.7.4
^ permalink raw reply
* Re: [PATCH 1/1] update balloon size in balloon "probe"
From: Michael S. Tsirkin @ 2016-09-23 18:58 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Konstantin Neumoin, linux-kernel, virtualization
In-Reply-To: <1474638477-8658-1-git-send-email-den@openvz.org>
On Fri, Sep 23, 2016 at 04:47:57PM +0300, Denis V. Lunev wrote:
> From: Konstantin Neumoin <kneumoin@virtuozzo.com>
>
> Patch
> Commit 3d2a3774c1b046f548ebea0391a602fd5685a307
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date: Tue Mar 10 11:55:08 2015 +1030
> virtio-balloon: do not call blocking ops when !TASK_RUNNING
> has added a regression. Original code with wait_event_interruptible
> checked the condition before start waiting and started balloon operations
> if necessary.
I don't get it, sorry.
+ add_wait_queue(&vb->config_change, &wait);
+ for (;;) {
+ if ((diff = towards_target(vb)) != 0 ||
+ vb->need_stats_update ||
+ kthread_should_stop() ||
+ freezing(current))
+ break;
+ wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
+ }
+ remove_wait_queue(&vb->config_change, &wait);
Seems to check the condition before waiting.
The issue is more likely with this patch:
commit fad7b7b27b6a168ca8ebc84482043886f837b89d
Author: Petr Mladek <pmladek@suse.com>
Date: Mon Jan 25 17:38:05 2016 +0100
virtio_balloon: Use a workqueue instead of "vballoon" kthread
> Right now balloon is not inflated if ballon target is set before the
> driver is loaded.
>
> Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e7003d..0a6c10f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -577,6 +577,10 @@ static int virtballoon_probe(struct virtio_device *vdev)
>
> virtio_device_ready(vdev);
>
> + if (towards_target(vb))
> + virtballoon_changed(vdev);
> + update_balloon_size(vb);
> +
> return 0;
>
> out_del_vqs:
I know we have same thing on restore, but it seems bogus
there as well:
if (towards_target(vb))
virtballoon_changed(vdev);
update_balloon_size(vb);
makes no sense because virtballoon_changed merely queues
the work.
> --
> 2.7.4
^ permalink raw reply
* [PATCH 1/1] update balloon size in balloon "probe"
From: Denis V. Lunev @ 2016-09-23 13:47 UTC (permalink / raw)
To: virtualization, linux-kernel; +Cc: den, Konstantin Neumoin, Michael S. Tsirkin
From: Konstantin Neumoin <kneumoin@virtuozzo.com>
Patch
Commit 3d2a3774c1b046f548ebea0391a602fd5685a307
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Tue Mar 10 11:55:08 2015 +1030
virtio-balloon: do not call blocking ops when !TASK_RUNNING
has added a regression. Original code with wait_event_interruptible
checked the condition before start waiting and started balloon operations
if necessary.
Right now balloon is not inflated if ballon target is set before the
driver is loaded.
Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: "Michael S. Tsirkin" <mst@redhat.com>
---
drivers/virtio/virtio_balloon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e7003d..0a6c10f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -577,6 +577,10 @@ static int virtballoon_probe(struct virtio_device *vdev)
virtio_device_ready(vdev);
+ if (towards_target(vb))
+ virtballoon_changed(vdev);
+ update_balloon_size(vb);
+
return 0;
out_del_vqs:
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Namhyung Kim @ 2016-09-23 5:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: virtio-dev, Tony Luck, Daniel P . Berrange, Kees Cook, kvm,
Radim Krčmář, Anton Vorontsov, qemu-devel,
Steven Rostedt, LKML, Minchan Kim, Michael S. Tsirkin,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <20160922122316.GB8221@stefanha-x1.localdomain>
On Thu, Sep 22, 2016 at 01:23:16PM +0100, Stefan Hajnoczi wrote:
> On Sun, Sep 04, 2016 at 11:38:59PM +0900, Namhyung Kim wrote:
> > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > + VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > + VirtQueueElement *elem;
> > + struct virtio_pstore_req req;
> > + struct virtio_pstore_res res;
> > + ssize_t len = 0;
> > + int ret;
> > +
> > + for (;;) {
> > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > + if (!elem) {
> > + return;
> > + }
> > +
> > + if (elem->out_num < 1 || elem->in_num < 1) {
> > + error_report("request or response buffer is missing");
> > + exit(1);
>
> The new virtio_error() function might be available, depending on when
> this patch series is merged. virtio_error() should be used instead of
> exit(1). See "[PATCH v5 0/9] virtio: avoid exit() when device enters
> invalid states" on qemu-devel.
Thanks for the info, will take a look.
>
> > + }
> > +
> > + if (elem->out_num > 2 || elem->in_num > 3) {
> > + error_report("invalid number of input/output buffer");
> > + exit(1);
> > + }
>
> The VIRTIO specification requires that flexible framing is supported.
> The device cannot make assumptions about the scatter-gather list. It
> must support any layout (e.g. even multiple 1-byte iovecs making up the
> buffer).
Ok.
>
> > +
> > + len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req));
> > + if (len != (ssize_t)sizeof(req)) {
> > + error_report("invalid request size: %ld", (long)len);
> > + exit(1);
> > + }
> > + res.cmd = req.cmd;
> > + res.type = req.type;
> > +
> > + switch (le16_to_cpu(req.cmd)) {
> > + case VIRTIO_PSTORE_CMD_OPEN:
> > + ret = virtio_pstore_do_open(s);
> > + break;
> > + case VIRTIO_PSTORE_CMD_CLOSE:
> > + ret = virtio_pstore_do_close(s);
> > + break;
> > + case VIRTIO_PSTORE_CMD_ERASE:
> > + ret = virtio_pstore_do_erase(s, &req);
> > + break;
> > + case VIRTIO_PSTORE_CMD_READ:
> > + ret = virtio_pstore_do_read(s, elem);
> > + if (ret == 1) {
> > + /* async channel io */
> > + continue;
> > + }
> > + break;
> > + case VIRTIO_PSTORE_CMD_WRITE:
> > + ret = virtio_pstore_do_write(s, elem, &req);
> > + if (ret == 1) {
> > + /* async channel io */
> > + continue;
> > + }
> > + break;
> > + default:
> > + ret = -1;
> > + break;
> > + }
> > +
> > + res.ret = ret;
>
> Missing cpu_to_le()?
Right!
>
> > +static void pstore_set_bufsize(Object *obj, Visitor *v,
> > + const char *name, void *opaque,
> > + Error **errp)
> > +{
> > + VirtIOPstore *s = opaque;
> > + Error *error = NULL;
> > + uint64_t value;
> > +
> > + visit_type_size(v, name, &value, &error);
> > + if (error) {
> > + error_propagate(errp, error);
> > + return;
> > + }
> > +
> > + if (value < 4096) {
> > + error_setg(&error, "Warning: too small buffer size: %"PRIu64, value);
>
> This is an error, not a warning. Please remove "Warning:" so it's clear
> to the user that this message caused QEMU to fail.
Will do.
Thanks,
Namhyung
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Namhyung Kim @ 2016-09-23 5:48 UTC (permalink / raw)
To: Stefan Hajnoczi, Kees Cook
Cc: virtio-dev, Tony Luck, Radim Krčmář, kvm,
Michael S. Tsirkin, Anton Vorontsov, Will Deacon, qemu-devel,
Steven Rostedt, LKML, Minchan Kim, Anthony Liguori, Colin Cross,
Paolo Bonzini, virtualization, Ingo Molnar
In-Reply-To: <20160922115744.GA8221@stefanha-x1.localdomain>
Hi Stefan,
On Thu, Sep 22, 2016 at 12:57:44PM +0100, Stefan Hajnoczi wrote:
> On Sun, Sep 04, 2016 at 11:38:58PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine. Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem. It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> >
> > It supports legacy PCI device using a 16K buffer by default and it's
> > configurable. It uses two virtqueues - one for (sync) read and another
> > for (async) write. Since it cannot wait for write finished, it supports
> > up to 128 concurrent IO.
>
> Please document the locks that this code relies on. It is generally not
> safe to call virtqueue_*() from multiple threads. I also wonder about
> locking for virtio_pstore->req_id and other fields. Are locks missing
> or is something in pstore ensuring safety?
Ok, I should use atomic inc for pstore->req_id. The open-read-close
are serialized by the read_mutex of pstore_info. Write can happend
anytime so I gave it a dedicate queue.
Erase is a problem though, normally it's only doable after mount
operation is done so no contention to the open-read-close. But if the
pstore_update_ms is set, timer routine can schedule a work to do the
open-read-close loop which might contend to erase.
I'm not sure how useful pstore_update_ms is and the descriptoin saids
"milliseconds before pstore updates its content "
"(default is -1, which means runtime updates are disabled; "
"enabling this option is not safe, it may lead to further "
"corruption on Oopses)")
>
> > +static int virt_pstore_open(struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_req *req;
> > + struct virtio_pstore_res *res;
> > + struct scatterlist sgo[1], sgi[1];
> > + struct scatterlist *sgs[2] = { sgo, sgi };
> > + unsigned int len;
> > +
> > + virt_pstore_get_reqs(vps, &req, &res);
> > +
> > + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_OPEN);
> > +
> > + sg_init_one(sgo, req, sizeof(*req));
> > + sg_init_one(sgi, res, sizeof(*res));
> > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > + virtqueue_kick(vps->vq[0]);
> > +
> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> > + return le32_to_cpu(res->ret);
>
> This assumes the device puts compatible Linux errno values in res->ret.
> The function doesn't need to return -errno if I'm reading fs/pstore/
> code correctly. You could return -1 on error to avoid making this
> assumption. The same applies to other res->ret usage below.
Ok.
>
> > +}
> > +
> > +static int virt_pstore_close(struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_req *req = &vps->req[vps->req_id];
> > + struct virtio_pstore_res *res = &vps->res[vps->req_id];
>
> Assigning &vps->req[vps->req_id]/&vps->res[vps->req_id] is unnecessary,
> virt_pstore_get_reqs() handles that below.
Ah, right.
>
> > + struct scatterlist sgo[1], sgi[1];
> > + struct scatterlist *sgs[2] = { sgo, sgi };
> > + unsigned int len;
> > +
> > + virt_pstore_get_reqs(vps, &req, &res);
> > +
> > + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_CLOSE);
> > +
> > + sg_init_one(sgo, req, sizeof(*req));
> > + sg_init_one(sgi, res, sizeof(*res));
> > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > + virtqueue_kick(vps->vq[0]);
> > +
> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> > + return le32_to_cpu(res->ret);
> > +}
> > +
> > +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
> > + int *count, struct timespec *time,
> > + char **buf, bool *compressed,
> > + ssize_t *ecc_notice_size,
> > + struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_req *req;
> > + struct virtio_pstore_res *res;
> > + struct virtio_pstore_fileinfo info;
> > + struct scatterlist sgo[1], sgi[3];
> > + struct scatterlist *sgs[2] = { sgo, sgi };
> > + unsigned int len;
> > + unsigned int flags;
> > + int ret;
> > + void *bf;
> > +
> > + virt_pstore_get_reqs(vps, &req, &res);
> > +
> > + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> > +
> > + sg_init_one(sgo, req, sizeof(*req));
> > + sg_init_table(sgi, 3);
> > + sg_set_buf(&sgi[0], res, sizeof(*res));
> > + sg_set_buf(&sgi[1], &info, sizeof(info));
> > + sg_set_buf(&sgi[2], psi->buf, psi->bufsize);
> > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > + virtqueue_kick(vps->vq[0]);
> > +
> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> > + if (len < sizeof(*res) + sizeof(info))
> > + return -1;
> > +
> > + ret = le32_to_cpu(res->ret);
> > + if (ret < 0)
> > + return ret;
> > +
> > + len = le32_to_cpu(info.len);
>
> We trust the device but a length check would be nice here to be clear
> that the memcpy() below is always safe:
>
> if (len > psi->bufsize)
> return -1;
Good point.
>
> > +
> > + bf = kmalloc(len, GFP_KERNEL);
> > + if (bf == NULL)
> > + return -ENOMEM;
>
> There's no point in returning -ENOMEM if we return -1 and res->ret
> above. The caller has no way of knowing the true meaning of the return
> value. I would return -1 to be clear that there are no error constants.
Ok.
>
> > +
> > + *id = le64_to_cpu(info.id);
> > + *type = from_virtio_type(info.type);
> > + *count = le32_to_cpu(info.count);
> > +
> > + flags = le32_to_cpu(info.flags);
> > + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED;
> > +
> > + time->tv_sec = le64_to_cpu(info.time_sec);
> > + time->tv_nsec = le32_to_cpu(info.time_nsec);
> > +
> > + memcpy(bf, psi->buf, len);
> > + *buf = bf;
> > +
> > + return len;
> > +}
> > +
> > +static int notrace virt_pstore_write(enum pstore_type_id type,
> > + enum kmsg_dump_reason reason,
> > + u64 *id, unsigned int part, int count,
> > + bool compressed, size_t size,
> > + struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_req *req;
> > + struct virtio_pstore_res *res;
> > + struct scatterlist sgo[2], sgi[1];
> > + struct scatterlist *sgs[2] = { sgo, sgi };
> > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
> > +
> > + if (vps->failed)
> > + return -1;
> > +
> > + *id = vps->req_id;
> > + virt_pstore_get_reqs(vps, &req, &res);
>
> This function does not wait for a response from the device so you cannot
> call virt_pstore_get_reqs() without risk of reusing an in-flight buffer.
Right. This part is debatable IMHO.
Generally pstore is used to dump oops buffer when kernel is dying.
This is an occasional event so I think it's ok with the current code.
The problematic case is when other writers (console, pmsg or ftrace)
are enabled. They might contend with the oops, but pstore serializes
them with a spinlock. But as you said it still has a risk of reusing
the in-flight buffer. I think it's okay that oops overwriting other,
but the reverse is not good. I tried to mitigate the problem by
managing the buffer position to spread out the write but it's not a
complete solution.
I thought simply stopping the guest when writing oops, or we might
create a way to tell pstore to stop other writers (until writing oops
finished) somehow.
Kees, what do you think?
>
> > +
> > + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_WRITE);
> > + req->type = to_virtio_type(type);
> > + req->flags = cpu_to_le32(flags);
> > +
> > + sg_init_table(sgo, 2);
> > + sg_set_buf(&sgo[0], req, sizeof(*req));
> > + sg_set_buf(&sgo[1], psi->buf, size);
> > + sg_init_one(sgi, res, sizeof(*res));
> > + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC);
>
> If we don't wait for request completion then virtqueue_add_sgs() could
> fail here if the virtqueue is already full.
Ah, didn't known that. So we need to wait for completion somehow.
>
> > + virtqueue_kick(vps->vq[1]);
> > +
> > + return 0;
> > +}
> > +
> > +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count,
> > + struct timespec time, struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_req *req;
> > + struct virtio_pstore_res *res;
> > + struct scatterlist sgo[1], sgi[1];
> > + struct scatterlist *sgs[2] = { sgo, sgi };
> > + unsigned int len;
> > +
> > + virt_pstore_get_reqs(vps, &req, &res);
> > +
> > + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_ERASE);
> > + req->type = to_virtio_type(type);
> > + req->id = cpu_to_le64(id);
> > + req->count = cpu_to_le32(count);
> > +
> > + sg_init_one(sgo, req, sizeof(*req));
> > + sg_init_one(sgi, res, sizeof(*res));
> > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > + virtqueue_kick(vps->vq[0]);
>
> Erase commands are sent on the "read" virtqueue, not the "write"
> virtqueue?
Yes, you can think they're sync and async queues. The write is the
only async operation.
>
> > +
> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> > + return le32_to_cpu(res->ret);
> > +}
> > +
> > +static int virt_pstore_init(struct virtio_pstore *vps)
> > +{
> > + struct pstore_info *psinfo = &vps->pstore;
> > + int err;
> > +
> > + if (!psinfo->bufsize)
> > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE;
> > +
> > + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL);
> > + if (!psinfo->buf) {
> > + pr_err("cannot allocate pstore buffer\n");
> > + return -ENOMEM;
> > + }
> > +
> > + psinfo->owner = THIS_MODULE;
> > + psinfo->name = "virtio";
> > + psinfo->open = virt_pstore_open;
> > + psinfo->close = virt_pstore_close;
> > + psinfo->read = virt_pstore_read;
> > + psinfo->erase = virt_pstore_erase;
> > + psinfo->write = virt_pstore_write;
> > + psinfo->flags = PSTORE_FLAGS_FRAGILE;
> > +
> > + psinfo->data = vps;
> > + spin_lock_init(&psinfo->buf_lock);
> > +
> > + err = pstore_register(psinfo);
> > + if (err)
> > + kfree(psinfo->buf);
>
> Should this be free_pages_exact() instead of kfree()?
Oops, right.
>
> Is it safe to call pstore_register() before the remaining initialization
> steps?
>
> My understanding is that if pstore is already mounted then requests will
> immediately be sent. In order to support this we'd need to initialize
> everything else first before calling pstore_register().
Fair enough. Will change.
>
> > +
> > + return err;
> > +}
> > +
> > +static int virt_pstore_exit(struct virtio_pstore *vps)
> > +{
> > + struct pstore_info *psinfo = &vps->pstore;
> > +
> > + pstore_unregister(psinfo);
> > +
> > + free_pages_exact(psinfo->buf, psinfo->bufsize);
> > + psinfo->buf = NULL;
> > + psinfo->bufsize = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int virtpstore_init_vqs(struct virtio_pstore *vps)
> > +{
> > + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check };
> > + const char *names[] = { "pstore_read", "pstore_write" };
> > +
> > + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq,
> > + callbacks, names);
> > +}
> > +
> > +static void virtpstore_init_config(struct virtio_pstore *vps)
> > +{
> > + u32 bufsize;
> > +
> > + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize);
> > +
> > + vps->pstore.bufsize = PAGE_ALIGN(bufsize);
> > +}
> > +
> > +static void virtpstore_confirm_config(struct virtio_pstore *vps)
> > +{
> > + u32 bufsize = vps->pstore.bufsize;
> > +
> > + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize,
> > + &bufsize);
> > +}
> > +
> > +static int virtpstore_probe(struct virtio_device *vdev)
> > +{
> > + struct virtio_pstore *vps;
> > + int err;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "driver init: config access disabled\n");
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL);
> > + if (!vps) {
> > + err = -ENOMEM;
> > + goto out;
> > + }
> > + vps->vdev = vdev;
> > +
> > + err = virtpstore_init_vqs(vps);
> > + if (err < 0)
> > + goto out_free;
> > +
> > + virtpstore_init_config(vps);
> > +
> > + err = virt_pstore_init(vps);
> > + if (err)
> > + goto out_del_vq;
> > +
> > + virtpstore_confirm_config(vps);
> > +
> > + init_waitqueue_head(&vps->acked);
> > +
> > + virtio_device_ready(vdev);
>
> This call is needed if the .probe() function uses virtqueues before
> returning. Right now it either doesn't use the virtqueues or it has
> already used them in virt_pstore_init(). Please move this before
> pstore_register().
Will do.
Thank you so much for your detailed review!
Namhyung
^ permalink raw reply
* Re: [PATCH v7 4/6] powerpc: lib/locks.c: Add cpu yield/wake helper function
From: Boqun Feng @ 2016-09-22 15:17 UTC (permalink / raw)
To: Pan Xinhui
Cc: peterz, benh, linux-kernel, waiman.long, virtualization, mingo,
paulus, mpe, paulmck, linuxppc-dev
In-Reply-To: <1474277037-15200-5-git-send-email-xinhui.pan@linux.vnet.ibm.com>
[-- Attachment #1.1: Type: text/plain, Size: 4005 bytes --]
Hi Xinhui,
On Mon, Sep 19, 2016 at 05:23:55AM -0400, Pan Xinhui wrote:
> Add two corresponding helper functions to support pv-qspinlock.
>
> For normal use, __spin_yield_cpu will confer current vcpu slices to the
> target vcpu(say, a lock holder). If target vcpu is not specified or it
> is in running state, such conferging to lpar happens or not depends.
>
> Because hcall itself will introduce latency and a little overhead. And
> we do NOT want to suffer any latency on some cases, e.g. in interrupt handler.
> The second parameter *confer* can indicate such case.
>
> __spin_wake_cpu is simpiler, it will wake up one vcpu regardless of its
> current vcpu state.
>
> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/spinlock.h | 4 +++
> arch/powerpc/lib/locks.c | 59 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 6aef8dd..abb6b0f 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -56,9 +56,13 @@
> /* We only yield to the hypervisor if we are in shared processor mode */
> #define SHARED_PROCESSOR (lppaca_shared_proc(local_paca->lppaca_ptr))
> extern void __spin_yield(arch_spinlock_t *lock);
> +extern void __spin_yield_cpu(int cpu, int confer);
> +extern void __spin_wake_cpu(int cpu);
> extern void __rw_yield(arch_rwlock_t *lock);
> #else /* SPLPAR */
> #define __spin_yield(x) barrier()
> +#define __spin_yield_cpu(x,y) barrier()
> +#define __spin_wake_cpu(x) barrier()
> #define __rw_yield(x) barrier()
> #define SHARED_PROCESSOR 0
> #endif
> diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
> index 6574626..892df7d 100644
> --- a/arch/powerpc/lib/locks.c
> +++ b/arch/powerpc/lib/locks.c
> @@ -23,6 +23,65 @@
> #include <asm/hvcall.h>
> #include <asm/smp.h>
>
> +/*
> + * confer our slices to a specified cpu and return. If it is already running or
> + * cpu is -1, then we will check confer. If confer is NULL, we will return
> + * otherwise we confer our slices to lpar.
> + */
> +void __spin_yield_cpu(int cpu, int confer)
> +{
> + unsigned int holder_cpu = cpu, yield_count;
> +
You don't need @holder_cpu at all, do you?
> + if (cpu == -1)
> + goto yield_to_lpar;
> +
> + BUG_ON(holder_cpu >= nr_cpu_ids);
> + yield_count = be32_to_cpu(lppaca_of(holder_cpu).yield_count);
> +
> + /* if cpu is running, confer slices to lpar conditionally*/
> + if ((yield_count & 1) == 0)
> + goto yield_to_lpar;
> +
> + plpar_hcall_norets(H_CONFER,
> + get_hard_smp_processor_id(holder_cpu), yield_count);
> + return;
> +
> +yield_to_lpar:
> + if (confer)
> + plpar_hcall_norets(H_CONFER, -1, 0);
> +}
> +EXPORT_SYMBOL_GPL(__spin_yield_cpu);
> +
> +void __spin_wake_cpu(int cpu)
> +{
> + unsigned int holder_cpu = cpu;
> +
Ditto.
Regards,
Boqun
> + BUG_ON(holder_cpu >= nr_cpu_ids);
> + /*
> + * NOTE: we should always do this hcall regardless of
> + * the yield_count of the holder_cpu.
> + * as thers might be a case like below;
> + * CPU 1 2
> + * yielded = true
> + * if (yielded)
> + * __spin_wake_cpu()
> + * __spin_yield_cpu()
> + *
> + * So we might lose a wake if we check the yield_count and
> + * return directly if the holder_cpu is running.
> + * IOW. do NOT code like below.
> + * yield_count = be32_to_cpu(lppaca_of(holder_cpu).yield_count);
> + * if ((yield_count & 1) == 0)
> + * return;
> + *
> + * a PROD hcall marks the target_cpu proded, which cause the next cede or confer
> + * called on the target_cpu invalid.
> + */
> + plpar_hcall_norets(H_PROD,
> + get_hard_smp_processor_id(holder_cpu));
> +}
> +EXPORT_SYMBOL_GPL(__spin_wake_cpu);
> +
> #ifndef CONFIG_QUEUED_SPINLOCKS
> void __spin_yield(arch_spinlock_t *lock)
> {
> --
> 2.4.11
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Stefan Hajnoczi @ 2016-09-22 12:23 UTC (permalink / raw)
To: Namhyung Kim
Cc: virtio-dev, Tony Luck, Daniel P . Berrange, Kees Cook, kvm,
Radim Krčmář, Namhyung Kim, Anton Vorontsov,
qemu-devel, Steven Rostedt, LKML, Minchan Kim, Michael S. Tsirkin,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <20160904143900.14850-3-namhyung@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 3211 bytes --]
On Sun, Sep 04, 2016 at 11:38:59PM +0900, Namhyung Kim wrote:
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> + VirtQueueElement *elem;
> + struct virtio_pstore_req req;
> + struct virtio_pstore_res res;
> + ssize_t len = 0;
> + int ret;
> +
> + for (;;) {
> + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> + if (!elem) {
> + return;
> + }
> +
> + if (elem->out_num < 1 || elem->in_num < 1) {
> + error_report("request or response buffer is missing");
> + exit(1);
The new virtio_error() function might be available, depending on when
this patch series is merged. virtio_error() should be used instead of
exit(1). See "[PATCH v5 0/9] virtio: avoid exit() when device enters
invalid states" on qemu-devel.
> + }
> +
> + if (elem->out_num > 2 || elem->in_num > 3) {
> + error_report("invalid number of input/output buffer");
> + exit(1);
> + }
The VIRTIO specification requires that flexible framing is supported.
The device cannot make assumptions about the scatter-gather list. It
must support any layout (e.g. even multiple 1-byte iovecs making up the
buffer).
> +
> + len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req));
> + if (len != (ssize_t)sizeof(req)) {
> + error_report("invalid request size: %ld", (long)len);
> + exit(1);
> + }
> + res.cmd = req.cmd;
> + res.type = req.type;
> +
> + switch (le16_to_cpu(req.cmd)) {
> + case VIRTIO_PSTORE_CMD_OPEN:
> + ret = virtio_pstore_do_open(s);
> + break;
> + case VIRTIO_PSTORE_CMD_CLOSE:
> + ret = virtio_pstore_do_close(s);
> + break;
> + case VIRTIO_PSTORE_CMD_ERASE:
> + ret = virtio_pstore_do_erase(s, &req);
> + break;
> + case VIRTIO_PSTORE_CMD_READ:
> + ret = virtio_pstore_do_read(s, elem);
> + if (ret == 1) {
> + /* async channel io */
> + continue;
> + }
> + break;
> + case VIRTIO_PSTORE_CMD_WRITE:
> + ret = virtio_pstore_do_write(s, elem, &req);
> + if (ret == 1) {
> + /* async channel io */
> + continue;
> + }
> + break;
> + default:
> + ret = -1;
> + break;
> + }
> +
> + res.ret = ret;
Missing cpu_to_le()?
> +static void pstore_set_bufsize(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + Error *error = NULL;
> + uint64_t value;
> +
> + visit_type_size(v, name, &value, &error);
> + if (error) {
> + error_propagate(errp, error);
> + return;
> + }
> +
> + if (value < 4096) {
> + error_setg(&error, "Warning: too small buffer size: %"PRIu64, value);
This is an error, not a warning. Please remove "Warning:" so it's clear
to the user that this message caused QEMU to fail.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Stefan Hajnoczi @ 2016-09-22 11:57 UTC (permalink / raw)
To: Namhyung Kim
Cc: virtio-dev, Tony Luck, Kees Cook, kvm,
Radim Krčmář, Anton Vorontsov, Will Deacon,
qemu-devel, Steven Rostedt, LKML, Minchan Kim, Michael S. Tsirkin,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <20160904143900.14850-2-namhyung@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 10474 bytes --]
On Sun, Sep 04, 2016 at 11:38:58PM +0900, Namhyung Kim wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine. Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem. It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
>
> It supports legacy PCI device using a 16K buffer by default and it's
> configurable. It uses two virtqueues - one for (sync) read and another
> for (async) write. Since it cannot wait for write finished, it supports
> up to 128 concurrent IO.
Please document the locks that this code relies on. It is generally not
safe to call virtqueue_*() from multiple threads. I also wonder about
locking for virtio_pstore->req_id and other fields. Are locks missing
or is something in pstore ensuring safety?
> +static int virt_pstore_open(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_OPEN);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + return le32_to_cpu(res->ret);
This assumes the device puts compatible Linux errno values in res->ret.
The function doesn't need to return -errno if I'm reading fs/pstore/
code correctly. You could return -1 on error to avoid making this
assumption. The same applies to other res->ret usage below.
> +}
> +
> +static int virt_pstore_close(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req = &vps->req[vps->req_id];
> + struct virtio_pstore_res *res = &vps->res[vps->req_id];
Assigning &vps->req[vps->req_id]/&vps->res[vps->req_id] is unnecessary,
virt_pstore_get_reqs() handles that below.
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_CLOSE);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + return le32_to_cpu(res->ret);
> +}
> +
> +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
> + int *count, struct timespec *time,
> + char **buf, bool *compressed,
> + ssize_t *ecc_notice_size,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct virtio_pstore_fileinfo info;
> + struct scatterlist sgo[1], sgi[3];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> + unsigned int flags;
> + int ret;
> + void *bf;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_table(sgi, 3);
> + sg_set_buf(&sgi[0], res, sizeof(*res));
> + sg_set_buf(&sgi[1], &info, sizeof(info));
> + sg_set_buf(&sgi[2], psi->buf, psi->bufsize);
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + if (len < sizeof(*res) + sizeof(info))
> + return -1;
> +
> + ret = le32_to_cpu(res->ret);
> + if (ret < 0)
> + return ret;
> +
> + len = le32_to_cpu(info.len);
We trust the device but a length check would be nice here to be clear
that the memcpy() below is always safe:
if (len > psi->bufsize)
return -1;
> +
> + bf = kmalloc(len, GFP_KERNEL);
> + if (bf == NULL)
> + return -ENOMEM;
There's no point in returning -ENOMEM if we return -1 and res->ret
above. The caller has no way of knowing the true meaning of the return
value. I would return -1 to be clear that there are no error constants.
> +
> + *id = le64_to_cpu(info.id);
> + *type = from_virtio_type(info.type);
> + *count = le32_to_cpu(info.count);
> +
> + flags = le32_to_cpu(info.flags);
> + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED;
> +
> + time->tv_sec = le64_to_cpu(info.time_sec);
> + time->tv_nsec = le32_to_cpu(info.time_nsec);
> +
> + memcpy(bf, psi->buf, len);
> + *buf = bf;
> +
> + return len;
> +}
> +
> +static int notrace virt_pstore_write(enum pstore_type_id type,
> + enum kmsg_dump_reason reason,
> + u64 *id, unsigned int part, int count,
> + bool compressed, size_t size,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[2], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
> +
> + if (vps->failed)
> + return -1;
> +
> + *id = vps->req_id;
> + virt_pstore_get_reqs(vps, &req, &res);
This function does not wait for a response from the device so you cannot
call virt_pstore_get_reqs() without risk of reusing an in-flight buffer.
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_WRITE);
> + req->type = to_virtio_type(type);
> + req->flags = cpu_to_le32(flags);
> +
> + sg_init_table(sgo, 2);
> + sg_set_buf(&sgo[0], req, sizeof(*req));
> + sg_set_buf(&sgo[1], psi->buf, size);
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC);
If we don't wait for request completion then virtqueue_add_sgs() could
fail here if the virtqueue is already full.
> + virtqueue_kick(vps->vq[1]);
> +
> + return 0;
> +}
> +
> +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count,
> + struct timespec time, struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_ERASE);
> + req->type = to_virtio_type(type);
> + req->id = cpu_to_le64(id);
> + req->count = cpu_to_le32(count);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
Erase commands are sent on the "read" virtqueue, not the "write"
virtqueue?
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + return le32_to_cpu(res->ret);
> +}
> +
> +static int virt_pstore_init(struct virtio_pstore *vps)
> +{
> + struct pstore_info *psinfo = &vps->pstore;
> + int err;
> +
> + if (!psinfo->bufsize)
> + psinfo->bufsize = VIRT_PSTORE_BUFSIZE;
> +
> + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL);
> + if (!psinfo->buf) {
> + pr_err("cannot allocate pstore buffer\n");
> + return -ENOMEM;
> + }
> +
> + psinfo->owner = THIS_MODULE;
> + psinfo->name = "virtio";
> + psinfo->open = virt_pstore_open;
> + psinfo->close = virt_pstore_close;
> + psinfo->read = virt_pstore_read;
> + psinfo->erase = virt_pstore_erase;
> + psinfo->write = virt_pstore_write;
> + psinfo->flags = PSTORE_FLAGS_FRAGILE;
> +
> + psinfo->data = vps;
> + spin_lock_init(&psinfo->buf_lock);
> +
> + err = pstore_register(psinfo);
> + if (err)
> + kfree(psinfo->buf);
Should this be free_pages_exact() instead of kfree()?
Is it safe to call pstore_register() before the remaining initialization
steps?
My understanding is that if pstore is already mounted then requests will
immediately be sent. In order to support this we'd need to initialize
everything else first before calling pstore_register().
> +
> + return err;
> +}
> +
> +static int virt_pstore_exit(struct virtio_pstore *vps)
> +{
> + struct pstore_info *psinfo = &vps->pstore;
> +
> + pstore_unregister(psinfo);
> +
> + free_pages_exact(psinfo->buf, psinfo->bufsize);
> + psinfo->buf = NULL;
> + psinfo->bufsize = 0;
> +
> + return 0;
> +}
> +
> +static int virtpstore_init_vqs(struct virtio_pstore *vps)
> +{
> + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check };
> + const char *names[] = { "pstore_read", "pstore_write" };
> +
> + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq,
> + callbacks, names);
> +}
> +
> +static void virtpstore_init_config(struct virtio_pstore *vps)
> +{
> + u32 bufsize;
> +
> + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize);
> +
> + vps->pstore.bufsize = PAGE_ALIGN(bufsize);
> +}
> +
> +static void virtpstore_confirm_config(struct virtio_pstore *vps)
> +{
> + u32 bufsize = vps->pstore.bufsize;
> +
> + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize,
> + &bufsize);
> +}
> +
> +static int virtpstore_probe(struct virtio_device *vdev)
> +{
> + struct virtio_pstore *vps;
> + int err;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "driver init: config access disabled\n");
> + return -EINVAL;
> + }
> +
> + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL);
> + if (!vps) {
> + err = -ENOMEM;
> + goto out;
> + }
> + vps->vdev = vdev;
> +
> + err = virtpstore_init_vqs(vps);
> + if (err < 0)
> + goto out_free;
> +
> + virtpstore_init_config(vps);
> +
> + err = virt_pstore_init(vps);
> + if (err)
> + goto out_del_vq;
> +
> + virtpstore_confirm_config(vps);
> +
> + init_waitqueue_head(&vps->acked);
> +
> + virtio_device_ready(vdev);
This call is needed if the .probe() function uses virtqueues before
returning. Right now it either doesn't use the virtqueues or it has
already used them in virt_pstore_init(). Please move this before
pstore_register().
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v7 6/6] powerpc: pSeries: Add pv-qspinlock build config/make
From: Michael Ellerman @ 2016-09-22 10:31 UTC (permalink / raw)
To: xinhui, kbuild test robot
Cc: peterz, benh, linux-kernel, waiman.long, virtualization, mingo,
paulus, kbuild-all, paulmck, linuxppc-dev
In-Reply-To: <57E37216.1030505@linux.vnet.ibm.com>
xinhui <xinhui.pan@linux.vnet.ibm.com> writes:
> hi, all
> ok, this patch set depends on
> https://patchwork.kernel.org/patch/8953981/ [V4] powerpc: Implement {cmp}xchg for u8 and u16
AKA: https://patchwork.ozlabs.org/patch/615480/
Sorry I saw the discussion on that and thought there'd be a new version.
But now I read the whole thread it looks like it was OK in the end.
I'll try and get it merged.
cheers
^ permalink raw reply
* Re: [PATCH v7 6/6] powerpc: pSeries: Add pv-qspinlock build config/make
From: xinhui @ 2016-09-22 5:54 UTC (permalink / raw)
To: kbuild test robot
Cc: peterz, mpe, linux-kernel, waiman.long, virtualization, mingo,
paulus, kbuild-all, benh, paulmck, linuxppc-dev
In-Reply-To: <201609191651.3BF51xRa%fengguang.wu@intel.com>
hi, all
ok, this patch set depends on
https://patchwork.kernel.org/patch/8953981/ [V4] powerpc: Implement {cmp}xchg for u8 and u16
sorry.
On 2016年09月19日 16:58, kbuild test robot wrote:
> Hi Pan,
>
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on v4.8-rc7 next-20160916]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
>
> url: https://github.com/0day-ci/linux/commits/Pan-Xinhui/Implement-qspinlock-pv-qspinlock-on-ppc/20160919-133130
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-allyesconfig (attached as .config)
> compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=powerpc
>
> All errors (new ones prefixed by >>):
>
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:326:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __cmpxchg");
> ^~~~~~~~~~~~~~~~
> In function '__cmpxchg',
> inlined from 'pv_wait_node' at kernel/locking/qspinlock_paravirt.h:328:3,
> inlined from '__pv_queued_spin_lock_slowpath' at kernel/locking/qspinlock.c:538:3:
> include/linux/compiler.h:491:38: error: call to '__compiletime_assert_326' declared with attribute error: Unsupported size for __cmpxchg
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^
> include/linux/compiler.h:474:4: note: in definition of macro '__compiletime_assert'
> prefix ## suffix(); \
> ^~~~~~
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:326:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __cmpxchg");
> ^~~~~~~~~~~~~~~~
> In function '__cmpxchg',
> inlined from 'pv_wait_head_or_lock' at kernel/locking/qspinlock_paravirt.h:109:10,
> inlined from '__pv_queued_spin_lock_slowpath' at kernel/locking/qspinlock.c:573:5:
> include/linux/compiler.h:491:38: error: call to '__compiletime_assert_326' declared with attribute error: Unsupported size for __cmpxchg
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^
> include/linux/compiler.h:474:4: note: in definition of macro '__compiletime_assert'
> prefix ## suffix(); \
> ^~~~~~
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:326:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __cmpxchg");
> ^~~~~~~~~~~~~~~~
> In function '__xchg_relaxed',
> inlined from 'pv_wait_head_or_lock' at kernel/locking/qspinlock_paravirt.h:442:8,
> inlined from '__pv_queued_spin_lock_slowpath' at kernel/locking/qspinlock.c:573:5:
> include/linux/compiler.h:491:38: error: call to '__compiletime_assert_113' declared with attribute error: Unsupported size for __xchg_local
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^
> include/linux/compiler.h:474:4: note: in definition of macro '__compiletime_assert'
> prefix ## suffix(); \
> ^~~~~~
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:113:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __xchg_local");
> ^~~~~~~~~~~~~~~~
> In function '__cmpxchg',
> inlined from 'pv_kick_node' at kernel/locking/qspinlock_paravirt.h:366:6,
> inlined from '__pv_queued_spin_lock_slowpath' at kernel/locking/qspinlock.c:616:2:
> include/linux/compiler.h:491:38: error: call to '__compiletime_assert_326' declared with attribute error: Unsupported size for __cmpxchg
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^
> include/linux/compiler.h:474:4: note: in definition of macro '__compiletime_assert'
> prefix ## suffix(); \
> ^~~~~~
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:326:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __cmpxchg");
> ^~~~~~~~~~~~~~~~
> In function '__xchg_relaxed',
> inlined from '__pv_queued_spin_lock_slowpath' at kernel/locking/qspinlock.c:184:14:
> include/linux/compiler.h:491:38: error: call to '__compiletime_assert_113' declared with attribute error: Unsupported size for __xchg_local
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^
> include/linux/compiler.h:474:4: note: in definition of macro '__compiletime_assert'
> prefix ## suffix(); \
> ^~~~~~
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:113:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __xchg_local");
> ^~~~~~~~~~~~~~~~
> In function '__cmpxchg_relaxed',
> inlined from '__pv_queued_spin_unlock' at kernel/locking/qspinlock_paravirt.h:547:11:
>>> include/linux/compiler.h:491:38: error: call to '__compiletime_assert_358' declared with attribute error: Unsupported size for __cmpxchg_relaxed
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^
> include/linux/compiler.h:474:4: note: in definition of macro '__compiletime_assert'
> prefix ## suffix(); \
> ^~~~~~
> include/linux/compiler.h:491:2: note: in expansion of macro '_compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bug.h:51:37: note: in expansion of macro 'compiletime_assert'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^~~~~~~~~~~~~~~~~~
> arch/powerpc/include/asm/cmpxchg.h:358:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> BUILD_BUG_ON_MSG(1, "Unsupported size for __cmpxchg_relaxed");
> ^~~~~~~~~~~~~~~~
>
> vim +/__compiletime_assert_358 +491 include/linux/compiler.h
>
> 9a8ab1c3 Daniel Santos 2013-02-21 475 __compiletime_error_fallback(__cond); \
> 9a8ab1c3 Daniel Santos 2013-02-21 476 } while (0)
> 9a8ab1c3 Daniel Santos 2013-02-21 477
> 9a8ab1c3 Daniel Santos 2013-02-21 478 #define _compiletime_assert(condition, msg, prefix, suffix) \
> 9a8ab1c3 Daniel Santos 2013-02-21 479 __compiletime_assert(condition, msg, prefix, suffix)
> 9a8ab1c3 Daniel Santos 2013-02-21 480
> 9a8ab1c3 Daniel Santos 2013-02-21 481 /**
> 9a8ab1c3 Daniel Santos 2013-02-21 482 * compiletime_assert - break build and emit msg if condition is false
> 9a8ab1c3 Daniel Santos 2013-02-21 483 * @condition: a compile-time constant condition to check
> 9a8ab1c3 Daniel Santos 2013-02-21 484 * @msg: a message to emit if condition is false
> 9a8ab1c3 Daniel Santos 2013-02-21 485 *
> 9a8ab1c3 Daniel Santos 2013-02-21 486 * In tradition of POSIX assert, this macro will break the build if the
> 9a8ab1c3 Daniel Santos 2013-02-21 487 * supplied condition is *false*, emitting the supplied error message if the
> 9a8ab1c3 Daniel Santos 2013-02-21 488 * compiler has support to do so.
> 9a8ab1c3 Daniel Santos 2013-02-21 489 */
> 9a8ab1c3 Daniel Santos 2013-02-21 490 #define compiletime_assert(condition, msg) \
> 9a8ab1c3 Daniel Santos 2013-02-21 @491 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> 9a8ab1c3 Daniel Santos 2013-02-21 492
> 47933ad4 Peter Zijlstra 2013-11-06 493 #define compiletime_assert_atomic_type(t) \
> 47933ad4 Peter Zijlstra 2013-11-06 494 compiletime_assert(__native_word(t), \
> 47933ad4 Peter Zijlstra 2013-11-06 495 "Need native word sized stores/loads for atomicity.")
> 47933ad4 Peter Zijlstra 2013-11-06 496
> 9c3cdc1f Linus Torvalds 2008-05-10 497 /*
> 9c3cdc1f Linus Torvalds 2008-05-10 498 * Prevent the compiler from merging or refetching accesses. The compiler
> 9c3cdc1f Linus Torvalds 2008-05-10 499 * is also forbidden from reordering successive instances of ACCESS_ONCE(),
>
> :::::: The code at line 491 was first introduced by commit
> :::::: 9a8ab1c39970a4938a72d94e6fd13be88a797590 bug.h, compiler.h: introduce compiletime_assert & BUILD_BUG_ON_MSG
>
> :::::: TO: Daniel Santos <daniel.santos@pobox.com>
> :::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: virtio_console: Less function calls in init_vqs() after error detection
From: SF Markus Elfring @ 2016-09-21 13:06 UTC (permalink / raw)
To: Amit Shah
Cc: Arnd Bergmann, Michael S. Tsirkin, Greg Kroah-Hartman,
kernel-janitors, LKML, virtualization, Julia Lawall
In-Reply-To: <20160921121038.GE1375@amit-lp.rh>
>> The kfree() function was called in up to five cases
>> by the init_vqs() function during error handling even if
>> the passed variable contained a null pointer.
>>
>> * Return directly after a call of the function "kmalloc_array" failed
>> at the beginning.
>>
>> * Split a condition check for memory allocation failures so that
>> each pointer from these function calls will be checked immediately.
>>
>> See also background information:
>> Topic "CWE-754: Improper check for unusual or exceptional conditions"
>> Link: https://cwe.mitre.org/data/definitions/754.html
>>
>> * Adjust jump targets according to the Linux coding style convention.
>
> So I've seen this series and I'm not yet sure how I feel about the
> patches - f.e. in this one, it adds more lines than it removes to
> achieve the same effect.
I find this consequence still debatable.
> I think the code is currently more readable than after these changes.
Thanks for your constructive feedback.
Can it be that an other software development concern is eventually overlooked?
> And even if kfree is called multiple times, it isn't a huge bother
I know also that the implementation of this function tolerates the passing
of null pointers.
> -- it's error case anyway, very unlikely to trigger, but keeps everything very readble.
I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.
* How much will it matter in general that four function call were performed
in this use case without checking their return values immediately?
* Should it usually be determined quicker if a required resource like
memory could be acquired before trying the next allocation?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 02/11] virtio_console: Less function calls in init_vqs() after error detection
From: Amit Shah @ 2016-09-21 12:10 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Arnd Bergmann, Michael S. Tsirkin, Greg Kroah-Hartman,
kernel-janitors, LKML, virtualization, Julia Lawall
In-Reply-To: <490b98e1-6129-f11f-55ff-94219ebce6d6@users.sourceforge.net>
Hi,
On (Wed) 14 Sep 2016 [16:01:28], SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 14:00:35 +0200
>
> The kfree() function was called in up to five cases
> by the init_vqs() function during error handling even if
> the passed variable contained a null pointer.
>
> * Return directly after a call of the function "kmalloc_array" failed
> at the beginning.
>
> * Split a condition check for memory allocation failures so that
> each pointer from these function calls will be checked immediately.
>
> See also background information:
> Topic "CWE-754: Improper check for unusual or exceptional conditions"
> Link: https://cwe.mitre.org/data/definitions/754.html
>
> * Adjust jump targets according to the Linux coding style convention.
So I've seen this series and I'm not yet sure how I feel about the
patches - f.e. in this one, it adds more lines than it removes to
achieve the same effect. I think the code is currently more readable
than after these changes. And even if kfree is called multiple times,
it isn't a huge bother -- it's error case anyway, very unlikely to
trigger, but keeps everything very readble.
Amit
^ permalink raw reply
* Re: [PATCH v9 17/19] drm/virtio: kconfig: Fix recursive dependency issue.
From: Emil Velikov @ 2016-09-21 12:09 UTC (permalink / raw)
To: Peter Griffin
Cc: devicetree, kernel, Arnd Bergmann, vinod.koul, lee.jones,
linux-remoteproc, patrice.chotard, ML dri-devel,
Linux-Kernel@Vger. Kernel. Org, David Airlie, dmaengine,
dan.j.williams, bjorn.andersson, open list:VIRTIO GPU DRIVER,
LAKML
In-Reply-To: <20160920083251.GB26093@griffinp-ThinkPad-X1-Carbon-2nd>
On 20 September 2016 at 09:32, Peter Griffin <peter.griffin@linaro.org> wrote:
> Hi Emil,
>
> On Tue, 20 Sep 2016, Emil Velikov wrote:
>
>> On 5 September 2016 at 14:16, Peter Griffin <peter.griffin@linaro.org> wrote:
>> > ST_SLIM_REMOTEPROC must select REMOTEPROC, which exposes the following
>> > recursive dependency.
>
>
>> >
>> From a humble skim through remoteproc, drm and a few other subsystems
>> I think the above is wrong. All the drivers (outside of remoteproc),
>> that I've seen, depend on the core component, they don't select it.
>
> I will let Bjorn comment on the remoteproc subsystem Kconfig design, and
> why it is like it is.
>
> For this particular SLIM_RPROC I have added it to Kconfig in keeping with all
> the other drivers in the remoteproc subsystem which has exposed this recursive
> dependency issue.
>
> For this particular kconfig symbol a quick grep reveals more drivers in
> the kernel using 'select', than 'depend on'
>
> git grep "select VIRTIO" | wc -l
> 14
>
> git grep "depends on VIRTIO" | wc -l
> 10
>
Might be worth taking a closer look into these at some point.
>
>> Furthermore most places explicitly hide the drivers from the menu if
>> the core component isn't enabled.
>
> Remoteproc subsystem takes a different approach, the core code is only enabled
> if a driver which relies on it is enabled. This IMHO makes sense, as
> remoteproc is not widely used (only a few particular ARM SoC's).
>
> It is true that for subsystems which rely on the core component being
> explicitly enabled, they often tend to hide drivers which depend on it
> from the menu unless it is. This also makes sense.
>
>>
>> Is there something that requires such a different/unusual behaviour in
>> remoteproc ?
>>
>
> I'm not sure it is that unusual...looking at config USB, it selects USB_COMMON in
> mfd subsystem, client drivers select MFD_CORE.
>
On the USB case I'm not sure what the reasoning behind the USB vs
USB_COMMON split. In seems that one could just fold them, but that's
another topic. On the MFD side... it follows the select {,mis,ab}use.
With one (the only one?) MFD driver not using/selecting MFD_CORE doing
it's own version of mfd_add_devices... which could be reworked,
possibly.
> I've added Arnd to this thread, as I've seen lots of Kconfig patches from him
> recently, maybe he has some thoughts on whether this is the correct fix or not?
>
Ack. Fwiw, I believe that the reasoning put by Jani is perfeclty
reasonable, but it'll be great to hear others as well.
Thanks
Emil
^ 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