Cleanup ringbuffer code also fix some issues in the util services.
Vitaly Kuznetsov (9):
Drivers: hv: utils: fix memory leak on on_msg() failure
Drivers: hv: utils: rename outmsg_lock
Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY mode
Drivers: hv: utils: fix crash when device is removed from host side
Drivers: hv: ring_buffer.c: fix comment style
Drivers: hv: ring_buffer: remove stray smp_read_barrier_depends()
Drivers: hv: ring_buffer: remove code duplication from
hv_ringbuffer_peek/read()
Drivers: hv: remove code duplication between
vmbus_recvpacket()/vmbus_recvpacket_raw()
Drivers: hv: ring_buffer: eliminate hv_ringbuffer_peek()
drivers/hv/channel.c | 81 +++------------
drivers/hv/hv_utils_transport.c | 112 ++++++++++++++++-----
drivers/hv/hv_utils_transport.h | 3 +-
drivers/hv/hyperv_vmbus.h | 11 +--
drivers/hv/ring_buffer.c | 218 +++++++++++---------------------------
include/linux/hyperv.h | 2 -
6 files changed, 172 insertions(+), 255 deletions(-)
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
inmsg should be freed in case of on_msg() failure to avoid memory leak.
Preserve the error code from on_msg().
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/hv_utils_transport.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 24b2766..40abe44 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -77,6 +77,7 @@ static ssize_t hvt_op_write(struct file *file, const char __user *buf,
{
struct hvutil_transport *hvt;
u8 *inmsg;
+ int ret;
hvt = container_of(file->f_op, struct hvutil_transport, fops);
@@ -84,11 +85,11 @@ static ssize_t hvt_op_write(struct file *file, const char __user *buf,
if (IS_ERR(inmsg))
return PTR_ERR(inmsg);
- if (hvt->on_msg(inmsg, count))
- return -EFAULT;
+ ret = hvt->on_msg(inmsg, count);
+
kfree(inmsg);
- return count;
+ return ret ? ret : count;
}
static unsigned int hvt_op_poll(struct file *file, poll_table *wait)
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
As a preparation to reusing outmsg_lock to protect test-and-set openrations
on 'mode' rename it the more general 'lock'.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/hv_utils_transport.c | 14 +++++++-------
drivers/hv/hv_utils_transport.h | 2 +-
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 40abe44..59c6f3d 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -27,11 +27,11 @@ static struct list_head hvt_list = LIST_HEAD_INIT(hvt_list);
static void hvt_reset(struct hvutil_transport *hvt)
{
- mutex_lock(&hvt->outmsg_lock);
+ mutex_lock(&hvt->lock);
kfree(hvt->outmsg);
hvt->outmsg = NULL;
hvt->outmsg_len = 0;
- mutex_unlock(&hvt->outmsg_lock);
+ mutex_unlock(&hvt->lock);
if (hvt->on_reset)
hvt->on_reset();
}
@@ -47,7 +47,7 @@ static ssize_t hvt_op_read(struct file *file, char __user *buf,
if (wait_event_interruptible(hvt->outmsg_q, hvt->outmsg_len > 0))
return -EINTR;
- mutex_lock(&hvt->outmsg_lock);
+ mutex_lock(&hvt->lock);
if (!hvt->outmsg) {
ret = -EAGAIN;
goto out_unlock;
@@ -68,7 +68,7 @@ static ssize_t hvt_op_read(struct file *file, char __user *buf,
hvt->outmsg_len = 0;
out_unlock:
- mutex_unlock(&hvt->outmsg_lock);
+ mutex_unlock(&hvt->lock);
return ret;
}
@@ -197,7 +197,7 @@ int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len)
return ret;
}
/* HVUTIL_TRANSPORT_CHARDEV */
- mutex_lock(&hvt->outmsg_lock);
+ mutex_lock(&hvt->lock);
if (hvt->outmsg) {
/* Previous message wasn't received */
ret = -EFAULT;
@@ -211,7 +211,7 @@ int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len)
} else
ret = -ENOMEM;
out_unlock:
- mutex_unlock(&hvt->outmsg_lock);
+ mutex_unlock(&hvt->lock);
return ret;
}
@@ -242,7 +242,7 @@ struct hvutil_transport *hvutil_transport_init(const char *name,
hvt->mdev.fops = &hvt->fops;
init_waitqueue_head(&hvt->outmsg_q);
- mutex_init(&hvt->outmsg_lock);
+ mutex_init(&hvt->lock);
spin_lock(&hvt_list_lock);
list_add(&hvt->list, &hvt_list);
diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
index 314c76c..bff4c92 100644
--- a/drivers/hv/hv_utils_transport.h
+++ b/drivers/hv/hv_utils_transport.h
@@ -38,7 +38,7 @@ struct hvutil_transport {
u8 *outmsg; /* message to the userspace */
int outmsg_len; /* its length */
wait_queue_head_t outmsg_q; /* poll/read wait queue */
- struct mutex outmsg_lock; /* protects outmsg */
+ struct mutex lock; /* protects struct members */
};
struct hvutil_transport *hvutil_transport_init(const char *name,
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
When Hyper-V host asks us to remove some util driver by closing the
appropriate channel there is no easy way to force the current file
descriptor holder to hang up but we can start to respond -EBADF to all
operations asking it to exit gracefully.
As we're setting hvt->mode from two separate contexts now we need to use
a proper locking.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/hv_utils_transport.c | 71 ++++++++++++++++++++++++++++++--------
drivers/hv/hv_utils_transport.h | 1 +
2 files changed, 57 insertions(+), 15 deletions(-)
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 59c6f3d..31c2f86 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -27,11 +27,9 @@ static struct list_head hvt_list = LIST_HEAD_INIT(hvt_list);
static void hvt_reset(struct hvutil_transport *hvt)
{
- mutex_lock(&hvt->lock);
kfree(hvt->outmsg);
hvt->outmsg = NULL;
hvt->outmsg_len = 0;
- mutex_unlock(&hvt->lock);
if (hvt->on_reset)
hvt->on_reset();
}
@@ -44,10 +42,17 @@ static ssize_t hvt_op_read(struct file *file, char __user *buf,
hvt = container_of(file->f_op, struct hvutil_transport, fops);
- if (wait_event_interruptible(hvt->outmsg_q, hvt->outmsg_len > 0))
+ if (wait_event_interruptible(hvt->outmsg_q, hvt->outmsg_len > 0 ||
+ hvt->mode != HVUTIL_TRANSPORT_CHARDEV))
return -EINTR;
mutex_lock(&hvt->lock);
+
+ if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) {
+ ret = -EBADF;
+ goto out_unlock;
+ }
+
if (!hvt->outmsg) {
ret = -EAGAIN;
goto out_unlock;
@@ -85,7 +90,10 @@ static ssize_t hvt_op_write(struct file *file, const char __user *buf,
if (IS_ERR(inmsg))
return PTR_ERR(inmsg);
- ret = hvt->on_msg(inmsg, count);
+ if (hvt->mode == HVUTIL_TRANSPORT_DESTROY)
+ ret = -EBADF;
+ else
+ ret = hvt->on_msg(inmsg, count);
kfree(inmsg);
@@ -99,6 +107,10 @@ static unsigned int hvt_op_poll(struct file *file, poll_table *wait)
hvt = container_of(file->f_op, struct hvutil_transport, fops);
poll_wait(file, &hvt->outmsg_q, wait);
+
+ if (hvt->mode == HVUTIL_TRANSPORT_DESTROY)
+ return -EBADF;
+
if (hvt->outmsg_len > 0)
return POLLIN | POLLRDNORM;
@@ -108,26 +120,39 @@ static unsigned int hvt_op_poll(struct file *file, poll_table *wait)
static int hvt_op_open(struct inode *inode, struct file *file)
{
struct hvutil_transport *hvt;
+ int ret = 0;
+ bool issue_reset = false;
hvt = container_of(file->f_op, struct hvutil_transport, fops);
- /*
- * Switching to CHARDEV mode. We switch bach to INIT when device
- * gets released.
- */
- if (hvt->mode == HVUTIL_TRANSPORT_INIT)
+ mutex_lock(&hvt->lock);
+
+ if (hvt->mode == HVUTIL_TRANSPORT_DESTROY) {
+ ret = -EBADF;
+ } else if (hvt->mode == HVUTIL_TRANSPORT_INIT) {
+ /*
+ * Switching to CHARDEV mode. We switch bach to INIT when
+ * device gets released.
+ */
hvt->mode = HVUTIL_TRANSPORT_CHARDEV;
+ }
else if (hvt->mode == HVUTIL_TRANSPORT_NETLINK) {
/*
* We're switching from netlink communication to using char
* device. Issue the reset first.
*/
- hvt_reset(hvt);
+ issue_reset = true;
hvt->mode = HVUTIL_TRANSPORT_CHARDEV;
- } else
- return -EBUSY;
+ } else {
+ ret = -EBUSY;
+ }
- return 0;
+ if (issue_reset)
+ hvt_reset(hvt);
+
+ mutex_unlock(&hvt->lock);
+
+ return ret;
}
static int hvt_op_release(struct inode *inode, struct file *file)
@@ -136,12 +161,15 @@ static int hvt_op_release(struct inode *inode, struct file *file)
hvt = container_of(file->f_op, struct hvutil_transport, fops);
- hvt->mode = HVUTIL_TRANSPORT_INIT;
+ mutex_lock(&hvt->lock);
+ if (hvt->mode != HVUTIL_TRANSPORT_DESTROY)
+ hvt->mode = HVUTIL_TRANSPORT_INIT;
/*
* Cleanup message buffers to avoid spurious messages when the daemon
* connects back.
*/
hvt_reset(hvt);
+ mutex_unlock(&hvt->lock);
return 0;
}
@@ -168,6 +196,7 @@ static void hvt_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
* Switching to NETLINK mode. Switching to CHARDEV happens when someone
* opens the device.
*/
+ mutex_lock(&hvt->lock);
if (hvt->mode == HVUTIL_TRANSPORT_INIT)
hvt->mode = HVUTIL_TRANSPORT_NETLINK;
@@ -175,6 +204,7 @@ static void hvt_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
hvt_found->on_msg(msg->data, msg->len);
else
pr_warn("hvt_cn_callback: unexpected netlink message!\n");
+ mutex_unlock(&hvt->lock);
}
int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len)
@@ -182,7 +212,8 @@ int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len)
struct cn_msg *cn_msg;
int ret = 0;
- if (hvt->mode == HVUTIL_TRANSPORT_INIT) {
+ if (hvt->mode == HVUTIL_TRANSPORT_INIT ||
+ hvt->mode == HVUTIL_TRANSPORT_DESTROY) {
return -EINVAL;
} else if (hvt->mode == HVUTIL_TRANSPORT_NETLINK) {
cn_msg = kzalloc(sizeof(*cn_msg) + len, GFP_ATOMIC);
@@ -198,6 +229,11 @@ int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len)
}
/* HVUTIL_TRANSPORT_CHARDEV */
mutex_lock(&hvt->lock);
+ if (hvt->mode != HVUTIL_TRANSPORT_CHARDEV) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
if (hvt->outmsg) {
/* Previous message wasn't received */
ret = -EFAULT;
@@ -268,6 +304,11 @@ err_free_hvt:
void hvutil_transport_destroy(struct hvutil_transport *hvt)
{
+ mutex_lock(&hvt->lock);
+ hvt->mode = HVUTIL_TRANSPORT_DESTROY;
+ wake_up_interruptible(&hvt->outmsg_q);
+ mutex_unlock(&hvt->lock);
+
spin_lock(&hvt_list_lock);
list_del(&hvt->list);
spin_unlock(&hvt_list_lock);
diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
index bff4c92..06254a1 100644
--- a/drivers/hv/hv_utils_transport.h
+++ b/drivers/hv/hv_utils_transport.h
@@ -25,6 +25,7 @@ enum hvutil_transport_mode {
HVUTIL_TRANSPORT_INIT = 0,
HVUTIL_TRANSPORT_NETLINK,
HVUTIL_TRANSPORT_CHARDEV,
+ HVUTIL_TRANSPORT_DESTROY,
};
struct hvutil_transport {
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
The crash is observed when a service is being disabled host side while
userspace daemon is connected to the device:
[ 90.244859] general protection fault: 0000 [#1] SMP
...
[ 90.800082] Call Trace:
[ 90.800082] [<ffffffff81187008>] __fput+0xc8/0x1f0
[ 90.800082] [<ffffffff8118716e>] ____fput+0xe/0x10
...
[ 90.800082] [<ffffffff81015278>] do_signal+0x28/0x580
[ 90.800082] [<ffffffff81086656>] ? finish_task_switch+0xa6/0x180
[ 90.800082] [<ffffffff81443ebf>] ? __schedule+0x28f/0x870
[ 90.800082] [<ffffffffa01ebbaa>] ? hvt_op_read+0x12a/0x140 [hv_utils]
...
The problem is that hvutil_transport_destroy() which does misc_deregister()
freeing the appropriate device is reachable by two paths: module unload
and from util_remove(). While module unload path is protected by .owner in
struct file_operations util_remove() path is not. Freeing the device while
someone holds an open fd for it is a show stopper.
In general, it is not possible to revoke an fd from all users so the only
way to solve the issue is to defer freeing the hvutil_transport structure.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/hv_utils_transport.c | 26 +++++++++++++++++++++++---
1 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 31c2f86..ee20b50 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -155,13 +155,22 @@ static int hvt_op_open(struct inode *inode, struct file *file)
return ret;
}
+static void hvt_transport_free(struct hvutil_transport *hvt)
+{
+ misc_deregister(&hvt->mdev);
+ kfree(hvt->outmsg);
+ kfree(hvt);
+}
+
static int hvt_op_release(struct inode *inode, struct file *file)
{
struct hvutil_transport *hvt;
+ int mode_old;
hvt = container_of(file->f_op, struct hvutil_transport, fops);
mutex_lock(&hvt->lock);
+ mode_old = hvt->mode;
if (hvt->mode != HVUTIL_TRANSPORT_DESTROY)
hvt->mode = HVUTIL_TRANSPORT_INIT;
/*
@@ -171,6 +180,9 @@ static int hvt_op_release(struct inode *inode, struct file *file)
hvt_reset(hvt);
mutex_unlock(&hvt->lock);
+ if (mode_old == HVUTIL_TRANSPORT_DESTROY)
+ hvt_transport_free(hvt);
+
return 0;
}
@@ -304,17 +316,25 @@ err_free_hvt:
void hvutil_transport_destroy(struct hvutil_transport *hvt)
{
+ int mode_old;
+
mutex_lock(&hvt->lock);
+ mode_old = hvt->mode;
hvt->mode = HVUTIL_TRANSPORT_DESTROY;
wake_up_interruptible(&hvt->outmsg_q);
mutex_unlock(&hvt->lock);
+ /*
+ * In case we were in 'chardev' mode we still have an open fd so we
+ * have to defer freeing the device. Netlink interface can be freed
+ * now.
+ */
spin_lock(&hvt_list_lock);
list_del(&hvt->list);
spin_unlock(&hvt_list_lock);
if (hvt->cn_id.idx > 0 && hvt->cn_id.val > 0)
cn_del_callback(&hvt->cn_id);
- misc_deregister(&hvt->mdev);
- kfree(hvt->outmsg);
- kfree(hvt);
+
+ if (mode_old != HVUTIL_TRANSPORT_CHARDEV)
+ hvt_transport_free(hvt);
}
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
Convert 6+-string comments repeating function names to normal kernel-style
comments and fix a couple of other comment style issues. No textual or
functional changes intended.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/ring_buffer.c | 135 +++++++++-------------------------------------
1 files changed, 26 insertions(+), 109 deletions(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 70a1a9a..7bca513 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -112,9 +112,7 @@ static bool hv_need_to_signal_on_read(u32 prev_write_sz,
u32 read_loc = rbi->ring_buffer->read_index;
u32 pending_sz = rbi->ring_buffer->pending_send_sz;
- /*
- * If the other end is not blocked on write don't bother.
- */
+ /* If the other end is not blocked on write don't bother. */
if (pending_sz == 0)
return false;
@@ -128,12 +126,7 @@ static bool hv_need_to_signal_on_read(u32 prev_write_sz,
return false;
}
-/*
- * hv_get_next_write_location()
- *
- * Get the next write location for the specified ring buffer
- *
- */
+/* Get the next write location for the specified ring buffer. */
static inline u32
hv_get_next_write_location(struct hv_ring_buffer_info *ring_info)
{
@@ -142,12 +135,7 @@ hv_get_next_write_location(struct hv_ring_buffer_info *ring_info)
return next;
}
-/*
- * hv_set_next_write_location()
- *
- * Set the next write location for the specified ring buffer
- *
- */
+/* Set the next write location for the specified ring buffer. */
static inline void
hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
u32 next_write_location)
@@ -155,11 +143,7 @@ hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
ring_info->ring_buffer->write_index = next_write_location;
}
-/*
- * hv_get_next_read_location()
- *
- * Get the next read location for the specified ring buffer
- */
+/* Get the next read location for the specified ring buffer. */
static inline u32
hv_get_next_read_location(struct hv_ring_buffer_info *ring_info)
{
@@ -169,10 +153,8 @@ hv_get_next_read_location(struct hv_ring_buffer_info *ring_info)
}
/*
- * hv_get_next_readlocation_withoffset()
- *
* Get the next read location + offset for the specified ring buffer.
- * This allows the caller to skip
+ * This allows the caller to skip.
*/
static inline u32
hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info,
@@ -186,13 +168,7 @@ hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info,
return next;
}
-/*
- *
- * hv_set_next_read_location()
- *
- * Set the next read location for the specified ring buffer
- *
- */
+/* Set the next read location for the specified ring buffer. */
static inline void
hv_set_next_read_location(struct hv_ring_buffer_info *ring_info,
u32 next_read_location)
@@ -201,12 +177,7 @@ hv_set_next_read_location(struct hv_ring_buffer_info *ring_info,
}
-/*
- *
- * hv_get_ring_buffer()
- *
- * Get the start of the ring buffer
- */
+/* Get the start of the ring buffer. */
static inline void *
hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
{
@@ -214,25 +185,14 @@ hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
}
-/*
- *
- * hv_get_ring_buffersize()
- *
- * Get the size of the ring buffer
- */
+/* Get the size of the ring buffer. */
static inline u32
hv_get_ring_buffersize(struct hv_ring_buffer_info *ring_info)
{
return ring_info->ring_datasize;
}
-/*
- *
- * hv_get_ring_bufferindices()
- *
- * Get the read and write indices as u64 of the specified ring buffer
- *
- */
+/* Get the read and write indices as u64 of the specified ring buffer. */
static inline u64
hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
{
@@ -240,12 +200,8 @@ hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
}
/*
- *
- * hv_copyfrom_ringbuffer()
- *
* Helper routine to copy to source from ring buffer.
* Assume there is enough room. Handles wrap-around in src case only!!
- *
*/
static u32 hv_copyfrom_ringbuffer(
struct hv_ring_buffer_info *ring_info,
@@ -277,12 +233,8 @@ static u32 hv_copyfrom_ringbuffer(
/*
- *
- * hv_copyto_ringbuffer()
- *
* Helper routine to copy from source to ring buffer.
* Assume there is enough room. Handles wrap-around in dest case only!!
- *
*/
static u32 hv_copyto_ringbuffer(
struct hv_ring_buffer_info *ring_info,
@@ -308,13 +260,7 @@ static u32 hv_copyto_ringbuffer(
return start_write_offset;
}
-/*
- *
- * hv_ringbuffer_get_debuginfo()
- *
- * Get various debug metrics for the specified ring buffer
- *
- */
+/* Get various debug metrics for the specified ring buffer. */
void hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
struct hv_ring_buffer_debug_info *debug_info)
{
@@ -337,13 +283,7 @@ void hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
}
}
-/*
- *
- * hv_ringbuffer_init()
- *
- *Initialize the ring buffer
- *
- */
+/* Initialize the ring buffer. */
int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
void *buffer, u32 buflen)
{
@@ -356,9 +296,7 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
ring_info->ring_buffer->read_index =
ring_info->ring_buffer->write_index = 0;
- /*
- * Set the feature bit for enabling flow control.
- */
+ /* Set the feature bit for enabling flow control. */
ring_info->ring_buffer->feature_bits.value = 1;
ring_info->ring_size = buflen;
@@ -369,24 +307,12 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
return 0;
}
-/*
- *
- * hv_ringbuffer_cleanup()
- *
- * Cleanup the ring buffer
- *
- */
+/* Cleanup the ring buffer. */
void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info)
{
}
-/*
- *
- * hv_ringbuffer_write()
- *
- * Write to the ring buffer
- *
- */
+/* Write to the ring buffer. */
int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
struct kvec *kv_list, u32 kv_count, bool *signal)
{
@@ -411,10 +337,11 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
&bytes_avail_toread,
&bytes_avail_towrite);
-
- /* If there is only room for the packet, assume it is full. */
- /* Otherwise, the next time around, we think the ring buffer */
- /* is empty since the read index == write index */
+ /*
+ * If there is only room for the packet, assume it is full.
+ * Otherwise, the next time around, we think the ring buffer
+ * is empty since the read index == write index.
+ */
if (bytes_avail_towrite <= totalbytes_towrite) {
spin_unlock_irqrestore(&outring_info->ring_lock, flags);
return -EAGAIN;
@@ -454,13 +381,7 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
}
-/*
- *
- * hv_ringbuffer_peek()
- *
- * Read without advancing the read index
- *
- */
+/* Read without advancing the read index. */
int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
void *Buffer, u32 buflen)
{
@@ -497,13 +418,7 @@ int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
}
-/*
- *
- * hv_ringbuffer_read()
- *
- * Read and advance the read index
- *
- */
+/* Read and advance the read index. */
int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
u32 buflen, u32 offset, bool *signal)
{
@@ -542,9 +457,11 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
sizeof(u64),
next_read_location);
- /* Make sure all reads are done before we update the read index since */
- /* the writer may start writing to the read area once the read index */
- /*is updated */
+ /*
+ * Make sure all reads are done before we update the read index since
+ * the writer may start writing to the read area once the read index
+ * is updated.
+ */
mb();
/* Update the read index */
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
smp_read_barrier_depends() does nothing on almost all arcitectures
including x86 and having it in the beginning of
hv_get_ringbuffer_availbytes() does not provide any guarantees anyway.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
include/linux/hyperv.h | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index acd995b..179ff33 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -141,8 +141,6 @@ hv_get_ringbuffer_availbytes(struct hv_ring_buffer_info *rbi,
{
u32 read_loc, write_loc, dsize;
- smp_read_barrier_depends();
-
/* Capture the read/write indices before they changed */
read_loc = rbi->ring_buffer->read_index;
write_loc = rbi->ring_buffer->write_index;
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
hv_ringbuffer_peek() does the same as hv_ringbuffer_read() without
advancing the read index. The only functional change this patch brings
is moving hv_need_to_signal_on_read() call under the ring_lock but this
function is just a couple of comparisons.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/ring_buffer.c | 68 +++++++++++++++++-----------------------------
1 files changed, 25 insertions(+), 43 deletions(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 7bca513..07f9408 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -380,47 +380,9 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
return 0;
}
-
-/* Read without advancing the read index. */
-int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
- void *Buffer, u32 buflen)
-{
- u32 bytes_avail_towrite;
- u32 bytes_avail_toread;
- u32 next_read_location = 0;
- unsigned long flags;
-
- spin_lock_irqsave(&Inring_info->ring_lock, flags);
-
- hv_get_ringbuffer_availbytes(Inring_info,
- &bytes_avail_toread,
- &bytes_avail_towrite);
-
- /* Make sure there is something to read */
- if (bytes_avail_toread < buflen) {
-
- spin_unlock_irqrestore(&Inring_info->ring_lock, flags);
-
- return -EAGAIN;
- }
-
- /* Convert to byte offset */
- next_read_location = hv_get_next_read_location(Inring_info);
-
- next_read_location = hv_copyfrom_ringbuffer(Inring_info,
- Buffer,
- buflen,
- next_read_location);
-
- spin_unlock_irqrestore(&Inring_info->ring_lock, flags);
-
- return 0;
-}
-
-
-/* Read and advance the read index. */
-int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
- u32 buflen, u32 offset, bool *signal)
+static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
+ void *buffer, u32 buflen, u32 offset,
+ bool *signal, bool advance)
{
u32 bytes_avail_towrite;
u32 bytes_avail_toread;
@@ -452,6 +414,9 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
buflen,
next_read_location);
+ if (!advance)
+ goto out_unlock;
+
next_read_location = hv_copyfrom_ringbuffer(inring_info,
&prev_indices,
sizeof(u64),
@@ -467,9 +432,26 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
/* Update the read index */
hv_set_next_read_location(inring_info, next_read_location);
- spin_unlock_irqrestore(&inring_info->ring_lock, flags);
-
*signal = hv_need_to_signal_on_read(bytes_avail_towrite, inring_info);
+out_unlock:
+ spin_unlock_irqrestore(&inring_info->ring_lock, flags);
return 0;
}
+
+/* Read from ring buffer without advancing the read index. */
+int hv_ringbuffer_peek(struct hv_ring_buffer_info *inring_info,
+ void *buffer, u32 buflen)
+{
+ return __hv_ringbuffer_read(inring_info, buffer, buflen,
+ 0, NULL, false);
+}
+
+/* Read from ring buffer and advance the read index. */
+int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
+ void *buffer, u32 buflen, u32 offset,
+ bool *signal)
+{
+ return __hv_ringbuffer_read(inring_info, buffer, buflen,
+ offset, signal, true);
+}
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
vmbus_recvpacket() and vmbus_recvpacket_raw() are almost identical but
there are two discrepancies:
1) vmbus_recvpacket() doesn't propagate errors from hv_ringbuffer_read()
which looks like it is not desired.
2) There is an error message printed in packetlen > bufferlen case in
vmbus_recvpacket(). I'm removing it as it is usless for users to see
such messages and /vmbus_recvpacket_raw() doesn't have it.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/channel.c | 65 +++++++++++++++++---------------------------------
1 files changed, 22 insertions(+), 43 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 2889d97..dd6de7f 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -922,8 +922,10 @@ EXPORT_SYMBOL_GPL(vmbus_sendpacket_multipagebuffer);
*
* Mainly used by Hyper-V drivers.
*/
-int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
- u32 bufferlen, u32 *buffer_actual_len, u64 *requestid)
+static inline int
+__vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
+ u32 bufferlen, u32 *buffer_actual_len, u64 *requestid,
+ bool raw)
{
struct vmpacket_descriptor desc;
u32 packetlen;
@@ -941,27 +943,34 @@ int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
return 0;
packetlen = desc.len8 << 3;
- userlen = packetlen - (desc.offset8 << 3);
+ if (!raw)
+ userlen = packetlen - (desc.offset8 << 3);
+ else
+ userlen = packetlen;
*buffer_actual_len = userlen;
- if (userlen > bufferlen) {
-
- pr_err("Buffer too small - got %d needs %d\n",
- bufferlen, userlen);
- return -ETOOSMALL;
- }
+ if (userlen > bufferlen)
+ return -ENOBUFS;
*requestid = desc.trans_id;
/* Copy over the packet to the user buffer */
ret = hv_ringbuffer_read(&channel->inbound, buffer, userlen,
- (desc.offset8 << 3), &signal);
+ raw ? 0 : desc.offset8 << 3, &signal);
if (signal)
vmbus_setevent(channel);
- return 0;
+ return ret;
+}
+
+int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
+ u32 bufferlen, u32 *buffer_actual_len,
+ u64 *requestid)
+{
+ return __vmbus_recvpacket(channel, buffer, bufferlen,
+ buffer_actual_len, requestid, false);
}
EXPORT_SYMBOL(vmbus_recvpacket);
@@ -972,37 +981,7 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
u32 bufferlen, u32 *buffer_actual_len,
u64 *requestid)
{
- struct vmpacket_descriptor desc;
- u32 packetlen;
- int ret;
- bool signal = false;
-
- *buffer_actual_len = 0;
- *requestid = 0;
-
-
- ret = hv_ringbuffer_peek(&channel->inbound, &desc,
- sizeof(struct vmpacket_descriptor));
- if (ret != 0)
- return 0;
-
-
- packetlen = desc.len8 << 3;
-
- *buffer_actual_len = packetlen;
-
- if (packetlen > bufferlen)
- return -ENOBUFS;
-
- *requestid = desc.trans_id;
-
- /* Copy over the entire packet to the user buffer */
- ret = hv_ringbuffer_read(&channel->inbound, buffer, packetlen, 0,
- &signal);
-
- if (signal)
- vmbus_setevent(channel);
-
- return ret;
+ return __vmbus_recvpacket(channel, buffer, bufferlen,
+ buffer_actual_len, requestid, true);
}
EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
--
1.7.4.1
From: Vitaly Kuznetsov <[email protected]>
Currently, there is only one user for hv_ringbuffer_read()/
hv_ringbuffer_peak() functions and the usage of these functions is:
- insecure as we drop ring_lock between them, someone else (in theory
only) can acquire it in between;
- non-optimal as we do a number of things (acquire/release the above
mentioned lock, calculate available space on the ring, ...) twice and
this path is performance-critical.
Remove hv_ringbuffer_peek() moving the logic from __vmbus_recvpacket() to
hv_ringbuffer_read().
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/channel.c | 30 +-------------------
drivers/hv/hyperv_vmbus.h | 11 ++-----
drivers/hv/ring_buffer.c | 65 +++++++++++++++++++++++++-------------------
3 files changed, 42 insertions(+), 64 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index dd6de7f..1161d68 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -927,37 +927,11 @@ __vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
u32 bufferlen, u32 *buffer_actual_len, u64 *requestid,
bool raw)
{
- struct vmpacket_descriptor desc;
- u32 packetlen;
- u32 userlen;
int ret;
bool signal = false;
- *buffer_actual_len = 0;
- *requestid = 0;
-
-
- ret = hv_ringbuffer_peek(&channel->inbound, &desc,
- sizeof(struct vmpacket_descriptor));
- if (ret != 0)
- return 0;
-
- packetlen = desc.len8 << 3;
- if (!raw)
- userlen = packetlen - (desc.offset8 << 3);
- else
- userlen = packetlen;
-
- *buffer_actual_len = userlen;
-
- if (userlen > bufferlen)
- return -ENOBUFS;
-
- *requestid = desc.trans_id;
-
- /* Copy over the packet to the user buffer */
- ret = hv_ringbuffer_read(&channel->inbound, buffer, userlen,
- raw ? 0 : desc.offset8 << 3, &signal);
+ ret = hv_ringbuffer_read(&channel->inbound, buffer, bufferlen,
+ buffer_actual_len, requestid, &signal, raw);
if (signal)
vmbus_setevent(channel);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 4d67e98..0411b7b 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -619,14 +619,9 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
struct kvec *kv_list,
u32 kv_count, bool *signal);
-int hv_ringbuffer_peek(struct hv_ring_buffer_info *ring_info, void *buffer,
- u32 buflen);
-
-int hv_ringbuffer_read(struct hv_ring_buffer_info *ring_info,
- void *buffer,
- u32 buflen,
- u32 offset, bool *signal);
-
+int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
+ void *buffer, u32 buflen, u32 *buffer_actual_len,
+ u64 *requestid, bool *signal, bool raw);
void hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
struct hv_ring_buffer_debug_info *debug_info);
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 07f9408..b53702c 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -380,30 +380,59 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
return 0;
}
-static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
- void *buffer, u32 buflen, u32 offset,
- bool *signal, bool advance)
+int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
+ void *buffer, u32 buflen, u32 *buffer_actual_len,
+ u64 *requestid, bool *signal, bool raw)
{
u32 bytes_avail_towrite;
u32 bytes_avail_toread;
u32 next_read_location = 0;
u64 prev_indices = 0;
unsigned long flags;
+ struct vmpacket_descriptor desc;
+ u32 offset;
+ u32 packetlen;
+ int ret = 0;
if (buflen <= 0)
return -EINVAL;
spin_lock_irqsave(&inring_info->ring_lock, flags);
+ *buffer_actual_len = 0;
+ *requestid = 0;
+
hv_get_ringbuffer_availbytes(inring_info,
&bytes_avail_toread,
&bytes_avail_towrite);
/* Make sure there is something to read */
- if (bytes_avail_toread < buflen) {
- spin_unlock_irqrestore(&inring_info->ring_lock, flags);
+ if (bytes_avail_toread < sizeof(desc)) {
+ /*
+ * No error is set when there is even no header, drivers are
+ * supposed to analyze buffer_actual_len.
+ */
+ goto out_unlock;
+ }
- return -EAGAIN;
+ next_read_location = hv_get_next_read_location(inring_info);
+ next_read_location = hv_copyfrom_ringbuffer(inring_info, &desc,
+ sizeof(desc),
+ next_read_location);
+
+ offset = raw ? 0 : (desc.offset8 << 3);
+ packetlen = (desc.len8 << 3) - offset;
+ *buffer_actual_len = packetlen;
+ *requestid = desc.trans_id;
+
+ if (bytes_avail_toread < packetlen + offset) {
+ ret = -EAGAIN;
+ goto out_unlock;
+ }
+
+ if (packetlen > buflen) {
+ ret = -ENOBUFS;
+ goto out_unlock;
}
next_read_location =
@@ -411,12 +440,9 @@ static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
next_read_location = hv_copyfrom_ringbuffer(inring_info,
buffer,
- buflen,
+ packetlen,
next_read_location);
- if (!advance)
- goto out_unlock;
-
next_read_location = hv_copyfrom_ringbuffer(inring_info,
&prev_indices,
sizeof(u64),
@@ -436,22 +462,5 @@ static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
out_unlock:
spin_unlock_irqrestore(&inring_info->ring_lock, flags);
- return 0;
-}
-
-/* Read from ring buffer without advancing the read index. */
-int hv_ringbuffer_peek(struct hv_ring_buffer_info *inring_info,
- void *buffer, u32 buflen)
-{
- return __hv_ringbuffer_read(inring_info, buffer, buflen,
- 0, NULL, false);
-}
-
-/* Read from ring buffer and advance the read index. */
-int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
- void *buffer, u32 buflen, u32 offset,
- bool *signal)
-{
- return __hv_ringbuffer_read(inring_info, buffer, buflen,
- offset, signal, true);
+ return ret;
}
--
1.7.4.1
> -----Original Message-----
> From: devel [mailto:[email protected]] On Behalf
> Of K. Y. Srinivasan
> Sent: Tuesday, December 15, 2015 11:02
> To: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: [PATCH 3/9] Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY
> mode
>
> From: Vitaly Kuznetsov <[email protected]>
>
> When Hyper-V host asks us to remove some util driver by closing the
> appropriate channel there is no easy way to force the current file
> descriptor holder to hang up but we can start to respond -EBADF to all
> operations asking it to exit gracefully.
>
> As we're setting hvt->mode from two separate contexts now we need to use
> a proper locking.
>
> ...
> @@ -99,6 +107,10 @@ static unsigned int hvt_op_poll(struct file *file,
> poll_table *wait)
> hvt = container_of(file->f_op, struct hvutil_transport, fops);
>
> poll_wait(file, &hvt->outmsg_q, wait);
> +
> + if (hvt->mode == HVUTIL_TRANSPORT_DESTROY)
> + return -EBADF;
> +
> if (hvt->outmsg_len > 0)
> return POLLIN | POLLRDNORM;
Hi Vitaly,
Should hvt_op_poll() return -EBADF -- I think it probably
should return POLLERR or POLLHUP?
-- Dexuan
Dexuan Cui <[email protected]> writes:
>> -----Original Message-----
>> From: devel [mailto:[email protected]] On Behalf
>> Of K. Y. Srinivasan
>> Sent: Tuesday, December 15, 2015 11:02
>> To: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: [PATCH 3/9] Drivers: hv: utils: introduce HVUTIL_TRANSPORT_DESTROY
>> mode
>>
>> From: Vitaly Kuznetsov <[email protected]>
>>
>> When Hyper-V host asks us to remove some util driver by closing the
>> appropriate channel there is no easy way to force the current file
>> descriptor holder to hang up but we can start to respond -EBADF to all
>> operations asking it to exit gracefully.
>>
>> As we're setting hvt->mode from two separate contexts now we need to use
>> a proper locking.
>>
>> ...
>> @@ -99,6 +107,10 @@ static unsigned int hvt_op_poll(struct file *file,
>> poll_table *wait)
>> hvt = container_of(file->f_op, struct hvutil_transport, fops);
>>
>> poll_wait(file, &hvt->outmsg_q, wait);
>> +
>> + if (hvt->mode == HVUTIL_TRANSPORT_DESTROY)
>> + return -EBADF;
>> +
>> if (hvt->outmsg_len > 0)
>> return POLLIN | POLLRDNORM;
>
> Hi Vitaly,
> Should hvt_op_poll() return -EBADF -- I think it probably
> should return POLLERR or POLLHUP?
Oh, sorry, my bad -- hvt_op_poll() returns unsigned int and -EBADF is
definitely inappropriate. I see this patch was already merged to
char-misc-testing so I'll send a follow-up patch to fix things up.
Thanks!
--
Vitaly