* [PATCH] drm/virtio: Add window server support
From: Tomeu Vizoso @ 2017-12-14 12:43 UTC (permalink / raw)
To: dri-devel
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, linux-kernel,
virtualization, Zach Reizner
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.
It is expected that a service in the guest will act as a proxy,
interacting with virtio-gpu to support unmodified clients. For some
features of the protocol, the hypervisor might have to intervene and
also parse the protocol data to properly bridge resources. The following
IOCTLs have been added to this effect:
*_WINSRV_CONNECT: Opens a connection to the compositor in the host,
returns a FD that represents this connection and on which the following
IOCTLs can be used. Callers are expected to poll this FD for new
messages from the compositor.
*_WINSRV_TX: Asks the hypervisor to forward a message to the compositor
*_WINSRV_RX: Returns all queued messages
Alongside protocol data that is opaque to the kernel, the client can
send file descriptors that reference GEM buffers allocated by
virtio-gpu. The guest proxy is expected to figure out when a client is
passing a FD that refers to shared memory in the guest and allocate a
virtio-gpu buffer of the same size with DRM_VIRTGPU_RESOURCE_CREATE.
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.
This has been tested with Wayland clients that make use of wl_shm to
pass buffers to the compositor, but is expected to work similarly for X
clients that make use of MIT-SHM with FD passing.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Zach Reizner <zachr@google.com>
---
Hi,
this work is based on the virtio_wl driver in the ChromeOS kernel by
Zach Reizner, currently at:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c
There's two features missing in this patch when compared with virtio_wl:
* Allow the guest access directly host memory, without having to resort
to TRANSFER_TO_HOST
* Pass FDs from host to guest (Wayland specifies that the compositor
shares keyboard data with the guest via a shared buffer)
I plan to work on this next, but I would like to get some comments on
the general approach so I can better choose which patch to follow.
Thanks,
Tomeu
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 39 ++++-
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 168 +++++++++++++++++++
drivers/gpu/drm/virtio/virtgpu_kms.c | 58 +++++--
drivers/gpu/drm/virtio/virtgpu_vq.c | 283 +++++++++++++++++++++++++++++++++
include/uapi/drm/virtgpu_drm.h | 29 ++++
include/uapi/linux/virtio_gpu.h | 39 +++++
6 files changed, 602 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index da2fb585fea4..268b386e1232 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -178,6 +178,8 @@ struct virtio_gpu_device {
struct virtio_gpu_queue ctrlq;
struct virtio_gpu_queue cursorq;
+ struct virtio_gpu_queue winsrv_rxq;
+ struct virtio_gpu_queue winsrv_txq;
struct kmem_cache *vbufs;
bool vqs_ready;
@@ -205,10 +207,32 @@ struct virtio_gpu_device {
struct virtio_gpu_fpriv {
uint32_t ctx_id;
+
+ struct list_head winsrv_conns; /* list of virtio_gpu_winsrv_conn */
+ spinlock_t winsrv_lock;
+};
+
+struct virtio_gpu_winsrv_rx_qentry {
+ struct virtio_gpu_winsrv_rx *cmd;
+ struct list_head next;
+};
+
+struct virtio_gpu_winsrv_conn {
+ struct virtio_gpu_device *vgdev;
+
+ spinlock_t lock;
+
+ int fd;
+ struct drm_file *drm_file;
+
+ struct list_head cmdq; /* queue of virtio_gpu_winsrv_rx_qentry */
+ wait_queue_head_t cmdwq;
+
+ struct list_head next;
};
/* virtio_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 10
+#define DRM_VIRTIO_NUM_IOCTLS 11
extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
/* virtio_kms.c */
@@ -318,9 +342,22 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
void virtio_gpu_ctrl_ack(struct virtqueue *vq);
void virtio_gpu_cursor_ack(struct virtqueue *vq);
void virtio_gpu_fence_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq);
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq);
void virtio_gpu_dequeue_ctrl_func(struct work_struct *work);
void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work);
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work);
void virtio_gpu_dequeue_fence_func(struct work_struct *work);
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev);
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_rx *cmd);
+int virtio_gpu_cmd_winsrv_connect(struct virtio_gpu_device *vgdev, int fd);
+void virtio_gpu_cmd_winsrv_disconnect(struct virtio_gpu_device *vgdev, int fd);
+int virtio_gpu_cmd_winsrv_tx(struct virtio_gpu_device *vgdev,
+ const char __user *buffer, u32 len,
+ int *fds, struct virtio_gpu_winsrv_conn *conn,
+ bool nonblock);
/* virtio_gpu_display.c */
int virtio_gpu_framebuffer_init(struct drm_device *dev,
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 0528edb4a2bf..2571cdafd594 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -25,6 +25,9 @@
* OTHER DEALINGS IN THE SOFTWARE.
*/
+#include <linux/anon_inodes.h>
+#include <linux/syscalls.h>
+
#include <drm/drmP.h>
#include <drm/virtgpu_drm.h>
#include <drm/ttm/ttm_execbuf_util.h>
@@ -527,6 +530,168 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
return 0;
}
+static unsigned int winsrv_poll(struct file *filp,
+ struct poll_table_struct *wait)
+{
+ struct virtio_gpu_winsrv_conn *conn = filp->private_data;
+ unsigned int mask = 0;
+
+ spin_lock(&conn->lock);
+ poll_wait(filp, &conn->cmdwq, wait);
+ if (!list_empty(&conn->cmdq))
+ mask |= POLLIN | POLLRDNORM;
+ spin_unlock(&conn->lock);
+
+ return mask;
+}
+
+static int winsrv_ioctl_rx(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_conn *conn,
+ struct drm_virtgpu_winsrv *cmd)
+{
+ struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp;
+ struct virtio_gpu_winsrv_rx *virtio_cmd;
+ int available_len = cmd->len;
+ int read_count = 0;
+
+ list_for_each_entry_safe(qentry, tmp, &conn->cmdq, next) {
+ virtio_cmd = qentry->cmd;
+ if (virtio_cmd->len > available_len)
+ return 0;
+
+ if (copy_to_user((void *)cmd->data + read_count,
+ virtio_cmd->data,
+ virtio_cmd->len)) {
+ /* return error unless we have some data to return */
+ if (read_count == 0)
+ return -EFAULT;
+ }
+
+ /*
+ * here we could export resource IDs to FDs, but no protocol
+ * as of today requires it
+ */
+
+ available_len -= virtio_cmd->len;
+ read_count += virtio_cmd->len;
+
+ virtio_gpu_queue_winsrv_rx_in(vgdev, virtio_cmd);
+
+ list_del(&qentry->next);
+ kfree(qentry);
+ }
+
+ cmd->len = read_count;
+
+ return 0;
+}
+
+static long winsrv_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct virtio_gpu_winsrv_conn *conn = filp->private_data;
+ struct virtio_gpu_device *vgdev = conn->vgdev;
+ struct drm_virtgpu_winsrv winsrv_cmd;
+ int ret;
+
+ if (_IOC_SIZE(cmd) > sizeof(winsrv_cmd))
+ return -EINVAL;
+
+ if (copy_from_user(&winsrv_cmd, (void __user *)arg,
+ _IOC_SIZE(cmd)) != 0)
+ return -EFAULT;
+
+ switch (cmd) {
+ case DRM_IOCTL_VIRTGPU_WINSRV_RX:
+ ret = winsrv_ioctl_rx(vgdev, conn, &winsrv_cmd);
+ if (copy_to_user((void __user *)arg, &winsrv_cmd,
+ _IOC_SIZE(cmd)) != 0)
+ return -EFAULT;
+
+ break;
+
+ case DRM_IOCTL_VIRTGPU_WINSRV_TX:
+ ret = virtio_gpu_cmd_winsrv_tx(vgdev,
+ (const char __user *) winsrv_cmd.data,
+ winsrv_cmd.len,
+ winsrv_cmd.fds,
+ conn,
+ filp->f_flags & O_NONBLOCK);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int winsrv_release(struct inode *inodep, struct file *filp)
+{
+ struct virtio_gpu_winsrv_conn *conn = filp->private_data;
+ struct virtio_gpu_device *vgdev = conn->vgdev;
+
+ virtio_gpu_cmd_winsrv_disconnect(vgdev, conn->fd);
+
+ list_del(&conn->next);
+ kfree(conn);
+
+ return 0;
+}
+
+static const struct file_operations winsrv_fops = {
+
+ .poll = winsrv_poll,
+ .unlocked_ioctl = winsrv_ioctl,
+ .release = winsrv_release,
+};
+
+static int virtio_gpu_winsrv_connect(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+ struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
+ struct drm_virtgpu_winsrv_connect *args = data;
+ struct virtio_gpu_winsrv_conn *conn;
+ int ret;
+
+ conn = kzalloc(sizeof(*conn), GFP_KERNEL);
+ if (!conn)
+ return -ENOMEM;
+
+ conn->vgdev = vgdev;
+ conn->drm_file = file;
+ spin_lock_init(&conn->lock);
+ INIT_LIST_HEAD(&conn->cmdq);
+ init_waitqueue_head(&conn->cmdwq);
+
+ ret = anon_inode_getfd("[virtgpu_winsrv]", &winsrv_fops, conn,
+ O_CLOEXEC | O_RDWR);
+ if (ret < 0)
+ goto free_conn;
+
+ conn->fd = ret;
+
+ ret = virtio_gpu_cmd_winsrv_connect(vgdev, conn->fd);
+ if (ret < 0)
+ goto close_fd;
+
+ spin_lock(&vfpriv->winsrv_lock);
+ list_add_tail(&conn->next, &vfpriv->winsrv_conns);
+ spin_unlock(&vfpriv->winsrv_lock);
+
+ args->fd = conn->fd;
+
+ return 0;
+
+close_fd:
+ sys_close(conn->fd);
+
+free_conn:
+ kfree(conn);
+
+ return ret;
+}
+
struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
@@ -558,4 +723,7 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_GET_CAPS, virtio_gpu_get_caps_ioctl,
DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+
+ DRM_IOCTL_DEF_DRV(VIRTGPU_WINSRV_CONNECT, virtio_gpu_winsrv_connect,
+ DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 6400506a06b0..ad7872037982 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -128,13 +128,15 @@ static void virtio_gpu_get_capsets(struct virtio_gpu_device *vgdev,
int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
{
static vq_callback_t *callbacks[] = {
- virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack
+ virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack,
+ virtio_gpu_winsrv_rx_read, virtio_gpu_winsrv_tx_ack
};
- static const char * const names[] = { "control", "cursor" };
+ static const char * const names[] = { "control", "cursor",
+ "winsrv-rx", "winsrv-tx" };
struct virtio_gpu_device *vgdev;
/* this will expand later */
- struct virtqueue *vqs[2];
+ struct virtqueue *vqs[4];
u32 num_scanouts, num_capsets;
int ret;
@@ -158,6 +160,10 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
init_waitqueue_head(&vgdev->resp_wq);
virtio_gpu_init_vq(&vgdev->ctrlq, virtio_gpu_dequeue_ctrl_func);
virtio_gpu_init_vq(&vgdev->cursorq, virtio_gpu_dequeue_cursor_func);
+ virtio_gpu_init_vq(&vgdev->winsrv_rxq,
+ virtio_gpu_dequeue_winsrv_rx_func);
+ virtio_gpu_init_vq(&vgdev->winsrv_txq,
+ virtio_gpu_dequeue_winsrv_tx_func);
vgdev->fence_drv.context = dma_fence_context_alloc(1);
spin_lock_init(&vgdev->fence_drv.lock);
@@ -175,13 +181,15 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
DRM_INFO("virgl 3d acceleration not supported by guest\n");
#endif
- ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
+ ret = virtio_find_vqs(vgdev->vdev, 4, vqs, callbacks, names, NULL);
if (ret) {
DRM_ERROR("failed to find virt queues\n");
goto err_vqs;
}
vgdev->ctrlq.vq = vqs[0];
vgdev->cursorq.vq = vqs[1];
+ vgdev->winsrv_rxq.vq = vqs[2];
+ vgdev->winsrv_txq.vq = vqs[3];
ret = virtio_gpu_alloc_vbufs(vgdev);
if (ret) {
DRM_ERROR("failed to alloc vbufs\n");
@@ -215,6 +223,9 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
goto err_modeset;
virtio_device_ready(vgdev->vdev);
+
+ virtio_gpu_fill_winsrv_rx(vgdev);
+
vgdev->vqs_ready = true;
if (num_capsets)
@@ -256,6 +267,8 @@ void virtio_gpu_driver_unload(struct drm_device *dev)
vgdev->vqs_ready = false;
flush_work(&vgdev->ctrlq.dequeue_work);
flush_work(&vgdev->cursorq.dequeue_work);
+ flush_work(&vgdev->winsrv_rxq.dequeue_work);
+ flush_work(&vgdev->winsrv_txq.dequeue_work);
flush_work(&vgdev->config_changed_work);
vgdev->vdev->config->del_vqs(vgdev->vdev);
@@ -274,25 +287,43 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
uint32_t id;
char dbgname[64], tmpname[TASK_COMM_LEN];
- /* can't create contexts without 3d renderer */
- if (!vgdev->has_virgl_3d)
- return 0;
-
- get_task_comm(tmpname, current);
- snprintf(dbgname, sizeof(dbgname), "%s", tmpname);
- dbgname[63] = 0;
/* allocate a virt GPU context for this opener */
vfpriv = kzalloc(sizeof(*vfpriv), GFP_KERNEL);
if (!vfpriv)
return -ENOMEM;
- virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname, &id);
+ /* can't create contexts without 3d renderer */
+ if (vgdev->has_virgl_3d) {
+ get_task_comm(tmpname, current);
+ snprintf(dbgname, sizeof(dbgname), "%s", tmpname);
+ dbgname[63] = 0;
+
+ virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname, &id);
+
+ vfpriv->ctx_id = id;
+ }
+
+ spin_lock_init(&vfpriv->winsrv_lock);
+ INIT_LIST_HEAD(&vfpriv->winsrv_conns);
- vfpriv->ctx_id = id;
file->driver_priv = vfpriv;
+
return 0;
}
+static void virtio_gpu_cleanup_conns(struct virtio_gpu_fpriv *vfpriv)
+{
+ struct virtio_gpu_winsrv_conn *conn, *tmp;
+ struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp2;
+
+ list_for_each_entry_safe(conn, tmp, &vfpriv->winsrv_conns, next) {
+ list_for_each_entry_safe(qentry, tmp2, &conn->cmdq, next) {
+ kfree(qentry);
+ }
+ kfree(conn);
+ }
+}
+
void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file *file)
{
struct virtio_gpu_device *vgdev = dev->dev_private;
@@ -303,6 +334,7 @@ void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file *file)
vfpriv = file->driver_priv;
+ virtio_gpu_cleanup_conns(vfpriv);
virtio_gpu_context_destroy(vgdev, vfpriv->ctx_id);
kfree(vfpriv);
file->driver_priv = NULL;
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9eb96fb2c147..93f2f86c19dd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -72,6 +72,67 @@ void virtio_gpu_cursor_ack(struct virtqueue *vq)
schedule_work(&vgdev->cursorq.dequeue_work);
}
+void virtio_gpu_winsrv_rx_read(struct virtqueue *vq)
+{
+ struct drm_device *dev = vq->vdev->priv;
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+
+ schedule_work(&vgdev->winsrv_rxq.dequeue_work);
+}
+
+void virtio_gpu_winsrv_tx_ack(struct virtqueue *vq)
+{
+ struct drm_device *dev = vq->vdev->priv;
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+
+ schedule_work(&vgdev->winsrv_txq.dequeue_work);
+}
+
+void virtio_gpu_queue_winsrv_rx_in(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_rx *cmd)
+{
+ struct virtqueue *vq = vgdev->winsrv_rxq.vq;
+ struct scatterlist sg[1];
+ int ret;
+
+ sg_init_one(sg, cmd, sizeof(*cmd));
+
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+retry:
+ ret = virtqueue_add_inbuf(vq, sg, 1, cmd, GFP_KERNEL);
+ if (ret == -ENOSPC) {
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+ wait_event(vgdev->winsrv_rxq.ack_queue, vq->num_free);
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+ goto retry;
+ }
+ virtqueue_kick(vq);
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+}
+
+void virtio_gpu_fill_winsrv_rx(struct virtio_gpu_device *vgdev)
+{
+ struct virtqueue *vq = vgdev->winsrv_rxq.vq;
+ struct virtio_gpu_winsrv_rx *cmd;
+ int ret = 0;
+
+ while (vq->num_free > 0) {
+ cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd) {
+ ret = -ENOMEM;
+ goto clear_queue;
+ }
+
+ virtio_gpu_queue_winsrv_rx_in(vgdev, cmd);
+ }
+
+ return;
+
+clear_queue:
+ while ((cmd = virtqueue_detach_unused_buf(vq)))
+ kfree(cmd);
+}
+
int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev)
{
vgdev->vbufs = kmem_cache_create("virtio-gpu-vbufs",
@@ -258,6 +319,96 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
wake_up(&vgdev->cursorq.ack_queue);
}
+void virtio_gpu_dequeue_winsrv_tx_func(struct work_struct *work)
+{
+ struct virtio_gpu_device *vgdev =
+ container_of(work, struct virtio_gpu_device,
+ winsrv_txq.dequeue_work);
+ struct virtio_gpu_vbuffer *vbuf;
+ int len;
+
+ spin_lock(&vgdev->winsrv_txq.qlock);
+ do {
+ while ((vbuf = virtqueue_get_buf(vgdev->winsrv_txq.vq, &len)))
+ free_vbuf(vgdev, vbuf);
+ } while (!virtqueue_enable_cb(vgdev->winsrv_txq.vq));
+ spin_unlock(&vgdev->winsrv_txq.qlock);
+
+ wake_up(&vgdev->winsrv_txq.ack_queue);
+}
+
+static struct virtio_gpu_winsrv_conn *find_conn(struct virtio_gpu_device *vgdev,
+ int fd)
+{
+ struct virtio_gpu_winsrv_conn *conn;
+ struct drm_device *ddev = vgdev->ddev;
+ struct drm_file *file;
+ struct virtio_gpu_fpriv *vfpriv;
+
+ mutex_lock(&ddev->filelist_mutex);
+ list_for_each_entry(file, &ddev->filelist, lhead) {
+ vfpriv = file->driver_priv;
+ spin_lock(&vfpriv->winsrv_lock);
+ list_for_each_entry(conn, &vfpriv->winsrv_conns, next) {
+ if (conn->fd == fd) {
+ spin_lock(&conn->lock);
+ spin_unlock(&vfpriv->winsrv_lock);
+ mutex_unlock(&ddev->filelist_mutex);
+ return conn;
+ }
+ }
+ spin_unlock(&vfpriv->winsrv_lock);
+ }
+ mutex_unlock(&ddev->filelist_mutex);
+
+ return NULL;
+}
+
+static void handle_rx_cmd(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_winsrv_rx *cmd)
+{
+ struct virtio_gpu_winsrv_conn *conn;
+ struct virtio_gpu_winsrv_rx_qentry *qentry;
+
+ conn = find_conn(vgdev, cmd->client_fd);
+ if (!conn) {
+ DRM_DEBUG("recv for unknown client fd %u\n", cmd->client_fd);
+ return;
+ }
+
+ qentry = kzalloc(sizeof(*qentry), GFP_KERNEL);
+ if (!qentry) {
+ spin_unlock(&conn->lock);
+ DRM_DEBUG("failed to allocate qentry for winsrv connection\n");
+ return;
+ }
+
+ qentry->cmd = cmd;
+
+ list_add_tail(&qentry->next, &conn->cmdq);
+ wake_up_interruptible(&conn->cmdwq);
+ spin_unlock(&conn->lock);
+}
+
+void virtio_gpu_dequeue_winsrv_rx_func(struct work_struct *work)
+{
+ struct virtio_gpu_device *vgdev =
+ container_of(work, struct virtio_gpu_device,
+ winsrv_rxq.dequeue_work);
+ struct virtio_gpu_winsrv_rx *cmd;
+ unsigned int len;
+
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+ while ((cmd = virtqueue_get_buf(vgdev->winsrv_rxq.vq, &len)) != NULL) {
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+ handle_rx_cmd(vgdev, cmd);
+ spin_lock(&vgdev->winsrv_rxq.qlock);
+ }
+ spin_unlock(&vgdev->winsrv_rxq.qlock);
+
+ virtqueue_kick(vgdev->winsrv_rxq.vq);
+}
+
static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
struct virtio_gpu_vbuffer *vbuf)
__releases(&vgdev->ctrlq.qlock)
@@ -380,6 +531,41 @@ static int virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
return ret;
}
+static int virtio_gpu_queue_winsrv_tx(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_vbuffer *vbuf)
+{
+ struct virtqueue *vq = vgdev->winsrv_txq.vq;
+ struct scatterlist *sgs[2], vcmd, vout;
+ int ret;
+
+ if (!vgdev->vqs_ready)
+ return -ENODEV;
+
+ sg_init_one(&vcmd, vbuf->buf, vbuf->size);
+ sgs[0] = &vcmd;
+
+ sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
+ sgs[1] = &vout;
+
+ spin_lock(&vgdev->winsrv_txq.qlock);
+retry:
+ ret = virtqueue_add_sgs(vq, sgs, 2, 0, vbuf, GFP_ATOMIC);
+ if (ret == -ENOSPC) {
+ spin_unlock(&vgdev->winsrv_txq.qlock);
+ wait_event(vgdev->winsrv_txq.ack_queue, vq->num_free);
+ spin_lock(&vgdev->winsrv_txq.qlock);
+ goto retry;
+ }
+
+ virtqueue_kick(vq);
+
+ spin_unlock(&vgdev->winsrv_txq.qlock);
+
+ if (!ret)
+ ret = vq->num_free;
+ return ret;
+}
+
/* just create gem objects for userspace and long lived objects,
just use dma_alloced pages for the queue objects? */
@@ -890,3 +1076,100 @@ void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev,
memcpy(cur_p, &output->cursor, sizeof(output->cursor));
virtio_gpu_queue_cursor(vgdev, vbuf);
}
+
+int virtio_gpu_cmd_winsrv_connect(struct virtio_gpu_device *vgdev, int fd)
+{
+ struct virtio_gpu_winsrv_connect *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+
+ cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_WINSRV_CONNECT);
+ cmd_p->client_fd = cpu_to_le32(fd);
+ return virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+}
+
+void virtio_gpu_cmd_winsrv_disconnect(struct virtio_gpu_device *vgdev, int fd)
+{
+ struct virtio_gpu_winsrv_disconnect *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+
+ cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_WINSRV_DISCONNECT);
+ cmd_p->client_fd = cpu_to_le32(fd);
+ virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+}
+
+int virtio_gpu_cmd_winsrv_tx(struct virtio_gpu_device *vgdev,
+ const char __user *buffer, u32 len,
+ int *fds, struct virtio_gpu_winsrv_conn *conn,
+ bool nonblock)
+{
+ int client_fd = conn->fd;
+ struct drm_file *file = conn->drm_file;
+ struct virtio_gpu_winsrv_tx *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+ uint32_t gem_handle;
+ struct drm_gem_object *gobj = NULL;
+ struct virtio_gpu_object *qobj = NULL;
+ int ret, i, fd;
+
+ cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_WINSRV_TX);
+
+ for (i = 0; i < VIRTIO_GPU_WINSRV_MAX_ALLOCS; i++) {
+ cmd_p->resource_ids[i] = -1;
+
+ fd = fds[i];
+ if (fd < 0)
+ break;
+
+ ret = drm_gem_prime_fd_to_handle(vgdev->ddev, file, fd,
+ &gem_handle);
+ if (ret != 0)
+ goto err_free_vbuf;
+
+ gobj = drm_gem_object_lookup(file, gem_handle);
+ if (gobj == NULL) {
+ ret = -ENOENT;
+ goto err_free_vbuf;
+ }
+
+ qobj = gem_to_virtio_gpu_obj(gobj);
+ cmd_p->resource_ids[i] = qobj->hw_res_handle;
+ }
+
+ cmd_p->client_fd = client_fd;
+ cmd_p->len = cpu_to_le32(len);
+
+ /* gets freed when the ring has consumed it */
+ vbuf->data_buf = kmalloc(cmd_p->len, GFP_KERNEL);
+ if (!vbuf->data_buf) {
+ DRM_ERROR("failed to allocate winsrv tx buffer\n");
+ ret = -ENOMEM;
+ goto err_free_vbuf;
+ }
+
+ vbuf->data_size = cmd_p->len;
+
+ if (copy_from_user(vbuf->data_buf, buffer, cmd_p->len)) {
+ ret = -EFAULT;
+ goto err_free_databuf;
+ }
+
+ virtio_gpu_queue_winsrv_tx(vgdev, vbuf);
+
+ return 0;
+
+err_free_databuf:
+ kfree(vbuf->data_buf);
+err_free_vbuf:
+ free_vbuf(vgdev, vbuf);
+
+ return ret;
+}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 91a31ffed828..89b0a1a707a7 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -46,6 +46,11 @@ extern "C" {
#define DRM_VIRTGPU_TRANSFER_TO_HOST 0x07
#define DRM_VIRTGPU_WAIT 0x08
#define DRM_VIRTGPU_GET_CAPS 0x09
+#define DRM_VIRTGPU_WINSRV_CONNECT 0x0a
+#define DRM_VIRTGPU_WINSRV_TX 0x0b
+#define DRM_VIRTGPU_WINSRV_RX 0x0c
+
+#define VIRTGPU_WINSRV_MAX_ALLOCS 28
struct drm_virtgpu_map {
__u64 offset; /* use for mmap system call */
@@ -132,6 +137,18 @@ struct drm_virtgpu_get_caps {
__u32 pad;
};
+struct drm_virtgpu_winsrv {
+ __s32 fds[VIRTGPU_WINSRV_MAX_ALLOCS];
+ __u64 data;
+ __u32 len;
+ __u32 pad;
+};
+
+struct drm_virtgpu_winsrv_connect {
+ __u32 fd; /* returned by kernel */
+ __u32 pad;
+};
+
#define DRM_IOCTL_VIRTGPU_MAP \
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_MAP, struct drm_virtgpu_map)
@@ -167,6 +184,18 @@ struct drm_virtgpu_get_caps {
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_GET_CAPS, \
struct drm_virtgpu_get_caps)
+#define DRM_IOCTL_VIRTGPU_WINSRV_CONNECT \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_WINSRV_CONNECT, \
+ struct drm_virtgpu_winsrv_connect)
+
+#define DRM_IOCTL_VIRTGPU_WINSRV_TX \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_WINSRV_TX, \
+ struct drm_virtgpu_winsrv)
+
+#define DRM_IOCTL_VIRTGPU_WINSRV_RX \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_WINSRV_RX, \
+ struct drm_virtgpu_winsrv)
+
#if defined(__cplusplus)
}
#endif
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 4b04ead26cd9..1be9cf203ab1 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -71,6 +71,12 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
VIRTIO_GPU_CMD_MOVE_CURSOR,
+ /* window server commands */
+ VIRTIO_GPU_CMD_WINSRV_CONNECT = 0x0400,
+ VIRTIO_GPU_CMD_WINSRV_DISCONNECT,
+ VIRTIO_GPU_CMD_WINSRV_TX,
+ VIRTIO_GPU_CMD_WINSRV_RX,
+
/* success responses */
VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
@@ -290,6 +296,39 @@ struct virtio_gpu_resp_capset {
__u8 capset_data[];
};
+/* VIRTIO_GPU_CMD_WINSRV_CONNECT */
+struct virtio_gpu_winsrv_connect {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+};
+
+/* VIRTIO_GPU_CMD_WINSRV_DISCONNECT */
+struct virtio_gpu_winsrv_disconnect {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+};
+
+#define VIRTIO_GPU_WINSRV_MAX_ALLOCS 28
+#define VIRTIO_GPU_WINSRV_TX_MAX_DATA 4096
+
+/* VIRTIO_GPU_CMD_WINSRV_TX */
+/* these commands are followed in the queue descriptor by protocol buffers */
+struct virtio_gpu_winsrv_tx {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+ __le32 len;
+ __le32 resource_ids[VIRTIO_GPU_WINSRV_MAX_ALLOCS];
+};
+
+/* VIRTIO_GPU_CMD_WINSRV_RX */
+struct virtio_gpu_winsrv_rx {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 client_fd;
+ __u8 data[VIRTIO_GPU_WINSRV_TX_MAX_DATA];
+ __le32 len;
+ __le32 resource_ids[VIRTIO_GPU_WINSRV_MAX_ALLOCS];
+};
+
#define VIRTIO_GPU_EVENT_DISPLAY (1 << 0)
struct virtio_gpu_config {
--
2.14.3
^ permalink raw reply related
* (unknown),
From: Solen win @ 2017-12-14 16:26 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 23 bytes --]
Solenwin@freshdesk.com
[-- Attachment #1.2: Type: text/html, Size: 141 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-14 18:12 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, quan.xu,
nilal, riel, cornelia.huck, mhocko, linux-kernel, amit.shah,
pbonzini, akpm, mgorman
In-Reply-To: <201712150129.BFC35949.FFtFOLSOJOQHVM@I-love.SAKURA.ne.jp>
On Fri, Dec 15, 2017 at 01:29:45AM +0900, Tetsuo Handa wrote:
> > > Also, one more thing you need to check. Have you checked how long does
> > > xb_find_next_set_bit(xb, 0, ULONG_MAX) on an empty xbitmap takes?
> > > If it causes soft lockup warning, should we add cond_resched() ?
> > > If yes, you have to document that this API might sleep. If no, you
> > > have to document that the caller of this API is responsible for
> > > not to pass such a large value range.
> >
> > Yes, that will take too long time. Probably we can document some
> > comments as a reminder for the callers.
>
> Then, I feel that API is poorly implemented. There is no need to brute-force
> when scanning [0, ULONG_MAX] range. If you eliminate exception path and
> redesign the data structure, xbitmap will become as simple as a sample
> implementation shown below. Not tested yet, but I think that this will be
> sufficient for what virtio-baloon wants to do; i.e. find consecutive "1" bits
> quickly. I didn't test whether finding "struct ulong_list_data" using radix
> tree can improve performance.
find_next_set_bit() is just badly implemented. There is no need to
redesign the data structure. It should be a simple matter of:
- look at ->head, see it is NULL, return false.
If bit 100 is set and you call find_next_set_bit(101, ULONG_MAX), it
should look at block 0, see there is a pointer to it, scan the block,
see there are no bits set above 100, then realise we're at the end of
the tree and stop.
If bit 2000 is set, and you call find_next_set_bit(2001, ULONG_MAX)
tit should look at block 1, see there's no bit set after bit 2001, then
look at the other blocks in the node, see that all the pointers are NULL
and stop.
This isn't rocket science, we already do something like this in the radix
tree and it'll be even easier to do in the XArray. Which I'm going back
to working on now.
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Michael S. Tsirkin @ 2017-12-14 18:48 UTC (permalink / raw)
To: Mark Rutland; +Cc: Cornelia Huck, weiping zhang, linux-kernel, virtualization
In-Reply-To: <20171213143413.e3efqns53333uf5g@lakrids.cambridge.arm.com>
On Wed, Dec 13, 2017 at 02:34:14PM +0000, Mark Rutland wrote:
> On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
> > On Tue, 12 Dec 2017 13:45:50 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > > Recent rework of the virtio_mmio probe/remove paths balanced a
> > > devm_ioremap() with an iounmap() rather than its devm variant. This ends
> > > up corrupting the devm datastructures, and results in the following
> > > boot-time splat on arm64 under QEMU 2.9.0:
> > >
> > > [ 3.450397] ------------[ cut here ]------------
> > > [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> > > [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
> > > [ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
> > > [ 3.475898]
> > > [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
> > > [ 3.513109] Hardware name: linux,dummy-virt (DT)
> > > [ 3.525382] Call trace:
> > > [ 3.531683] dump_backtrace+0x0/0x368
> > > [ 3.543921] show_stack+0x20/0x30
> > > [ 3.547767] dump_stack+0x108/0x164
> > > [ 3.559584] panic+0x25c/0x51c
> > > [ 3.569184] __warn+0x29c/0x31c
> > > [ 3.576023] report_bug+0x1d4/0x290
> > > [ 3.586069] bug_handler.part.2+0x40/0x100
> > > [ 3.597820] bug_handler+0x4c/0x88
> > > [ 3.608400] brk_handler+0x11c/0x218
> > > [ 3.613430] do_debug_exception+0xe8/0x318
> > > [ 3.627370] el1_dbg+0x18/0x78
> > > [ 3.634037] __vunmap+0x1b8/0x220
> > > [ 3.648747] vunmap+0x6c/0xc0
> > > [ 3.653864] __iounmap+0x44/0x58
> > > [ 3.659771] devm_ioremap_release+0x34/0x68
> > > [ 3.672983] release_nodes+0x404/0x880
> > > [ 3.683543] devres_release_all+0x6c/0xe8
> > > [ 3.695692] driver_probe_device+0x250/0x828
> > > [ 3.706187] __driver_attach+0x190/0x210
> > > [ 3.717645] bus_for_each_dev+0x14c/0x1f0
> > > [ 3.728633] driver_attach+0x48/0x78
> > > [ 3.740249] bus_add_driver+0x26c/0x5b8
> > > [ 3.752248] driver_register+0x16c/0x398
> > > [ 3.757211] __platform_driver_register+0xd8/0x128
> > > [ 3.770860] virtio_mmio_init+0x1c/0x24
> > > [ 3.782671] do_one_initcall+0xe0/0x398
> > > [ 3.791890] kernel_init_freeable+0x594/0x660
> > > [ 3.798514] kernel_init+0x18/0x190
> > > [ 3.810220] ret_from_fork+0x10/0x18
> > >
> > > To fix this, we can simply rip out the explicit cleanup that the devm
> > > infrastructure will do for us when our probe function returns an error
> > > code, or when our remove function returns.
> > >
> > > We only need to ensure that we call put_device() if a call to
> > > register_virtio_device() fails in the probe path.
> > >
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
> > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: weiping zhang <zhangweiping@didichuxing.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > ---
> > > drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
> > > 1 file changed, 9 insertions(+), 34 deletions(-)
> >
> > In the hope that I have grokked the devm_* interface by now,
> >
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> Thanks!
>
> Michael, could you please queue this as a fix for v4.15?
>
> This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2,
> impacting our automated regression testing, and I'd very much like to
> get back to testing pure mainline rather than mainline + local fixes.
>
> Thanks,
> Mark.
Yep, plan to.
Thanks!
^ permalink raw reply
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: Michael S. Tsirkin @ 2017-12-14 19:13 UTC (permalink / raw)
To: weiping zhang; +Cc: cohuck, Greg Kroah-Hartman, virtualization
In-Reply-To: <e397f504c0a796481d8aaf0e558cc8425286012c.1513083885.git.zhangweiping@didichuxing.com>
On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> As mentioned at drivers/base/core.c:
> /*
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up the
> * reference initialized in this function instead.
> */
> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
annotations went to stable, should this go there too?
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
OK but this relies on users knowing that register_virtio_device
calls device_register. I think we want to add a comment
to register_virtio_device.
Also the cleanup is uglified.
I really think the right thing would be to change device_register making
it safe to kfree. People have the right to expect register on failure to
have no effect.
That just might be too hard to implement though.
For now, my suggestion - add a variable.
> ---
> drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..91d20f7 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> pci_set_master(pci_dev);
>
> rc = register_virtio_device(&vp_dev->vdev);
> - if (rc)
> - goto err_register;
> + if (rc) {
> + if (vp_dev->ioaddr)
> + virtio_pci_legacy_remove(vp_dev);
> + else
> + virtio_pci_modern_remove(vp_dev);
> + pci_disable_device(pci_dev);
> + put_device(&vp_dev->vdev.dev);
> + }
>
> - return 0;
> + return rc;
>
> -err_register:
> - if (vp_dev->ioaddr)
> - virtio_pci_legacy_remove(vp_dev);
> - else
> - virtio_pci_modern_remove(vp_dev);
> err_probe:
> pci_disable_device(pci_dev);
> err_enable_device:
> --
> 2.9.4
I'd prefer something like the below.
--->
virtio_pci: don't kfree device on register failure
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1c4797e..995ab03 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
static int virtio_pci_probe(struct pci_dev *pci_dev,
const struct pci_device_id *id)
{
- struct virtio_pci_device *vp_dev;
+ struct virtio_pci_device *vp_dev, *reg_dev = NULL;
int rc;
/* allocate our structure and fill it out */
@@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
pci_set_master(pci_dev);
rc = register_virtio_device(&vp_dev->vdev);
+ /* NOTE: device is considered registered even if register failed. */
+ reg_dev = vp_dev;
if (rc)
goto err_register;
@@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
err_probe:
pci_disable_device(pci_dev);
err_enable_device:
- kfree(vp_dev);
+ if (reg_dev)
+ put_device(dev);
+ else
+ kfree(vp_dev);
return rc;
}
^ permalink raw reply related
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: weiping zhang @ 2017-12-15 1:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cornelia Huck, weiping zhang, Greg Kroah-Hartman, virtualization
In-Reply-To: <20171214205538-mutt-send-email-mst@kernel.org>
2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>:
> On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
>> As mentioned at drivers/base/core.c:
>> /*
>> * NOTE: _Never_ directly free @dev after calling this function, even
>> * if it returned an error! Always use put_device() to give up the
>> * reference initialized in this function instead.
>> */
>> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
>
> seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> annotations went to stable, should this go there too?
>
just let people know the detail reason of using put_device.
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> OK but this relies on users knowing that register_virtio_device
> calls device_register. I think we want to add a comment
> to register_virtio_device.
>
> Also the cleanup is uglified.
>
> I really think the right thing would be to change device_register making
> it safe to kfree. People have the right to expect register on failure to
> have no effect.
>
> That just might be too hard to implement though.
>
> For now, my suggestion - add a variable.
>
>> ---
>> drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index 1c4797e..91d20f7 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>> pci_set_master(pci_dev);
>>
>> rc = register_virtio_device(&vp_dev->vdev);
>> - if (rc)
>> - goto err_register;
>> + if (rc) {
>> + if (vp_dev->ioaddr)
>> + virtio_pci_legacy_remove(vp_dev);
>> + else
>> + virtio_pci_modern_remove(vp_dev);
>> + pci_disable_device(pci_dev);
>> + put_device(&vp_dev->vdev.dev);
>> + }
>>
>> - return 0;
>> + return rc;
>>
>> -err_register:
>> - if (vp_dev->ioaddr)
>> - virtio_pci_legacy_remove(vp_dev);
>> - else
>> - virtio_pci_modern_remove(vp_dev);
>> err_probe:
>> pci_disable_device(pci_dev);
>> err_enable_device:
>> --
>> 2.9.4
>
> I'd prefer something like the below.
>
> --->
>
> virtio_pci: don't kfree device on register failure
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..995ab03 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
> static int virtio_pci_probe(struct pci_dev *pci_dev,
> const struct pci_device_id *id)
> {
> - struct virtio_pci_device *vp_dev;
> + struct virtio_pci_device *vp_dev, *reg_dev = NULL;
> int rc;
>
> /* allocate our structure and fill it out */
> @@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> pci_set_master(pci_dev);
>
> rc = register_virtio_device(&vp_dev->vdev);
> + /* NOTE: device is considered registered even if register failed. */
> + reg_dev = vp_dev;
> if (rc)
> goto err_register;
>
> @@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> err_probe:
> pci_disable_device(pci_dev);
> err_enable_device:
> - kfree(vp_dev);
> + if (reg_dev)
> + put_device(dev);
> + else
> + kfree(vp_dev);
> return rc;
> }
looks more cleaner and same coding style.
Need I send V3 or apply your patch directly ?
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: Michael S. Tsirkin @ 2017-12-15 1:48 UTC (permalink / raw)
To: weiping zhang
Cc: Cornelia Huck, weiping zhang, Greg Kroah-Hartman, virtualization
In-Reply-To: <CAA70yB4jyroEGd+rKHMA_AJ93JGTC8Q4qADBs87q3z7x9XwEMQ@mail.gmail.com>
On Fri, Dec 15, 2017 at 09:38:42AM +0800, weiping zhang wrote:
> 2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>:
> > On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> >> As mentioned at drivers/base/core.c:
> >> /*
> >> * NOTE: _Never_ directly free @dev after calling this function, even
> >> * if it returned an error! Always use put_device() to give up the
> >> * reference initialized in this function instead.
> >> */
> >> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
> >
> > seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> > annotations went to stable, should this go there too?
> >
> just let people know the detail reason of using put_device.
> >> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >
> > OK but this relies on users knowing that register_virtio_device
> > calls device_register. I think we want to add a comment
> > to register_virtio_device.
> >
> > Also the cleanup is uglified.
> >
> > I really think the right thing would be to change device_register making
> > it safe to kfree. People have the right to expect register on failure to
> > have no effect.
> >
> > That just might be too hard to implement though.
> >
> > For now, my suggestion - add a variable.
> >
> >> ---
> >> drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
> >> 1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index 1c4797e..91d20f7 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> >> pci_set_master(pci_dev);
> >>
> >> rc = register_virtio_device(&vp_dev->vdev);
> >> - if (rc)
> >> - goto err_register;
> >> + if (rc) {
> >> + if (vp_dev->ioaddr)
> >> + virtio_pci_legacy_remove(vp_dev);
> >> + else
> >> + virtio_pci_modern_remove(vp_dev);
> >> + pci_disable_device(pci_dev);
> >> + put_device(&vp_dev->vdev.dev);
> >> + }
> >>
> >> - return 0;
> >> + return rc;
> >>
> >> -err_register:
> >> - if (vp_dev->ioaddr)
> >> - virtio_pci_legacy_remove(vp_dev);
> >> - else
> >> - virtio_pci_modern_remove(vp_dev);
> >> err_probe:
> >> pci_disable_device(pci_dev);
> >> err_enable_device:
> >> --
> >> 2.9.4
> >
> > I'd prefer something like the below.
> >
> > --->
> >
> > virtio_pci: don't kfree device on register failure
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 1c4797e..995ab03 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
> > static int virtio_pci_probe(struct pci_dev *pci_dev,
> > const struct pci_device_id *id)
> > {
> > - struct virtio_pci_device *vp_dev;
> > + struct virtio_pci_device *vp_dev, *reg_dev = NULL;
> > int rc;
> >
> > /* allocate our structure and fill it out */
> > @@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > pci_set_master(pci_dev);
> >
> > rc = register_virtio_device(&vp_dev->vdev);
> > + /* NOTE: device is considered registered even if register failed. */
> > + reg_dev = vp_dev;
> > if (rc)
> > goto err_register;
> >
> > @@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > err_probe:
> > pci_disable_device(pci_dev);
> > err_enable_device:
> > - kfree(vp_dev);
> > + if (reg_dev)
> > + put_device(dev);
> > + else
> > + kfree(vp_dev);
> > return rc;
> > }
> looks more cleaner and same coding style.
> Need I send V3 or apply your patch directly ?
Pls post v3 updating all patches to this style.
Also just to make sure, none of this is a regression and none
of this causes actual known issues right?
I think it's preferrable to defer to next merge cycle unless this
is a regression.
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: weiping zhang @ 2017-12-15 1:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mark Rutland, Cornelia Huck, weiping zhang,
Linux Kernel Mailing List, virtualization
In-Reply-To: <20171214204808-mutt-send-email-mst@kernel.org>
2017-12-15 2:48 GMT+08:00 Michael S. Tsirkin <mst@redhat.com>:
> On Wed, Dec 13, 2017 at 02:34:14PM +0000, Mark Rutland wrote:
>> On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
>> > On Tue, 12 Dec 2017 13:45:50 +0000
>> > Mark Rutland <mark.rutland@arm.com> wrote:
>> >
>> > > Recent rework of the virtio_mmio probe/remove paths balanced a
>> > > devm_ioremap() with an iounmap() rather than its devm variant. This ends
>> > > up corrupting the devm datastructures, and results in the following
>> > > boot-time splat on arm64 under QEMU 2.9.0:
>> > >
>> > > [ 3.450397] ------------[ cut here ]------------
>> > > [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
>> > > [ 3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525 __vunmap+0x1b8/0x220
>> > > [ 3.475898] Kernel panic - not syncing: panic_on_warn set ...
>> > > [ 3.475898]
>> > > [ 3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
>> > > [ 3.513109] Hardware name: linux,dummy-virt (DT)
>> > > [ 3.525382] Call trace:
>> > > [ 3.531683] dump_backtrace+0x0/0x368
>> > > [ 3.543921] show_stack+0x20/0x30
>> > > [ 3.547767] dump_stack+0x108/0x164
>> > > [ 3.559584] panic+0x25c/0x51c
>> > > [ 3.569184] __warn+0x29c/0x31c
>> > > [ 3.576023] report_bug+0x1d4/0x290
>> > > [ 3.586069] bug_handler.part.2+0x40/0x100
>> > > [ 3.597820] bug_handler+0x4c/0x88
>> > > [ 3.608400] brk_handler+0x11c/0x218
>> > > [ 3.613430] do_debug_exception+0xe8/0x318
>> > > [ 3.627370] el1_dbg+0x18/0x78
>> > > [ 3.634037] __vunmap+0x1b8/0x220
>> > > [ 3.648747] vunmap+0x6c/0xc0
>> > > [ 3.653864] __iounmap+0x44/0x58
>> > > [ 3.659771] devm_ioremap_release+0x34/0x68
>> > > [ 3.672983] release_nodes+0x404/0x880
>> > > [ 3.683543] devres_release_all+0x6c/0xe8
>> > > [ 3.695692] driver_probe_device+0x250/0x828
>> > > [ 3.706187] __driver_attach+0x190/0x210
>> > > [ 3.717645] bus_for_each_dev+0x14c/0x1f0
>> > > [ 3.728633] driver_attach+0x48/0x78
>> > > [ 3.740249] bus_add_driver+0x26c/0x5b8
>> > > [ 3.752248] driver_register+0x16c/0x398
>> > > [ 3.757211] __platform_driver_register+0xd8/0x128
>> > > [ 3.770860] virtio_mmio_init+0x1c/0x24
>> > > [ 3.782671] do_one_initcall+0xe0/0x398
>> > > [ 3.791890] kernel_init_freeable+0x594/0x660
>> > > [ 3.798514] kernel_init+0x18/0x190
>> > > [ 3.810220] ret_from_fork+0x10/0x18
>> > >
>> > > To fix this, we can simply rip out the explicit cleanup that the devm
>> > > infrastructure will do for us when our probe function returns an error
>> > > code, or when our remove function returns.
>> > >
>> > > We only need to ensure that we call put_device() if a call to
>> > > register_virtio_device() fails in the probe path.
>> > >
>> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
>> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for virtio_mmio_remove")
>> > > Cc: Cornelia Huck <cohuck@redhat.com>
>> > > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > > Cc: weiping zhang <zhangweiping@didichuxing.com>
>> > > Cc: virtualization@lists.linux-foundation.org
>> > > ---
>> > > drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
>> > > 1 file changed, 9 insertions(+), 34 deletions(-)
>> >
>> > In the hope that I have grokked the devm_* interface by now,
>> >
>> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>
>> Thanks!
>>
>> Michael, could you please queue this as a fix for v4.15?
>>
>> This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2,
>> impacting our automated regression testing, and I'd very much like to
>> get back to testing pure mainline rather than mainline + local fixes.
>>
>> Thanks,
>> Mark.
>
> Yep, plan to.
> Thanks!
Sorry to bother again,
As we know if we call device_register we should keep vdev alive until
dev.release be called. As discuss with Cornelia before,
> - return register_virtio_device(&vm_dev->vdev);
> + rc = register_virtio_device(&vm_dev->vdev);
> + if (rc)
> + goto put_dev;
> + return 0;
> +put_dev:
> + put_device(&vm_dev->vdev.dev);
> Here you give up the extra reference from device_initialize(), which
> may or may not be the last reference (since you don't know if
> device_add() had already exposed the struct device to other code that
> might have acquired a reference). As the device has an empty release
> function, touching the device structure after that is not a real
> problem, but...
cuase devm_ interface will free vdev if probe fail, even dev.ref count not
dev to 0. So devm_ interface, may be not very suitable for device_register.
______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v19 2/7] xbitmap: potential improvement
From: kbuild test robot @ 2017-12-15 3:07 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, kbuild-all, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1513079759-14169-3-git-send-email-wei.w.wang@intel.com>
[-- Attachment #1: Type: text/plain, Size: 2729 bytes --]
Hi Wei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc3 next-20171214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/Virtio-balloon-Enhancement/20171215-100525
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
Note: the linux-review/Wei-Wang/Virtio-balloon-Enhancement/20171215-100525 HEAD 607ddba072bf7f9c9cbacedaccad7c42c5c7149c builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
>> lib/xbitmap.c:80:6: error: conflicting types for 'xb_clear_bit'
void xb_clear_bit(struct xb *xb, unsigned long bit)
^~~~~~~~~~~~
In file included from lib/xbitmap.c:2:0:
include/linux/xbitmap.h:37:5: note: previous declaration of 'xb_clear_bit' was here
int xb_clear_bit(struct xb *xb, unsigned long bit);
^~~~~~~~~~~~
vim +/xb_clear_bit +80 lib/xbitmap.c
71
72 /**
73 * xb_clear_bit - clear a bit in the xbitmap
74 * @xb: the xbitmap tree used to record the bit
75 * @bit: index of the bit to clear
76 *
77 * This function is used to clear a bit in the xbitmap. If all the bits of the
78 * bitmap are 0, the bitmap will be freed.
79 */
> 80 void xb_clear_bit(struct xb *xb, unsigned long bit)
81 {
82 unsigned long index = bit / IDA_BITMAP_BITS;
83 struct radix_tree_root *root = &xb->xbrt;
84 struct radix_tree_node *node;
85 void **slot;
86 struct ida_bitmap *bitmap;
87 unsigned long ebit;
88
89 bit %= IDA_BITMAP_BITS;
90 ebit = bit + 2;
91
92 bitmap = __radix_tree_lookup(root, index, &node, &slot);
93 if (radix_tree_exception(bitmap)) {
94 unsigned long tmp = (unsigned long)bitmap;
95
96 if (ebit >= BITS_PER_LONG)
97 return;
98 tmp &= ~(1UL << ebit);
99 if (tmp == RADIX_TREE_EXCEPTIONAL_ENTRY)
100 __radix_tree_delete(root, node, slot);
101 else
102 rcu_assign_pointer(*slot, (void *)tmp);
103 return;
104 }
105
106 if (!bitmap)
107 return;
108
109 __clear_bit(bit, bitmap->bitmap);
110 if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS)) {
111 kfree(bitmap);
112 __radix_tree_delete(root, node, slot);
113 }
114 }
115 EXPORT_SYMBOL(xb_clear_bit);
116
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6769 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v19 1/7] xbitmap: Introduce xbitmap
From: kbuild test robot @ 2017-12-15 11:05 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, kbuild-all, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1513079759-14169-2-git-send-email-wei.w.wang@intel.com>
Hi Matthew,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.15-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/Virtio-balloon-Enhancement/20171215-100525
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
vim +29 lib/xbitmap.c
5
6 /**
7 * xb_set_bit - set a bit in the xbitmap
8 * @xb: the xbitmap tree used to record the bit
9 * @bit: index of the bit to set
10 *
11 * This function is used to set a bit in the xbitmap. If the bitmap that @bit
12 * resides in is not there, the per-cpu ida_bitmap will be taken.
13 *
14 * Returns: 0 on success. %-EAGAIN indicates that @bit was not set.
15 */
16 int xb_set_bit(struct xb *xb, unsigned long bit)
17 {
18 int err;
19 unsigned long index = bit / IDA_BITMAP_BITS;
20 struct radix_tree_root *root = &xb->xbrt;
21 struct radix_tree_node *node;
22 void **slot;
23 struct ida_bitmap *bitmap;
24 unsigned long ebit;
25
26 bit %= IDA_BITMAP_BITS;
27 ebit = bit + 2;
28
> 29 err = __radix_tree_create(root, index, 0, &node, &slot);
30 if (err)
31 return err;
32 bitmap = rcu_dereference_raw(*slot);
33 if (radix_tree_exception(bitmap)) {
34 unsigned long tmp = (unsigned long)bitmap;
35
36 if (ebit < BITS_PER_LONG) {
37 tmp |= 1UL << ebit;
38 rcu_assign_pointer(*slot, (void *)tmp);
39 return 0;
40 }
41 bitmap = this_cpu_xchg(ida_bitmap, NULL);
42 if (!bitmap)
43 return -EAGAIN;
44 memset(bitmap, 0, sizeof(*bitmap));
45 bitmap->bitmap[0] = tmp >> RADIX_TREE_EXCEPTIONAL_SHIFT;
46 rcu_assign_pointer(*slot, bitmap);
47 }
48
49 if (!bitmap) {
50 if (ebit < BITS_PER_LONG) {
51 bitmap = (void *)((1UL << ebit) |
52 RADIX_TREE_EXCEPTIONAL_ENTRY);
> 53 __radix_tree_replace(root, node, slot, bitmap, NULL);
54 return 0;
55 }
56 bitmap = this_cpu_xchg(ida_bitmap, NULL);
57 if (!bitmap)
58 return -EAGAIN;
59 memset(bitmap, 0, sizeof(*bitmap));
60 __radix_tree_replace(root, node, slot, bitmap, NULL);
61 }
62
63 __set_bit(bit, bitmap->bitmap);
64 return 0;
65 }
66 EXPORT_SYMBOL(xb_set_bit);
67
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: Cornelia Huck @ 2017-12-15 12:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Greg Kroah-Hartman, weiping zhang, virtualization
In-Reply-To: <20171214205538-mutt-send-email-mst@kernel.org>
On Thu, 14 Dec 2017 21:13:28 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> > As mentioned at drivers/base/core.c:
> > /*
> > * NOTE: _Never_ directly free @dev after calling this function, even
> > * if it returned an error! Always use put_device() to give up the
> > * reference initialized in this function instead.
> > */
> > so we don't free vp_dev until vp_dev->vdev.dev.release be called.
>
> seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> annotations went to stable, should this go there too?
>
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> OK but this relies on users knowing that register_virtio_device
> calls device_register. I think we want to add a comment
> to register_virtio_device.
>
> Also the cleanup is uglified.
>
> I really think the right thing would be to change device_register making
> it safe to kfree. People have the right to expect register on failure to
> have no effect.
>
> That just might be too hard to implement though.
Yes. The main problem is that device_register() at some point makes the
structure visible to others, at which point they may obtain a
reference. If that happened, you cannot clean up unless that other
party gave up their reference -- which means your only chance to get
this right is the current put_device() approach.
It *is* problematic if all of that stuff is hidden behind too many
calling layers. If you have the device_initialize() -> device_add()
calling sequence, having to do a put_device() on failure is much more
obvious. But as you usually don't pass in a pure struct device but
something embedding it, the put_device() needs to be done on the
outermost level.
Commenting can help here, as would probably a static checker for that
code pattern.
^ permalink raw reply
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: Cornelia Huck @ 2017-12-15 12:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Greg Kroah-Hartman, weiping zhang, virtualization
In-Reply-To: <20171215034700-mutt-send-email-mst@kernel.org>
On Fri, 15 Dec 2017 03:48:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Also just to make sure, none of this is a regression and none
> of this causes actual known issues right?
>
> I think it's preferrable to defer to next merge cycle unless this
> is a regression.
I noticed this while looking at the cleanup path for
regsiter_virtio_device(), I don't think any actual problems have been
seen in the wild (just a latent bug). Deferring to the next merge
window seems reasonable.
^ permalink raw reply
* Re: [PATCH v19 1/7] xbitmap: Introduce xbitmap
From: Matthew Wilcox @ 2017-12-15 13:24 UTC (permalink / raw)
To: kbuild test robot
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, kbuild-all, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201712151837.MQq7hdgk%fengguang.wu@intel.com>
On Fri, Dec 15, 2017 at 07:05:07PM +0800, kbuild test robot wrote:
> 21 struct radix_tree_node *node;
> 22 void **slot;
^^^
missing __rcu annotation here.
Wei, could you fold that change into your next round? Thanks!
^ permalink raw reply
* [PULL] vhost: regression fixes
From: Michael S. Tsirkin @ 2017-12-15 18:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: mark.rutland, kvm, mst, netdev, cohuck, linux-kernel,
virtualization, zhangweiping
The following changes since commit 03e9f8a05bce7330bcd9c5cc54c8e42d0fcbf993:
virtio_net: fix return value check in receive_mergeable() (2017-12-07 18:34:52 +0200)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to c2e90800aef22e7ea14ea7560ba99993f11d3616:
virtio_mmio: fix devm cleanup (2017-12-14 21:01:40 +0200)
----------------------------------------------------------------
virtio: regression fixes
Fixes two issues in the latest kernel.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Mark Rutland (1):
virtio_mmio: fix devm cleanup
Michael S. Tsirkin (1):
ptr_ring: fix up after recent ptr_ring changes
drivers/virtio/virtio_mmio.c | 43 +++++++++-------------------------------
tools/virtio/ringtest/ptr_ring.c | 29 +++++++++++++++++++++------
2 files changed, 32 insertions(+), 40 deletions(-)
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Michael S. Tsirkin @ 2017-12-15 18:26 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201712160121.BEJ26052.HOFFOOQFMLtSVJ@I-love.SAKURA.ne.jp>
On Sat, Dec 16, 2017 at 01:21:52AM +0900, Tetsuo Handa wrote:
> My understanding is that virtio-balloon wants to handle sparsely spreaded
> unsigned long values (which is PATCH 4/7) and wants to find all chunks of
> consecutive "1" bits efficiently. Therefore, I guess that holding the values
> in ascending order at store time is faster than sorting the values at read
> time.
Are you asking why is a bitmap used here, as opposed to a tree? It's
not just store versus read. There's also the issue that memory can get
highly fragmented, if it is, the number of 1s is potentially very high.
A bitmap can use as little as 1 bit per value, it is hard to beat in
this respect.
--
MST
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-15 18:42 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1513079759-14169-4-git-send-email-wei.w.wang@intel.com>
On Tue, Dec 12, 2017 at 07:55:55PM +0800, Wei Wang wrote:
> +int xb_preload_and_set_bit(struct xb *xb, unsigned long bit, gfp_t gfp);
I'm struggling to understand when one would use this. The xb_ API
requires you to handle your own locking. But specifying GFP flags
here implies you can sleep. So ... um ... there's no locking?
> +void xb_clear_bit_range(struct xb *xb, unsigned long start, unsigned long end);
That's xb_zero() which you deleted with the previous patch ... remember,
keep things as close as possible to the bitmap API.
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-15 18:49 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, quan.xu,
nilal, riel, cornelia.huck, mhocko, linux-kernel, amit.shah,
pbonzini, akpm, mgorman
In-Reply-To: <201712160121.BEJ26052.HOFFOOQFMLtSVJ@I-love.SAKURA.ne.jp>
On Sat, Dec 16, 2017 at 01:21:52AM +0900, Tetsuo Handa wrote:
> My understanding is that virtio-balloon wants to handle sparsely spreaded
> unsigned long values (which is PATCH 4/7) and wants to find all chunks of
> consecutive "1" bits efficiently. Therefore, I guess that holding the values
> in ascending order at store time is faster than sorting the values at read
> time. I don't know how to use radix tree API, but I think that B+ tree API
> suits for holding the values in ascending order.
>
> We wait for Wei to post radix tree version combined into one patch and then
> compare performance between radix tree version and B+ tree version (shown
> below)?
Sure. We all benefit from some friendly competition. Even if a
competition between trees might remind one of the Entmoot ;-)
But let's not hold back -- let's figure out some good workloads to use
in our competition. And we should also decide on the API / locking
constraints. And of course we should compete based on not just speed,
but also memory consumption (both as a runtime overhead for a given set
of bits and as code size). If you can replace the IDR, you get to count
that savings against the cost of your implementation.
Here's the API I'm looking at right now. The user need take no lock;
the locking (spinlock) is handled internally to the implementation.
void xbit_init(struct xbitmap *xb);
int xbit_alloc(struct xbitmap *, unsigned long bit, gfp_t);
int xbit_alloc_range(struct xbitmap *, unsigned long start,
unsigned long nbits, gfp_t);
int xbit_set(struct xbitmap *, unsigned long bit, gfp_t);
bool xbit_test(struct xbitmap *, unsigned long bit);
int xbit_clear(struct xbitmap *, unsigned long bit);
int xbit_zero(struct xbitmap *, unsigned long start, unsigned long nbits);
int xbit_fill(struct xbitmap *, unsigned long start, unsigned long nbits,
gfp_t);
unsigned long xbit_find_clear(struct xbitmap *, unsigned long start,
unsigned long max);
unsigned long xbit_find_set(struct xbitmap *, unsigned long start,
unsigned long max);
> static bool set_ulong(struct ulong_list_head *head, const unsigned long value)
> {
> if (!ptr) {
> ptr = kzalloc(sizeof(*ptr), GFP_NOWAIT | __GFP_NOWARN);
> if (!ptr)
> goto out1;
> ptr->bitmap = kzalloc(BITMAP_LEN / 8,
> GFP_NOWAIT | __GFP_NOWARN);
> if (!ptr->bitmap)
> goto out2;
> if (btree_insertl(&head->btree, ~segment, ptr,
> GFP_NOWAIT | __GFP_NOWARN))
> goto out3;
> out3:
> kfree(ptr->bitmap);
> out2:
> kfree(ptr);
> out1:
> return false;
> }
And what is the user supposed to do if this returns false? How do they
make headway? The xb_ API is clear -- you call xb_prealloc and that
ensures forward progress.
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-15 19:22 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, quan.xu,
nilal, riel, cornelia.huck, mhocko, linux-kernel, amit.shah,
pbonzini, akpm, mgorman
In-Reply-To: <20171215184915.GB27160@bombadil.infradead.org>
On Fri, Dec 15, 2017 at 10:49:15AM -0800, Matthew Wilcox wrote:
> Here's the API I'm looking at right now. The user need take no lock;
> the locking (spinlock) is handled internally to the implementation.
I looked at the API some more and found some flaws:
- how does xbit_alloc communicate back which bit it allocated?
- What if xbit_find_set() is called on a completely empty array with
a range of 0, ULONG_MAX -- there's no invalid number to return.
- xbit_clear() can't return an error. Neither can xbit_zero().
- Need to add __must_check to various return values to discourage sloppy
programming
So I modify the proposed API we compete with thusly:
bool xbit_test(struct xbitmap *, unsigned long bit);
int __must_check xbit_set(struct xbitmap *, unsigned long bit, gfp_t);
void xbit_clear(struct xbitmap *, unsigned long bit);
int __must_check xbit_alloc(struct xbitmap *, unsigned long *bit, gfp_t);
int __must_check xbit_fill(struct xbitmap *, unsigned long start,
unsigned long nbits, gfp_t);
void xbit_zero(struct xbitmap *, unsigned long start, unsigned long nbits);
int __must_check xbit_alloc_range(struct xbitmap *, unsigned long *bit,
unsigned long nbits, gfp_t);
bool xbit_find_clear(struct xbitmap *, unsigned long *start, unsigned long max);
bool xbit_find_set(struct xbitmap *, unsigned long *start, unsigned long max);
(I'm a little sceptical about the API accepting 'max' for the find
functions and 'nbits' in the fill/zero/alloc_range functions, but I think
that matches how people want to use it, and it matches how bitmap.h works)
^ permalink raw reply
* Re: [PATCH] drm/virtio: Add window server support
From: kbuild test robot @ 2017-12-16 0:50 UTC (permalink / raw)
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, linux-kernel,
dri-devel, virtualization, kbuild-all
In-Reply-To: <20171214124323.10139-1-tomeu.vizoso@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]
Hi Tomeu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.15-rc3]
[cannot apply to drm/drm-next drm-exynos/exynos-drm/for-next next-20171215]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tomeu-Vizoso/drm-virtio-Add-window-server-support/20171216-081939
config: i386-randconfig-x016-201750 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/gpu/drm/virtio/virtgpu_ioctl.c: In function 'winsrv_ioctl_rx':
>> drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
if (copy_to_user((void *)cmd->data + read_count,
^
drivers/gpu/drm/virtio/virtgpu_ioctl.c: In function 'winsrv_ioctl':
drivers/gpu/drm/virtio/virtgpu_ioctl.c:615:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
(const char __user *) winsrv_cmd.data,
^
vim +562 drivers/gpu/drm/virtio/virtgpu_ioctl.c
547
548 static int winsrv_ioctl_rx(struct virtio_gpu_device *vgdev,
549 struct virtio_gpu_winsrv_conn *conn,
550 struct drm_virtgpu_winsrv *cmd)
551 {
552 struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp;
553 struct virtio_gpu_winsrv_rx *virtio_cmd;
554 int available_len = cmd->len;
555 int read_count = 0;
556
557 list_for_each_entry_safe(qentry, tmp, &conn->cmdq, next) {
558 virtio_cmd = qentry->cmd;
559 if (virtio_cmd->len > available_len)
560 return 0;
561
> 562 if (copy_to_user((void *)cmd->data + read_count,
563 virtio_cmd->data,
564 virtio_cmd->len)) {
565 /* return error unless we have some data to return */
566 if (read_count == 0)
567 return -EFAULT;
568 }
569
570 /*
571 * here we could export resource IDs to FDs, but no protocol
572 * as of today requires it
573 */
574
575 available_len -= virtio_cmd->len;
576 read_count += virtio_cmd->len;
577
578 virtio_gpu_queue_winsrv_rx_in(vgdev, virtio_cmd);
579
580 list_del(&qentry->next);
581 kfree(qentry);
582 }
583
584 cmd->len = read_count;
585
586 return 0;
587 }
588
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23999 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drm/virtio: Add window server support
From: kbuild test robot @ 2017-12-16 0:58 UTC (permalink / raw)
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, linux-kernel,
dri-devel, virtualization, kbuild-all
In-Reply-To: <20171214124323.10139-1-tomeu.vizoso@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 4459 bytes --]
Hi Tomeu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.15-rc3]
[cannot apply to drm/drm-next drm-exynos/exynos-drm/for-next next-20171215]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tomeu-Vizoso/drm-virtio-Add-window-server-support/20171216-081939
config: i386-randconfig-x002-201750 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/wait.h:7,
from include/linux/wait_bit.h:8,
from include/linux/fs.h:6,
from include/uapi/linux/aio_abi.h:31,
from include/linux/syscalls.h:72,
from drivers/gpu/drm/virtio/virtgpu_ioctl.c:29:
drivers/gpu/drm/virtio/virtgpu_ioctl.c: In function 'winsrv_ioctl_rx':
drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
if (copy_to_user((void *)cmd->data + read_count,
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:3: note: in expansion of macro 'if'
if (copy_to_user((void *)cmd->data + read_count,
^~
drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
if (copy_to_user((void *)cmd->data + read_count,
^
include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:3: note: in expansion of macro 'if'
if (copy_to_user((void *)cmd->data + read_count,
^~
drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:20: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
if (copy_to_user((void *)cmd->data + read_count,
^
include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^~~~
>> drivers/gpu/drm/virtio/virtgpu_ioctl.c:562:3: note: in expansion of macro 'if'
if (copy_to_user((void *)cmd->data + read_count,
^~
drivers/gpu/drm/virtio/virtgpu_ioctl.c: In function 'winsrv_ioctl':
drivers/gpu/drm/virtio/virtgpu_ioctl.c:615:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
(const char __user *) winsrv_cmd.data,
^
vim +/if +562 drivers/gpu/drm/virtio/virtgpu_ioctl.c
547
548 static int winsrv_ioctl_rx(struct virtio_gpu_device *vgdev,
549 struct virtio_gpu_winsrv_conn *conn,
550 struct drm_virtgpu_winsrv *cmd)
551 {
552 struct virtio_gpu_winsrv_rx_qentry *qentry, *tmp;
553 struct virtio_gpu_winsrv_rx *virtio_cmd;
554 int available_len = cmd->len;
555 int read_count = 0;
556
557 list_for_each_entry_safe(qentry, tmp, &conn->cmdq, next) {
558 virtio_cmd = qentry->cmd;
559 if (virtio_cmd->len > available_len)
560 return 0;
561
> 562 if (copy_to_user((void *)cmd->data + read_count,
563 virtio_cmd->data,
564 virtio_cmd->len)) {
565 /* return error unless we have some data to return */
566 if (read_count == 0)
567 return -EFAULT;
568 }
569
570 /*
571 * here we could export resource IDs to FDs, but no protocol
572 * as of today requires it
573 */
574
575 available_len -= virtio_cmd->len;
576 read_count += virtio_cmd->len;
577
578 virtio_gpu_queue_winsrv_rx_in(vgdev, virtio_cmd);
579
580 list_del(&qentry->next);
581 kfree(qentry);
582 }
583
584 cmd->len = read_count;
585
586 return 0;
587 }
588
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27343 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-16 5:05 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, quan.xu,
nilal, riel, cornelia.huck, mhocko, linux-kernel, amit.shah,
pbonzini, akpm, mgorman
In-Reply-To: <201712161331.ABI26579.OtOMFSOLHVFFQJ@I-love.SAKURA.ne.jp>
On Sat, Dec 16, 2017 at 01:31:24PM +0900, Tetsuo Handa wrote:
> Michael S. Tsirkin wrote:
> > On Sat, Dec 16, 2017 at 01:21:52AM +0900, Tetsuo Handa wrote:
> > > My understanding is that virtio-balloon wants to handle sparsely spreaded
> > > unsigned long values (which is PATCH 4/7) and wants to find all chunks of
> > > consecutive "1" bits efficiently. Therefore, I guess that holding the values
> > > in ascending order at store time is faster than sorting the values at read
> > > time.
What makes you think that the radix tree (also xbitmap, also idr) doesn't
sort the values at store time?
> I'm asking whether we really need to invent a new library module (i.e.
> PATCH 1/7 + PATCH 2/7 + PATCH 3/7) for virtio-balloon compared to mine.
>
> What virtio-balloon needs is ability to
>
> (1) record any integer value in [0, ULONG_MAX] range
>
> (2) fetch all recorded values, with consecutive values combined in
> min,max (or start,count) form for efficiently
>
> and I wonder whether we need to invent complete API set which
> Matthew Wilcox and Wei Wang are planning for generic purpose.
The xbitmap absolutely has that ability. And making it generic code
means more people see it, use it, debug it, optimise it. I originally
wrote the implementation for bcache, when Kent was complaining we didn't
have such a thing. His needs weren't as complex as Wei's, which is why
I hadn't implemented everything that Wei needed.
^ permalink raw reply
* Re: [PATCH v19 1/7] xbitmap: Introduce xbitmap
From: Wei Wang @ 2017-12-16 10:10 UTC (permalink / raw)
To: Matthew Wilcox, kbuild test robot
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, kbuild-all, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171215132405.GB10348@bombadil.infradead.org>
On 12/15/2017 09:24 PM, Matthew Wilcox wrote:
> On Fri, Dec 15, 2017 at 07:05:07PM +0800, kbuild test robot wrote:
>> 21 struct radix_tree_node *node;
>> 22 void **slot;
> ^^^
> missing __rcu annotation here.
>
> Wei, could you fold that change into your next round? Thanks!
>
Sure, I'll do. Thanks for your time on this patch series.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-16 10:12 UTC (permalink / raw)
To: Matthew Wilcox
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171215184256.GA27160@bombadil.infradead.org>
On 12/16/2017 02:42 AM, Matthew Wilcox wrote:
> On Tue, Dec 12, 2017 at 07:55:55PM +0800, Wei Wang wrote:
>> +int xb_preload_and_set_bit(struct xb *xb, unsigned long bit, gfp_t gfp);
> I'm struggling to understand when one would use this. The xb_ API
> requires you to handle your own locking. But specifying GFP flags
> here implies you can sleep. So ... um ... there's no locking?
In the regular use cases, people would do xb_preload() before taking the
lock, and the xb_set/clear within the lock.
In the virtio-balloon usage, we have a large number of bits to set with
the balloon_lock being held (we're not unlocking for each bit), so we
used the above wrapper to do preload and set within the balloon_lock,
and passed in GFP_NOWAIT to avoid sleeping. Probably we can change to
put this wrapper implementation to virtio-balloon, since it would not be
useful for the regular cases.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-16 10:14 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201712150129.BFC35949.FFtFOLSOJOQHVM@I-love.SAKURA.ne.jp>
On 12/15/2017 12:29 AM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> I used the example of xb_clear_bit_range(), and xb_find_next_bit() is
>> the same fundamentally. Please let me know if anywhere still looks fuzzy.
> I don't think it is the same for xb_find_next_bit() with set == 0.
>
> + if (radix_tree_exception(bmap)) {
> + unsigned long tmp = (unsigned long)bmap;
> + unsigned long ebit = bit + 2;
> +
> + if (ebit >= BITS_PER_LONG)
> + continue;
> + if (set)
> + ret = find_next_bit(&tmp, BITS_PER_LONG, ebit);
> + else
> + ret = find_next_zero_bit(&tmp, BITS_PER_LONG,
> + ebit);
> + if (ret < BITS_PER_LONG)
> + return ret - 2 + IDA_BITMAP_BITS * index;
>
> What I'm saying is that find_next_zero_bit() will not be called if you do
> "if (ebit >= BITS_PER_LONG) continue;" before calling find_next_zero_bit().
>
> When scanning "0000000000000000000000000000000000000000000000000000000000000001",
> "bit < BITS_PER_LONG - 2" case finds "0" in this word but
> "bit >= BITS_PER_LONG - 2" case finds "0" in next word or segment.
>
> I can't understand why this is correct behavior. It is too much puzzling.
>
OK, I'll post out a version without the exceptional path.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: Michael S. Tsirkin @ 2017-12-16 22:13 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Greg Kroah-Hartman, weiping zhang, virtualization
In-Reply-To: <20171215132127.71c33352.cohuck@redhat.com>
On Fri, Dec 15, 2017 at 01:21:27PM +0100, Cornelia Huck wrote:
> On Thu, 14 Dec 2017 21:13:28 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> > > As mentioned at drivers/base/core.c:
> > > /*
> > > * NOTE: _Never_ directly free @dev after calling this function, even
> > > * if it returned an error! Always use put_device() to give up the
> > > * reference initialized in this function instead.
> > > */
> > > so we don't free vp_dev until vp_dev->vdev.dev.release be called.
> >
> > seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> > annotations went to stable, should this go there too?
> >
> > > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >
> > OK but this relies on users knowing that register_virtio_device
> > calls device_register. I think we want to add a comment
> > to register_virtio_device.
> >
> > Also the cleanup is uglified.
> >
> > I really think the right thing would be to change device_register making
> > it safe to kfree. People have the right to expect register on failure to
> > have no effect.
> >
> > That just might be too hard to implement though.
>
> Yes. The main problem is that device_register() at some point makes the
> structure visible to others, at which point they may obtain a
> reference. If that happened, you cannot clean up unless that other
> party gave up their reference -- which means your only chance to get
> this right is the current put_device() approach.
>
> It *is* problematic if all of that stuff is hidden behind too many
> calling layers. If you have the device_initialize() -> device_add()
> calling sequence, having to do a put_device() on failure is much more
> obvious. But as you usually don't pass in a pure struct device but
> something embedding it, the put_device() needs to be done on the
> outermost level.
>
> Commenting can help here, as would probably a static checker for that
> code pattern.
A semantic patch is probably the best we can do here.
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox