Linux virtualization list
 help / color / mirror / Atom feed
* [PATCH] virtio_console: initialize vtermno value for ports
From: Pankaj Gupta @ 2019-03-19  6:04 UTC (permalink / raw)
  To: linux-kernel, virtualization; +Cc: pagupta, arnd, mst, gregkh, amit, siliu

For regular serial ports we do not initialize value of vtermno 
variable. A garbage value is assigned for non console ports.
The value can be observed as a random integer with [1]. 

[1] vim /sys/kernel/debug/virtio-ports/vport*p*

This patch initialize the value of vtermno for console serial 
ports to '1' and regular serial ports are initiaized to '0'.

Reported-by: siliu@redhat.com
Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 drivers/char/virtio_console.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fbeb71953526..05dbfdb9f4af 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -75,7 +75,7 @@ struct ports_driver_data {
 	/* All the console devices handled by this driver */
 	struct list_head consoles;
 };
-static struct ports_driver_data pdrvdata;
+static struct ports_driver_data pdrvdata = { .next_vtermno = 1};
 
 static DEFINE_SPINLOCK(pdrvdata_lock);
 static DECLARE_COMPLETION(early_console_added);
@@ -1394,6 +1394,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 	port->async_queue = NULL;
 
 	port->cons.ws.ws_row = port->cons.ws.ws_col = 0;
+	port->cons.vtermno = 0;
 
 	port->host_connected = port->guest_connected = false;
 	port->stats = (struct port_stats) { 0 };
-- 
2.20.1

^ permalink raw reply related

* Re: [PATCH] virtio_net: remove hcpu from virtnet_clean_affinity
From: David Miller @ 2019-03-19  5:26 UTC (permalink / raw)
  To: peterx; +Cc: netdev, virtualization, linux-kernel, mst
In-Reply-To: <20190318065606.17100-1-peterx@redhat.com>

From: Peter Xu <peterx@redhat.com>
Date: Mon, 18 Mar 2019 14:56:06 +0800

> The variable is never used.
> 
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: virtualization@lists.linux-foundation.org
> CC: netdev@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Peter Xu <peterx@redhat.com>

This looks rather uncontroversial and straightforward so I've applied
this to net-next.

Thanks.

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Dongli Zhang @ 2019-03-19  2:22 UTC (permalink / raw)
  To: Jason Wang, Cornelia Huck
  Cc: linux-block, axboe, virtualization, linux-kernel, mst
In-Reply-To: <1df52766-88fb-6b23-d160-b891c3017133@redhat.com>

Hi Jason,

On 3/18/19 3:47 PM, Jason Wang wrote:
> 
> On 2019/3/15 下午8:41, Cornelia Huck wrote:
>> On Fri, 15 Mar 2019 12:50:11 +0800
>> Jason Wang <jasowang@redhat.com> wrote:
>>
>>> Or something like I proposed several years ago?
>>> https://do-db2.lkml.org/lkml/2014/12/25/169
>>>
>>> Btw, for virtio-net, I think we actually want to go for having a maximum
>>> number of supported queues like what hardware did. This would be useful
>>> for e.g cpu hotplug or XDP (requires per cpu TX queue). But the current
>>> vector allocation doesn't support this which will results all virtqueues
>>> to share a single vector. We may indeed need more flexible policy here.
>> I think it should be possible for the driver to give the transport
>> hints how to set up their queues/interrupt structures. (The driver
>> probably knows best about its requirements.) Perhaps whether a queue is
>> high or low frequency, or whether it should be low latency, or even
>> whether two queues could share a notification mechanism without
>> drawbacks. It's up to the transport to make use of that information, if
>> possible.
> 
> 
> Exactly and it was what the above series tried to do by providing hints of e.g
> which queues want to share a notification.
> 

I read about your patch set on providing more flexibility of queue-to-vector
mapping.

One use case of the patch set is we would be able to enable more queues when
there is limited number of vectors.

Another use case we may classify queues as hight priority or low priority as
mentioned by Cornelia.

For virtio-blk, we may extend virtio-blk based on this patch set to enable
something similar to write_queues/poll_queues in nvme, when (set->nr_maps != 1).


Yet, the question I am asking in this email thread is for a difference scenario.

The issue is not we are not having enough vectors (although this is why only 1
vector is allocated for all virtio-blk queues). As so far virtio-blk has
(set->nr_maps == 1), block layer would limit the number of hw queues by
nr_cpu_ids, we indeed do not need more than nr_cpu_ids hw queues in virtio-blk.

That's why I ask why not change the flow as below options when the number of
supported hw queues is more than nr_cpu_ids (and set->nr_maps == 1. virtio-blk
does not set nr_maps and block layer would set it to 1 when the driver does not
specify with a value):

option 1:
As what nvme and xen-netfront do, limit the hw queue number by nr_cpu_ids.

option 2:
If the vectors is not enough, use the max number vector (indeed nr_cpu_ids) as
number of hw queues.

option 3:
We should allow more vectors even the block layer would support at most
nr_cpu_ids queues.


I understand a new policy for queue-vector mapping is very helpful. I am just
asking the question from block layer's point of view.

Thank you very much!

Dongli Zhang
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH v3 1/2] drm/virtio: Add window server support
From: Tomeu Vizoso @ 2019-03-18 12:47 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Michael S. Tsirkin, David Airlie, linux-kernel, dri-devel,
	virtualization, Zach Reizner, kernel
In-Reply-To: <49785e0d-936a-c3b4-62dd-aafc7083a942@collabora.com>

[Tomasz wants to comment, adding him to CC]

On 2/5/18 9:19 AM, Tomeu Vizoso wrote:
> On 1 February 2018 at 17:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> Hi,
>>
>> Sorry for joining the party late. Had a broken finger and was
>> offline for a bunch of weeks (and a buif backlog afterwards ...).
> 
> Hi, no problem, hope it's fine now.
> 
>>> This is to allow clients running within VMs to be able to
>>> communicate with a compositor in the host. Clients will use the
>>> communication protocol that the compositor supports, and virtio-gpu
>>> will assist with making buffers available in both sides, and
>>> copying content as needed.
>>
>> Why not use virtio-vsock to run the wayland protocol? I don't like
>> the idea to duplicate something with very simliar functionality in 
>> virtio-gpu.
> 
> The reason for abandoning that approach was the type of objects that
> could be shared via virtio-vsock would be extremely limited. Besides
> that being potentially confusing to users, it would mean from the
> implementation side that either virtio-vsock would gain a dependency on
> the drm subsystem, or an appropriate abstraction for shareable buffers
> would need to be added for little gain.
> 
> Another factor that was taken into account was that the complexity
> required for implementing passing protocol data around was very small
> when compared with the buffer sharing mechanism.
> 
>>> It is expected that a service in the guest will act as a proxy, 
>>> interacting with virtio-gpu to support unmodified clients.
>>
>> If you have a guest proxy anyway using virtio-sock for the protocol 
>> stream and virtio-gpu for buffer sharing (and some day 3d rendering
>> too) should work fine I think.
> 
> If I understand correctly your proposal, virtio-gpu would be used for
> creating buffers that could be shared across domains, but something
> equivalent to SCM_RIGHTS would still be needed in virtio-vsock?
> 
> If so, that's what was planned initially, with the concern being that we
> would be adding a bunch of complexity to virtio-vsock that would be only
> used in this specific use case. Then we would also need to figure out
> how virtio-vsock would be able to work with buffers from virtio-gpu
> (either direct dependency or a new abstraction).
> 
> If the mechanics of passing presentation data were very complex, I think
> this approach would have more merit. But as you can see from the code,
> it isn't that bad.
> 
>>> When the client notifies the compositor that it can read from that
> buffer,
>>> the proxy should copy the contents from the SHM region to the
>>> virtio-gpu resource and call DRM_VIRTGPU_TRANSFER_TO_HOST.
>>
>> What is the plan for the host side? I see basically two options. Either 
>> implement the host wayland proxy directly in qemu. Or
>> implement it as separate process, which then needs some help from
>> qemu to get access to the buffers. The later would allow qemu running
>> independant from the desktop session.
> 
> Regarding synchronizing buffers, this will stop becoming needed in
> subsequent commits as all shared memory is allocated in the host and
> mapped to the guest via KVM_SET_USER_MEMORY_REGION.
> 
> This is already the case for buffers passed from the compositor to the
> clients (see patch 2/2), and I'm working on the equivalent for buffers
> from the guest to the host (clients still have to create buffers with
> DRM_VIRTGPU_RESOURCE_CREATE but they will be only backend by host memory
> so no calls to DRM_VIRTGPU_TRANSFER_TO_HOST are needed).
> 
> But in the case that we still need a proxy for some reason on the host
> side, I think it would be better to have it in the same process where
> virtio-gpu is implemented. In crosvm's case it would be in a process
> separate from the main VMM, as device processes are isolated from each
> other with minijail (see
> https://chromium.googlesource.com/chromiumos/platform/crosvm/ ).
> 
> Regards,
> 
> Tomeu
> 

^ permalink raw reply

* Re: [PATCH v2 5/6] drm/virtio: drop fencing in virtio_gpu_resource_create_ioctl
From: Gerd Hoffmann @ 2019-03-18 11:35 UTC (permalink / raw)
  To: Gurchetan Singh
  Cc: David Airlie, open list, ML dri-devel,
	open list:VIRTIO GPU DRIVER
In-Reply-To: <CAAfnVB=z36CtTsCiEy1uVTNvE9DVuSDKLNdMVFO+tBJqiQ7s1A@mail.gmail.com>

  Hi,

> virtio_gpu_execbuffer_ioctl
> virtio_gpu_resource_create_ioctl with fence resource
> virtio_gpu_wait_ioctl with that fence resource --> associated with a
> GL wait on the host side

After a long break (sidetracked with other stuff) I've finally sent v3
which should address this.

cheers,
  Gerd

^ permalink raw reply

* [PATCH v3 5/5] drm/virtio: rework resource creation workflow.
From: Gerd Hoffmann @ 2019-03-18 11:33 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER,
	Noralf Trønnes, Daniel Vetter, Gurchetan Singh
In-Reply-To: <20190318113332.10900-1-kraxel@redhat.com>

This patch moves the virtio_gpu_cmd_create_resource() call (which
notifies the host about the new resource created) into the
virtio_gpu_object_create() function.  That way we can call
virtio_gpu_cmd_create_resource() before ttm_bo_init(), so the host
already knows about the object when ttm initializes the object and calls
our driver callbacks.

Specifically the object is already created when the
virtio_gpu_ttm_tt_bind() callback invokes virtio_gpu_object_attach(),
so the extra virtio_gpu_object_attach() calls done after
virtio_gpu_object_create() are not needed any more.

The fence support for the create ioctl becomes a bit more tricky though.
The code moved into virtio_gpu_object_create() too.  We first submit the
(fenced) virtio_gpu_cmd_create_resource() command, then initialize the
ttm object, and finally attach just created object to the fence for the
command in case it didn't finish yet.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    | 18 +++++--
 drivers/gpu/drm/virtio/virtgpu_fence.c  |  4 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c    | 19 ++-----
 drivers/gpu/drm/virtio/virtgpu_ioctl.c  | 72 ++++---------------------
 drivers/gpu/drm/virtio/virtgpu_object.c | 47 ++++++++++++++--
 drivers/gpu/drm/virtio/virtgpu_vq.c     | 14 ++---
 6 files changed, 83 insertions(+), 91 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9b01cb34a616..491dec0712b3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -55,7 +55,9 @@ struct virtio_gpu_object_params {
 	uint32_t width;
 	uint32_t height;
 	unsigned long size;
+	bool dumb;
 	/* 3d */
+	bool virgl;
 	uint32_t target;
 	uint32_t bind;
 	uint32_t depth;
@@ -219,6 +221,9 @@ struct virtio_gpu_fpriv {
 /* virtio_ioctl.c */
 #define DRM_VIRTIO_NUM_IOCTLS 10
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
+int virtio_gpu_object_list_validate(struct ww_acquire_ctx *ticket,
+				    struct list_head *head);
+void virtio_gpu_unref_list(struct list_head *head);
 
 /* virtio_kms.c */
 int virtio_gpu_init(struct drm_device *dev);
@@ -241,7 +246,8 @@ void virtio_gpu_gem_object_close(struct drm_gem_object *obj,
 				 struct drm_file *file);
 struct virtio_gpu_object*
 virtio_gpu_alloc_object(struct drm_device *dev,
-			struct virtio_gpu_object_params *params);
+			struct virtio_gpu_object_params *params,
+			struct virtio_gpu_fence *fence);
 int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 				struct drm_device *dev,
 				struct drm_mode_create_dumb *args);
@@ -258,7 +264,8 @@ int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev);
 void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev);
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 				    struct virtio_gpu_object *bo,
-				    struct virtio_gpu_object_params *params);
+				    struct virtio_gpu_object_params *params,
+				    struct virtio_gpu_fence *fence);
 void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
 				   uint32_t resource_id);
 void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
@@ -317,7 +324,8 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
 void
 virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
 				  struct virtio_gpu_object *bo,
-				  struct virtio_gpu_object_params *params);
+				  struct virtio_gpu_object_params *params,
+				  struct virtio_gpu_fence *fence);
 void virtio_gpu_ctrl_ack(struct virtqueue *vq);
 void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_fence_ack(struct virtqueue *vq);
@@ -345,6 +353,7 @@ void virtio_gpu_ttm_fini(struct virtio_gpu_device *vgdev);
 int virtio_gpu_mmap(struct file *filp, struct vm_area_struct *vma);
 
 /* virtio_gpu_fence.c */
+bool virtio_fence_signaled(struct dma_fence *f);
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(
 	struct virtio_gpu_device *vgdev);
 int virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
@@ -356,7 +365,8 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
 /* virtio_gpu_object */
 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 			     struct virtio_gpu_object_params *params,
-			     struct virtio_gpu_object **bo_ptr);
+			     struct virtio_gpu_object **bo_ptr,
+			     struct virtio_gpu_fence *fence);
 void virtio_gpu_object_kunmap(struct virtio_gpu_object *bo);
 int virtio_gpu_object_kmap(struct virtio_gpu_object *bo);
 int virtio_gpu_object_get_sg_table(struct virtio_gpu_device *qdev,
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 21bd4c4a32d1..87d1966192f4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -36,7 +36,7 @@ static const char *virtio_get_timeline_name(struct dma_fence *f)
 	return "controlq";
 }
 
-static bool virtio_signaled(struct dma_fence *f)
+bool virtio_fence_signaled(struct dma_fence *f)
 {
 	struct virtio_gpu_fence *fence = to_virtio_fence(f);
 
@@ -62,7 +62,7 @@ static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
 static const struct dma_fence_ops virtio_fence_ops = {
 	.get_driver_name     = virtio_get_driver_name,
 	.get_timeline_name   = virtio_get_timeline_name,
-	.signaled            = virtio_signaled,
+	.signaled            = virtio_fence_signaled,
 	.fence_value_str     = virtio_fence_value_str,
 	.timeline_value_str  = virtio_timeline_value_str,
 };
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index ac15863643e7..1e49e08dd545 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -36,13 +36,14 @@ void virtio_gpu_gem_free_object(struct drm_gem_object *gem_obj)
 
 struct virtio_gpu_object*
 virtio_gpu_alloc_object(struct drm_device *dev,
-			struct virtio_gpu_object_params *params)
+			struct virtio_gpu_object_params *params,
+			struct virtio_gpu_fence *fence)
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_object *obj;
 	int ret;
 
-	ret = virtio_gpu_object_create(vgdev, params, &obj);
+	ret = virtio_gpu_object_create(vgdev, params, &obj, fence);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -59,7 +60,7 @@ int virtio_gpu_gem_create(struct drm_file *file,
 	int ret;
 	u32 handle;
 
-	obj = virtio_gpu_alloc_object(dev, params);
+	obj = virtio_gpu_alloc_object(dev, params, NULL);
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
@@ -82,9 +83,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 				struct drm_device *dev,
 				struct drm_mode_create_dumb *args)
 {
-	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_gem_object *gobj;
-	struct virtio_gpu_object *obj;
 	struct virtio_gpu_object_params params = { 0 };
 	int ret;
 	uint32_t pitch;
@@ -100,20 +99,12 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	params.width = args->width;
 	params.height = args->height;
 	params.size = args->size;
+	params.dumb = true;
 	ret = virtio_gpu_gem_create(file_priv, dev, &params, &gobj,
 				    &args->handle);
 	if (ret)
 		goto fail;
 
-	obj = gem_to_virtio_gpu_obj(gobj);
-	virtio_gpu_cmd_create_resource(vgdev, obj, &params);
-
-	/* attach the object to the resource */
-	ret = virtio_gpu_object_attach(vgdev, obj, NULL);
-	if (ret)
-		goto fail;
-
-	obj->dumb = true;
 	args->pitch = pitch;
 	return ret;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 143898b0d628..949a264985fc 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -54,8 +54,8 @@ static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
 					 &virtio_gpu_map->offset);
 }
 
-static int virtio_gpu_object_list_validate(struct ww_acquire_ctx *ticket,
-					   struct list_head *head)
+int virtio_gpu_object_list_validate(struct ww_acquire_ctx *ticket,
+				    struct list_head *head)
 {
 	struct ttm_operation_ctx ctx = { false, false };
 	struct ttm_validate_buffer *buf;
@@ -79,7 +79,7 @@ static int virtio_gpu_object_list_validate(struct ww_acquire_ctx *ticket,
 	return 0;
 }
 
-static void virtio_gpu_unref_list(struct list_head *head)
+void virtio_gpu_unref_list(struct list_head *head)
 {
 	struct ttm_validate_buffer *buf;
 	struct ttm_buffer_object *bo;
@@ -275,14 +275,11 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_virtgpu_resource_create *rc = data;
+	struct virtio_gpu_fence *fence;
 	int ret;
 	struct virtio_gpu_object *qobj;
 	struct drm_gem_object *obj;
 	uint32_t handle = 0;
-	struct list_head validate_list;
-	struct ttm_validate_buffer mainbuf;
-	struct virtio_gpu_fence *fence = NULL;
-	struct ww_acquire_ctx ticket;
 	struct virtio_gpu_object_params params = { 0 };
 
 	if (vgdev->has_virgl_3d == false) {
@@ -298,14 +295,12 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 			return -EINVAL;
 	}
 
-	INIT_LIST_HEAD(&validate_list);
-	memset(&mainbuf, 0, sizeof(struct ttm_validate_buffer));
-
 	params.format = rc->format;
 	params.width = rc->width;
 	params.height = rc->height;
 	params.size = rc->size;
 	if (vgdev->has_virgl_3d) {
+		params.virgl = true;
 		params.target = rc->target;
 		params.bind = rc->bind;
 		params.depth = rc->depth;
@@ -318,72 +313,25 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	if (params.size == 0)
 		params.size = PAGE_SIZE;
 
-	qobj = virtio_gpu_alloc_object(dev, &params);
+	fence = virtio_gpu_fence_alloc(vgdev);
+	if (!fence)
+		return -ENOMEM;
+	qobj = virtio_gpu_alloc_object(dev, &params, fence);
+	dma_fence_put(&fence->f);
 	if (IS_ERR(qobj))
 		return PTR_ERR(qobj);
 	obj = &qobj->gem_base;
 
-	if (!vgdev->has_virgl_3d) {
-		virtio_gpu_cmd_create_resource(vgdev, qobj, &params);
-
-		ret = virtio_gpu_object_attach(vgdev, qobj, NULL);
-	} else {
-		/* use a gem reference since unref list undoes them */
-		drm_gem_object_get(&qobj->gem_base);
-		mainbuf.bo = &qobj->tbo;
-		list_add(&mainbuf.head, &validate_list);
-
-		ret = virtio_gpu_object_list_validate(&ticket, &validate_list);
-		if (ret) {
-			DRM_DEBUG("failed to validate\n");
-			goto fail_unref;
-		}
-
-		fence = virtio_gpu_fence_alloc(vgdev);
-		if (!fence) {
-			ret = -ENOMEM;
-			goto fail_backoff;
-		}
-
-		virtio_gpu_cmd_resource_create_3d(vgdev, qobj, &params);
-		ret = virtio_gpu_object_attach(vgdev, qobj, fence);
-		if (ret) {
-			dma_fence_put(&fence->f);
-			goto fail_backoff;
-		}
-		ttm_eu_fence_buffer_objects(&ticket, &validate_list, &fence->f);
-	}
-
 	ret = drm_gem_handle_create(file_priv, obj, &handle);
 	if (ret) {
-
 		drm_gem_object_release(obj);
-		if (vgdev->has_virgl_3d) {
-			virtio_gpu_unref_list(&validate_list);
-			dma_fence_put(&fence->f);
-		}
 		return ret;
 	}
 	drm_gem_object_put_unlocked(obj);
 
 	rc->res_handle = qobj->hw_res_handle; /* similiar to a VM address */
 	rc->bo_handle = handle;
-
-	if (vgdev->has_virgl_3d) {
-		virtio_gpu_unref_list(&validate_list);
-		dma_fence_put(&fence->f);
-	}
 	return 0;
-fail_backoff:
-	ttm_eu_backoff_reservation(&ticket, &validate_list);
-fail_unref:
-	if (vgdev->has_virgl_3d) {
-		virtio_gpu_unref_list(&validate_list);
-		dma_fence_put(&fence->f);
-	}
-//fail_obj:
-//	drm_gem_object_handle_unreference_unlocked(obj);
-	return ret;
 }
 
 static int virtio_gpu_resource_info_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 538b93417f70..b2da31310d24 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -23,6 +23,8 @@
  * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <drm/ttm/ttm_execbuf_util.h>
+
 #include "virtgpu_drv.h"
 
 static int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
@@ -92,7 +94,8 @@ static void virtio_gpu_init_ttm_placement(struct virtio_gpu_object *vgbo)
 
 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 			     struct virtio_gpu_object_params *params,
-			     struct virtio_gpu_object **bo_ptr)
+			     struct virtio_gpu_object **bo_ptr,
+			     struct virtio_gpu_fence *fence)
 {
 	struct virtio_gpu_object *bo;
 	size_t acc_size;
@@ -118,9 +121,15 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 		kfree(bo);
 		return ret;
 	}
-	bo->dumb = false;
+	bo->dumb = params->dumb;
+
+	if (params->virgl) {
+		virtio_gpu_cmd_resource_create_3d(vgdev, bo, params, fence);
+	} else {
+		virtio_gpu_cmd_create_resource(vgdev, bo, params, fence);
+	}
+
 	virtio_gpu_init_ttm_placement(bo);
-
 	ret = ttm_bo_init(&vgdev->mman.bdev, &bo->tbo, params->size,
 			  ttm_bo_type_device, &bo->placement, 0,
 			  true, acc_size, NULL, NULL,
@@ -129,6 +138,38 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 	if (ret != 0)
 		return ret;
 
+	if (fence) {
+		struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
+		struct list_head validate_list;
+		struct ttm_validate_buffer mainbuf;
+		struct ww_acquire_ctx ticket;
+		unsigned long irq_flags;
+		bool signaled;
+
+		INIT_LIST_HEAD(&validate_list);
+		memset(&mainbuf, 0, sizeof(struct ttm_validate_buffer));
+
+		/* use a gem reference since unref list undoes them */
+		drm_gem_object_get(&bo->gem_base);
+		mainbuf.bo = &bo->tbo;
+		list_add(&mainbuf.head, &validate_list);
+
+		ret = virtio_gpu_object_list_validate(&ticket, &validate_list);
+		if (ret == 0) {
+			spin_lock_irqsave(&drv->lock, irq_flags);
+			signaled = virtio_fence_signaled(&fence->f);
+			if (!signaled)
+				/* virtio create command still in flight */
+				ttm_eu_fence_buffer_objects(&ticket, &validate_list,
+							    &fence->f);
+			spin_unlock_irqrestore(&drv->lock, irq_flags);
+			if (signaled)
+				/* virtio create command finished */
+				ttm_eu_backoff_reservation(&ticket, &validate_list);
+		}
+		virtio_gpu_unref_list(&validate_list);
+	}
+
 	*bo_ptr = bo;
 	return 0;
 }
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index ca93ec6ca3c5..e62fe24b1a2e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -376,7 +376,8 @@ static int virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
 /* create a basic resource */
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 				    struct virtio_gpu_object *bo,
-				    struct virtio_gpu_object_params *params)
+				    struct virtio_gpu_object_params *params,
+				    struct virtio_gpu_fence *fence)
 {
 	struct virtio_gpu_resource_create_2d *cmd_p;
 	struct virtio_gpu_vbuffer *vbuf;
@@ -390,7 +391,7 @@ void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 	cmd_p->width = cpu_to_le32(params->width);
 	cmd_p->height = cpu_to_le32(params->height);
 
-	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+	virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence);
 	bo->created = true;
 }
 
@@ -826,7 +827,8 @@ void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,
 void
 virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
 				  struct virtio_gpu_object *bo,
-				  struct virtio_gpu_object_params *params)
+				  struct virtio_gpu_object_params *params,
+				  struct virtio_gpu_fence *fence)
 {
 	struct virtio_gpu_resource_create_3d *cmd_p;
 	struct virtio_gpu_vbuffer *vbuf;
@@ -848,7 +850,7 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
 	cmd_p->nr_samples = cpu_to_le32(params->nr_samples);
 	cmd_p->flags = cpu_to_le32(params->flags);
 
-	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+	virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence);
 	bo->created = true;
 }
 
@@ -932,8 +934,8 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
 	struct scatterlist *sg;
 	int si, nents;
 
-	if (!obj->created)
-		return 0;
+	if (WARN_ON_ONCE(!obj->created))
+		return -EINVAL;
 
 	if (!obj->pages) {
 		int ret;
-- 
2.18.1

^ permalink raw reply related

* [PATCH v3 4/5] drm/virtio: params struct for virtio_gpu_cmd_create_resource_3d()
From: Gerd Hoffmann @ 2019-03-18 11:33 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER,
	Noralf Trønnes, Daniel Vetter, Gurchetan Singh
In-Reply-To: <20190318113332.10900-1-kraxel@redhat.com>

Add 3d resource parameters to virtio_gpu_object_params struct.  With
that in place we can use it for virtio_gpu_cmd_resource_create_3d()
calls.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 10 +++++++++-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 25 ++++++++++---------------
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 16 +++++++++++++---
 3 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 5b38c397afbc..9b01cb34a616 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -55,6 +55,14 @@ struct virtio_gpu_object_params {
 	uint32_t width;
 	uint32_t height;
 	unsigned long size;
+	/* 3d */
+	uint32_t target;
+	uint32_t bind;
+	uint32_t depth;
+	uint32_t array_size;
+	uint32_t last_level;
+	uint32_t nr_samples;
+	uint32_t flags;
 };
 
 struct virtio_gpu_object {
@@ -309,7 +317,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
 void
 virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
 				  struct virtio_gpu_object *bo,
-				  struct virtio_gpu_resource_create_3d *rc_3d);
+				  struct virtio_gpu_object_params *params);
 void virtio_gpu_ctrl_ack(struct virtqueue *vq);
 void virtio_gpu_cursor_ack(struct virtqueue *vq);
 void virtio_gpu_fence_ack(struct virtqueue *vq);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index d1751ee488e5..143898b0d628 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -283,7 +283,6 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	struct ttm_validate_buffer mainbuf;
 	struct virtio_gpu_fence *fence = NULL;
 	struct ww_acquire_ctx ticket;
-	struct virtio_gpu_resource_create_3d rc_3d;
 	struct virtio_gpu_object_params params = { 0 };
 
 	if (vgdev->has_virgl_3d == false) {
@@ -306,7 +305,15 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	params.width = rc->width;
 	params.height = rc->height;
 	params.size = rc->size;
-
+	if (vgdev->has_virgl_3d) {
+		params.target = rc->target;
+		params.bind = rc->bind;
+		params.depth = rc->depth;
+		params.array_size = rc->array_size;
+		params.last_level = rc->last_level;
+		params.nr_samples = rc->nr_samples;
+		params.flags = rc->flags;
+	}
 	/* allocate a single page size object */
 	if (params.size == 0)
 		params.size = PAGE_SIZE;
@@ -332,25 +339,13 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 			goto fail_unref;
 		}
 
-		rc_3d.resource_id = cpu_to_le32(qobj->hw_res_handle);
-		rc_3d.target = cpu_to_le32(rc->target);
-		rc_3d.format = cpu_to_le32(rc->format);
-		rc_3d.bind = cpu_to_le32(rc->bind);
-		rc_3d.width = cpu_to_le32(rc->width);
-		rc_3d.height = cpu_to_le32(rc->height);
-		rc_3d.depth = cpu_to_le32(rc->depth);
-		rc_3d.array_size = cpu_to_le32(rc->array_size);
-		rc_3d.last_level = cpu_to_le32(rc->last_level);
-		rc_3d.nr_samples = cpu_to_le32(rc->nr_samples);
-		rc_3d.flags = cpu_to_le32(rc->flags);
-
 		fence = virtio_gpu_fence_alloc(vgdev);
 		if (!fence) {
 			ret = -ENOMEM;
 			goto fail_backoff;
 		}
 
-		virtio_gpu_cmd_resource_create_3d(vgdev, qobj, &rc_3d);
+		virtio_gpu_cmd_resource_create_3d(vgdev, qobj, &params);
 		ret = virtio_gpu_object_attach(vgdev, qobj, fence);
 		if (ret) {
 			dma_fence_put(&fence->f);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 363b8b85775e..ca93ec6ca3c5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -826,7 +826,7 @@ void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,
 void
 virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
 				  struct virtio_gpu_object *bo,
-				  struct virtio_gpu_resource_create_3d *rc_3d)
+				  struct virtio_gpu_object_params *params)
 {
 	struct virtio_gpu_resource_create_3d *cmd_p;
 	struct virtio_gpu_vbuffer *vbuf;
@@ -834,9 +834,19 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
 	cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
 	memset(cmd_p, 0, sizeof(*cmd_p));
 
-	*cmd_p = *rc_3d;
 	cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_CREATE_3D);
-	cmd_p->hdr.flags = 0;
+	cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
+	cmd_p->format = cpu_to_le32(params->format);
+	cmd_p->width = cpu_to_le32(params->width);
+	cmd_p->height = cpu_to_le32(params->height);
+
+	cmd_p->target = cpu_to_le32(params->target);
+	cmd_p->bind = cpu_to_le32(params->bind);
+	cmd_p->depth = cpu_to_le32(params->depth);
+	cmd_p->array_size = cpu_to_le32(params->array_size);
+	cmd_p->last_level = cpu_to_le32(params->last_level);
+	cmd_p->nr_samples = cpu_to_le32(params->nr_samples);
+	cmd_p->flags = cpu_to_le32(params->flags);
 
 	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
 	bo->created = true;
-- 
2.18.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related

* [PATCH v3 3/5] drm/virtio: params struct for virtio_gpu_cmd_create_resource()
From: Gerd Hoffmann @ 2019-03-18 11:33 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER,
	Noralf Trønnes, Daniel Vetter, Gurchetan Singh
In-Reply-To: <20190318113332.10900-1-kraxel@redhat.com>

Add format, width and height fields to the virtio_gpu_object_params
struct.  With that in place we can use the parameter struct for
virtio_gpu_cmd_create_resource() calls too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 ++++---
 drivers/gpu/drm/virtio/virtgpu_gem.c   |  8 ++++----
 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  6 ++++--
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 10 ++++------
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 16746c0fc0cf..5b38c397afbc 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -51,6 +51,9 @@
 #define DRIVER_PATCHLEVEL 0
 
 struct virtio_gpu_object_params {
+	uint32_t format;
+	uint32_t width;
+	uint32_t height;
 	unsigned long size;
 };
 
@@ -247,9 +250,7 @@ int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev);
 void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev);
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 				    struct virtio_gpu_object *bo,
-				    uint32_t format,
-				    uint32_t width,
-				    uint32_t height);
+				    struct virtio_gpu_object_params *params);
 void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
 				   uint32_t resource_id);
 void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index ffeb734003e4..ac15863643e7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -88,7 +88,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	struct virtio_gpu_object_params params = { 0 };
 	int ret;
 	uint32_t pitch;
-	uint32_t format;
 
 	if (args->bpp != 32)
 		return -EINVAL;
@@ -97,16 +96,17 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	args->size = pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
+	params.format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
+	params.width = args->width;
+	params.height = args->height;
 	params.size = args->size;
 	ret = virtio_gpu_gem_create(file_priv, dev, &params, &gobj,
 				    &args->handle);
 	if (ret)
 		goto fail;
 
-	format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
 	obj = gem_to_virtio_gpu_obj(gobj);
-	virtio_gpu_cmd_create_resource(vgdev, obj, format,
-				       args->width, args->height);
+	virtio_gpu_cmd_create_resource(vgdev, obj, &params);
 
 	/* attach the object to the resource */
 	ret = virtio_gpu_object_attach(vgdev, obj, NULL);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 3b05320bcf73..d1751ee488e5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -302,6 +302,9 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	INIT_LIST_HEAD(&validate_list);
 	memset(&mainbuf, 0, sizeof(struct ttm_validate_buffer));
 
+	params.format = rc->format;
+	params.width = rc->width;
+	params.height = rc->height;
 	params.size = rc->size;
 
 	/* allocate a single page size object */
@@ -314,8 +317,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	obj = &qobj->gem_base;
 
 	if (!vgdev->has_virgl_3d) {
-		virtio_gpu_cmd_create_resource(vgdev, qobj, rc->format,
-					       rc->width, rc->height);
+		virtio_gpu_cmd_create_resource(vgdev, qobj, &params);
 
 		ret = virtio_gpu_object_attach(vgdev, qobj, NULL);
 	} else {
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 6bc2008b0d0d..363b8b85775e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -376,9 +376,7 @@ static int virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
 /* create a basic resource */
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 				    struct virtio_gpu_object *bo,
-				    uint32_t format,
-				    uint32_t width,
-				    uint32_t height)
+				    struct virtio_gpu_object_params *params)
 {
 	struct virtio_gpu_resource_create_2d *cmd_p;
 	struct virtio_gpu_vbuffer *vbuf;
@@ -388,9 +386,9 @@ void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 
 	cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_CREATE_2D);
 	cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
-	cmd_p->format = cpu_to_le32(format);
-	cmd_p->width = cpu_to_le32(width);
-	cmd_p->height = cpu_to_le32(height);
+	cmd_p->format = cpu_to_le32(params->format);
+	cmd_p->width = cpu_to_le32(params->width);
+	cmd_p->height = cpu_to_le32(params->height);
 
 	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
 	bo->created = true;
-- 
2.18.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related

* [PATCH v3 2/5] drm/virtio: use struct to pass params to virtio_gpu_object_create()
From: Gerd Hoffmann @ 2019-03-18 11:33 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER,
	Noralf Trønnes, Daniel Vetter, Gurchetan Singh
In-Reply-To: <20190318113332.10900-1-kraxel@redhat.com>

Create virtio_gpu_object_params, use that to pass object parameters to
virtio_gpu_object_create.  This is just the first step, followup patches
will add more parameters to the struct.  The plan is to use the struct
for all object parameters.

Drop unused "kernel" parameter for virtio_gpu_alloc_object(), it is
unused and always false.

Also drop "pinned" parameter.  virtio-gpu doesn't shuffle around
objects, so effecively they all are pinned anyway.  Hardcode
TTM_PL_FLAG_NO_EVICT so ttm knows.  Doesn't change much for the moment
as virtio-gpu supports TTM_PL_FLAG_TT only so there is no opportunity to
move around objects.  That'll probably change in the future though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    | 14 +++++++-----
 drivers/gpu/drm/virtio/virtgpu_gem.c    | 16 ++++++++------
 drivers/gpu/drm/virtio/virtgpu_ioctl.c  | 10 ++++-----
 drivers/gpu/drm/virtio/virtgpu_object.c | 29 ++++++++++---------------
 4 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 86a264cee362..16746c0fc0cf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -50,6 +50,10 @@
 #define DRIVER_MINOR 1
 #define DRIVER_PATCHLEVEL 0
 
+struct virtio_gpu_object_params {
+	unsigned long size;
+};
+
 struct virtio_gpu_object {
 	struct drm_gem_object gem_base;
 	uint32_t hw_res_handle;
@@ -217,16 +221,16 @@ int virtio_gpu_gem_init(struct virtio_gpu_device *vgdev);
 void virtio_gpu_gem_fini(struct virtio_gpu_device *vgdev);
 int virtio_gpu_gem_create(struct drm_file *file,
 			  struct drm_device *dev,
-			  uint64_t size,
+			  struct virtio_gpu_object_params *params,
 			  struct drm_gem_object **obj_p,
 			  uint32_t *handle_p);
 int virtio_gpu_gem_object_open(struct drm_gem_object *obj,
 			       struct drm_file *file);
 void virtio_gpu_gem_object_close(struct drm_gem_object *obj,
 				 struct drm_file *file);
-struct virtio_gpu_object *virtio_gpu_alloc_object(struct drm_device *dev,
-						  size_t size, bool kernel,
-						  bool pinned);
+struct virtio_gpu_object*
+virtio_gpu_alloc_object(struct drm_device *dev,
+			struct virtio_gpu_object_params *params);
 int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 				struct drm_device *dev,
 				struct drm_mode_create_dumb *args);
@@ -342,7 +346,7 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
 
 /* virtio_gpu_object */
 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
-			     unsigned long size, bool kernel, bool pinned,
+			     struct virtio_gpu_object_params *params,
 			     struct virtio_gpu_object **bo_ptr);
 void virtio_gpu_object_kunmap(struct virtio_gpu_object *bo);
 int virtio_gpu_object_kmap(struct virtio_gpu_object *bo);
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index f06586393974..ffeb734003e4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -34,15 +34,15 @@ void virtio_gpu_gem_free_object(struct drm_gem_object *gem_obj)
 		virtio_gpu_object_unref(&obj);
 }
 
-struct virtio_gpu_object *virtio_gpu_alloc_object(struct drm_device *dev,
-						  size_t size, bool kernel,
-						  bool pinned)
+struct virtio_gpu_object*
+virtio_gpu_alloc_object(struct drm_device *dev,
+			struct virtio_gpu_object_params *params)
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_object *obj;
 	int ret;
 
-	ret = virtio_gpu_object_create(vgdev, size, kernel, pinned, &obj);
+	ret = virtio_gpu_object_create(vgdev, params, &obj);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -51,7 +51,7 @@ struct virtio_gpu_object *virtio_gpu_alloc_object(struct drm_device *dev,
 
 int virtio_gpu_gem_create(struct drm_file *file,
 			  struct drm_device *dev,
-			  uint64_t size,
+			  struct virtio_gpu_object_params *params,
 			  struct drm_gem_object **obj_p,
 			  uint32_t *handle_p)
 {
@@ -59,7 +59,7 @@ int virtio_gpu_gem_create(struct drm_file *file,
 	int ret;
 	u32 handle;
 
-	obj = virtio_gpu_alloc_object(dev, size, false, false);
+	obj = virtio_gpu_alloc_object(dev, params);
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
@@ -85,6 +85,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_gem_object *gobj;
 	struct virtio_gpu_object *obj;
+	struct virtio_gpu_object_params params = { 0 };
 	int ret;
 	uint32_t pitch;
 	uint32_t format;
@@ -96,7 +97,8 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	args->size = pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
-	ret = virtio_gpu_gem_create(file_priv, dev, args->size, &gobj,
+	params.size = args->size;
+	ret = virtio_gpu_gem_create(file_priv, dev, &params, &gobj,
 				    &args->handle);
 	if (ret)
 		goto fail;
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 14ce8188c052..3b05320bcf73 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -279,12 +279,12 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	struct virtio_gpu_object *qobj;
 	struct drm_gem_object *obj;
 	uint32_t handle = 0;
-	uint32_t size;
 	struct list_head validate_list;
 	struct ttm_validate_buffer mainbuf;
 	struct virtio_gpu_fence *fence = NULL;
 	struct ww_acquire_ctx ticket;
 	struct virtio_gpu_resource_create_3d rc_3d;
+	struct virtio_gpu_object_params params = { 0 };
 
 	if (vgdev->has_virgl_3d == false) {
 		if (rc->depth > 1)
@@ -302,13 +302,13 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	INIT_LIST_HEAD(&validate_list);
 	memset(&mainbuf, 0, sizeof(struct ttm_validate_buffer));
 
-	size = rc->size;
+	params.size = rc->size;
 
 	/* allocate a single page size object */
-	if (size == 0)
-		size = PAGE_SIZE;
+	if (params.size == 0)
+		params.size = PAGE_SIZE;
 
-	qobj = virtio_gpu_alloc_object(dev, size, false, false);
+	qobj = virtio_gpu_alloc_object(dev, &params);
 	if (IS_ERR(qobj))
 		return PTR_ERR(qobj);
 	obj = &qobj->gem_base;
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index e7e946035027..538b93417f70 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -74,39 +74,33 @@ static void virtio_gpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
 	kfree(bo);
 }
 
-static void virtio_gpu_init_ttm_placement(struct virtio_gpu_object *vgbo,
-					  bool pinned)
+static void virtio_gpu_init_ttm_placement(struct virtio_gpu_object *vgbo)
 {
 	u32 c = 1;
-	u32 pflag = pinned ? TTM_PL_FLAG_NO_EVICT : 0;
 
 	vgbo->placement.placement = &vgbo->placement_code;
 	vgbo->placement.busy_placement = &vgbo->placement_code;
 	vgbo->placement_code.fpfn = 0;
 	vgbo->placement_code.lpfn = 0;
 	vgbo->placement_code.flags =
-		TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT | pflag;
+		TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT |
+		TTM_PL_FLAG_NO_EVICT;
 	vgbo->placement.num_placement = c;
 	vgbo->placement.num_busy_placement = c;
 
 }
 
 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
-			     unsigned long size, bool kernel, bool pinned,
+			     struct virtio_gpu_object_params *params,
 			     struct virtio_gpu_object **bo_ptr)
 {
 	struct virtio_gpu_object *bo;
-	enum ttm_bo_type type;
 	size_t acc_size;
 	int ret;
 
-	if (kernel)
-		type = ttm_bo_type_kernel;
-	else
-		type = ttm_bo_type_device;
 	*bo_ptr = NULL;
 
-	acc_size = ttm_bo_dma_acc_size(&vgdev->mman.bdev, size,
+	acc_size = ttm_bo_dma_acc_size(&vgdev->mman.bdev, params->size,
 				       sizeof(struct virtio_gpu_object));
 
 	bo = kzalloc(sizeof(struct virtio_gpu_object), GFP_KERNEL);
@@ -117,19 +111,20 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 		kfree(bo);
 		return ret;
 	}
-	size = roundup(size, PAGE_SIZE);
-	ret = drm_gem_object_init(vgdev->ddev, &bo->gem_base, size);
+	params->size = roundup(params->size, PAGE_SIZE);
+	ret = drm_gem_object_init(vgdev->ddev, &bo->gem_base, params->size);
 	if (ret != 0) {
 		virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
 		kfree(bo);
 		return ret;
 	}
 	bo->dumb = false;
-	virtio_gpu_init_ttm_placement(bo, pinned);
+	virtio_gpu_init_ttm_placement(bo);
 
-	ret = ttm_bo_init(&vgdev->mman.bdev, &bo->tbo, size, type,
-			  &bo->placement, 0, !kernel, acc_size,
-			  NULL, NULL, &virtio_gpu_ttm_bo_destroy);
+	ret = ttm_bo_init(&vgdev->mman.bdev, &bo->tbo, params->size,
+			  ttm_bo_type_device, &bo->placement, 0,
+			  true, acc_size, NULL, NULL,
+			  &virtio_gpu_ttm_bo_destroy);
 	/* ttm_bo_init failure will call the destroy */
 	if (ret != 0)
 		return ret;
-- 
2.18.1

^ permalink raw reply related

* [PATCH v3 1/5] drm/virtio: move virtio_gpu_object_{attach, detach} calls.
From: Gerd Hoffmann @ 2019-03-18 11:33 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER,
	Noralf Trønnes, Daniel Vetter, Gurchetan Singh
In-Reply-To: <20190318113332.10900-1-kraxel@redhat.com>

Drop the dummy ttm backend implementation, add a real one for
TTM_PL_FLAG_TT objects.  The bin/unbind callbacks will call
virtio_gpu_object_{attach,detach}, to update the object state
on the host side, instead of invoking those calls from the
move_notify() callback.

With that in place the move and move_notify callbacks are not
needed any more, so drop them.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/virtio/virtgpu_ttm.c | 92 ++++++++--------------------
 1 file changed, 24 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index 4bfbf25fabff..77407976c707 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -194,42 +194,45 @@ static void virtio_gpu_ttm_io_mem_free(struct ttm_bo_device *bdev,
  */
 struct virtio_gpu_ttm_tt {
 	struct ttm_dma_tt		ttm;
-	struct virtio_gpu_device	*vgdev;
-	u64				offset;
+	struct virtio_gpu_object        *obj;
 };
 
-static int virtio_gpu_ttm_backend_bind(struct ttm_tt *ttm,
-				       struct ttm_mem_reg *bo_mem)
+static int virtio_gpu_ttm_tt_bind(struct ttm_tt *ttm,
+				  struct ttm_mem_reg *bo_mem)
 {
-	struct virtio_gpu_ttm_tt *gtt = (void *)ttm;
+	struct virtio_gpu_ttm_tt *gtt =
+		container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
+	struct virtio_gpu_device *vgdev =
+		virtio_gpu_get_vgdev(gtt->obj->tbo.bdev);
 
-	gtt->offset = (unsigned long)(bo_mem->start << PAGE_SHIFT);
-	if (!ttm->num_pages)
-		WARN(1, "nothing to bind %lu pages for mreg %p back %p!\n",
-		     ttm->num_pages, bo_mem, ttm);
-
-	/* Not implemented */
+	virtio_gpu_object_attach(vgdev, gtt->obj, NULL);
 	return 0;
 }
 
-static int virtio_gpu_ttm_backend_unbind(struct ttm_tt *ttm)
+static int virtio_gpu_ttm_tt_unbind(struct ttm_tt *ttm)
 {
-	/* Not implemented */
+	struct virtio_gpu_ttm_tt *gtt =
+		container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
+	struct virtio_gpu_device *vgdev =
+		virtio_gpu_get_vgdev(gtt->obj->tbo.bdev);
+
+	virtio_gpu_object_detach(vgdev, gtt->obj);
 	return 0;
 }
 
-static void virtio_gpu_ttm_backend_destroy(struct ttm_tt *ttm)
+static void virtio_gpu_ttm_tt_destroy(struct ttm_tt *ttm)
 {
-	struct virtio_gpu_ttm_tt *gtt = (void *)ttm;
+	struct virtio_gpu_ttm_tt *gtt =
+		container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
 
 	ttm_dma_tt_fini(&gtt->ttm);
 	kfree(gtt);
 }
 
-static struct ttm_backend_func virtio_gpu_backend_func = {
-	.bind = &virtio_gpu_ttm_backend_bind,
-	.unbind = &virtio_gpu_ttm_backend_unbind,
-	.destroy = &virtio_gpu_ttm_backend_destroy,
+static struct ttm_backend_func virtio_gpu_tt_func = {
+	.bind = &virtio_gpu_ttm_tt_bind,
+	.unbind = &virtio_gpu_ttm_tt_unbind,
+	.destroy = &virtio_gpu_ttm_tt_destroy,
 };
 
 static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
@@ -242,8 +245,8 @@ static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
 	gtt = kzalloc(sizeof(struct virtio_gpu_ttm_tt), GFP_KERNEL);
 	if (gtt == NULL)
 		return NULL;
-	gtt->ttm.ttm.func = &virtio_gpu_backend_func;
-	gtt->vgdev = vgdev;
+	gtt->ttm.ttm.func = &virtio_gpu_tt_func;
+	gtt->obj = container_of(bo, struct virtio_gpu_object, tbo);
 	if (ttm_dma_tt_init(&gtt->ttm, bo, page_flags)) {
 		kfree(gtt);
 		return NULL;
@@ -251,51 +254,6 @@ static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
 	return &gtt->ttm.ttm;
 }
 
-static void virtio_gpu_move_null(struct ttm_buffer_object *bo,
-				 struct ttm_mem_reg *new_mem)
-{
-	struct ttm_mem_reg *old_mem = &bo->mem;
-
-	BUG_ON(old_mem->mm_node != NULL);
-	*old_mem = *new_mem;
-	new_mem->mm_node = NULL;
-}
-
-static int virtio_gpu_bo_move(struct ttm_buffer_object *bo, bool evict,
-			      struct ttm_operation_ctx *ctx,
-			      struct ttm_mem_reg *new_mem)
-{
-	int ret;
-
-	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
-	if (ret)
-		return ret;
-
-	virtio_gpu_move_null(bo, new_mem);
-	return 0;
-}
-
-static void virtio_gpu_bo_move_notify(struct ttm_buffer_object *tbo,
-				      bool evict,
-				      struct ttm_mem_reg *new_mem)
-{
-	struct virtio_gpu_object *bo;
-	struct virtio_gpu_device *vgdev;
-
-	bo = container_of(tbo, struct virtio_gpu_object, tbo);
-	vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
-
-	if (!new_mem || (new_mem->placement & TTM_PL_FLAG_SYSTEM)) {
-		if (bo->hw_res_handle)
-			virtio_gpu_object_detach(vgdev, bo);
-
-	} else if (new_mem->placement & TTM_PL_FLAG_TT) {
-		if (bo->hw_res_handle) {
-			virtio_gpu_object_attach(vgdev, bo, NULL);
-		}
-	}
-}
-
 static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
 {
 	struct virtio_gpu_object *bo;
@@ -314,11 +272,9 @@ static struct ttm_bo_driver virtio_gpu_bo_driver = {
 	.init_mem_type = &virtio_gpu_init_mem_type,
 	.eviction_valuable = ttm_bo_eviction_valuable,
 	.evict_flags = &virtio_gpu_evict_flags,
-	.move = &virtio_gpu_bo_move,
 	.verify_access = &virtio_gpu_verify_access,
 	.io_mem_reserve = &virtio_gpu_ttm_io_mem_reserve,
 	.io_mem_free = &virtio_gpu_ttm_io_mem_free,
-	.move_notify = &virtio_gpu_bo_move_notify,
 	.swap_notify = &virtio_gpu_bo_swap_notify,
 };
 
-- 
2.18.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Jason Wang @ 2019-03-18  7:47 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: axboe, mst, linux-kernel, virtualization, linux-block
In-Reply-To: <20190315134112.7d63348c.cohuck@redhat.com>


On 2019/3/15 下午8:41, Cornelia Huck wrote:
> On Fri, 15 Mar 2019 12:50:11 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Or something like I proposed several years ago?
>> https://do-db2.lkml.org/lkml/2014/12/25/169
>>
>> Btw, for virtio-net, I think we actually want to go for having a maximum
>> number of supported queues like what hardware did. This would be useful
>> for e.g cpu hotplug or XDP (requires per cpu TX queue). But the current
>> vector allocation doesn't support this which will results all virtqueues
>> to share a single vector. We may indeed need more flexible policy here.
> I think it should be possible for the driver to give the transport
> hints how to set up their queues/interrupt structures. (The driver
> probably knows best about its requirements.) Perhaps whether a queue is
> high or low frequency, or whether it should be low latency, or even
> whether two queues could share a notification mechanism without
> drawbacks. It's up to the transport to make use of that information, if
> possible.


Exactly and it was what the above series tried to do by providing hints 
of e.g which queues want to share a notification.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* [PATCH] virtio_net: remove hcpu from virtnet_clean_affinity
From: Peter Xu @ 2019-03-18  6:56 UTC (permalink / raw)
  To: linux-kernel; +Cc: netdev, virtualization, Michael S . Tsirkin

The variable is never used.

CC: Michael S. Tsirkin <mst@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: virtualization@lists.linux-foundation.org
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 drivers/net/virtio_net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7eb38ea9ba56..1b03c4b6ebff 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1925,7 +1925,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev,
 	return 0;
 }
 
-static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
+static void virtnet_clean_affinity(struct virtnet_info *vi)
 {
 	int i;
 
@@ -1949,7 +1949,7 @@ static void virtnet_set_affinity(struct virtnet_info *vi)
 	int stride;
 
 	if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
-		virtnet_clean_affinity(vi, -1);
+		virtnet_clean_affinity(vi);
 		return;
 	}
 
@@ -1999,7 +1999,7 @@ static int virtnet_cpu_down_prep(unsigned int cpu, struct hlist_node *node)
 	struct virtnet_info *vi = hlist_entry_safe(node, struct virtnet_info,
 						   node);
 
-	virtnet_clean_affinity(vi, cpu);
+	virtnet_clean_affinity(vi);
 	return 0;
 }
 
@@ -2735,7 +2735,7 @@ static void virtnet_del_vqs(struct virtnet_info *vi)
 {
 	struct virtio_device *vdev = vi->vdev;
 
-	virtnet_clean_affinity(vi, -1);
+	virtnet_clean_affinity(vi);
 
 	vdev->config->del_vqs(vdev);
 
-- 
2.17.1

^ permalink raw reply related

* [summary] virtio network device failover writeup
From: Michael S. Tsirkin @ 2019-03-17 13:55 UTC (permalink / raw)
  To: Si-Wei Liu
  Cc: vuhuong, Jiri Pirko, Jakub Kicinski, Sridhar Samudrala,
	Alexander Duyck, virtualization, liran.alon, Netdev,
	boris.ostrovsky, David Miller, ogerlitz

Hi all,
I've put up a blog post with a summary of where network
device failover stands and some open issues.
Not sure where best to host it, I just put it up on blogspot:
https://mstsirkin.blogspot.com/2019/03/virtio-network-device-failover-support.html

Comments, corrections are welcome!

-- 
MST

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Cornelia Huck @ 2019-03-15 12:41 UTC (permalink / raw)
  To: Jason Wang; +Cc: axboe, mst, linux-kernel, virtualization, linux-block
In-Reply-To: <537e6420-8994-43d6-1d4d-ccb6e0fafa0b@redhat.com>

On Fri, 15 Mar 2019 12:50:11 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Or something like I proposed several years ago? 
> https://do-db2.lkml.org/lkml/2014/12/25/169
> 
> Btw, for virtio-net, I think we actually want to go for having a maximum 
> number of supported queues like what hardware did. This would be useful 
> for e.g cpu hotplug or XDP (requires per cpu TX queue). But the current 
> vector allocation doesn't support this which will results all virtqueues 
> to share a single vector. We may indeed need more flexible policy here.

I think it should be possible for the driver to give the transport
hints how to set up their queues/interrupt structures. (The driver
probably knows best about its requirements.) Perhaps whether a queue is
high or low frequency, or whether it should be low latency, or even
whether two queues could share a notification mechanism without
drawbacks. It's up to the transport to make use of that information, if
possible.

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Jason Wang @ 2019-03-15  4:50 UTC (permalink / raw)
  To: Dongli Zhang, Cornelia Huck, mst
  Cc: linux-block, axboe, linux-kernel, virtualization
In-Reply-To: <a3f24cfa-5160-0ccc-decc-a48362c8de19@oracle.com>


On 2019/3/14 下午2:12, Dongli Zhang wrote:
>
> On 3/13/19 5:39 PM, Cornelia Huck wrote:
>> On Wed, 13 Mar 2019 11:26:04 +0800
>> Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>
>>> On 3/13/19 1:33 AM, Cornelia Huck wrote:
>>>> On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
>>>> Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>>>    
>>>>> I observed that there is one msix vector for config and one shared vector
>>>>> for all queues in below qemu cmdline, when the num-queues for virtio-blk
>>>>> is more than the number of possible cpus:
>>>>>
>>>>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
>>>>>
>>>>> # cat /proc/interrupts
>>>>>             CPU0       CPU1       CPU2       CPU3
>>>>> ... ...
>>>>>   24:          0          0          0          0   PCI-MSI 65536-edge      virtio0-config
>>>>>   25:          0          0          0         59   PCI-MSI 65537-edge      virtio0-virtqueues
>>>>> ... ...
>>>>>
>>>>>
>>>>> However, when num-queues is the same as number of possible cpus:
>>>>>
>>>>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
>>>>>
>>>>> # cat /proc/interrupts
>>>>>             CPU0       CPU1       CPU2       CPU3
>>>>> ... ...
>>>>>   24:          0          0          0          0   PCI-MSI 65536-edge      virtio0-config
>>>>>   25:          2          0          0          0   PCI-MSI 65537-edge      virtio0-req.0
>>>>>   26:          0         35          0          0   PCI-MSI 65538-edge      virtio0-req.1
>>>>>   27:          0          0         32          0   PCI-MSI 65539-edge      virtio0-req.2
>>>>>   28:          0          0          0          0   PCI-MSI 65540-edge      virtio0-req.3
>>>>> ... ...
>>>>>
>>>>> In above case, there is one msix vector per queue.
>>>> Please note that this is pci-specific...
>>>>    
>>>>>
>>>>> This is because the max number of queues is not limited by the number of
>>>>> possible cpus.
>>>>>
>>>>> By default, nvme (regardless about write_queues and poll_queues) and
>>>>> xen-blkfront limit the number of queues with num_possible_cpus().
>>>> ...and these are probably pci-specific as well.
>>> Not pci-specific, but per-cpu as well.
>> Ah, I meant that those are pci devices.
>>
>>>>    
>>>>>
>>>>> Is this by design on purpose, or can we fix with below?
>>>>>
>>>>>
>>>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>>>> index 4bc083b..df95ce3 100644
>>>>> --- a/drivers/block/virtio_blk.c
>>>>> +++ b/drivers/block/virtio_blk.c
>>>>> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
>>>>>   	if (err)
>>>>>   		num_vqs = 1;
>>>>>   
>>>>> +	num_vqs = min(num_possible_cpus(), num_vqs);
>>>>> +
>>>>>   	vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
>>>>>   	if (!vblk->vqs)
>>>>>   		return -ENOMEM;
>>>> virtio-blk, however, is not pci-specific.
>>>>
>>>> If we are using the ccw transport on s390, a completely different
>>>> interrupt mechanism is in use ('floating' interrupts, which are not
>>>> per-cpu). A check like that should therefore not go into the generic
>>>> driver.
>>>>    
>>> So far there seems two options.
>>>
>>> The 1st option is to ask the qemu user to always specify "-num-queues" with the
>>> same number of vcpus when running x86 guest with pci for virtio-blk or
>>> virtio-scsi, in order to assign a vector for each queue.
>> That does seem like an extra burden for the user: IIUC, things work
>> even if you have too many queues, it's just not optimal. It sounds like
>> something that can be done by a management layer (e.g. libvirt), though.
>>
>>> Or, is it fine for virtio folks to add a new hook to 'struct virtio_config_ops'
>>> so that different platforms (e.g., pci or ccw) would use different ways to limit
>>> the max number of queues in guest, with something like below?
>> That sounds better, as both transports and drivers can opt-in here.
>>
>> However, maybe it would be even better to try to come up with a better
>> strategy of allocating msix vectors in virtio-pci. More vectors in the
>> num_queues > num_cpus case, even if they still need to be shared?
>> Individual vectors for n-1 cpus and then a shared one for the remaining
>> queues?
>>
>> It might even be device-specific: Have some low-traffic status queues
>> share a vector, and provide an individual vector for high-traffic
>> queues. Would need some device<->transport interface, obviously.
>>
> This sounds a little bit similar to multiple hctx maps?
>
> So far, as virtio-blk only supports set->nr_maps = 1, no matter how many hw
> queues are assigned for virtio-blk, blk_mq_alloc_tag_set() would use at most
> nr_cpu_ids hw queues.
>
> 2981 int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
> ... ...
> 3021         /*
> 3022          * There is no use for more h/w queues than cpus if we just have
> 3023          * a single map
> 3024          */
> 3025         if (set->nr_maps == 1 && set->nr_hw_queues > nr_cpu_ids)
> 3026                 set->nr_hw_queues = nr_cpu_ids;
>
> Even the block layer would limit the number of hw queues by nr_cpu_ids when
> (set->nr_maps == 1).
>
> That's why I think virtio-blk should use the similar solution as nvme
> (regardless about write_queues and poll_queues) and xen-blkfront.
>
> Added Jason again. I do not know why the mailing list of
> virtualization@lists.linux-foundation.org always filters out Jason's email...
>
>
> Dongli Zhang


Or something like I proposed several years ago? 
https://do-db2.lkml.org/lkml/2014/12/25/169

Btw, for virtio-net, I think we actually want to go for having a maximum 
number of supported queues like what hardware did. This would be useful 
for e.g cpu hotplug or XDP (requires per cpu TX queue). But the current 
vector allocation doesn't support this which will results all virtqueues 
to share a single vector. We may indeed need more flexible policy here.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-15  4:39 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Christoph Hellwig, linux-parisc, kvm, Michael S. Tsirkin, netdev,
	linux-kernel, virtualization, James Bottomley, linux-mm,
	David Miller, linux-arm-kernel
In-Reply-To: <20190314193333.GA24542@redhat.com>


On 2019/3/15 上午3:33, Andrea Arcangeli wrote:
> Hello Jason,
>
> On Thu, Mar 14, 2019 at 09:49:03PM +0800, Jason Wang wrote:
>> Yes since we don't want to slow down 32bit.
> If you've a lot of ram there's no justification to stick to a 32bit
> kernel, so I don't think there's need to maintain a separate model
> just for 32bit. I really wouldn't care about the performance of 32bit
> with >700MB of RAM if that would cause any maintenance burden. Let's
> focus on the best 64bit implementation that will work equally
> optimally on 32bit with <= 700M of RAM.


Yes, but probably there are still some reasons of keeping copy_user() 
friends as a fallback. When we have a large virtqueue, the ring may 
occupy more than one page. This means the VA might not be contiguous 
when using kmap(). Instead of doing tricks in the accessories, maybe 
it's or simpler better just fall back to copy_user() in this case. And 
we meet the similar issue when software device IOTLB is used for vhost. 
And in the following example for gup, we can simply do a fallback when 
we race with the invalidation.

Michael also tends to keep the copy_user(), he suggested to use 
copy_user() for VIVT archs then there's no need for a explicit 
flush_dcache_page(). And he also want a module parameter for falling 
back to copy_user() for e.g debugging purpose.


>
> Talking to Jerome about the set_page_dirty issue, he raised the point
> of what happens if two thread calls a mmu notifier invalidate
> simultaneously. The first mmu notifier could call set_page_dirty and
> then proceed in try_to_free_buffers or page_mkclean and then the
> concurrent mmu notifier that arrives second, then must not call
> set_page_dirty a second time.
>
> With KVM sptes mappings and vhost mappings you would call
> set_page_dirty (if you invoked gup with FOLL_WRITE) only when
> effectively tearing down any secondary mapping (you've got pointers in
> both cases for the mapping). So there's no way to risk a double
> set_page_dirty from concurrent mmu notifier invalidate because the
> invalidate takes a lock when it has to teardown the mapping and so
> set_page_dirty is only run in the first invalidate method and not in
> the second. In the spte case even better, as you wouldn't need to call
> it even at teardown time unless the spte is dirty (if shadow mask
> allows dirty sptes with EPT2 or NPT or shadow MMU).


I see, the sounds indeed better.


>
> If you instead had to invalidate a secondary MMU mapping that isn't
> tracked by the driver (again: not vhost nor KVM case), you could have
> used the dirty bit of the kernel pagetable to call set_page_dirty and
> disambiguate but that's really messy, and it would prevent the use of
> gigapages in the direct mapping too and it'd require vmap for 4k
> tracking.
>
> To make sure set_page_dirty is run a single time no matter if the
> invalidate known when a mapping is tear down, I suggested the below
> model:
>
>    access = FOLL_WRITE
>
> repeat:
>    page = gup_fast(access)
>    put_page(page) /* need a way to drop FOLL_GET from gup_fast instead! */
>
>    spin_lock(mmu_notifier_lock);
>    if (race with invalidate) {
>      spin_unlock..
>      goto repeat;
>    }
>    if (access == FOLL_WRITE)
>      set_page_dirty(page)
>    establish writable mapping in secondary MMU on page
>    spin_unlock
>
> (replace spin_lock with mutex_lock for vhost of course if you stick to
> a mutex and _start/_end instead of non-sleepable ->invalidate_range)


Yes, I probably stick to the vq mutex since the invalidation needs to be 
synchronized with the whole packet processing routine.


> "race with invalidate" is the usual "mmu_notifier_retry" in kvm_host.h
> to be implemented for vhost.
>
> We could add a FOLL_DIRTY flag to add to FOLL_TOUCH to move the
> set_page_dirty inside GUP forced (currently it's not forced if the
> linux pte is already dirty). And we could remove FOLL_GET.
>
> Then if you have the ability to disambiguate which is the first
> invalidate that tears down a mapping to any given page (vhost can do
> that trivially, it's just a pointer to a page struct to kmap), in the
> mmu notifier invalidate just before dropping the spinlock you would
> do this check:
>
> def vhost_invalidate_range_start():
>     [..]
>     spin_lock(mmu_notifier_lock);
>     [..]
>     if (vhost->page_pointer) {
>        if (access == FOLL_WRITE)
> 	VM_WARN_ON(!PageDirty(vhost->page_pointer));
>        vhost->page_pointer = NULL;
>        /* no put_page, already done at gup time */
>     }
>     spin_unlock(..
>
> Generally speaking set_page_dirty is safer done after the last
> modification to the data of the page. However the way stable page
> works, as long as the mmu notifier invalidate didn't run, the PG_dirty
> cannot go away.


Ok.


>
> So this model solves the issue with guaranteeing a single
> set_page_dirty is run before page_mkclean or try_to_free_buffers can
> run, even for drivers that implement the invalidate as a generic "TLB
> flush" across the whole secondary MMU and that cannot disambiguate the
> first invalidate from a second invalidate if they're issued
> concurrently on the same address by two different CPUs.
>
> So for those drivers that can disambiguate trivially (like KVM and
> vhost) we'll just add a debug check in the invalidate to validate the
> common code for all mmu notifier users.
>
> This is the solution for RDMA hardware and everything that can support
> mmu notifiers too and they can take infinitely long secondary MMU
> mappins without interfering with stable pages at all (i.e. long term
> pins but without pinning) perfectly safe and transparent to the whole
> stable page code.
>
> I think O_DIRECT for stable pages shall be solved taking the page lock
> or writeback lock or a new rwsem in the inode that is taken for
> writing by page_mkclean and try_to_free_buffers and for reading by
> outstanding O_DIRECT in flight I/O, like I suggested probably ages ago
> but then we only made GUP take the page pin, which is fine for mmu
> notifier actually (except those didn't exist back then). To solve
> O_DIRECT we can leverage the 100% guarantee that the pin will be
> dropped ASAP and stop page_mkclean and stop or trylock in
> try_to_free_buffers in such case.
>
> mm_take_all_locks is major hurdle that prevents usage in O_DIRECT
> case, even if we "cache it" if you fork(); write; exit() in a loop
> it'll still cause heavy lock overhead. MMU notifier registration isn't
> intended to happen in fast and frequent paths like the write()
> syscall. Dropping mm_take_all_locks would bring other downsides: a
> regular quiescent point can happen in between _start/_end and _start
> must be always called first all the mmu notifier retry counters we
> rely on would break. One way would be to add srcu_read_lock _before_
> you can call mm_has_mm_has_notifiers(mm), then yes we could replace
> mm_take_all_locks with synchronize_srcu. It would save a lot of CPU
> and a ton of locked operations, but it'd potentially increase the
> latency of the registration so the first O_DIRECT write() in a process
> could still potentially stall (still better than mm_take_all_locks
> which would use a lot more CPU and hurt SMP scalability in threaded
> programs). The downside is all VM fast paths would get some overhead
> because of srcu_read_lock even when mmu notifier is not registered,
> which is what we carefully avoided by taking a larger hit in the
> registration with mm_take_all_locks. This is why I don't think mmu
> notifier is a good solution to solve O_DIRECT stable pages even in
> theory O_DIRECT could use the exact same model as vhost to solve
> stable pages.
>
> If we solve O_DIRECT with new per-page locking or a new rwsem inode
> lock leveraging the fact we're guaranteed the pin to go away ASAP,
> what's left is the case of PCI devices mapping userland memory for
> indefinite amount of time that cannot support MMU notifier because of
> hardware limitations.


Yes and this is part of the issue we met in vhost TX zerocopy code. 
What's more interesting is that a skb could be held of a software layer 
e.g qdisc as well. MMU notifier could be used for dealing with such 
software holding e.g copy packets into new pages. But this may requires 
more thoughts since this may race with networking stack.


>
> Mentioning virtualization as a case taking long term PIN is incorrect,
> that didn't happen since the introduction of MMU notifier.
>
> vfio for device assignment to KVM takes the long term pins, but that's
> because the iommus may not support the mmu notifier, mmu notifier
> could solve the vfio case too.


This might require the support of page faults from IOMMU hardware.


>
> PCI devices that pretend to keep a constant mapping on userland
> virtual memory and that cannot support MMU notifier because they lack
> a secondary MMU, cripple the Linux VM and there's no solution to
> that. Even if we solve the stable pages problem, they will still
> practically disable all advanced VM features.
>
> I think it would be ok to solve the stable pages in 3 different ways:
>
> 1) mmu notifier as above when mmu_notifier_register doesn't need to
>     run often and the hardware can support it
>
> 2) O_DIRECT with new locking stopping page_mkclean/try_to_free_buffers
>     until I/O completion, leveraging the fact the pin&lock are
>     guaranteed to be dropped&released ASAP
>
> 3) something else for pci devices that cannot support MMU notifier
>     because of hardware limitations, bounce buffers would be fine as
>     well
>
> I'm not even sure if in 3) it is worth worrying about being able to
> routinely flush to disk the dirty data, but then bounce buffers could
> solve that. Altering the page mapped in the pte sounds like a complex
> solution when you could copy the physical page just before issuing the
> I/O in the writeback code. To find if a GUP pin exists it's enough to
> check what KSM does:
>
> 		/*
> 		 * Check that no O_DIRECT or similar I/O is in progress on the
> 		 * page
> 		 */
> 		if (page_mapcount(page) + 1 + swapped != page_count(page)) {
>
> That can give false positives (even through random pins coming from
> speculative cache lookups), but not false negatives.
>
> Thanks,
> Andrea


Thanks for the patient and detailed explanation with lots of 
backgrounds. It help to understand the whole picture a lot.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Andrea Arcangeli @ 2019-03-14 19:33 UTC (permalink / raw)
  To: Jason Wang
  Cc: Christoph Hellwig, linux-parisc, kvm, Michael S. Tsirkin, netdev,
	linux-kernel, virtualization, James Bottomley, linux-mm,
	David Miller, linux-arm-kernel
In-Reply-To: <74015196-2e18-4fe0-50ac-0c9d497315c7@redhat.com>

Hello Jason,

On Thu, Mar 14, 2019 at 09:49:03PM +0800, Jason Wang wrote:
> Yes since we don't want to slow down 32bit.

If you've a lot of ram there's no justification to stick to a 32bit
kernel, so I don't think there's need to maintain a separate model
just for 32bit. I really wouldn't care about the performance of 32bit
with >700MB of RAM if that would cause any maintenance burden. Let's
focus on the best 64bit implementation that will work equally
optimally on 32bit with <= 700M of RAM.

Talking to Jerome about the set_page_dirty issue, he raised the point
of what happens if two thread calls a mmu notifier invalidate
simultaneously. The first mmu notifier could call set_page_dirty and
then proceed in try_to_free_buffers or page_mkclean and then the
concurrent mmu notifier that arrives second, then must not call
set_page_dirty a second time.

With KVM sptes mappings and vhost mappings you would call
set_page_dirty (if you invoked gup with FOLL_WRITE) only when
effectively tearing down any secondary mapping (you've got pointers in
both cases for the mapping). So there's no way to risk a double
set_page_dirty from concurrent mmu notifier invalidate because the
invalidate takes a lock when it has to teardown the mapping and so
set_page_dirty is only run in the first invalidate method and not in
the second. In the spte case even better, as you wouldn't need to call
it even at teardown time unless the spte is dirty (if shadow mask
allows dirty sptes with EPT2 or NPT or shadow MMU).

If you instead had to invalidate a secondary MMU mapping that isn't
tracked by the driver (again: not vhost nor KVM case), you could have
used the dirty bit of the kernel pagetable to call set_page_dirty and
disambiguate but that's really messy, and it would prevent the use of
gigapages in the direct mapping too and it'd require vmap for 4k
tracking.

To make sure set_page_dirty is run a single time no matter if the
invalidate known when a mapping is tear down, I suggested the below
model:

  access = FOLL_WRITE

repeat:
  page = gup_fast(access)
  put_page(page) /* need a way to drop FOLL_GET from gup_fast instead! */

  spin_lock(mmu_notifier_lock);
  if (race with invalidate) {
    spin_unlock..
    goto repeat;
  }
  if (access == FOLL_WRITE)
    set_page_dirty(page)
  establish writable mapping in secondary MMU on page
  spin_unlock

(replace spin_lock with mutex_lock for vhost of course if you stick to
a mutex and _start/_end instead of non-sleepable ->invalidate_range)

"race with invalidate" is the usual "mmu_notifier_retry" in kvm_host.h
to be implemented for vhost.

We could add a FOLL_DIRTY flag to add to FOLL_TOUCH to move the
set_page_dirty inside GUP forced (currently it's not forced if the
linux pte is already dirty). And we could remove FOLL_GET.

Then if you have the ability to disambiguate which is the first
invalidate that tears down a mapping to any given page (vhost can do
that trivially, it's just a pointer to a page struct to kmap), in the
mmu notifier invalidate just before dropping the spinlock you would
do this check:

def vhost_invalidate_range_start():
   [..]
   spin_lock(mmu_notifier_lock);
   [..]
   if (vhost->page_pointer) {
      if (access == FOLL_WRITE)
	VM_WARN_ON(!PageDirty(vhost->page_pointer));
      vhost->page_pointer = NULL;
      /* no put_page, already done at gup time */
   }
   spin_unlock(..

Generally speaking set_page_dirty is safer done after the last
modification to the data of the page. However the way stable page
works, as long as the mmu notifier invalidate didn't run, the PG_dirty
cannot go away.

So this model solves the issue with guaranteeing a single
set_page_dirty is run before page_mkclean or try_to_free_buffers can
run, even for drivers that implement the invalidate as a generic "TLB
flush" across the whole secondary MMU and that cannot disambiguate the
first invalidate from a second invalidate if they're issued
concurrently on the same address by two different CPUs.

So for those drivers that can disambiguate trivially (like KVM and
vhost) we'll just add a debug check in the invalidate to validate the
common code for all mmu notifier users.

This is the solution for RDMA hardware and everything that can support
mmu notifiers too and they can take infinitely long secondary MMU
mappins without interfering with stable pages at all (i.e. long term
pins but without pinning) perfectly safe and transparent to the whole
stable page code.

I think O_DIRECT for stable pages shall be solved taking the page lock
or writeback lock or a new rwsem in the inode that is taken for
writing by page_mkclean and try_to_free_buffers and for reading by
outstanding O_DIRECT in flight I/O, like I suggested probably ages ago
but then we only made GUP take the page pin, which is fine for mmu
notifier actually (except those didn't exist back then). To solve
O_DIRECT we can leverage the 100% guarantee that the pin will be
dropped ASAP and stop page_mkclean and stop or trylock in
try_to_free_buffers in such case.

mm_take_all_locks is major hurdle that prevents usage in O_DIRECT
case, even if we "cache it" if you fork(); write; exit() in a loop
it'll still cause heavy lock overhead. MMU notifier registration isn't
intended to happen in fast and frequent paths like the write()
syscall. Dropping mm_take_all_locks would bring other downsides: a
regular quiescent point can happen in between _start/_end and _start
must be always called first all the mmu notifier retry counters we
rely on would break. One way would be to add srcu_read_lock _before_
you can call mm_has_mm_has_notifiers(mm), then yes we could replace
mm_take_all_locks with synchronize_srcu. It would save a lot of CPU
and a ton of locked operations, but it'd potentially increase the
latency of the registration so the first O_DIRECT write() in a process
could still potentially stall (still better than mm_take_all_locks
which would use a lot more CPU and hurt SMP scalability in threaded
programs). The downside is all VM fast paths would get some overhead
because of srcu_read_lock even when mmu notifier is not registered,
which is what we carefully avoided by taking a larger hit in the
registration with mm_take_all_locks. This is why I don't think mmu
notifier is a good solution to solve O_DIRECT stable pages even in
theory O_DIRECT could use the exact same model as vhost to solve
stable pages.

If we solve O_DIRECT with new per-page locking or a new rwsem inode
lock leveraging the fact we're guaranteed the pin to go away ASAP,
what's left is the case of PCI devices mapping userland memory for
indefinite amount of time that cannot support MMU notifier because of
hardware limitations.

Mentioning virtualization as a case taking long term PIN is incorrect,
that didn't happen since the introduction of MMU notifier.

vfio for device assignment to KVM takes the long term pins, but that's
because the iommus may not support the mmu notifier, mmu notifier
could solve the vfio case too.

PCI devices that pretend to keep a constant mapping on userland
virtual memory and that cannot support MMU notifier because they lack
a secondary MMU, cripple the Linux VM and there's no solution to
that. Even if we solve the stable pages problem, they will still
practically disable all advanced VM features.

I think it would be ok to solve the stable pages in 3 different ways:

1) mmu notifier as above when mmu_notifier_register doesn't need to
   run often and the hardware can support it

2) O_DIRECT with new locking stopping page_mkclean/try_to_free_buffers
   until I/O completion, leveraging the fact the pin&lock are
   guaranteed to be dropped&released ASAP

3) something else for pci devices that cannot support MMU notifier
   because of hardware limitations, bounce buffers would be fine as
   well

I'm not even sure if in 3) it is worth worrying about being able to
routinely flush to disk the dirty data, but then bounce buffers could
solve that. Altering the page mapped in the pte sounds like a complex
solution when you could copy the physical page just before issuing the
I/O in the writeback code. To find if a GUP pin exists it's enough to
check what KSM does:

		/*
		 * Check that no O_DIRECT or similar I/O is in progress on the
		 * page
		 */
		if (page_mapcount(page) + 1 + swapped != page_count(page)) {

That can give false positives (even through random pins coming from
speculative cache lookups), but not false negatives.

Thanks,
Andrea

^ permalink raw reply

* [PATCH 15/38] vfs: Convert virtio_balloon to fs_context
From: David Howells @ 2019-03-14 16:10 UTC (permalink / raw)
  To: viro
  Cc: Michael S. Tsirkin, linux-kernel, virtualization, dhowells,
	linux-fsdevel
In-Reply-To: <155257972443.13720.11743171471060355965.stgit@warthog.procyon.org.uk>

Signed-off-by: David Howells <dhowells@redhat.com>
cc: "Michael S. Tsirkin" <mst@redhat.com>
cc: Jason Wang <jasowang@redhat.com>
cc: virtualization@lists.linux-foundation.org
---

 drivers/virtio/virtio_balloon.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f19061b585a4..89d67c8aa719 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -757,21 +757,22 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
 
 	return MIGRATEPAGE_SUCCESS;
 }
+#include <linux/fs_context.h>
 
-static struct dentry *balloon_mount(struct file_system_type *fs_type,
-		int flags, const char *dev_name, void *data)
-{
-	static const struct dentry_operations ops = {
-		.d_dname = simple_dname,
-	};
+static const struct dentry_operations balloon_dops = {
+	.d_dname = simple_dname,
+};
 
-	return mount_pseudo(fs_type, "balloon-kvm:", NULL, &ops,
-				BALLOON_KVM_MAGIC);
+static int balloon_init_fs_context(struct fs_context *fc)
+{
+	return vfs_init_pseudo_fs_context(fc, "balloon-kvm:",
+					  NULL, NULL,
+					  &balloon_dops, BALLOON_KVM_MAGIC);
 }
 
 static struct file_system_type balloon_fs = {
 	.name           = "balloon-kvm",
-	.mount          = balloon_mount,
+	.init_fs_context = balloon_init_fs_context,
 	.kill_sb        = kill_anon_super,
 };

^ permalink raw reply related

* [PATCH 00/38] VFS: Convert trivial filesystems and more
From: David Howells @ 2019-03-14 16:08 UTC (permalink / raw)
  To: viro
  Cc: Uma Krishnan, linux-aio, linux-efi, linux-ia64,
	Sergey Senozhatsky, Michael S. Tsirkin, David Airlie, dri-devel,
	virtualization, Keith Busch, Chris Mason, Joel Fernandes,
	Todd Kjos, Manoj N. Kumar, Christoph Hellwig, devel,
	Matthew Garrett, Stefano Stabellini, Dave Jiang, Paul Moore,
	linux-scsi, linux-nvdimm, linux-rdma, Vishal Verma,
	Boris Ostrovsky


Hi Al,

Here's a set of patches that:

 (1) Provides a convenience member in struct fs_context that is OR'd into
     sb->s_iflags by sget_fc().

 (2) Provides a convenience vfs_init_pseudo_fs_context() helper function
     for doing most of the work in mounting a pseudo filesystem.

 (3) Converts all the trivial filesystems that have no arguments to
     fs_context.

 (4) Converts binderfs (which was trivial before January).

 (5) Converts ramfs, tmpfs, rootfs and devtmpfs.

 (6) Kills off mount_pseudo(), mount_pseudo_xattr(), mount_ns(),
     sget_userns().

The patches can be found here also:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git

on branch:

	mount-api-viro

David
---
David Howells (38):
      vfs: Provide sb->s_iflags settings in fs_context struct
      vfs: Provide a mount_pseudo-replacement for fs_context
      vfs: Convert aio to fs_context
      vfs: Convert anon_inodes to fs_context
      vfs: Convert bdev to fs_context
      vfs: Convert nsfs to fs_context
      vfs: Convert pipe to fs_context
      vfs: Convert zsmalloc to fs_context
      vfs: Convert sockfs to fs_context
      vfs: Convert dax to fs_context
      vfs: Convert drm to fs_context
      vfs: Convert ia64 perfmon to fs_context
      vfs: Convert cxl to fs_context
      vfs: Convert ocxlflash to fs_context
      vfs: Convert virtio_balloon to fs_context
      vfs: Convert btrfs_test to fs_context
      vfs: Kill off mount_pseudo() and mount_pseudo_xattr()
      vfs: Use sget_fc() for pseudo-filesystems
      vfs: Convert binderfs to fs_context
      vfs: Convert nfsctl to fs_context
      vfs: Convert rpc_pipefs to fs_context
      vfs: Kill off mount_ns()
      vfs: Kill sget_userns()
      vfs: Convert binfmt_misc to fs_context
      vfs: Convert configfs to fs_context
      vfs: Convert efivarfs to fs_context
      vfs: Convert fusectl to fs_context
      vfs: Convert qib_fs/ipathfs to fs_context
      vfs: Convert ibmasmfs to fs_context
      vfs: Convert oprofilefs to fs_context
      vfs: Convert gadgetfs to fs_context
      vfs: Convert xenfs to fs_context
      vfs: Convert openpromfs to fs_context
      vfs: Convert apparmorfs to fs_context
      vfs: Convert securityfs to fs_context
      vfs: Convert selinuxfs to fs_context
      vfs: Convert smackfs to fs_context
      tmpfs, devtmpfs, ramfs, rootfs: Convert to fs_context


 arch/ia64/kernel/perfmon.c         |   14 +
 drivers/android/binderfs.c         |  173 +++++++++-------
 drivers/base/devtmpfs.c            |   16 +
 drivers/dax/super.c                |   13 +
 drivers/gpu/drm/drm_drv.c          |   14 +
 drivers/infiniband/hw/qib/qib_fs.c |   26 ++
 drivers/misc/cxl/api.c             |   10 -
 drivers/misc/ibmasm/ibmasmfs.c     |   21 +-
 drivers/oprofile/oprofilefs.c      |   20 +-
 drivers/scsi/cxlflash/ocxl_hw.c    |   21 +-
 drivers/usb/gadget/legacy/inode.c  |   21 +-
 drivers/virtio/virtio_balloon.c    |   19 +-
 drivers/xen/xenfs/super.c          |   21 +-
 fs/aio.c                           |   15 +
 fs/anon_inodes.c                   |   12 +
 fs/binfmt_misc.c                   |   20 +-
 fs/block_dev.c                     |   14 +
 fs/btrfs/tests/btrfs-tests.c       |   13 +
 fs/configfs/mount.c                |   20 +-
 fs/efivarfs/super.c                |   20 +-
 fs/fuse/control.c                  |   20 +-
 fs/libfs.c                         |   91 ++++++--
 fs/nfsd/nfsctl.c                   |   33 ++-
 fs/nsfs.c                          |   13 +
 fs/openpromfs/inode.c              |   20 +-
 fs/pipe.c                          |   12 +
 fs/ramfs/inode.c                   |  104 ++++++---
 fs/super.c                         |  106 ++--------
 include/linux/fs.h                 |   21 --
 include/linux/fs_context.h         |    8 +
 include/linux/ramfs.h              |    6 -
 include/linux/shmem_fs.h           |    4 
 init/do_mounts.c                   |   12 -
 mm/shmem.c                         |  396 ++++++++++++++++++++++++------------
 mm/zsmalloc.c                      |   19 +-
 net/socket.c                       |   14 +
 net/sunrpc/rpc_pipe.c              |   34 ++-
 security/apparmor/apparmorfs.c     |   20 +-
 security/inode.c                   |   21 +-
 security/selinux/selinuxfs.c       |   20 +-
 security/smack/smackfs.c           |   34 ++-
 41 files changed, 902 insertions(+), 609 deletions(-)

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Dongli Zhang @ 2019-03-14 16:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: axboe, mst, linux-kernel, virtualization, linux-block
In-Reply-To: <20190314131339.1b61fff6.cohuck@redhat.com>



On 03/14/2019 08:13 PM, Cornelia Huck wrote:
> On Thu, 14 Mar 2019 14:12:32 +0800
> Dongli Zhang <dongli.zhang@oracle.com> wrote:
> 
>> On 3/13/19 5:39 PM, Cornelia Huck wrote:
>>> On Wed, 13 Mar 2019 11:26:04 +0800
>>> Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>>   
>>>> On 3/13/19 1:33 AM, Cornelia Huck wrote:  
>>>>> On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
>>>>> Dongli Zhang <dongli.zhang@oracle.com> wrote:
> 
>>>>>> Is this by design on purpose, or can we fix with below?
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>>>>> index 4bc083b..df95ce3 100644
>>>>>> --- a/drivers/block/virtio_blk.c
>>>>>> +++ b/drivers/block/virtio_blk.c
>>>>>> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
>>>>>>  	if (err)
>>>>>>  		num_vqs = 1;
>>>>>>  
>>>>>> +	num_vqs = min(num_possible_cpus(), num_vqs);
>>>>>> +
>>>>>>  	vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
>>>>>>  	if (!vblk->vqs)
>>>>>>  		return -ENOMEM;    
>>>>>
>>>>> virtio-blk, however, is not pci-specific.
>>>>>
>>>>> If we are using the ccw transport on s390, a completely different
>>>>> interrupt mechanism is in use ('floating' interrupts, which are not
>>>>> per-cpu). A check like that should therefore not go into the generic
>>>>> driver.
>>>>>     
>>>>
>>>> So far there seems two options.
>>>>
>>>> The 1st option is to ask the qemu user to always specify "-num-queues" with the
>>>> same number of vcpus when running x86 guest with pci for virtio-blk or
>>>> virtio-scsi, in order to assign a vector for each queue.  
>>>
>>> That does seem like an extra burden for the user: IIUC, things work
>>> even if you have too many queues, it's just not optimal. It sounds like
>>> something that can be done by a management layer (e.g. libvirt), though.
>>>   
>>>> Or, is it fine for virtio folks to add a new hook to 'struct virtio_config_ops'
>>>> so that different platforms (e.g., pci or ccw) would use different ways to limit
>>>> the max number of queues in guest, with something like below?  
>>>
>>> That sounds better, as both transports and drivers can opt-in here.
>>>
>>> However, maybe it would be even better to try to come up with a better
>>> strategy of allocating msix vectors in virtio-pci. More vectors in the
>>> num_queues > num_cpus case, even if they still need to be shared?
>>> Individual vectors for n-1 cpus and then a shared one for the remaining
>>> queues?
>>>
>>> It might even be device-specific: Have some low-traffic status queues
>>> share a vector, and provide an individual vector for high-traffic
>>> queues. Would need some device<->transport interface, obviously.
>>>   
>>
>> This sounds a little bit similar to multiple hctx maps?
>>
>> So far, as virtio-blk only supports set->nr_maps = 1, no matter how many hw
>> queues are assigned for virtio-blk, blk_mq_alloc_tag_set() would use at most
>> nr_cpu_ids hw queues.
>>
>> 2981 int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
>> ... ...
>> 3021         /*
>> 3022          * There is no use for more h/w queues than cpus if we just have
>> 3023          * a single map
>> 3024          */
>> 3025         if (set->nr_maps == 1 && set->nr_hw_queues > nr_cpu_ids)
>> 3026                 set->nr_hw_queues = nr_cpu_ids;
>>
>> Even the block layer would limit the number of hw queues by nr_cpu_ids when
>> (set->nr_maps == 1).
> 
> Correct me if I'm wrong, but there seem to be two kinds of limitations
> involved here:
> - Allocation of msix vectors by the virtio-pci transport. We end up
>   with shared vectors if we have more virtqueues than vcpus. Other
>   transports may or may not have similar issues, but essentially, this
>   is something that applies to all kind of virtio devices attached via
>   the virtio-pci transport.

It depends.

For virtio-net, we need to specify the number of available vectors on qemu side,
e.g.,:

-device virtio-net-pci,netdev=tapnet,mq=true,vectors=16

This parameter is specific for virtio-net.

Suppose if 'queues=8' while 'vectors=16', as 2*8+1 > 16, there will be lack of
vectors and guest would not be able to assign one vector for each queue.

I was tortured by this long time ago and it seems qemu would minimize the memory
allocation and the default 'vectors' is 3.

BTW, why cannot we have a more consistent configuration for most qemu devices,
e.g., so far:

virtio-blk use 'num-queues'
nvme use 'num_queues'
virtio-net use 'queues' for tap :)

> - The block layer limits the number of hw queues to the number of
>   vcpus. This applies only to virtio devices that interact with the
>   block layer, but regardless of the virtio transport.

Yes: virtio-blk and virtio-scsi.

> 
>> That's why I think virtio-blk should use the similar solution as nvme
>> (regardless about write_queues and poll_queues) and xen-blkfront.
> 
> Ok, the hw queues limit from above would be an argument to limit to
> #vcpus in the virtio-blk driver, regardless of the transport used. (No
> idea if there are better ways to deal with this, I'm not familiar with
> the interface.)
> 
> For virtio devices that don't interact with the block layer and are
> attached via the virtio-pci transport, it might still make sense to
> revisit vector allocation.
> 

As mentioned above, we need to specify 'vectors' for virtio-net as the default
value is only 3 (config + tx + rx). That would make a little difference?

Dongli Zhang

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Dongli Zhang @ 2019-03-14 15:36 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: linux-block, axboe, linux-kernel, virtualization
In-Reply-To: <20190314082926-mutt-send-email-mst@kernel.org>



On 03/14/2019 08:32 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 12, 2019 at 10:22:46AM -0700, Dongli Zhang wrote:
>> I observed that there is one msix vector for config and one shared vector
>> for all queues in below qemu cmdline, when the num-queues for virtio-blk
>> is more than the number of possible cpus:
>>
>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"
> 
> So why do this?

I observed this when I was testing virtio-blk and block layer.

I just assign a very large 'num-queues' to virtio-blk and keep changing the
number of vcpu in order to study blk-mq.

The num-queues for nvme (qemu) is by default 64, while it is 1 for virtio-blk.

> 
>> # cat /proc/interrupts 
>>            CPU0       CPU1       CPU2       CPU3
>> ... ...
>>  24:          0          0          0          0   PCI-MSI 65536-edge      virtio0-config
>>  25:          0          0          0         59   PCI-MSI 65537-edge      virtio0-virtqueues
>> ... ...
>>
>>
>> However, when num-queues is the same as number of possible cpus:
>>
>> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
>>
>> # cat /proc/interrupts 
>>            CPU0       CPU1       CPU2       CPU3
>> ... ... 
>>  24:          0          0          0          0   PCI-MSI 65536-edge      virtio0-config
>>  25:          2          0          0          0   PCI-MSI 65537-edge      virtio0-req.0
>>  26:          0         35          0          0   PCI-MSI 65538-edge      virtio0-req.1
>>  27:          0          0         32          0   PCI-MSI 65539-edge      virtio0-req.2
>>  28:          0          0          0          0   PCI-MSI 65540-edge      virtio0-req.3
>> ... ...
>>
>> In above case, there is one msix vector per queue.
>>
>>
>> This is because the max number of queues is not limited by the number of
>> possible cpus.
>>
>> By default, nvme (regardless about write_queues and poll_queues) and
>> xen-blkfront limit the number of queues with num_possible_cpus().
>>
>>
>> Is this by design on purpose, or can we fix with below?
>>
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 4bc083b..df95ce3 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
>>  	if (err)
>>  		num_vqs = 1;
>>  
>> +	num_vqs = min(num_possible_cpus(), num_vqs);
>> +
>>  	vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
>>  	if (!vblk->vqs)
>>  		return -ENOMEM;
>> --
>>
>>
>> PS: The same issue is applicable to virtio-scsi as well.
>>
>> Thank you very much!
>>
>> Dongli Zhang
> 
> I don't think this will address the issue if there's vcpu hotplug though.
> Because it's not about num_possible_cpus it's about the # of active VCPUs,
> right? Does block hangle CPU hotplug generally?
> We could maybe address that by switching vq to msi vector mapping in
> a cpu hotplug notifier...
> 

It looks it is about num_possible_cpus/nr_cpu_ids for cpu hotplug.


For instance, below is when only 2 out of 6 cpus are initialized while
virtio-blk has 6 queues.

"-smp 2,maxcpus=6" and "-device
virtio-blk-pci,drive=drive0,id=disk0,num-queues=6,iothread=io1"

# cat /sys/devices/system/cpu/present
0-1
# cat /sys/devices/system/cpu/possible
0-5
# cat /proc/interrupts  | grep virtio
 24:          0          0   PCI-MSI 65536-edge      virtio0-config
 25:       1864          0   PCI-MSI 65537-edge      virtio0-req.0
 26:          0       1069   PCI-MSI 65538-edge      virtio0-req.1
 27:          0          0   PCI-MSI 65539-edge      virtio0-req.2
 28:          0          0   PCI-MSI 65540-edge      virtio0-req.3
 29:          0          0   PCI-MSI 65541-edge      virtio0-req.4
 30:          0          0   PCI-MSI 65542-edge      virtio0-req.5

6 + 1 irqs are assigned even 4 out of 6 cpus are still offline.


Below is about the nvme emulated by qemu. While 2 out of 6 cpus are initial
assigned, nvme has 64 queues by default.

"-smp 2,maxcpus=6" and "-device nvme,drive=drive1,serial=deadbeaf1"

# cat /sys/devices/system/cpu/present
0-1
# cat /sys/devices/system/cpu/possible
0-5
# cat /proc/interrupts | grep nvme
 31:          0         16   PCI-MSI 81920-edge      nvme0q0
 32:         35          0   PCI-MSI 81921-edge      nvme0q1
 33:          0         42   PCI-MSI 81922-edge      nvme0q2
 34:          0          0   PCI-MSI 81923-edge      nvme0q3
 35:          0          0   PCI-MSI 81924-edge      nvme0q4
 36:          0          0   PCI-MSI 81925-edge      nvme0q5
 37:          0          0   PCI-MSI 81926-edge      nvme0q6

6 io queues are assigned with irq, although only 2 cpus are online.


When only 2 out of 48 cpus are online, there are 48 hctx created by block layer.

"-smp 2,maxcpus=48" and "-device
virtio-blk-pci,drive=drive0,id=disk0,num-queues=48,iothread=io1"

# ls /sys/kernel/debug/block/vda/ | grep hctx | wc -l
48


The above indicates the number of hw queues/irq is related to
num_possible_cpus/nr_cpu_ids.

Dongli Zhang

^ permalink raw reply

* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-14 13:49 UTC (permalink / raw)
  To: Michael S. Tsirkin, James Bottomley
  Cc: Andrea Arcangeli, linux-parisc, kvm, netdev, linux-kernel,
	virtualization, Christoph Hellwig, linux-mm, David Miller,
	linux-arm-kernel
In-Reply-To: <20190314064004-mutt-send-email-mst@kernel.org>


On 2019/3/14 下午6:42, Michael S. Tsirkin wrote:
>>>>> Which means after we fix vhost to add the flush_dcache_page after
>>>>> kunmap, Parisc will get a double hit (but it also means Parisc
>>>>> was the only one of those archs needed explicit cache flushes,
>>>>> where vhost worked correctly so far.. so it kinds of proofs your
>>>>> point of giving up being the safe choice).
>>>> What double hit?  If there's no cache to flush then cache flush is
>>>> a no-op.  It's also a highly piplineable no-op because the CPU has
>>>> the L1 cache within easy reach.  The only event when flush takes a
>>>> large amount time is if we actually have dirty data to write back
>>>> to main memory.
>>> I've heard people complaining that on some microarchitectures even
>>> no-op cache flushes are relatively expensive.  Don't ask me why,
>>> but if we can easily avoid double flushes we should do that.
>> It's still not entirely free for us.  Our internal cache line is around
>> 32 bytes (some have 16 and some have 64) but that means we need 128
>> flushes for a page ... we definitely can't pipeline them all.  So I
>> agree duplicate flush elimination would be a small improvement.
>>
>> James
> I suspect we'll keep the copyXuser path around for 32 bit anyway -
> right Jason?


Yes since we don't want to slow down 32bit.

Thanks


> So we can also keep using that on parisc...
>
> -- 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Michael S. Tsirkin @ 2019-03-14 12:32 UTC (permalink / raw)
  To: Dongli Zhang; +Cc: linux-block, axboe, linux-kernel, virtualization
In-Reply-To: <e4afe4c5-0262-4500-aeec-60f30734b4fc@default>

On Tue, Mar 12, 2019 at 10:22:46AM -0700, Dongli Zhang wrote:
> I observed that there is one msix vector for config and one shared vector
> for all queues in below qemu cmdline, when the num-queues for virtio-blk
> is more than the number of possible cpus:
> 
> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=6"

So why do this?

> # cat /proc/interrupts 
>            CPU0       CPU1       CPU2       CPU3
> ... ...
>  24:          0          0          0          0   PCI-MSI 65536-edge      virtio0-config
>  25:          0          0          0         59   PCI-MSI 65537-edge      virtio0-virtqueues
> ... ...
> 
> 
> However, when num-queues is the same as number of possible cpus:
> 
> qemu: "-smp 4" while "-device virtio-blk-pci,drive=drive-0,id=virtblk0,num-queues=4"
> 
> # cat /proc/interrupts 
>            CPU0       CPU1       CPU2       CPU3
> ... ... 
>  24:          0          0          0          0   PCI-MSI 65536-edge      virtio0-config
>  25:          2          0          0          0   PCI-MSI 65537-edge      virtio0-req.0
>  26:          0         35          0          0   PCI-MSI 65538-edge      virtio0-req.1
>  27:          0          0         32          0   PCI-MSI 65539-edge      virtio0-req.2
>  28:          0          0          0          0   PCI-MSI 65540-edge      virtio0-req.3
> ... ...
> 
> In above case, there is one msix vector per queue.
> 
> 
> This is because the max number of queues is not limited by the number of
> possible cpus.
> 
> By default, nvme (regardless about write_queues and poll_queues) and
> xen-blkfront limit the number of queues with num_possible_cpus().
> 
> 
> Is this by design on purpose, or can we fix with below?
> 
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4bc083b..df95ce3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
>  	if (err)
>  		num_vqs = 1;
>  
> +	num_vqs = min(num_possible_cpus(), num_vqs);
> +
>  	vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
>  	if (!vblk->vqs)
>  		return -ENOMEM;
> --
> 
> 
> PS: The same issue is applicable to virtio-scsi as well.
> 
> Thank you very much!
> 
> Dongli Zhang

I don't think this will address the issue if there's vcpu hotplug though.
Because it's not about num_possible_cpus it's about the # of active VCPUs,
right? Does block hangle CPU hotplug generally?
We could maybe address that by switching vq to msi vector mapping in
a cpu hotplug notifier...

-- 
MST

^ permalink raw reply

* Re: virtio-blk: should num_vqs be limited by num_possible_cpus()?
From: Cornelia Huck @ 2019-03-14 12:13 UTC (permalink / raw)
  To: Dongli Zhang; +Cc: axboe, mst, linux-kernel, virtualization, linux-block
In-Reply-To: <a3f24cfa-5160-0ccc-decc-a48362c8de19@oracle.com>

On Thu, 14 Mar 2019 14:12:32 +0800
Dongli Zhang <dongli.zhang@oracle.com> wrote:

> On 3/13/19 5:39 PM, Cornelia Huck wrote:
> > On Wed, 13 Mar 2019 11:26:04 +0800
> > Dongli Zhang <dongli.zhang@oracle.com> wrote:
> >   
> >> On 3/13/19 1:33 AM, Cornelia Huck wrote:  
> >>> On Tue, 12 Mar 2019 10:22:46 -0700 (PDT)
> >>> Dongli Zhang <dongli.zhang@oracle.com> wrote:

> >>>> Is this by design on purpose, or can we fix with below?
> >>>>
> >>>>
> >>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> >>>> index 4bc083b..df95ce3 100644
> >>>> --- a/drivers/block/virtio_blk.c
> >>>> +++ b/drivers/block/virtio_blk.c
> >>>> @@ -513,6 +513,8 @@ static int init_vq(struct virtio_blk *vblk)
> >>>>  	if (err)
> >>>>  		num_vqs = 1;
> >>>>  
> >>>> +	num_vqs = min(num_possible_cpus(), num_vqs);
> >>>> +
> >>>>  	vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
> >>>>  	if (!vblk->vqs)
> >>>>  		return -ENOMEM;    
> >>>
> >>> virtio-blk, however, is not pci-specific.
> >>>
> >>> If we are using the ccw transport on s390, a completely different
> >>> interrupt mechanism is in use ('floating' interrupts, which are not
> >>> per-cpu). A check like that should therefore not go into the generic
> >>> driver.
> >>>     
> >>
> >> So far there seems two options.
> >>
> >> The 1st option is to ask the qemu user to always specify "-num-queues" with the
> >> same number of vcpus when running x86 guest with pci for virtio-blk or
> >> virtio-scsi, in order to assign a vector for each queue.  
> > 
> > That does seem like an extra burden for the user: IIUC, things work
> > even if you have too many queues, it's just not optimal. It sounds like
> > something that can be done by a management layer (e.g. libvirt), though.
> >   
> >> Or, is it fine for virtio folks to add a new hook to 'struct virtio_config_ops'
> >> so that different platforms (e.g., pci or ccw) would use different ways to limit
> >> the max number of queues in guest, with something like below?  
> > 
> > That sounds better, as both transports and drivers can opt-in here.
> > 
> > However, maybe it would be even better to try to come up with a better
> > strategy of allocating msix vectors in virtio-pci. More vectors in the
> > num_queues > num_cpus case, even if they still need to be shared?
> > Individual vectors for n-1 cpus and then a shared one for the remaining
> > queues?
> > 
> > It might even be device-specific: Have some low-traffic status queues
> > share a vector, and provide an individual vector for high-traffic
> > queues. Would need some device<->transport interface, obviously.
> >   
> 
> This sounds a little bit similar to multiple hctx maps?
> 
> So far, as virtio-blk only supports set->nr_maps = 1, no matter how many hw
> queues are assigned for virtio-blk, blk_mq_alloc_tag_set() would use at most
> nr_cpu_ids hw queues.
> 
> 2981 int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
> ... ...
> 3021         /*
> 3022          * There is no use for more h/w queues than cpus if we just have
> 3023          * a single map
> 3024          */
> 3025         if (set->nr_maps == 1 && set->nr_hw_queues > nr_cpu_ids)
> 3026                 set->nr_hw_queues = nr_cpu_ids;
> 
> Even the block layer would limit the number of hw queues by nr_cpu_ids when
> (set->nr_maps == 1).

Correct me if I'm wrong, but there seem to be two kinds of limitations
involved here:
- Allocation of msix vectors by the virtio-pci transport. We end up
  with shared vectors if we have more virtqueues than vcpus. Other
  transports may or may not have similar issues, but essentially, this
  is something that applies to all kind of virtio devices attached via
  the virtio-pci transport.
- The block layer limits the number of hw queues to the number of
  vcpus. This applies only to virtio devices that interact with the
  block layer, but regardless of the virtio transport.

> That's why I think virtio-blk should use the similar solution as nvme
> (regardless about write_queues and poll_queues) and xen-blkfront.

Ok, the hw queues limit from above would be an argument to limit to
#vcpus in the virtio-blk driver, regardless of the transport used. (No
idea if there are better ways to deal with this, I'm not familiar with
the interface.)

For virtio devices that don't interact with the block layer and are
attached via the virtio-pci transport, it might still make sense to
revisit vector allocation.

^ permalink raw reply

* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Michael S. Tsirkin @ 2019-03-14 10:42 UTC (permalink / raw)
  To: James Bottomley
  Cc: Andrea Arcangeli, linux-parisc, kvm, netdev, linux-kernel,
	virtualization, Christoph Hellwig, linux-mm, David Miller,
	linux-arm-kernel
In-Reply-To: <1552495028.3022.37.camel@HansenPartnership.com>

On Wed, Mar 13, 2019 at 09:37:08AM -0700, James Bottomley wrote:
> On Wed, 2019-03-13 at 09:05 -0700, Christoph Hellwig wrote:
> > On Tue, Mar 12, 2019 at 01:53:37PM -0700, James Bottomley wrote:
> > > I've got to say: optimize what?  What code do we ever have in the
> > > kernel that kmap's a page and then doesn't do anything with it? You
> > > can
> > > guarantee that on kunmap the page is either referenced (needs
> > > invalidating) or updated (needs flushing). The in-kernel use of
> > > kmap is
> > > always
> > > 
> > > kmap
> > > do something with the mapped page
> > > kunmap
> > > 
> > > In a very short interval.  It seems just a simplification to make
> > > kunmap do the flush if needed rather than try to have the users
> > > remember.  The thing which makes this really simple is that on most
> > > architectures flush and invalidate is the same operation.  If you
> > > really want to optimize you can use the referenced and dirty bits
> > > on the kmapped pte to tell you what operation to do, but if your
> > > flush is your invalidate, you simply assume the data needs flushing
> > > on kunmap without checking anything.
> > 
> > I agree that this would be a good way to simplify the API.   Now
> > we'd just need volunteers to implement this for all architectures
> > that need cache flushing and then remove the explicit flushing in
> > the callers..
> 
> Well, it's already done on parisc ...  I can help with this if we agree
> it's the best way forward.  It's really only architectures that
> implement flush_dcache_page that would need modifying.
> 
> It may also improve performance because some kmap/use/flush/kunmap
> sequences have flush_dcache_page() instead of
> flush_kernel_dcache_page() and the former is hugely expensive and
> usually unnecessary because GUP already flushed all the user aliases.
> 
> In the interests of full disclosure the reason we do it for parisc is
> because our later machines have problems even with clean aliases.  So
> on most VIPT systems, doing kmap/read/kunmap creates a fairly harmless
> clean alias.  Technically it should be invalidated, because if you
> remap the same page to the same colour you get cached stale data, but
> in practice the data is expired from the cache long before that
> happens, so the problem is almost never seen if the flush is forgotten.
>  Our problem is on the P9xxx processor: they have a L1/L2 VIPT L3 PIPT
> cache.  As the L1/L2 caches expire clean data, they place the expiring
> contents into L3, but because L3 is PIPT, the stale alias suddenly
> becomes the default for any read of they physical page because any
> update which dirtied the cache line often gets written to main memory
> and placed into the L3 as clean *before* the clean alias in L1/L2 gets
> expired, so the older clean alias replaces it.
> 
> Our only recourse is to kill all aliases with prejudice before the
> kernel loses ownership.
> 
> > > > Which means after we fix vhost to add the flush_dcache_page after
> > > > kunmap, Parisc will get a double hit (but it also means Parisc
> > > > was the only one of those archs needed explicit cache flushes,
> > > > where vhost worked correctly so far.. so it kinds of proofs your
> > > > point of giving up being the safe choice).
> > > 
> > > What double hit?  If there's no cache to flush then cache flush is
> > > a no-op.  It's also a highly piplineable no-op because the CPU has
> > > the L1 cache within easy reach.  The only event when flush takes a
> > > large amount time is if we actually have dirty data to write back
> > > to main memory.
> > 
> > I've heard people complaining that on some microarchitectures even
> > no-op cache flushes are relatively expensive.  Don't ask me why,
> > but if we can easily avoid double flushes we should do that.
> 
> It's still not entirely free for us.  Our internal cache line is around
> 32 bytes (some have 16 and some have 64) but that means we need 128
> flushes for a page ... we definitely can't pipeline them all.  So I
> agree duplicate flush elimination would be a small improvement.
> 
> James

I suspect we'll keep the copyXuser path around for 32 bit anyway -
right Jason?
So we can also keep using that on parisc...

-- 
MST

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox