* Re: [PATCH 0/7] virtio: console: Fixes, new flexible way of discovering ports
From: Amit Shah @ 2010-03-30 5:20 UTC (permalink / raw)
To: Rusty Russell; +Cc: mst, virtualization
In-Reply-To: <201003301546.14309.rusty@rustcorp.com.au>
On (Tue) Mar 30 2010 [15:46:14], Rusty Russell wrote:
> On Tue, 23 Mar 2010 07:27:53 pm Amit Shah wrote:
> > Hello all,
> >
> > This new patchset switches over to using the control queue for port
> > discovery so that we can stay in sync with the host for port
> > enumeration and also don't use bitmaps to limit us to config space
> > sizes.
> >
> > This changes the ABI, so it would be better to merge this for 2.6.34
> > so that we don't have to worry about released kernels with the older
> > ABI.
>
> This looks horribly like development during the merge window.
Yep, let's go ahead with disabling multiport for now and enabling it
later for .35.
Thanks,
Amit
^ permalink raw reply
* Re: [PATCH 4/4] virtio: disable multiport console support.
From: Amit Shah @ 2010-03-30 6:29 UTC (permalink / raw)
To: Rusty Russell; +Cc: Michael S. Tsirkin, virtualization
In-Reply-To: <201003301549.43186.rusty@rustcorp.com.au>
On (Tue) Mar 30 2010 [15:49:43], Rusty Russell wrote:
> Unfortunately there proved to be at least one bug which requires an
> ABI change, so we're best off not introducing multiport support until
> 2.6.35.
>
> While I generally left the multiport code paths intact, I really wanted
> to remove the ABI defines from the header, which meant some quite deep cuts.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> To: Amit Shah <amit.shah@redhat.com>
> ---
> drivers/char/virtio_console.c | 274 -----------------------------------------
> include/linux/virtio_console.h | 19 --
> 2 files changed, 4 insertions(+), 289 deletions(-)
Instead of this big patch that causes a lot of churn (and we'll have to
re-introduce most of this later anyway), how about this one? Less churn,
same effect (tested, works fine):
From 3d12454057e618d58a8bb203197cfaa351e8aee8 Mon Sep 17 00:00:00 2001
From: Amit Shah <amit.shah@redhat.com>
Date: Tue, 30 Mar 2010 11:42:59 +0530
Subject: [PATCH] virtio: console: Disable multiport support
The host and guest currently enumerate ports independently. This can
lead to problems after several hot-plug/unplug operations and
migrations. The fix is to let the management software instantiate ports
at fixed locations and the host lets the guest know which 'id' to add a
new port to.
Since this is an intrusive change, and we don't want to support the
current ABI, disable multiport support for now and re-enable it for the
next merge window with the other changes.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 44288ce..c51eb5e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1418,6 +1418,8 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
multiport = false;
portdev->config.nr_ports = 1;
portdev->config.max_nr_ports = 1;
+#if 0
+ /* Multiport will be enabled after some design fixes */
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
multiport = true;
vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
@@ -1443,6 +1445,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
/* Let the Host know we support multiple ports.*/
vdev->config->finalize_features(vdev);
+#endif
err = init_vqs(portdev);
if (err < 0) {
@@ -1525,7 +1528,6 @@ static struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
- VIRTIO_CONSOLE_F_MULTIPORT,
};
static struct virtio_driver virtio_console = {
--
1.6.2.5
^ permalink raw reply related
* Re: [PATCH 3/4] virtio: console makes incorrect assumption about virtio API
From: Amit Shah @ 2010-03-30 6:30 UTC (permalink / raw)
To: Rusty Russell; +Cc: Michael S. Tsirkin, virtualization
In-Reply-To: <201003301549.22395.rusty@rustcorp.com.au>
On (Tue) Mar 30 2010 [15:49:22], Rusty Russell wrote:
> The get_buf() API sets the second arg to the number of bytes *written*
> by the other side; in this case it should be zero as these are output buffers.
>
> lguest gets this right (obviously kvm's console doesn't), resulting in
> continual buildup of console writes.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> To: Amit Shah <amit.shah@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
> ---
> drivers/char/virtio_console.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -415,20 +415,16 @@ static ssize_t send_buf(struct port *por
> out_vq->vq_ops->kick(out_vq);
>
> if (ret < 0) {
> - len = 0;
> + in_count = 0;
> goto fail;
> }
>
> - /*
> - * Wait till the host acknowledges it pushed out the data we
> - * sent. Also ensure we return to userspace the number of
> - * bytes that were successfully consumed by the host.
> - */
> + /* Wait till the host acknowledges it pushed out the data we sent. */
> while (!out_vq->vq_ops->get_buf(out_vq, &len))
> cpu_relax();
> fail:
> /* We're expected to return the amount of data we wrote */
> - return len;
> + return in_count;
> }
>
> /*
>
Amit
--
http://log.amitshah.net/
^ permalink raw reply
* CfP with Extended Deadline 5th Workshop on Virtualization in High-Performance Cloud Computing (VHPC'10)
From: Michael Alexander @ 2010-03-30 17:11 UTC (permalink / raw)
To: virtualization
Apologies if you received multiple copies of this message.
=================================================================
CALL FOR PAPERS
5th Workshop on
Virtualization in High-Performance Cloud Computing
VHPC'10
as part of Euro-Par 2010, Island of Ischia-Naples, Italy
=================================================================
Date: August 31, 2010
Euro-Par 2009: http://www.europar2010.org/
Workshop URL: http://vhpc.org
SUBMISSION DEADLINE:
Abstracts: April 4, 2010 (extended)
Full Paper: June 19, 2010 (extended)
Scope:
Virtualization has become a common abstraction layer in modern data
centers, enabling resource owners to manage complex infrastructure
independently of their applications. Conjointly virtualization is
becoming a driving technology for a manifold of industry grade IT
services. Piloted by the Amazon Elastic Computing Cloud services, the
cloud concept includes the notion of a separation between resource
owners and users, adding services such as hosted application
frameworks and queuing. Utilizing the same infrastructure, clouds
carry significant potential for use in high-performance scientific
computing. The ability of clouds to provide for requests and releases
of vast computing resource dynamically and close to the marginal cost
of providing the services is unprecedented in the history of
scientific and commercial computing.
Distributed computing concepts that leverage federated resource access
are popular within the grid community, but have not seen previously
desired deployed levels so far. Also, many of the scientific
datacenters have not adopted virtualization or cloud concepts yet.
This workshop aims to bring together industrial providers with the
scientific community in order to foster discussion, collaboration and
mutual exchange of knowledge and experience.
The workshop will be one day in length, composed of 20 min paper
presentations, each followed by 10 min discussion sections.
Presentations may be accompanied by interactive demonstrations. It
concludes with a 30 min panel discussion by presenters.
TOPICS
Topics include, but are not limited to, the following subjects:
- Virtualization in cloud, cluster and grid HPC environments
- VM cloud, cluster load distribution algorithms
- Cloud, cluster and grid filesystems
- QoS and and service level guarantees
- Cloud programming models, APIs and databases
- Software as a service (SaaS)
- Cloud provisioning
- Virtualized I/O
- VMMs and storage virtualization
- MPI, PVM on virtual machines
- High-performance network virtualization
- High-speed interconnects
- Hypervisor extensions
- Tools for cluster and grid computing
- Xen/other VMM cloud/cluster/grid tools
- Raw device access from VMs
- Cloud reliability, fault-tolerance, and security
- Cloud load balancing
- VMs - power efficiency
- Network architectures for VM-based environments
- VMMs/Hypervisors
- Hardware support for virtualization
- Fault tolerant VM environments
- Workload characterizations for VM-based environments
- Bottleneck management
- Metering
- VM-based cloud performance modeling
- Cloud security, access control and data integrity
- Performance management and tuning hosts and guest VMs
- VMM performance tuning on various load types
- Research and education use cases
- Cloud use cases
- Management of VM environments and clouds
- Deployment of VM-based environments
PAPER SUBMISSION
Papers submitted to the workshop will be reviewed by at least two
members of the program committee and external reviewers. Submissions
should include abstract, key words, the e-mail address of the
corresponding author, and must not exceed 10 pages, including tables
and figures at a main font size no smaller than 11 point. Submission
of a paper should be regarded as a commitment that, should the paper
be accepted, at least one of the authors will register and attend the
conference to present the work.
Accepted papers will be published in the Springer LNCS series - the
format must be according to the Springer LNCS Style. Initial
submissions are in PDF, accepted papers will be requested to provided
source files.
Format Guidelines: http://www.springer.de/comp/lncs/authors.html
Submission Link: http://edas.info/newPaper.php?c=8553
IMPORTANT DATES
April 4 - Abstract submission due (extended)
May 19 - Full paper submission (extended)
July 14 - Acceptance notification
August 3 - Camera-ready version due
August 31 - September 3 - conference
CHAIR
Michael Alexander (chair), scaledinfra technologies GmbH, Austria
Gianluigi Zanetti (co-chair), CRS4, Italy
PROGRAM COMMITTEE
Padmashree Apparao, Intel Corp., USA
Volker Buege, University of Karlsruhe, Germany
Roberto Canonico, University of Napoli Federico II, Italy
Tommaso Cucinotta, Scuola Superiore Sant'Anna, Italy
Werner Fischer, Thomas Krenn AG, Germany
William Gardner, University of Guelph, Canada
Wolfgang Gentzsch, DEISA. Max Planck Gesellschaft, Germany
Derek Groen, UVA, The Netherlands
Marcus Hardt, Forschungszentrum Karlsruhe, Germany
Sverre Jarp, CERN, Switzerland
Shantenu Jha, Louisiana State University, USA
Xuxian Jiang, NC State, USA
Kenji Kaneda, Google, Japan
Yves Kemp, DESY Hamburg, Germany
Ignacio Llorente, Universidad Complutense de Madrid, Spain
Naoya Maruyama, Tokyo Institute of Technology, Japan
Jean-Marc Menaud, Ecole des Mines de Nantes, France
Anastassios Nano, National Technical University of Athens, Greece
Oliver Oberst, Karlsruhe Institute of Technology, Germany
Jose Renato Santos, HP Labs, USA
Borja Sotomayor, University of Chicago, USA
Yoshio Turner, HP Labs, USA
Kurt Tuschku, University of Vienna, Austria
Lizhe Wang, Indiana University, USA
Chao-Tung Yang, Tunghai University, Taiwan
DURATION: Workshop Duration is one day.
GENERAL INFORMATION
The workshop will be held as part of Euro-Par 2010,
Island of Ischia-Naples, Italy.
Euro-Par 2010: http://www.europar2010.org/
^ permalink raw reply
* [PATCH][QEMU][VHOST]fix feature bit handling for mergeable rx buffers
From: David L Stevens @ 2010-03-30 19:21 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization
This patch adds mergeable receive buffer support to qemu-kvm,
to allow enabling it when vhost_net supports it.
It also adds a missing call to vhost_net_ack_features() to
push acked features to vhost_net.
The patch is relative to Michael Tsirkin's qemu-kvm git tree.
+-DLS
Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
diff -ruNp qemu-kvm.mst/hw/vhost_net.c qemu-kvm.dls/hw/vhost_net.c
--- qemu-kvm.mst/hw/vhost_net.c 2010-03-03 13:39:07.000000000 -0800
+++ qemu-kvm.dls/hw/vhost_net.c 2010-03-29 20:37:34.000000000 -0700
@@ -5,6 +5,7 @@
#include <sys/ioctl.h>
#include <linux/vhost.h>
#include <linux/virtio_ring.h>
+#include <linux/if_tun.h>
#include <netpacket/packet.h>
#include <net/ethernet.h>
#include <net/if.h>
@@ -38,15 +39,6 @@ unsigned vhost_net_get_features(struct v
return features;
}
-void vhost_net_ack_features(struct vhost_net *net, unsigned features)
-{
- net->dev.acked_features = net->dev.backend_features;
- if (features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY))
- net->dev.acked_features |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
- if (features & (1 << VIRTIO_RING_F_INDIRECT_DESC))
- net->dev.acked_features |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
-}
-
static int vhost_net_get_fd(VLANClientState *backend)
{
switch (backend->info->type) {
@@ -58,6 +50,25 @@ static int vhost_net_get_fd(VLANClientSt
}
}
+void vhost_net_ack_features(struct vhost_net *net, unsigned features)
+{
+ int vnet_hdr_sz = sizeof(struct virtio_net_hdr);
+
+ net->dev.acked_features = net->dev.backend_features;
+ if (features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY))
+ net->dev.acked_features |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+ if (features & (1 << VIRTIO_RING_F_INDIRECT_DESC))
+ net->dev.acked_features |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
+ if (features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
+ net->dev.acked_features |= (1 << VIRTIO_NET_F_MRG_RXBUF);
+ vnet_hdr_sz = sizeof(struct virtio_net_hdr_mrg_rxbuf);
+ }
+#ifdef TUNSETVNETHDRSZ
+ if (ioctl(vhost_net_get_fd(net->vc), TUNSETVNETHDRSZ, &vnet_hdr_sz) < 0)
+ perror("TUNSETVNETHDRSZ");
+#endif /* TUNSETVNETHDRSZ */
+}
+
struct vhost_net *vhost_net_init(VLANClientState *backend, int devfd)
{
int r;
diff -ruNp qemu-kvm.mst/hw/virtio-net.c qemu-kvm.dls/hw/virtio-net.c
--- qemu-kvm.mst/hw/virtio-net.c 2010-03-03 13:39:07.000000000 -0800
+++ qemu-kvm.dls/hw/virtio-net.c 2010-03-29 16:15:46.000000000 -0700
@@ -211,12 +211,16 @@ static void virtio_net_set_features(Virt
n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
if (n->has_vnet_hdr) {
+ struct vhost_net *vhost_net = tap_get_vhost_net(n->nic->nc.peer);
+
tap_set_offload(n->nic->nc.peer,
(features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
(features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
(features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
(features >> VIRTIO_NET_F_GUEST_ECN) & 1,
(features >> VIRTIO_NET_F_GUEST_UFO) & 1);
+ if (vhost_net)
+ vhost_net_ack_features(vhost_net, features);
}
}
^ permalink raw reply
* [PATCH v2] Add Mergeable RX buffer feature to vhost_net
From: David Stevens @ 2010-03-31 1:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <20100308080733.GC7482@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 22372 bytes --]
This patch adds support for the Mergeable Receive Buffers feature to
vhost_net.
Changes:
1) generalize descriptor allocation functions to allow multiple
descriptors per packet
2) add socket "peek" to know datalen at buffer allocation time
3) change notification to enable a multi-buffer max packet, rather
than the single-buffer run until completely empty
Changes from previous revision:
1) incorporate review comments from Michael Tsirkin
2) assume use of TUNSETVNETHDRSZ ioctl by qemu, which simplifies vnet
header
processing
3) fixed notification code to only affect the receive side
Signed-Off-By: David L Stevens <dlstevens@us.ibm.com>
[in-line for review, attached for applying w/o whitespace mangling]
diff -ruNp net-next-p0/drivers/vhost/net.c net-next-p3/drivers/vhost/net.c
--- net-next-p0/drivers/vhost/net.c 2010-03-22 12:04:38.000000000
-0700
+++ net-next-p3/drivers/vhost/net.c 2010-03-30 12:50:57.000000000
-0700
@@ -54,26 +54,6 @@ struct vhost_net {
enum vhost_net_poll_state tx_poll_state;
};
-/* Pop first len bytes from iovec. Return number of segments used. */
-static int move_iovec_hdr(struct iovec *from, struct iovec *to,
- size_t len, int iov_count)
-{
- int seg = 0;
- size_t size;
- while (len && seg < iov_count) {
- size = min(from->iov_len, len);
- to->iov_base = from->iov_base;
- to->iov_len = size;
- from->iov_len -= size;
- from->iov_base += size;
- len -= size;
- ++from;
- ++to;
- ++seg;
- }
- return seg;
-}
-
/* Caller must have TX VQ lock */
static void tx_poll_stop(struct vhost_net *net)
{
@@ -97,7 +77,8 @@ static void tx_poll_start(struct vhost_n
static void handle_tx(struct vhost_net *net)
{
struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
- unsigned head, out, in, s;
+ unsigned out, in;
+ struct iovec head;
struct msghdr msg = {
.msg_name = NULL,
.msg_namelen = 0,
@@ -108,8 +89,8 @@ static void handle_tx(struct vhost_net *
};
size_t len, total_len = 0;
int err, wmem;
- size_t hdr_size;
struct socket *sock = rcu_dereference(vq->private_data);
+
if (!sock)
return;
@@ -127,22 +108,19 @@ static void handle_tx(struct vhost_net *
if (wmem < sock->sk->sk_sndbuf / 2)
tx_poll_stop(net);
- hdr_size = vq->hdr_size;
for (;;) {
- head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in,
- NULL, NULL);
+ head.iov_base = (void *)vhost_get_vq_desc(&net->dev, vq,
+ vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
NULL);
/* Nothing new? Wait for eventfd to tell us they
refilled. */
- if (head == vq->num) {
+ if (head.iov_base == (void *)vq->num) {
wmem = atomic_read(&sock->sk->sk_wmem_alloc);
if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
tx_poll_start(net, sock);
set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
break;
}
- if (unlikely(vhost_enable_notify(vq))) {
+ if (unlikely(vhost_enable_notify(vq, 0))) {
vhost_disable_notify(vq);
continue;
}
@@ -154,27 +132,30 @@ static void handle_tx(struct vhost_net *
break;
}
/* Skip header. TODO: support TSO. */
- s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
msg.msg_iovlen = out;
- len = iov_length(vq->iov, out);
+ head.iov_len = len = iov_length(vq->iov, out);
+
/* Sanity check */
if (!len) {
- vq_err(vq, "Unexpected header len for TX: "
- "%zd expected %zd\n",
- iov_length(vq->hdr, s), hdr_size);
+ vq_err(vq, "Unexpected buffer len for TX: %zd ",
len);
break;
}
- /* TODO: Check specific error and bomb out unless ENOBUFS?
*/
err = sock->ops->sendmsg(NULL, sock, &msg, len);
if (unlikely(err < 0)) {
- vhost_discard_vq_desc(vq);
- tx_poll_start(net, sock);
+ if (err == -EAGAIN) {
+ vhost_discard_vq_desc(vq, 1);
+ tx_poll_start(net, sock);
+ } else {
+ vq_err(vq, "sendmsg: errno %d\n", -err);
+ /* drop packet; do not discard/resend */
+ vhost_add_used_and_signal(&net->dev, vq,
&head,
+ 1, 0);
+ }
break;
}
if (err != len)
- pr_err("Truncated TX packet: "
- " len %d != %zd\n", err, len);
- vhost_add_used_and_signal(&net->dev, vq, head, 0);
+ pr_err("Truncated TX packet: len %d != %zd\n",
err, len);
+ vhost_add_used_and_signal(&net->dev, vq, &head, 1, 0);
total_len += len;
if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
vhost_poll_queue(&vq->poll);
@@ -186,12 +167,25 @@ static void handle_tx(struct vhost_net *
unuse_mm(net->dev.mm);
}
+static int vhost_head_len(struct sock *sk)
+{
+ struct sk_buff *head;
+ int len = 0;
+
+ lock_sock(sk);
+ head = skb_peek(&sk->sk_receive_queue);
+ if (head)
+ len = head->len;
+ release_sock(sk);
+ return len;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_rx(struct vhost_net *net)
{
struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
- unsigned head, out, in, log, s;
+ unsigned in, log;
struct vhost_log *vq_log;
struct msghdr msg = {
.msg_name = NULL,
@@ -202,34 +196,25 @@ static void handle_rx(struct vhost_net *
.msg_flags = MSG_DONTWAIT,
};
- struct virtio_net_hdr hdr = {
- .flags = 0,
- .gso_type = VIRTIO_NET_HDR_GSO_NONE
- };
-
size_t len, total_len = 0;
- int err;
- size_t hdr_size;
+ int err, headcount, datalen;
struct socket *sock = rcu_dereference(vq->private_data);
+
if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
return;
use_mm(net->dev.mm);
mutex_lock(&vq->mutex);
vhost_disable_notify(vq);
- hdr_size = vq->hdr_size;
vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
vq->log : NULL;
- for (;;) {
- head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in,
- vq_log, &log);
+ while ((datalen = vhost_head_len(sock->sk))) {
+ headcount = vhost_get_heads(vq, datalen, &in, vq_log,
&log);
/* OK, now we need to know about added descriptors. */
- if (head == vq->num) {
- if (unlikely(vhost_enable_notify(vq))) {
+ if (!headcount) {
+ if (unlikely(vhost_enable_notify(vq, 1))) {
/* They have slipped one in as we were
* doing that: check again. */
vhost_disable_notify(vq);
@@ -240,46 +225,42 @@ static void handle_rx(struct vhost_net *
break;
}
/* We don't need to be notified again. */
- if (out) {
- vq_err(vq, "Unexpected descriptor format for RX: "
- "out %d, int %d\n",
- out, in);
- break;
- }
- /* Skip header. TODO: support TSO/mergeable rx buffers. */
- s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
+ if (vq->rxmaxheadcount < headcount)
+ vq->rxmaxheadcount = headcount;
+ /* Skip header. TODO: support TSO. */
msg.msg_iovlen = in;
len = iov_length(vq->iov, in);
/* Sanity check */
if (!len) {
- vq_err(vq, "Unexpected header len for RX: "
- "%zd expected %zd\n",
- iov_length(vq->hdr, s), hdr_size);
+ vq_err(vq, "Unexpected buffer len for RX: %zd\n",
len);
break;
}
err = sock->ops->recvmsg(NULL, sock, &msg,
len, MSG_DONTWAIT | MSG_TRUNC);
- /* TODO: Check specific error and bomb out unless EAGAIN?
*/
if (err < 0) {
- vhost_discard_vq_desc(vq);
+ vhost_discard_vq_desc(vq, headcount);
break;
}
/* TODO: Should check and handle checksum. */
+ if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
{
+ struct virtio_net_hdr_mrg_rxbuf *vhdr =
+ (struct virtio_net_hdr_mrg_rxbuf *)
+ vq->iov[0].iov_base;
+ /* add num_bufs */
+ if (put_user(headcount, &vhdr->num_buffers)) {
+ vq_err(vq, "Failed to write num_buffers");
+ vhost_discard_vq_desc(vq, headcount);
+ break;
+ }
+ }
if (err > len) {
pr_err("Discarded truncated rx packet: "
" len %d > %zd\n", err, len);
- vhost_discard_vq_desc(vq);
+ vhost_discard_vq_desc(vq, headcount);
continue;
}
len = err;
- err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr,
hdr_size);
- if (err) {
- vq_err(vq, "Unable to write vnet_hdr at addr %p:
%d\n",
- vq->iov->iov_base, err);
- break;
- }
- len += hdr_size;
- vhost_add_used_and_signal(&net->dev, vq, head, len);
+ vhost_add_used_and_signal(&net->dev,vq,vq->heads,headcount,1);
if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, len);
total_len += len;
@@ -560,9 +541,6 @@ done:
static int vhost_net_set_features(struct vhost_net *n, u64 features)
{
- size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
- sizeof(struct virtio_net_hdr) : 0;
- int i;
mutex_lock(&n->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&n->dev)) {
@@ -571,11 +549,6 @@ static int vhost_net_set_features(struct
}
n->dev.acked_features = features;
smp_wmb();
- for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
- mutex_lock(&n->vqs[i].mutex);
- n->vqs[i].hdr_size = hdr_size;
- mutex_unlock(&n->vqs[i].mutex);
- }
vhost_net_flush(n);
mutex_unlock(&n->dev.mutex);
return 0;
diff -ruNp net-next-p0/drivers/vhost/vhost.c
net-next-p3/drivers/vhost/vhost.c
--- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
-0700
+++ net-next-p3/drivers/vhost/vhost.c 2010-03-29 20:12:42.000000000
-0700
@@ -113,7 +113,7 @@ static void vhost_vq_reset(struct vhost_
vq->used_flags = 0;
vq->log_used = false;
vq->log_addr = -1ull;
- vq->hdr_size = 0;
+ vq->rxmaxheadcount = 0;
vq->private_data = NULL;
vq->log_base = NULL;
vq->error_ctx = NULL;
@@ -410,6 +410,7 @@ static long vhost_set_vring(struct vhost
vq->last_avail_idx = s.num;
/* Forget the cached index value. */
vq->avail_idx = vq->last_avail_idx;
+ vq->rxmaxheadcount = 0;
break;
case VHOST_GET_VRING_BASE:
s.index = idx;
@@ -856,6 +857,48 @@ static unsigned get_indirect(struct vhos
return 0;
}
+/* This is a multi-head version of vhost_get_vq_desc
+ * @vq - the relevant virtqueue
+ * datalen - data length we'll be reading
+ * @iovcount - returned count of io vectors we fill
+ * @log - vhost log
+ * @log_num - log offset
+ */
+unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
*iovcount,
+ struct vhost_log *log, unsigned int *log_num)
+{
+ struct iovec *heads = vq->heads;
+ int out, in = 0;
+ int seg = 0; /* iov index */
+ int hc = 0; /* head count */
+
+ while (datalen > 0) {
+ if (hc >= VHOST_NET_MAX_SG) {
+ vhost_discard_vq_desc(vq, hc);
+ return 0;
+ }
+ heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev,
vq,
+ vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out, &in,
log,
+ log_num);
+ if (heads[hc].iov_base == (void *)vq->num) {
+ vhost_discard_vq_desc(vq, hc);
+ return 0;
+ }
+ if (out || in <= 0) {
+ vq_err(vq, "unexpected descriptor format for RX: "
+ "out %d, in %d\n", out, in);
+ vhost_discard_vq_desc(vq, hc);
+ return 0;
+ }
+ heads[hc].iov_len = iov_length(vq->iov+seg, in);
+ datalen -= heads[hc].iov_len;
+ hc++;
+ seg += in;
+ }
+ *iovcount = seg;
+ return hc;
+}
+
/* This looks in the virtqueue and for the first available buffer, and
converts
* it to an iovec for convenient access. Since descriptors consist of
some
* number of output then some number of input descriptors, it's actually
two
@@ -981,31 +1024,36 @@ unsigned vhost_get_vq_desc(struct vhost_
}
/* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
-void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
+void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n)
{
- vq->last_avail_idx--;
+ vq->last_avail_idx -= n;
}
/* After we've used one of their buffers, we tell them about it. We'll
then
* want to notify the guest, using eventfd. */
-int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
len)
+int vhost_add_used(struct vhost_virtqueue *vq, struct iovec *heads, int
count)
{
- struct vring_used_elem *used;
+ struct vring_used_elem *used = 0;
+ int i;
- /* The virtqueue contains a ring of used buffers. Get a pointer
to the
- * next entry in that used ring. */
- used = &vq->used->ring[vq->last_used_idx % vq->num];
- if (put_user(head, &used->id)) {
- vq_err(vq, "Failed to write used id");
- return -EFAULT;
- }
- if (put_user(len, &used->len)) {
- vq_err(vq, "Failed to write used len");
- return -EFAULT;
+ if (count <= 0)
+ return -EINVAL;
+
+ for (i = 0; i < count; ++i) {
+ used = &vq->used->ring[vq->last_used_idx % vq->num];
+ if (put_user((unsigned)heads[i].iov_base, &used->id)) {
+ vq_err(vq, "Failed to write used id");
+ return -EFAULT;
+ }
+ if (put_user(heads[i].iov_len, &used->len)) {
+ vq_err(vq, "Failed to write used len");
+ return -EFAULT;
+ }
+ vq->last_used_idx++;
}
/* Make sure buffer is written before we update index. */
smp_wmb();
- if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
+ if (put_user(vq->last_used_idx, &vq->used->idx)) {
vq_err(vq, "Failed to increment used idx");
return -EFAULT;
}
@@ -1023,22 +1071,35 @@ int vhost_add_used(struct vhost_virtqueu
if (vq->log_ctx)
eventfd_signal(vq->log_ctx, 1);
}
- vq->last_used_idx++;
return 0;
}
+int vhost_available(struct vhost_virtqueue *vq)
+{
+ int avail;
+
+ if (!vq->rxmaxheadcount) /* haven't got any yet */
+ return 1;
+ avail = vq->avail_idx - vq->last_avail_idx;
+ if (avail < 0)
+ avail += 0x10000; /* wrapped */
+ return avail;
+}
+
/* This actually signals the guest, using eventfd. */
-void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq, bool
recvr)
{
__u16 flags = 0;
+
if (get_user(flags, &vq->avail->flags)) {
vq_err(vq, "Failed to get flags");
return;
}
- /* If they don't want an interrupt, don't signal, unless empty. */
+ /* If they don't want an interrupt, don't signal, unless
+ * empty or receiver can't get a max-sized packet. */
if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
- (vq->avail_idx != vq->last_avail_idx ||
+ (!recvr || vhost_available(vq) >= vq->rxmaxheadcount ||
!vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
return;
@@ -1050,14 +1111,14 @@ void vhost_signal(struct vhost_dev *dev,
/* And here's the combo meal deal. Supersize me! */
void vhost_add_used_and_signal(struct vhost_dev *dev,
struct vhost_virtqueue *vq,
- unsigned int head, int len)
+ struct iovec *heads, int count, bool recvr)
{
- vhost_add_used(vq, head, len);
- vhost_signal(dev, vq);
+ vhost_add_used(vq, heads, count);
+ vhost_signal(dev, vq, recvr);
}
/* OK, now we need to know about added descriptors. */
-bool vhost_enable_notify(struct vhost_virtqueue *vq)
+bool vhost_enable_notify(struct vhost_virtqueue *vq, bool recvr)
{
u16 avail_idx;
int r;
@@ -1080,6 +1141,8 @@ bool vhost_enable_notify(struct vhost_vi
return false;
}
+ if (recvr && vq->rxmaxheadcount)
+ return (avail_idx - vq->last_avail_idx) >=
vq->rxmaxheadcount;
return avail_idx != vq->last_avail_idx;
}
diff -ruNp net-next-p0/drivers/vhost/vhost.h
net-next-p3/drivers/vhost/vhost.h
--- net-next-p0/drivers/vhost/vhost.h 2010-03-22 12:04:38.000000000
-0700
+++ net-next-p3/drivers/vhost/vhost.h 2010-03-29 20:07:17.000000000
-0700
@@ -82,9 +82,9 @@ struct vhost_virtqueue {
u64 log_addr;
struct iovec indirect[VHOST_NET_MAX_SG];
- struct iovec iov[VHOST_NET_MAX_SG];
- struct iovec hdr[VHOST_NET_MAX_SG];
- size_t hdr_size;
+ struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
+ struct iovec heads[VHOST_NET_MAX_SG];
+ int rxmaxheadcount;
/* We use a kind of RCU to access private pointer.
* All readers access it from workqueue, which makes it possible
to
* flush the workqueue instead of synchronize_rcu. Therefore
readers do
@@ -120,18 +120,20 @@ long vhost_dev_ioctl(struct vhost_dev *,
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
int vhost_log_access_ok(struct vhost_dev *);
+unsigned vhost_get_heads(struct vhost_virtqueue *, int datalen, int
*iovcount,
+ struct vhost_log *log, unsigned int *log_num);
unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
struct iovec iov[], unsigned int iov_count,
unsigned int *out_num, unsigned int *in_num,
struct vhost_log *log, unsigned int *log_num);
-void vhost_discard_vq_desc(struct vhost_virtqueue *);
+void vhost_discard_vq_desc(struct vhost_virtqueue *, int);
-int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
-void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
+int vhost_add_used(struct vhost_virtqueue *, struct iovec *heads, int
count);
void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue
*,
- unsigned int head, int len);
+ struct iovec *heads, int count, bool
recvr);
+void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *, bool);
void vhost_disable_notify(struct vhost_virtqueue *);
-bool vhost_enable_notify(struct vhost_virtqueue *);
+bool vhost_enable_notify(struct vhost_virtqueue *, bool);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
@@ -149,7 +151,8 @@ enum {
VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
(1 << VIRTIO_RING_F_INDIRECT_DESC) |
(1 << VHOST_F_LOG_ALL) |
- (1 << VHOST_NET_F_VIRTIO_NET_HDR),
+ (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
+ (1 << VIRTIO_NET_F_MRG_RXBUF),
};
static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
[-- Attachment #2: MRXB-3.patch --]
[-- Type: application/octet-stream, Size: 16655 bytes --]
diff -ruNp net-next-p0/drivers/vhost/net.c net-next-p3/drivers/vhost/net.c
--- net-next-p0/drivers/vhost/net.c 2010-03-22 12:04:38.000000000 -0700
+++ net-next-p3/drivers/vhost/net.c 2010-03-30 12:50:57.000000000 -0700
@@ -54,26 +54,6 @@ struct vhost_net {
enum vhost_net_poll_state tx_poll_state;
};
-/* Pop first len bytes from iovec. Return number of segments used. */
-static int move_iovec_hdr(struct iovec *from, struct iovec *to,
- size_t len, int iov_count)
-{
- int seg = 0;
- size_t size;
- while (len && seg < iov_count) {
- size = min(from->iov_len, len);
- to->iov_base = from->iov_base;
- to->iov_len = size;
- from->iov_len -= size;
- from->iov_base += size;
- len -= size;
- ++from;
- ++to;
- ++seg;
- }
- return seg;
-}
-
/* Caller must have TX VQ lock */
static void tx_poll_stop(struct vhost_net *net)
{
@@ -97,7 +77,8 @@ static void tx_poll_start(struct vhost_n
static void handle_tx(struct vhost_net *net)
{
struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
- unsigned head, out, in, s;
+ unsigned out, in;
+ struct iovec head;
struct msghdr msg = {
.msg_name = NULL,
.msg_namelen = 0,
@@ -108,8 +89,8 @@ static void handle_tx(struct vhost_net *
};
size_t len, total_len = 0;
int err, wmem;
- size_t hdr_size;
struct socket *sock = rcu_dereference(vq->private_data);
+
if (!sock)
return;
@@ -127,22 +108,19 @@ static void handle_tx(struct vhost_net *
if (wmem < sock->sk->sk_sndbuf / 2)
tx_poll_stop(net);
- hdr_size = vq->hdr_size;
for (;;) {
- head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in,
- NULL, NULL);
+ head.iov_base = (void *)vhost_get_vq_desc(&net->dev, vq,
+ vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL, NULL);
/* Nothing new? Wait for eventfd to tell us they refilled. */
- if (head == vq->num) {
+ if (head.iov_base == (void *)vq->num) {
wmem = atomic_read(&sock->sk->sk_wmem_alloc);
if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
tx_poll_start(net, sock);
set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
break;
}
- if (unlikely(vhost_enable_notify(vq))) {
+ if (unlikely(vhost_enable_notify(vq, 0))) {
vhost_disable_notify(vq);
continue;
}
@@ -154,27 +132,30 @@ static void handle_tx(struct vhost_net *
break;
}
/* Skip header. TODO: support TSO. */
- s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
msg.msg_iovlen = out;
- len = iov_length(vq->iov, out);
+ head.iov_len = len = iov_length(vq->iov, out);
+
/* Sanity check */
if (!len) {
- vq_err(vq, "Unexpected header len for TX: "
- "%zd expected %zd\n",
- iov_length(vq->hdr, s), hdr_size);
+ vq_err(vq, "Unexpected buffer len for TX: %zd ", len);
break;
}
- /* TODO: Check specific error and bomb out unless ENOBUFS? */
err = sock->ops->sendmsg(NULL, sock, &msg, len);
if (unlikely(err < 0)) {
- vhost_discard_vq_desc(vq);
- tx_poll_start(net, sock);
+ if (err == -EAGAIN) {
+ vhost_discard_vq_desc(vq, 1);
+ tx_poll_start(net, sock);
+ } else {
+ vq_err(vq, "sendmsg: errno %d\n", -err);
+ /* drop packet; do not discard/resend */
+ vhost_add_used_and_signal(&net->dev, vq, &head,
+ 1, 0);
+ }
break;
}
if (err != len)
- pr_err("Truncated TX packet: "
- " len %d != %zd\n", err, len);
- vhost_add_used_and_signal(&net->dev, vq, head, 0);
+ pr_err("Truncated TX packet: len %d != %zd\n", err, len);
+ vhost_add_used_and_signal(&net->dev, vq, &head, 1, 0);
total_len += len;
if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
vhost_poll_queue(&vq->poll);
@@ -186,12 +167,25 @@ static void handle_tx(struct vhost_net *
unuse_mm(net->dev.mm);
}
+static int vhost_head_len(struct sock *sk)
+{
+ struct sk_buff *head;
+ int len = 0;
+
+ lock_sock(sk);
+ head = skb_peek(&sk->sk_receive_queue);
+ if (head)
+ len = head->len;
+ release_sock(sk);
+ return len;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_rx(struct vhost_net *net)
{
struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
- unsigned head, out, in, log, s;
+ unsigned in, log;
struct vhost_log *vq_log;
struct msghdr msg = {
.msg_name = NULL,
@@ -202,34 +196,25 @@ static void handle_rx(struct vhost_net *
.msg_flags = MSG_DONTWAIT,
};
- struct virtio_net_hdr hdr = {
- .flags = 0,
- .gso_type = VIRTIO_NET_HDR_GSO_NONE
- };
-
size_t len, total_len = 0;
- int err;
- size_t hdr_size;
+ int err, headcount, datalen;
struct socket *sock = rcu_dereference(vq->private_data);
+
if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
return;
use_mm(net->dev.mm);
mutex_lock(&vq->mutex);
vhost_disable_notify(vq);
- hdr_size = vq->hdr_size;
vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
vq->log : NULL;
- for (;;) {
- head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in,
- vq_log, &log);
+ while ((datalen = vhost_head_len(sock->sk))) {
+ headcount = vhost_get_heads(vq, datalen, &in, vq_log, &log);
/* OK, now we need to know about added descriptors. */
- if (head == vq->num) {
- if (unlikely(vhost_enable_notify(vq))) {
+ if (!headcount) {
+ if (unlikely(vhost_enable_notify(vq, 1))) {
/* They have slipped one in as we were
* doing that: check again. */
vhost_disable_notify(vq);
@@ -240,46 +225,42 @@ static void handle_rx(struct vhost_net *
break;
}
/* We don't need to be notified again. */
- if (out) {
- vq_err(vq, "Unexpected descriptor format for RX: "
- "out %d, int %d\n",
- out, in);
- break;
- }
- /* Skip header. TODO: support TSO/mergeable rx buffers. */
- s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
+ if (vq->rxmaxheadcount < headcount)
+ vq->rxmaxheadcount = headcount;
+ /* Skip header. TODO: support TSO. */
msg.msg_iovlen = in;
len = iov_length(vq->iov, in);
/* Sanity check */
if (!len) {
- vq_err(vq, "Unexpected header len for RX: "
- "%zd expected %zd\n",
- iov_length(vq->hdr, s), hdr_size);
+ vq_err(vq, "Unexpected buffer len for RX: %zd\n", len);
break;
}
err = sock->ops->recvmsg(NULL, sock, &msg,
len, MSG_DONTWAIT | MSG_TRUNC);
- /* TODO: Check specific error and bomb out unless EAGAIN? */
if (err < 0) {
- vhost_discard_vq_desc(vq);
+ vhost_discard_vq_desc(vq, headcount);
break;
}
/* TODO: Should check and handle checksum. */
+ if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
+ struct virtio_net_hdr_mrg_rxbuf *vhdr =
+ (struct virtio_net_hdr_mrg_rxbuf *)
+ vq->iov[0].iov_base;
+ /* add num_bufs */
+ if (put_user(headcount, &vhdr->num_buffers)) {
+ vq_err(vq, "Failed to write num_buffers");
+ vhost_discard_vq_desc(vq, headcount);
+ break;
+ }
+ }
if (err > len) {
pr_err("Discarded truncated rx packet: "
" len %d > %zd\n", err, len);
- vhost_discard_vq_desc(vq);
+ vhost_discard_vq_desc(vq, headcount);
continue;
}
len = err;
- err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
- if (err) {
- vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
- vq->iov->iov_base, err);
- break;
- }
- len += hdr_size;
- vhost_add_used_and_signal(&net->dev, vq, head, len);
+ vhost_add_used_and_signal(&net->dev,vq,vq->heads,headcount,1);
if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, len);
total_len += len;
@@ -560,9 +541,6 @@ done:
static int vhost_net_set_features(struct vhost_net *n, u64 features)
{
- size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
- sizeof(struct virtio_net_hdr) : 0;
- int i;
mutex_lock(&n->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&n->dev)) {
@@ -571,11 +549,6 @@ static int vhost_net_set_features(struct
}
n->dev.acked_features = features;
smp_wmb();
- for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
- mutex_lock(&n->vqs[i].mutex);
- n->vqs[i].hdr_size = hdr_size;
- mutex_unlock(&n->vqs[i].mutex);
- }
vhost_net_flush(n);
mutex_unlock(&n->dev.mutex);
return 0;
diff -ruNp net-next-p0/drivers/vhost/vhost.c net-next-p3/drivers/vhost/vhost.c
--- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000 -0700
+++ net-next-p3/drivers/vhost/vhost.c 2010-03-29 20:12:42.000000000 -0700
@@ -113,7 +113,7 @@ static void vhost_vq_reset(struct vhost_
vq->used_flags = 0;
vq->log_used = false;
vq->log_addr = -1ull;
- vq->hdr_size = 0;
+ vq->rxmaxheadcount = 0;
vq->private_data = NULL;
vq->log_base = NULL;
vq->error_ctx = NULL;
@@ -410,6 +410,7 @@ static long vhost_set_vring(struct vhost
vq->last_avail_idx = s.num;
/* Forget the cached index value. */
vq->avail_idx = vq->last_avail_idx;
+ vq->rxmaxheadcount = 0;
break;
case VHOST_GET_VRING_BASE:
s.index = idx;
@@ -856,6 +857,48 @@ static unsigned get_indirect(struct vhos
return 0;
}
+/* This is a multi-head version of vhost_get_vq_desc
+ * @vq - the relevant virtqueue
+ * datalen - data length we'll be reading
+ * @iovcount - returned count of io vectors we fill
+ * @log - vhost log
+ * @log_num - log offset
+ */
+unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int *iovcount,
+ struct vhost_log *log, unsigned int *log_num)
+{
+ struct iovec *heads = vq->heads;
+ int out, in = 0;
+ int seg = 0; /* iov index */
+ int hc = 0; /* head count */
+
+ while (datalen > 0) {
+ if (hc >= VHOST_NET_MAX_SG) {
+ vhost_discard_vq_desc(vq, hc);
+ return 0;
+ }
+ heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev, vq,
+ vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out, &in, log,
+ log_num);
+ if (heads[hc].iov_base == (void *)vq->num) {
+ vhost_discard_vq_desc(vq, hc);
+ return 0;
+ }
+ if (out || in <= 0) {
+ vq_err(vq, "unexpected descriptor format for RX: "
+ "out %d, in %d\n", out, in);
+ vhost_discard_vq_desc(vq, hc);
+ return 0;
+ }
+ heads[hc].iov_len = iov_length(vq->iov+seg, in);
+ datalen -= heads[hc].iov_len;
+ hc++;
+ seg += in;
+ }
+ *iovcount = seg;
+ return hc;
+}
+
/* This looks in the virtqueue and for the first available buffer, and converts
* it to an iovec for convenient access. Since descriptors consist of some
* number of output then some number of input descriptors, it's actually two
@@ -981,31 +1024,36 @@ unsigned vhost_get_vq_desc(struct vhost_
}
/* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
-void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
+void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n)
{
- vq->last_avail_idx--;
+ vq->last_avail_idx -= n;
}
/* After we've used one of their buffers, we tell them about it. We'll then
* want to notify the guest, using eventfd. */
-int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
+int vhost_add_used(struct vhost_virtqueue *vq, struct iovec *heads, int count)
{
- struct vring_used_elem *used;
+ struct vring_used_elem *used = 0;
+ int i;
- /* The virtqueue contains a ring of used buffers. Get a pointer to the
- * next entry in that used ring. */
- used = &vq->used->ring[vq->last_used_idx % vq->num];
- if (put_user(head, &used->id)) {
- vq_err(vq, "Failed to write used id");
- return -EFAULT;
- }
- if (put_user(len, &used->len)) {
- vq_err(vq, "Failed to write used len");
- return -EFAULT;
+ if (count <= 0)
+ return -EINVAL;
+
+ for (i = 0; i < count; ++i) {
+ used = &vq->used->ring[vq->last_used_idx % vq->num];
+ if (put_user((unsigned)heads[i].iov_base, &used->id)) {
+ vq_err(vq, "Failed to write used id");
+ return -EFAULT;
+ }
+ if (put_user(heads[i].iov_len, &used->len)) {
+ vq_err(vq, "Failed to write used len");
+ return -EFAULT;
+ }
+ vq->last_used_idx++;
}
/* Make sure buffer is written before we update index. */
smp_wmb();
- if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
+ if (put_user(vq->last_used_idx, &vq->used->idx)) {
vq_err(vq, "Failed to increment used idx");
return -EFAULT;
}
@@ -1023,22 +1071,35 @@ int vhost_add_used(struct vhost_virtqueu
if (vq->log_ctx)
eventfd_signal(vq->log_ctx, 1);
}
- vq->last_used_idx++;
return 0;
}
+int vhost_available(struct vhost_virtqueue *vq)
+{
+ int avail;
+
+ if (!vq->rxmaxheadcount) /* haven't got any yet */
+ return 1;
+ avail = vq->avail_idx - vq->last_avail_idx;
+ if (avail < 0)
+ avail += 0x10000; /* wrapped */
+ return avail;
+}
+
/* This actually signals the guest, using eventfd. */
-void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq, bool recvr)
{
__u16 flags = 0;
+
if (get_user(flags, &vq->avail->flags)) {
vq_err(vq, "Failed to get flags");
return;
}
- /* If they don't want an interrupt, don't signal, unless empty. */
+ /* If they don't want an interrupt, don't signal, unless
+ * empty or receiver can't get a max-sized packet. */
if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
- (vq->avail_idx != vq->last_avail_idx ||
+ (!recvr || vhost_available(vq) >= vq->rxmaxheadcount ||
!vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
return;
@@ -1050,14 +1111,14 @@ void vhost_signal(struct vhost_dev *dev,
/* And here's the combo meal deal. Supersize me! */
void vhost_add_used_and_signal(struct vhost_dev *dev,
struct vhost_virtqueue *vq,
- unsigned int head, int len)
+ struct iovec *heads, int count, bool recvr)
{
- vhost_add_used(vq, head, len);
- vhost_signal(dev, vq);
+ vhost_add_used(vq, heads, count);
+ vhost_signal(dev, vq, recvr);
}
/* OK, now we need to know about added descriptors. */
-bool vhost_enable_notify(struct vhost_virtqueue *vq)
+bool vhost_enable_notify(struct vhost_virtqueue *vq, bool recvr)
{
u16 avail_idx;
int r;
@@ -1080,6 +1141,8 @@ bool vhost_enable_notify(struct vhost_vi
return false;
}
+ if (recvr && vq->rxmaxheadcount)
+ return (avail_idx - vq->last_avail_idx) >= vq->rxmaxheadcount;
return avail_idx != vq->last_avail_idx;
}
diff -ruNp net-next-p0/drivers/vhost/vhost.h net-next-p3/drivers/vhost/vhost.h
--- net-next-p0/drivers/vhost/vhost.h 2010-03-22 12:04:38.000000000 -0700
+++ net-next-p3/drivers/vhost/vhost.h 2010-03-29 20:07:17.000000000 -0700
@@ -82,9 +82,9 @@ struct vhost_virtqueue {
u64 log_addr;
struct iovec indirect[VHOST_NET_MAX_SG];
- struct iovec iov[VHOST_NET_MAX_SG];
- struct iovec hdr[VHOST_NET_MAX_SG];
- size_t hdr_size;
+ struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
+ struct iovec heads[VHOST_NET_MAX_SG];
+ int rxmaxheadcount;
/* We use a kind of RCU to access private pointer.
* All readers access it from workqueue, which makes it possible to
* flush the workqueue instead of synchronize_rcu. Therefore readers do
@@ -120,18 +120,20 @@ long vhost_dev_ioctl(struct vhost_dev *,
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
int vhost_log_access_ok(struct vhost_dev *);
+unsigned vhost_get_heads(struct vhost_virtqueue *, int datalen, int *iovcount,
+ struct vhost_log *log, unsigned int *log_num);
unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
struct iovec iov[], unsigned int iov_count,
unsigned int *out_num, unsigned int *in_num,
struct vhost_log *log, unsigned int *log_num);
-void vhost_discard_vq_desc(struct vhost_virtqueue *);
+void vhost_discard_vq_desc(struct vhost_virtqueue *, int);
-int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
-void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
+int vhost_add_used(struct vhost_virtqueue *, struct iovec *heads, int count);
void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue *,
- unsigned int head, int len);
+ struct iovec *heads, int count, bool recvr);
+void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *, bool);
void vhost_disable_notify(struct vhost_virtqueue *);
-bool vhost_enable_notify(struct vhost_virtqueue *);
+bool vhost_enable_notify(struct vhost_virtqueue *, bool);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
@@ -149,7 +151,8 @@ enum {
VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
(1 << VIRTIO_RING_F_INDIRECT_DESC) |
(1 << VHOST_F_LOG_ALL) |
- (1 << VHOST_NET_F_VIRTIO_NET_HDR),
+ (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
+ (1 << VIRTIO_NET_F_MRG_RXBUF),
};
static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
^ permalink raw reply
* Re: [PATCH][QEMU][VHOST]fix feature bit handling for mergeable rx buffers
From: Michael S. Tsirkin @ 2010-03-31 10:12 UTC (permalink / raw)
To: David L Stevens; +Cc: kvm, virtualization
In-Reply-To: <201003301921.o2UJL5EH003473@lab1.dls>
On Tue, Mar 30, 2010 at 12:21:05PM -0700, David L Stevens wrote:
>
> This patch adds mergeable receive buffer support to qemu-kvm,
> to allow enabling it when vhost_net supports it.
>
> It also adds a missing call to vhost_net_ack_features() to
> push acked features to vhost_net.
>
> The patch is relative to Michael Tsirkin's qemu-kvm git tree.
>
> +-DLS
>
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
Patch is well-formed, thanks!
BTW, where qemu-km and qemu share code, it is better
to send patch against qemu.
Since qemu-kvm generally carries kernel headers it needs, it might also
be a good idea to add if_tun to the kvm header directory.
> diff -ruNp qemu-kvm.mst/hw/vhost_net.c qemu-kvm.dls/hw/vhost_net.c
> --- qemu-kvm.mst/hw/vhost_net.c 2010-03-03 13:39:07.000000000 -0800
> +++ qemu-kvm.dls/hw/vhost_net.c 2010-03-29 20:37:34.000000000 -0700
> @@ -5,6 +5,7 @@
> #include <sys/ioctl.h>
> #include <linux/vhost.h>
> #include <linux/virtio_ring.h>
> +#include <linux/if_tun.h>
> #include <netpacket/packet.h>
> #include <net/ethernet.h>
> #include <net/if.h>
> @@ -38,15 +39,6 @@ unsigned vhost_net_get_features(struct v
> return features;
> }
>
> -void vhost_net_ack_features(struct vhost_net *net, unsigned features)
> -{
> - net->dev.acked_features = net->dev.backend_features;
> - if (features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY))
> - net->dev.acked_features |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
> - if (features & (1 << VIRTIO_RING_F_INDIRECT_DESC))
> - net->dev.acked_features |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
> -}
> -
> static int vhost_net_get_fd(VLANClientState *backend)
> {
> switch (backend->info->type) {
> @@ -58,6 +50,25 @@ static int vhost_net_get_fd(VLANClientSt
> }
> }
>
> +void vhost_net_ack_features(struct vhost_net *net, unsigned features)
> +{
> + int vnet_hdr_sz = sizeof(struct virtio_net_hdr);
> +
> + net->dev.acked_features = net->dev.backend_features;
> + if (features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY))
> + net->dev.acked_features |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
> + if (features & (1 << VIRTIO_RING_F_INDIRECT_DESC))
> + net->dev.acked_features |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
> + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> + net->dev.acked_features |= (1 << VIRTIO_NET_F_MRG_RXBUF);
> + vnet_hdr_sz = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> + }
> +#ifdef TUNSETVNETHDRSZ
> + if (ioctl(vhost_net_get_fd(net->vc), TUNSETVNETHDRSZ, &vnet_hdr_sz) < 0)
> + perror("TUNSETVNETHDRSZ");
> +#endif /* TUNSETVNETHDRSZ */
Note that this will break injecting raw packets after migration
(that code assumes header size is sizeof(struct virtio_net_hdr)).
Need to fix it.
So I think we are better off adding tap_set_vnet_header_size.
That function would:
- Remember current value, return success and do nothing
if value is not changed.
- Try to change and return error if not.
- To get doubly sure, we can call TUNSETVNETHDRSZ with
sizeof(struct virtio_net_hdr) at startup. If we get success
once we should further on as well, otherwise we can exit.
An additional benefit is that tap driver will stay
localized in one file.
> +}
> +
> struct vhost_net *vhost_net_init(VLANClientState *backend, int devfd)
> {
> int r;
> diff -ruNp qemu-kvm.mst/hw/virtio-net.c qemu-kvm.dls/hw/virtio-net.c
> --- qemu-kvm.mst/hw/virtio-net.c 2010-03-03 13:39:07.000000000 -0800
> +++ qemu-kvm.dls/hw/virtio-net.c 2010-03-29 16:15:46.000000000 -0700
> @@ -211,12 +211,16 @@ static void virtio_net_set_features(Virt
> n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
>
> if (n->has_vnet_hdr) {
> + struct vhost_net *vhost_net = tap_get_vhost_net(n->nic->nc.peer);
> +
> tap_set_offload(n->nic->nc.peer,
> (features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
> (features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
> (features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
> (features >> VIRTIO_NET_F_GUEST_ECN) & 1,
> (features >> VIRTIO_NET_F_GUEST_UFO) & 1);
> + if (vhost_net)
> + vhost_net_ack_features(vhost_net, features);
Good catch, thanks! I'll apply this separately.
> }
> }
>
^ permalink raw reply
* Re: [PATCH v2] Add Mergeable RX buffer feature to vhost_net
From: Michael S. Tsirkin @ 2010-03-31 12:02 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <OF35D5EAC9.45A7DC5F-ON882576F7.0006F45E-882576F7.0007AB62@us.ibm.com>
On Tue, Mar 30, 2010 at 07:23:48PM -0600, David Stevens wrote:
> This patch adds support for the Mergeable Receive Buffers feature to
> vhost_net.
>
> Changes:
> 1) generalize descriptor allocation functions to allow multiple
> descriptors per packet
> 2) add socket "peek" to know datalen at buffer allocation time
> 3) change notification to enable a multi-buffer max packet, rather
> than the single-buffer run until completely empty
>
> Changes from previous revision:
> 1) incorporate review comments from Michael Tsirkin
> 2) assume use of TUNSETVNETHDRSZ ioctl by qemu, which simplifies vnet
> header
> processing
> 3) fixed notification code to only affect the receive side
>
> Signed-Off-By: David L Stevens <dlstevens@us.ibm.com>
>
> [in-line for review, attached for applying w/o whitespace mangling]
attached patch seems to be whiespace damaged as well.
Does the origin pass checkpatch.pl for you?
> diff -ruNp net-next-p0/drivers/vhost/net.c net-next-p3/drivers/vhost/net.c
> --- net-next-p0/drivers/vhost/net.c 2010-03-22 12:04:38.000000000
> -0700
> +++ net-next-p3/drivers/vhost/net.c 2010-03-30 12:50:57.000000000
> -0700
> @@ -54,26 +54,6 @@ struct vhost_net {
> enum vhost_net_poll_state tx_poll_state;
> };
>
> -/* Pop first len bytes from iovec. Return number of segments used. */
> -static int move_iovec_hdr(struct iovec *from, struct iovec *to,
> - size_t len, int iov_count)
> -{
> - int seg = 0;
> - size_t size;
> - while (len && seg < iov_count) {
> - size = min(from->iov_len, len);
> - to->iov_base = from->iov_base;
> - to->iov_len = size;
> - from->iov_len -= size;
> - from->iov_base += size;
> - len -= size;
> - ++from;
> - ++to;
> - ++seg;
> - }
> - return seg;
> -}
> -
> /* Caller must have TX VQ lock */
> static void tx_poll_stop(struct vhost_net *net)
> {
> @@ -97,7 +77,8 @@ static void tx_poll_start(struct vhost_n
> static void handle_tx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> - unsigned head, out, in, s;
> + unsigned out, in;
> + struct iovec head;
> struct msghdr msg = {
> .msg_name = NULL,
> .msg_namelen = 0,
> @@ -108,8 +89,8 @@ static void handle_tx(struct vhost_net *
> };
> size_t len, total_len = 0;
> int err, wmem;
> - size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
> +
> if (!sock)
> return;
>
> @@ -127,22 +108,19 @@ static void handle_tx(struct vhost_net *
>
> if (wmem < sock->sk->sk_sndbuf / 2)
> tx_poll_stop(net);
> - hdr_size = vq->hdr_size;
>
> for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> - NULL, NULL);
> + head.iov_base = (void *)vhost_get_vq_desc(&net->dev, vq,
> + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
> NULL);
I this casting confusing.
Is it really expensive to add an array of heads so that
we do not need to cast?
> /* Nothing new? Wait for eventfd to tell us they
> refilled. */
> - if (head == vq->num) {
> + if (head.iov_base == (void *)vq->num) {
> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
> tx_poll_start(net, sock);
> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> break;
> }
> - if (unlikely(vhost_enable_notify(vq))) {
> + if (unlikely(vhost_enable_notify(vq, 0))) {
> vhost_disable_notify(vq);
> continue;
> }
> @@ -154,27 +132,30 @@ static void handle_tx(struct vhost_net *
> break;
> }
> /* Skip header. TODO: support TSO. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
> msg.msg_iovlen = out;
> - len = iov_length(vq->iov, out);
> + head.iov_len = len = iov_length(vq->iov, out);
> +
> /* Sanity check */
> if (!len) {
> - vq_err(vq, "Unexpected header len for TX: "
> - "%zd expected %zd\n",
> - iov_length(vq->hdr, s), hdr_size);
> + vq_err(vq, "Unexpected buffer len for TX: %zd ",
> len);
> break;
> }
> - /* TODO: Check specific error and bomb out unless ENOBUFS?
> */
> err = sock->ops->sendmsg(NULL, sock, &msg, len);
> if (unlikely(err < 0)) {
> - vhost_discard_vq_desc(vq);
> - tx_poll_start(net, sock);
> + if (err == -EAGAIN) {
> + vhost_discard_vq_desc(vq, 1);
> + tx_poll_start(net, sock);
> + } else {
> + vq_err(vq, "sendmsg: errno %d\n", -err);
> + /* drop packet; do not discard/resend */
> + vhost_add_used_and_signal(&net->dev, vq,
> &head,
> + 1, 0);
> + }
> break;
> }
> if (err != len)
> - pr_err("Truncated TX packet: "
> - " len %d != %zd\n", err, len);
> - vhost_add_used_and_signal(&net->dev, vq, head, 0);
> + pr_err("Truncated TX packet: len %d != %zd\n",
> err, len);
> + vhost_add_used_and_signal(&net->dev, vq, &head, 1, 0);
> total_len += len;
> if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> vhost_poll_queue(&vq->poll);
> @@ -186,12 +167,25 @@ static void handle_tx(struct vhost_net *
> unuse_mm(net->dev.mm);
> }
>
> +static int vhost_head_len(struct sock *sk)
> +{
> + struct sk_buff *head;
> + int len = 0;
> +
> + lock_sock(sk);
> + head = skb_peek(&sk->sk_receive_queue);
> + if (head)
> + len = head->len;
> + release_sock(sk);
> + return len;
> +}
> +
> /* Expects to be always run from workqueue - which acts as
> * read-size critical section for our kind of RCU. */
> static void handle_rx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> - unsigned head, out, in, log, s;
> + unsigned in, log;
> struct vhost_log *vq_log;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -202,34 +196,25 @@ static void handle_rx(struct vhost_net *
> .msg_flags = MSG_DONTWAIT,
> };
>
> - struct virtio_net_hdr hdr = {
> - .flags = 0,
> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> - };
> -
> size_t len, total_len = 0;
> - int err;
> - size_t hdr_size;
> + int err, headcount, datalen;
> struct socket *sock = rcu_dereference(vq->private_data);
> +
> if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> return;
>
> use_mm(net->dev.mm);
> mutex_lock(&vq->mutex);
> vhost_disable_notify(vq);
> - hdr_size = vq->hdr_size;
>
> vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
>
> - for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> - vq_log, &log);
> + while ((datalen = vhost_head_len(sock->sk))) {
> + headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> &log);
> /* OK, now we need to know about added descriptors. */
> - if (head == vq->num) {
> - if (unlikely(vhost_enable_notify(vq))) {
> + if (!headcount) {
> + if (unlikely(vhost_enable_notify(vq, 1))) {
> /* They have slipped one in as we were
> * doing that: check again. */
> vhost_disable_notify(vq);
> @@ -240,46 +225,42 @@ static void handle_rx(struct vhost_net *
> break;
> }
> /* We don't need to be notified again. */
> - if (out) {
> - vq_err(vq, "Unexpected descriptor format for RX: "
> - "out %d, int %d\n",
> - out, in);
> - break;
> - }
> - /* Skip header. TODO: support TSO/mergeable rx buffers. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> + if (vq->rxmaxheadcount < headcount)
> + vq->rxmaxheadcount = headcount;
This seems the only place where we set the rxmaxheadcount
value. Maybe it can be moved out of vhost.c to net.c?
If vhost library needs this it can get it as function
parameter.
> + /* Skip header. TODO: support TSO. */
You seem to have removed the code that skips the header.
Won't this break non-GSO backends such as raw?
> msg.msg_iovlen = in;
> len = iov_length(vq->iov, in);
> /* Sanity check */
> if (!len) {
> - vq_err(vq, "Unexpected header len for RX: "
> - "%zd expected %zd\n",
> - iov_length(vq->hdr, s), hdr_size);
> + vq_err(vq, "Unexpected buffer len for RX: %zd\n",
> len);
> break;
> }
> err = sock->ops->recvmsg(NULL, sock, &msg,
> len, MSG_DONTWAIT | MSG_TRUNC);
> - /* TODO: Check specific error and bomb out unless EAGAIN?
> */
Do you think it's not a good idea?
> if (err < 0) {
> - vhost_discard_vq_desc(vq);
> + vhost_discard_vq_desc(vq, headcount);
> break;
> }
I think we should detect and discard truncated messages,
since len might not be reliable if userspace pulls
a packet from under us. Also, if new packet is
shorter than the old one, there's no truncation but
headcount is wrong.
So simplest fix IMO would be to compare err with expected len.
If there's a difference, we hit the race and so
we would discard the packet.
> /* TODO: Should check and handle checksum. */
> + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> {
> + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> + (struct virtio_net_hdr_mrg_rxbuf *)
> + vq->iov[0].iov_base;
> + /* add num_bufs */
> + if (put_user(headcount, &vhdr->num_buffers)) {
> + vq_err(vq, "Failed to write num_buffers");
> + vhost_discard_vq_desc(vq, headcount);
Let's do memcpy_to_iovecend etc so that we do not assume layout.
This is also why we need move_iovec: sendmsg might modify the iovec.
It would also be nice not to corrupt memory and
get a reasonable error if buffer size
that we get is smaller than expected header size.
> + break;
> + }
> + }
> if (err > len) {
> pr_err("Discarded truncated rx packet: "
> " len %d > %zd\n", err, len);
> - vhost_discard_vq_desc(vq);
> + vhost_discard_vq_desc(vq, headcount);
> continue;
> }
> len = err;
> - err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr,
> hdr_size);
> - if (err) {
> - vq_err(vq, "Unable to write vnet_hdr at addr %p:
> %d\n",
> - vq->iov->iov_base, err);
> - break;
> - }
> - len += hdr_size;
This seems to break non-GSO backends as well.
> - vhost_add_used_and_signal(&net->dev, vq, head, len);
> + vhost_add_used_and_signal(&net->dev,vq,vq->heads,headcount,1);
> if (unlikely(vq_log))
> vhost_log_write(vq, vq_log, log, len);
> total_len += len;
> @@ -560,9 +541,6 @@ done:
>
> static int vhost_net_set_features(struct vhost_net *n, u64 features)
> {
> - size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> - sizeof(struct virtio_net_hdr) : 0;
> - int i;
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev)) {
> @@ -571,11 +549,6 @@ static int vhost_net_set_features(struct
> }
> n->dev.acked_features = features;
> smp_wmb();
> - for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> - mutex_lock(&n->vqs[i].mutex);
> - n->vqs[i].hdr_size = hdr_size;
> - mutex_unlock(&n->vqs[i].mutex);
I expect the above is a leftover from the previous version
which calculated header size in kernel?
> - }
> vhost_net_flush(n);
> mutex_unlock(&n->dev.mutex);
> return 0;
> diff -ruNp net-next-p0/drivers/vhost/vhost.c
> net-next-p3/drivers/vhost/vhost.c
> --- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
> -0700
> +++ net-next-p3/drivers/vhost/vhost.c 2010-03-29 20:12:42.000000000
> -0700
> @@ -113,7 +113,7 @@ static void vhost_vq_reset(struct vhost_
> vq->used_flags = 0;
> vq->log_used = false;
> vq->log_addr = -1ull;
> - vq->hdr_size = 0;
> + vq->rxmaxheadcount = 0;
> vq->private_data = NULL;
> vq->log_base = NULL;
> vq->error_ctx = NULL;
> @@ -410,6 +410,7 @@ static long vhost_set_vring(struct vhost
> vq->last_avail_idx = s.num;
> /* Forget the cached index value. */
> vq->avail_idx = vq->last_avail_idx;
> + vq->rxmaxheadcount = 0;
> break;
> case VHOST_GET_VRING_BASE:
> s.index = idx;
> @@ -856,6 +857,48 @@ static unsigned get_indirect(struct vhos
> return 0;
> }
>
> +/* This is a multi-head version of vhost_get_vq_desc
How about:
version of vhost_get_vq_desc that returns multiple
descriptors
> + * @vq - the relevant virtqueue
> + * datalen - data length we'll be reading
> + * @iovcount - returned count of io vectors we fill
> + * @log - vhost log
> + * @log_num - log offset
return value?
Also - why unsigned?
> + */
> +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
Would vhost_get_vq_desc_multiple be a better name?
> *iovcount,
> + struct vhost_log *log, unsigned int *log_num)
> +{
> + struct iovec *heads = vq->heads;
I think it's better to pass in heads array than take it from vq->heads.
> + int out, in = 0;
Why is in initialized here?
> + int seg = 0; /* iov index */
> + int hc = 0; /* head count */
> +
> + while (datalen > 0) {
Can't this simply call vhost_get_vq_desc in a loop somehow,
or use a common function that both this and vhost_get_vq_desc call?
> + if (hc >= VHOST_NET_MAX_SG) {
> + vhost_discard_vq_desc(vq, hc);
> + return 0;
> + }
> + heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev,
> vq,
> + vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out, &in,
> log,
> + log_num);
> + if (heads[hc].iov_base == (void *)vq->num) {
> + vhost_discard_vq_desc(vq, hc);
> + return 0;
> + }
> + if (out || in <= 0) {
> + vq_err(vq, "unexpected descriptor format for RX: "
> + "out %d, in %d\n", out, in);
> + vhost_discard_vq_desc(vq, hc);
goto err above might help simplify cleanup.
> + return 0;
> + }
> + heads[hc].iov_len = iov_length(vq->iov+seg, in);
> + datalen -= heads[hc].iov_len;
> + hc++;
> + seg += in;
> + }
> + *iovcount = seg;
> + return hc;
> +}
> +
> /* This looks in the virtqueue and for the first available buffer, and
> converts
> * it to an iovec for convenient access. Since descriptors consist of
> some
> * number of output then some number of input descriptors, it's actually
> two
> @@ -981,31 +1024,36 @@ unsigned vhost_get_vq_desc(struct vhost_
> }
>
> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
> -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> +void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n)
> {
> - vq->last_avail_idx--;
> + vq->last_avail_idx -= n;
> }
>
> /* After we've used one of their buffers, we tell them about it. We'll
> then
> * want to notify the guest, using eventfd. */
> -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> len)
> +int vhost_add_used(struct vhost_virtqueue *vq, struct iovec *heads, int
> count)
count is always 0 for send, right?
I think it is better to have two APIs here as well:
vhost_add_used
vhost_add_used_multiple
we can use functions to avoid code duplication.
> {
> - struct vring_used_elem *used;
> + struct vring_used_elem *used = 0;
why is used initialized here?
> + int i;
>
> - /* The virtqueue contains a ring of used buffers. Get a pointer
> to the
> - * next entry in that used ring. */
> - used = &vq->used->ring[vq->last_used_idx % vq->num];
> - if (put_user(head, &used->id)) {
> - vq_err(vq, "Failed to write used id");
> - return -EFAULT;
> - }
> - if (put_user(len, &used->len)) {
> - vq_err(vq, "Failed to write used len");
> - return -EFAULT;
> + if (count <= 0)
> + return -EINVAL;
> +
> + for (i = 0; i < count; ++i) {
> + used = &vq->used->ring[vq->last_used_idx % vq->num];
> + if (put_user((unsigned)heads[i].iov_base, &used->id)) {
> + vq_err(vq, "Failed to write used id");
> + return -EFAULT;
> + }
> + if (put_user(heads[i].iov_len, &used->len)) {
> + vq_err(vq, "Failed to write used len");
> + return -EFAULT;
> + }
> + vq->last_used_idx++;
I think we should update last_used_idx on success only,
at the end. Simply use last_used_idx + count
instead of last_used_idx + 1.
> }
> /* Make sure buffer is written before we update index. */
> smp_wmb();
> - if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
> + if (put_user(vq->last_used_idx, &vq->used->idx)) {
> vq_err(vq, "Failed to increment used idx");
> return -EFAULT;
> }
> @@ -1023,22 +1071,35 @@ int vhost_add_used(struct vhost_virtqueu
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> - vq->last_used_idx++;
> return 0;
> }
>
> +int vhost_available(struct vhost_virtqueue *vq)
since this function is non-static, please document what it does.
> +{
> + int avail;
> +
> + if (!vq->rxmaxheadcount) /* haven't got any yet */
> + return 1;
since seems to make net - specific asumptions.
How about moving this check out to net.c?
> + avail = vq->avail_idx - vq->last_avail_idx;
> + if (avail < 0)
> + avail += 0x10000; /* wrapped */
A branch that is likely non-taken. Also, rxmaxheadcount
is really unlikely to get as large as half the ring.
So this just wastes cycles?
> + return avail;
> +}
> +
> /* This actually signals the guest, using eventfd. */
> -void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> +void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq, bool
> recvr)
This one is not elegant. receive is net.c concept, let's not
put it in vhost.c. Pass in headcount if you must.
> {
> __u16 flags = 0;
> +
> if (get_user(flags, &vq->avail->flags)) {
> vq_err(vq, "Failed to get flags");
> return;
> }
>
> - /* If they don't want an interrupt, don't signal, unless empty. */
> + /* If they don't want an interrupt, don't signal, unless
> + * empty or receiver can't get a max-sized packet. */
> if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> - (vq->avail_idx != vq->last_avail_idx ||
> + (!recvr || vhost_available(vq) >= vq->rxmaxheadcount ||
Is the above really worth the complexity?
Guests can't rely on this kind of fuzzy logic, can they?
Do you see this helping performance at all?
> !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
> return;
>
> @@ -1050,14 +1111,14 @@ void vhost_signal(struct vhost_dev *dev,
> /* And here's the combo meal deal. Supersize me! */
> void vhost_add_used_and_signal(struct vhost_dev *dev,
> struct vhost_virtqueue *vq,
> - unsigned int head, int len)
> + struct iovec *heads, int count, bool recvr)
> {
> - vhost_add_used(vq, head, len);
> - vhost_signal(dev, vq);
> + vhost_add_used(vq, heads, count);
> + vhost_signal(dev, vq, recvr);
> }
>
> /* OK, now we need to know about added descriptors. */
> -bool vhost_enable_notify(struct vhost_virtqueue *vq)
> +bool vhost_enable_notify(struct vhost_virtqueue *vq, bool recvr)
> {
> u16 avail_idx;
> int r;
> @@ -1080,6 +1141,8 @@ bool vhost_enable_notify(struct vhost_vi
> return false;
> }
>
> + if (recvr && vq->rxmaxheadcount)
> + return (avail_idx - vq->last_avail_idx) >=
> vq->rxmaxheadcount;
The fuzzy logic behind rxmaxheadcount might be a good
optimization, but I am not comfortable using it
for correctness. Maybe vhost_enable_notify should get
the last head so we can redo poll when another one
is added.
> return avail_idx != vq->last_avail_idx;
> }
>
> diff -ruNp net-next-p0/drivers/vhost/vhost.h
> net-next-p3/drivers/vhost/vhost.h
> --- net-next-p0/drivers/vhost/vhost.h 2010-03-22 12:04:38.000000000
> -0700
> +++ net-next-p3/drivers/vhost/vhost.h 2010-03-29 20:07:17.000000000
> -0700
> @@ -82,9 +82,9 @@ struct vhost_virtqueue {
> u64 log_addr;
>
> struct iovec indirect[VHOST_NET_MAX_SG];
> - struct iovec iov[VHOST_NET_MAX_SG];
> - struct iovec hdr[VHOST_NET_MAX_SG];
> - size_t hdr_size;
> + struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
VHOST_NET_MAX_SG should already include iovec vnet hdr.
> + struct iovec heads[VHOST_NET_MAX_SG];
> + int rxmaxheadcount;
> /* We use a kind of RCU to access private pointer.
> * All readers access it from workqueue, which makes it possible
> to
> * flush the workqueue instead of synchronize_rcu. Therefore
> readers do
> @@ -120,18 +120,20 @@ long vhost_dev_ioctl(struct vhost_dev *,
> int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> int vhost_log_access_ok(struct vhost_dev *);
>
> +unsigned vhost_get_heads(struct vhost_virtqueue *, int datalen, int
> *iovcount,
> + struct vhost_log *log, unsigned int *log_num);
> unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
> struct iovec iov[], unsigned int iov_count,
> unsigned int *out_num, unsigned int *in_num,
> struct vhost_log *log, unsigned int *log_num);
> -void vhost_discard_vq_desc(struct vhost_virtqueue *);
> +void vhost_discard_vq_desc(struct vhost_virtqueue *, int);
>
> -int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
> -void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> +int vhost_add_used(struct vhost_virtqueue *, struct iovec *heads, int
> count);
> void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue
> *,
> - unsigned int head, int len);
> + struct iovec *heads, int count, bool
> recvr);
> +void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *, bool);
> void vhost_disable_notify(struct vhost_virtqueue *);
> -bool vhost_enable_notify(struct vhost_virtqueue *);
> +bool vhost_enable_notify(struct vhost_virtqueue *, bool);
>
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> unsigned int log_num, u64 len);
> @@ -149,7 +151,8 @@ enum {
> VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
> (1 << VIRTIO_RING_F_INDIRECT_DESC) |
> (1 << VHOST_F_LOG_ALL) |
> - (1 << VHOST_NET_F_VIRTIO_NET_HDR),
> + (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
> + (1 << VIRTIO_NET_F_MRG_RXBUF),
> };
>
> static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
>
^ permalink raw reply
* Re: [PATCH 4/4] virtio: disable multiport console support.
From: Michael S. Tsirkin @ 2010-03-31 18:35 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, virtualization
In-Reply-To: <201003301549.43186.rusty@rustcorp.com.au>
On Tue, Mar 30, 2010 at 03:49:43PM +1030, Rusty Russell wrote:
> Unfortunately there proved to be at least one bug which requires an
> ABI change, so we're best off not introducing multiport support until
> 2.6.35.
>
> While I generally left the multiport code paths intact, I really wanted
> to remove the ABI defines from the header, which meant some quite deep cuts.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> To: Amit Shah <amit.shah@redhat.com>
> ---
> diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> --- a/include/linux/virtio_console.h
> +++ b/include/linux/virtio_console.h
> @@ -12,7 +12,6 @@
>
> /* Feature bits */
> #define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
> -#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
>
> struct virtio_console_config {
> /* colums of the screens */
> @@ -25,24 +24,6 @@ struct virtio_console_config {
> __u32 nr_ports;
> } __attribute__((packed));
We also want to remove nr_ports, correct?
^ permalink raw reply
* Re: [PATCH 4/4] virtio: disable multiport console support.
From: Michael S. Tsirkin @ 2010-03-31 18:56 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, virtualization
In-Reply-To: <201003301549.43186.rusty@rustcorp.com.au>
Here's a much smaller patch that also removes multiport
from user visible headers completely.
What do you think? Compile-tested only.
->8
Move MULTIPORT feature and related config changes
out of exported headers, and disable the feature
at runtime.
At this point, it seems less risky to keep code around
until we can enable it than rip it out completely.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/char/virtio_console.c | 47 ++++++++++++++++++++++++++++++++++-------
include/linux/virtio_console.h | 23 --------------------
2 files changed, 39 insertions(+), 31 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 44288ce..2a360de 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -32,6 +32,35 @@
#include <linux/workqueue.h>
#include "hvc_console.h"
+/* Moved here from .h file in order to disable MULTIPORT. */
+#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
+
+struct virtio_console_multiport_conf {
+ struct virtio_console_config config;
+ /* max. number of ports this device can hold */
+ __u32 max_nr_ports;
+ /* number of ports added so far */
+ __u32 nr_ports;
+} __attribute__((packed));
+
+/*
+ * A message that's passed between the Host and the Guest for a
+ * particular port.
+ */
+struct virtio_console_control {
+ __u32 id; /* Port number */
+ __u16 event; /* The kind of control event (see below) */
+ __u16 value; /* Extra information for the key */
+};
+
+/* Some events for control messages */
+#define VIRTIO_CONSOLE_PORT_READY 0
+#define VIRTIO_CONSOLE_CONSOLE_PORT 1
+#define VIRTIO_CONSOLE_RESIZE 2
+#define VIRTIO_CONSOLE_PORT_OPEN 3
+#define VIRTIO_CONSOLE_PORT_NAME 4
+#define VIRTIO_CONSOLE_PORT_REMOVE 5
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -120,7 +149,7 @@ struct ports_device {
spinlock_t cvq_lock;
/* The current config space is stored here */
- struct virtio_console_config config;
+ struct virtio_console_multiport_conf config;
/* The virtio device we're associated with */
struct virtio_device *vdev;
@@ -1217,7 +1246,7 @@ fail:
*/
static void config_work_handler(struct work_struct *work)
{
- struct virtio_console_config virtconconf;
+ struct virtio_console_multiport_conf virtconconf;
struct ports_device *portdev;
struct virtio_device *vdev;
int err;
@@ -1226,7 +1255,8 @@ static void config_work_handler(struct work_struct *work)
vdev = portdev->vdev;
vdev->config->get(vdev,
- offsetof(struct virtio_console_config, nr_ports),
+ offsetof(struct virtio_console_multiport_conf,
+ nr_ports),
&virtconconf.nr_ports,
sizeof(virtconconf.nr_ports));
@@ -1422,12 +1452,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
multiport = true;
vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
- nr_ports),
+ vdev->config->get(vdev,
+ offsetof(struct virtio_console_multiport_conf,
+ nr_ports),
&portdev->config.nr_ports,
sizeof(portdev->config.nr_ports));
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
- max_nr_ports),
+ vdev->config->get(vdev,
+ offsetof(struct virtio_console_multiport_conf,
+ max_nr_ports),
&portdev->config.max_nr_ports,
sizeof(portdev->config.max_nr_ports));
if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
@@ -1525,7 +1557,6 @@ static struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
- VIRTIO_CONSOLE_F_MULTIPORT,
};
static struct virtio_driver virtio_console = {
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ae4f039..92228a8 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -12,37 +12,14 @@
/* Feature bits */
#define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
-#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
struct virtio_console_config {
/* colums of the screens */
__u16 cols;
/* rows of the screens */
__u16 rows;
- /* max. number of ports this device can hold */
- __u32 max_nr_ports;
- /* number of ports added so far */
- __u32 nr_ports;
} __attribute__((packed));
-/*
- * A message that's passed between the Host and the Guest for a
- * particular port.
- */
-struct virtio_console_control {
- __u32 id; /* Port number */
- __u16 event; /* The kind of control event (see below) */
- __u16 value; /* Extra information for the key */
-};
-
-/* Some events for control messages */
-#define VIRTIO_CONSOLE_PORT_READY 0
-#define VIRTIO_CONSOLE_CONSOLE_PORT 1
-#define VIRTIO_CONSOLE_RESIZE 2
-#define VIRTIO_CONSOLE_PORT_OPEN 3
-#define VIRTIO_CONSOLE_PORT_NAME 4
-#define VIRTIO_CONSOLE_PORT_REMOVE 5
-
#ifdef __KERNEL__
int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
#endif /* __KERNEL__ */
^ permalink raw reply related
* Re: [PATCH v2] Add Mergeable RX buffer feature to vhost_net
From: David Stevens @ 2010-03-31 22:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <20100331120228.GH31085@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/31/2010 05:02:28 AM:
>
> attached patch seems to be whiespace damaged as well.
> Does the origin pass checkpatch.pl for you?
Yes, but I probably broke it in the transfer -- will be more
careful with the next revision.
> > + head.iov_base = (void *)vhost_get_vq_desc(&net->dev,
vq,
> > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
> > NULL);
>
> I this casting confusing.
> Is it really expensive to add an array of heads so that
> we do not need to cast?
It needs the heads and the lengths, which looks a lot
like an iovec. I was trying to resist adding a new
struct XXX { unsigned head; unsigned len; } just for this,
but I could make these parallel arrays, one with head index and
the other with length.
> > + if (vq->rxmaxheadcount < headcount)
> > + vq->rxmaxheadcount = headcount;
>
> This seems the only place where we set the rxmaxheadcount
> value. Maybe it can be moved out of vhost.c to net.c?
> If vhost library needs this it can get it as function
> parameter.
I can move it to vhost_get_heads(), sure.
>
> > + /* Skip header. TODO: support TSO. */
>
> You seem to have removed the code that skips the header.
> Won't this break non-GSO backends such as raw?
From our prior discussion, what I had in mind was that
we'd remove all special-header processing by using the ioctl
you added for TUN and later, an equivalent ioctl for raw (when
supported in qemu-kvm). Qemu would arrange headers with tap
(and later raw) to get what the guest expects, and vhost then
just passes all data as-is between the socket and the ring.
That not only removes the different-header-len code
for mergeable buffers, but also eliminates making a copy of the
header for every packet as before. Vhost has no need to do
anything with the header, or even know its length. It also
doesn't have to care what the type of the backend is-- raw or
tap.
I think that simplifies everything, but it does mean that
raw socket support requires a header ioctl for the different
vnethdr sizes if the guest wants a vnethdr with and without
mergeable buffers. Actually, I thought this was your idea and
the point of the TUNSETVNETHDRSZ ioctl, but I guess you had
something else in mind?
> > - /* TODO: Check specific error and bomb out unless
EAGAIN?
> > */
>
> Do you think it's not a good idea?
EAGAIN is not possible after the change, because we don't
even enter the loop unless we have an skb on the read queue; the
other cases bomb out, so I figured the comment for future work is
now done. :-)
>
> > if (err < 0) {
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard_vq_desc(vq, headcount);
> > break;
> > }
>
> I think we should detect and discard truncated messages,
> since len might not be reliable if userspace pulls
> a packet from under us. Also, if new packet is
> shorter than the old one, there's no truncation but
> headcount is wrong.
>
> So simplest fix IMO would be to compare err with expected len.
> If there's a difference, we hit the race and so
> we would discard the packet.
Sure.
>
>
> > /* TODO: Should check and handle checksum. */
> > + if (vhost_has_feature(&net->dev,
VIRTIO_NET_F_MRG_RXBUF))
> > {
> > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > + (struct virtio_net_hdr_mrg_rxbuf *)
> > + vq->iov[0].iov_base;
> > + /* add num_bufs */
> > + if (put_user(headcount, &vhdr->num_buffers)) {
> > + vq_err(vq, "Failed to write
num_buffers");
> > + vhost_discard_vq_desc(vq, headcount);
>
> Let's do memcpy_to_iovecend etc so that we do not assume layout.
> This is also why we need move_iovec: sendmsg might modify the iovec.
> It would also be nice not to corrupt memory and
> get a reasonable error if buffer size
> that we get is smaller than expected header size.
I wanted to avoid the header copy here, with the reasonable
restriction
that the guest gives you a buffer at least big enough for the vnet_hdr. A
length check alone (on iov[0].iov_len) could enforce that without copying
headers back and forth to support silly cases like 8-byte buffers or
num_buffers spanning multiple iovecs, and I think paying the price of the
copy on every packet to support guests that broken isn't worth it.
So, my preference here would be to add:
if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
struct virtio_net_mrg_rxbuf *vhdr...
if (vq->iov[0].iov_len < sizeof(*vhdr)) {
vq_err(vq, "tiny buffers (len %d < %d) are not
supported",
vq->iov[0].iov_len, sizeof(*vhdr));
break;
}
/* add num_bufs */
...
Does that work for you?
> > - if (err) {
> > - vq_err(vq, "Unable to write vnet_hdr at addr
%p:
> > %d\n",
> > - vq->iov->iov_base, err);
> > - break;
> > - }
> > - len += hdr_size;
>
> This seems to break non-GSO backends as well.
I don't have any to test with w/ current qemu-kvm, but again, I thought
your plan was to remove special header processing from vhost and let
the socket end do it via ioctl. Wouldn't a similar ioctl for raw
sockets when supported by qemu do that?
> > }
> > n->dev.acked_features = features;
> > smp_wmb();
> > - for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> > - mutex_lock(&n->vqs[i].mutex);
> > - n->vqs[i].hdr_size = hdr_size;
> > - mutex_unlock(&n->vqs[i].mutex);
>
> I expect the above is a leftover from the previous version
> which calculated header size in kernel?
Right. With the ioctl, and assuming raw gets a similar
one, we don't need to know anything about the header size in
vhost, except that qemu needs to make sure mergeable rxbufs is
only set when the socket has the larger vnet_hdr that includes
the num_bufs field.
> > @@ -410,6 +410,7 @@ static long vhost_set_vring(struct vhost
> > vq->last_avail_idx = s.num;
> > /* Forget the cached index value. */
> > vq->avail_idx = vq->last_avail_idx;
> > + vq->rxmaxheadcount = 0;
> > break;
> > case VHOST_GET_VRING_BASE:
> > s.index = idx;
> > @@ -856,6 +857,48 @@ static unsigned get_indirect(struct vhos
> > return 0;
> > }
> >
> > +/* This is a multi-head version of vhost_get_vq_desc
>
> How about:
> version of vhost_get_vq_desc that returns multiple
> descriptors
OK.
>
> > + * @vq - the relevant virtqueue
> > + * datalen - data length we'll be reading
> > + * @iovcount - returned count of io vectors we fill
> > + * @log - vhost log
> > + * @log_num - log offset
>
> return value?
> Also - why unsigned?
Returned value is the number of heads, which is
0 or greater.
>
> > + */
> > +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
>
> Would vhost_get_vq_desc_multiple be a better name?
26 chars for the function name with indentation and
argument list might not be pretty on the calls. What about
changing vhost_get_desc() and vhost_get_desc_n() [make them
both shorter]? ("_n" indicating >= 1 instead of just one).
> > + struct vhost_log *log, unsigned int *log_num)
> > +{
> > + struct iovec *heads = vq->heads;
>
> I think it's better to pass in heads array than take it from vq->heads.
I'd actually prefer to go the other way, and remove the
log stuff, for example, since they are accessible from vq which
we have already. Adding heads make it look less similar to the
arg list for vhost_get_vq_desc(), but I'm more interested in
getting the feature than particular arg lists, so your call.
>
> > + int out, in = 0;
>
> Why is in initialized here?
Needed in the previous version, but not here.
>
> > + int seg = 0; /* iov index */
> > + int hc = 0; /* head count */
> > +
> > + while (datalen > 0) {
>
> Can't this simply call vhost_get_vq_desc in a loop somehow,
> or use a common function that both this and vhost_get_vq_desc call?
It is calling vhost_get_vq_desc() in this loop; that's how it
gets the indiviual heads and iov entries. The rest of it is just
massaging the offsets so vhost_get_vq_desc puts them all in the
right places and tracks the heads and lengths correctly for add_used.
>
> > + if (hc >= VHOST_NET_MAX_SG) {
> > + vhost_discard_vq_desc(vq, hc);
> > + return 0;
> > + }
> > + heads[hc].iov_base = (void
*)vhost_get_vq_desc(vq->dev,
> > vq,
> > + vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out,
&in,
> > log,
> > + log_num);
> > + if (heads[hc].iov_base == (void *)vq->num) {
> > + vhost_discard_vq_desc(vq, hc);
> > + return 0;
> > + }
> > + if (out || in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for
RX: "
> > + "out %d, in %d\n", out, in);
> > + vhost_discard_vq_desc(vq, hc);
>
> goto err above might help simplify cleanup.
I generally would rather see the error cleanup explicitly where the
error detection and message are, for code readability, but I can change
it to a goto here if you think that's clearer.
> > -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> > len)
> > +int vhost_add_used(struct vhost_virtqueue *vq, struct iovec *heads,
int
> > count)
>
> count is always 0 for send, right?
> I think it is better to have two APIs here as well:
> vhost_add_used
> vhost_add_used_multiple
> we can use functions to avoid code duplication.
>
> > {
> > - struct vring_used_elem *used;
> > + struct vring_used_elem *used = 0;
>
> why is used initialized here?
This is to remove a compiler warning that "used" may
be used unitialized, for a case that can't happen (count <= 0).
> > + vq->last_used_idx++;
>
> I think we should update last_used_idx on success only,
> at the end. Simply use last_used_idx + count
> instead of last_used_idx + 1.
OK.
> > @@ -1023,22 +1071,35 @@ int vhost_add_used(struct vhost_virtqueu
> > if (vq->log_ctx)
> > eventfd_signal(vq->log_ctx, 1);
> > }
> > - vq->last_used_idx++;
> > return 0;
> > }
> >
> > +int vhost_available(struct vhost_virtqueue *vq)
>
> since this function is non-static, please document what it does.
OK.
> > +{
> > + int avail;
> > +
> > + if (!vq->rxmaxheadcount) /* haven't got any yet */
> > + return 1;
>
> since seems to make net - specific asumptions.
> How about moving this check out to net.c?
I can, but its only user is vhost_signal() in this file.
> > + avail = vq->avail_idx - vq->last_avail_idx;
> > + if (avail < 0)
> > + avail += 0x10000; /* wrapped */
>
> A branch that is likely non-taken. Also, rxmaxheadcount
> is really unlikely to get as large as half the ring.
> So this just wastes cycles?
The indexes are free-running counters, so if
avail_idx has overflowed but last_avail_idx has not
then the result will be (incorrectly) negative. It
has nothing to do with how many buffers are in use--
consider avail_idx = 2 and last_avail_idx = 65534,
then avail = will be -65532, but we want it to be 4.
>
> > + return avail;
> > +}
> > +
> > /* This actually signals the guest, using eventfd. */
> > -void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > +void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq,
bool
> > recvr)
>
> This one is not elegant. receive is net.c concept, let's not
> put it in vhost.c. Pass in headcount if you must.
Mainly what I was after here is to not affect signal's
for the transmit side. I thought about splitting it into a
separate function entirely for the transmit, but came up with
this, instead. I can't tell if it's a receiver from the vq,
but I don't want to override NOTIFY_ON_EMPTY or the headcount
check (which has nothing to do with transmitters) for the
transmitter at all.
The vq doesn't tell you if it is a receiver or not,
so I either need a flag as argument to tell, or I need to
split into a transmit vhost_signal and a receive vhost_signal.
I can do the available check before calling vhost_signal,
but then I need to remove the NOTIFY check entirely so it'll
still notify for the receiver case when available is not
enough but still not 0.
So, I think the options are:
1) a flag
2) separate transmit or receive signal functions
3) move NOTIFY_ON_EMPTY out of vhost_signal and
check that or available (for recvr) before
calling vhost_signal().
>
> > {
> > __u16 flags = 0;
> > +
> > if (get_user(flags, &vq->avail->flags)) {
> > vq_err(vq, "Failed to get flags");
> > return;
> > }
> >
> > - /* If they don't want an interrupt, don't signal, unless
empty. */
> > + /* If they don't want an interrupt, don't signal, unless
> > + * empty or receiver can't get a max-sized packet. */
> > if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> > - (vq->avail_idx != vq->last_avail_idx ||
> > + (!recvr || vhost_available(vq) >= vq->rxmaxheadcount ||
>
> Is the above really worth the complexity?
> Guests can't rely on this kind of fuzzy logic, can they?
> Do you see this helping performance at all?
It definitely hurts performance to allocate max-sized buffers
and then free up the excess (the translate_desc is expensive). It is
a heuristic, but it's one that keeps the notifies flowing without
having to fail on a packet to reenable them.
The notify code has to change because there is no recovery
if we need 5 buffers and have only 3, while notification is
disabled unless we go all the way to 0. We can't honor NOTIFY_ON_EMPTY
in that case because it will never proceed. We can remove the
NOTIFY_ON_EMPTY check in vhost_signal and do it in the caller; then allow
receiver to fail a buffer allocation and signal unconditionally,
but we cannot wait for it to be 0 even if NOTIFY_ON_EMPTY is set--
that's a deadlock.
> > /* OK, now we need to know about added descriptors. */
> > -bool vhost_enable_notify(struct vhost_virtqueue *vq)
> > +bool vhost_enable_notify(struct vhost_virtqueue *vq, bool recvr)
> > {
> > u16 avail_idx;
> > int r;
> > @@ -1080,6 +1141,8 @@ bool vhost_enable_notify(struct vhost_vi
> > return false;
> > }
> >
> > + if (recvr && vq->rxmaxheadcount)
> > + return (avail_idx - vq->last_avail_idx) >=
> > vq->rxmaxheadcount;
>
> The fuzzy logic behind rxmaxheadcount might be a good
> optimization, but I am not comfortable using it
> for correctness. Maybe vhost_enable_notify should get
> the last head so we can redo poll when another one
> is added.
I tried requiring at least 64K here always, but as
I said before, the cost of doing the translate_desc and
then throwing those away for small packets killed performance.
I also considered using a highwater mark on the ring, but
potentially differing buffer sizes makes that less useful,
and a small ring may only have enough space for 1 max-sized
packet.
Maybe we should just remove NOTIFY_ON_EMPTY if we
fail a packet allocation; I can try that out.
Another alternative I considered was splitting out
the translate_desc stuff in the hopes that we could found
out how many buffers we need in a less-expensive way; then
we could go for max-sized and free the excess without too
much overhead, maybe.
Anyway, I'll look harder at working around this.
> > struct iovec indirect[VHOST_NET_MAX_SG];
> > - struct iovec iov[VHOST_NET_MAX_SG];
> > - struct iovec hdr[VHOST_NET_MAX_SG];
> > - size_t hdr_size;
> > + struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr
*/
>
> VHOST_NET_MAX_SG should already include iovec vnet hdr.
That's actually an artifact from the previous patch. I
was arranging the iovec to have just the header needed by the
guest and this simplified the bounds checking. It can be
removed if we leave header size management to the socket side,
but if vhost is going to be doing vnet_hdr manipulation for
raw sockets, then it'll depend on what that new code needs
to do to manage the header, I suppose.
+-DLS
^ permalink raw reply
* Re: [PATCH v2] Add Mergeable RX buffer feature to vhost_net
From: Michael S. Tsirkin @ 2010-04-01 10:54 UTC (permalink / raw)
To: David Stevens; +Cc: netdev, kvm-owner, kvm, virtualization
In-Reply-To: <OF5F520CEF.01718090-ON882576F7.006C8152-882576F7.0079481F@us.ibm.com>
On Wed, Mar 31, 2010 at 03:04:43PM -0700, David Stevens wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 03/31/2010 05:02:28 AM:
> >
> > attached patch seems to be whiespace damaged as well.
> > Does the origin pass checkpatch.pl for you?
>
> Yes, but I probably broke it in the transfer -- will be more
> careful with the next revision.
>
>
>
> > > + head.iov_base = (void *)vhost_get_vq_desc(&net->dev,
> vq,
> > > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
>
> > > NULL);
> >
> > I this casting confusing.
> > Is it really expensive to add an array of heads so that
> > we do not need to cast?
>
> It needs the heads and the lengths, which looks a lot
> like an iovec. I was trying to resist adding a new
> struct XXX { unsigned head; unsigned len; } just for this,
> but I could make these parallel arrays, one with head index and
> the other with length.
>
> > > + if (vq->rxmaxheadcount < headcount)
> > > + vq->rxmaxheadcount = headcount;
> >
> > This seems the only place where we set the rxmaxheadcount
> > value. Maybe it can be moved out of vhost.c to net.c?
> > If vhost library needs this it can get it as function
> > parameter.
>
> I can move it to vhost_get_heads(), sure.
> >
> > > + /* Skip header. TODO: support TSO. */
> >
> > You seem to have removed the code that skips the header.
> > Won't this break non-GSO backends such as raw?
>
> From our prior discussion, what I had in mind was that
> we'd remove all special-header processing by using the ioctl
> you added for TUN and later, an equivalent ioctl for raw (when
> supported in qemu-kvm). Qemu would arrange headers with tap
> (and later raw) to get what the guest expects, and vhost then
> just passes all data as-is between the socket and the ring.
> That not only removes the different-header-len code
> for mergeable buffers, but also eliminates making a copy of the
> header for every packet as before.
Yes but please note that in practice if this is what we do,
then vhost header size is 0 and then there is no actual copy.
> Vhost has no need to do
> anything with the header, or even know its length. It also
> doesn't have to care what the type of the backend is-- raw or
> tap.
> I think that simplifies everything, but it does mean that
> raw socket support requires a header ioctl for the different
> vnethdr sizes if the guest wants a vnethdr with and without
> mergeable buffers. Actually, I thought this was your idea and
> the point of the TUNSETVNETHDRSZ ioctl, but I guess you had
> something else in mind?
Yes. However, we also have an ioctl that sets vhost header size, and it
allows supporting simple backends which do not support an (equivalent
of) TUNSETVNETHDRSZ. We have two of these now: packet and macvtap.
So I thought that unless there are gains in code simplicity and/or
performance we can keep supporting these just as well.
> > > - /* TODO: Check specific error and bomb out unless
> EAGAIN?
> > > */
> >
> > Do you think it's not a good idea?
>
> EAGAIN is not possible after the change, because we don't
> even enter the loop unless we have an skb on the read queue; the
> other cases bomb out, so I figured the comment for future work is
> now done. :-)
Guest could be buggy so we'll get EFAULT.
If skb is taken off the rx queue (as below), we might get EAGAIN.
> >
> > > if (err < 0) {
> > > - vhost_discard_vq_desc(vq);
> > > + vhost_discard_vq_desc(vq, headcount);
> > > break;
> > > }
> >
> > I think we should detect and discard truncated messages,
> > since len might not be reliable if userspace pulls
> > a packet from under us. Also, if new packet is
> > shorter than the old one, there's no truncation but
> > headcount is wrong.
> >
> > So simplest fix IMO would be to compare err with expected len.
> > If there's a difference, we hit the race and so
> > we would discard the packet.
>
> Sure.
>
> >
> >
> > > /* TODO: Should check and handle checksum. */
> > > + if (vhost_has_feature(&net->dev,
> VIRTIO_NET_F_MRG_RXBUF))
> > > {
> > > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > > + (struct virtio_net_hdr_mrg_rxbuf *)
> > > + vq->iov[0].iov_base;
> > > + /* add num_bufs */
> > > + if (put_user(headcount, &vhdr->num_buffers)) {
> > > + vq_err(vq, "Failed to write
> num_buffers");
> > > + vhost_discard_vq_desc(vq, headcount);
> >
> > Let's do memcpy_to_iovecend etc so that we do not assume layout.
> > This is also why we need move_iovec: sendmsg might modify the iovec.
> > It would also be nice not to corrupt memory and
> > get a reasonable error if buffer size
> > that we get is smaller than expected header size.
>
> I wanted to avoid the header copy here, with the reasonable
> restriction
> that the guest gives you a buffer at least big enough for the vnet_hdr.
> A
> length check alone (on iov[0].iov_len) could enforce that without copying
> headers back and forth to support silly cases like 8-byte buffers or
> num_buffers spanning multiple iovecs, and I think paying the price of the
> copy on every packet to support guests that broken isn't worth it.
In practice, when the first iovec includes the header what
we will copy is iov[0].iov_len + iov[0].iov_base.
We also still have the problem that sendmsg might modify the iovec,
(for tap it doesn't) so using iov_len after sendmsg isn't legal,
you need to copy it anyway.
> So, my preference here would be to add:
>
> if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> struct virtio_net_mrg_rxbuf *vhdr...
>
> if (vq->iov[0].iov_len < sizeof(*vhdr)) {
> vq_err(vq, "tiny buffers (len %d < %d) are not
> supported",
> vq->iov[0].iov_len, sizeof(*vhdr));
> break;
> }
> /* add num_bufs */
> ...
>
> Does that work for you?
Seems more code, doesn't it? Do you really believe this is a performance gain?
It looks a bit like prepature optimization ...
> > > - if (err) {
> > > - vq_err(vq, "Unable to write vnet_hdr at addr
> %p:
> > > %d\n",
> > > - vq->iov->iov_base, err);
> > > - break;
> > > - }
> > > - len += hdr_size;
> >
> > This seems to break non-GSO backends as well.
>
> I don't have any to test with w/ current qemu-kvm,
I can dig up the demo, but there's also the macvtap backend. Try using that.
> but again, I thought
> your plan was to remove special header processing from vhost and let
> the socket end do it via ioctl. Wouldn't a similar ioctl for raw
> sockets when supported by qemu do that?
I just don't want to require that backends support GSO/vnet header,
so vhost needs an ability to skip the header.
> > > }
> > > n->dev.acked_features = features;
> > > smp_wmb();
> > > - for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> > > - mutex_lock(&n->vqs[i].mutex);
> > > - n->vqs[i].hdr_size = hdr_size;
> > > - mutex_unlock(&n->vqs[i].mutex);
> >
> > I expect the above is a leftover from the previous version
> > which calculated header size in kernel?
>
> Right. With the ioctl, and assuming raw gets a similar
> one, we don't need to know anything about the header size in
> vhost, except that qemu needs to make sure mergeable rxbufs is
> only set when the socket has the larger vnet_hdr that includes
> the num_bufs field.
>
Note that vhost doesn't really *know* about header, per se,
it just skips the first X bytes in iovec.
> > > @@ -410,6 +410,7 @@ static long vhost_set_vring(struct vhost
> > > vq->last_avail_idx = s.num;
> > > /* Forget the cached index value. */
> > > vq->avail_idx = vq->last_avail_idx;
> > > + vq->rxmaxheadcount = 0;
> > > break;
> > > case VHOST_GET_VRING_BASE:
> > > s.index = idx;
> > > @@ -856,6 +857,48 @@ static unsigned get_indirect(struct vhos
> > > return 0;
> > > }
> > >
> > > +/* This is a multi-head version of vhost_get_vq_desc
> >
> > How about:
> > version of vhost_get_vq_desc that returns multiple
> > descriptors
>
> OK.
>
> >
> > > + * @vq - the relevant virtqueue
> > > + * datalen - data length we'll be reading
> > > + * @iovcount - returned count of io vectors we fill
> > > + * @log - vhost log
> > > + * @log_num - log offset
> >
> > return value?
> > Also - why unsigned?
>
> Returned value is the number of heads, which is
> 0 or greater.
The advantage of using int is that you'll be able to return
an error code. Good for debugging.
> >
> > > + */
> > > +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
>
> >
> > Would vhost_get_vq_desc_multiple be a better name?
>
> 26 chars for the function name with indentation and
> argument list might not be pretty on the calls. What about
> changing vhost_get_desc() and vhost_get_desc_n() [make them
> both shorter]? ("_n" indicating >= 1 instead of just one).
OK.
> > > + struct vhost_log *log, unsigned int *log_num)
> > > +{
> > > + struct iovec *heads = vq->heads;
> >
> > I think it's better to pass in heads array than take it from vq->heads.
>
> I'd actually prefer to go the other way, and remove the
> log stuff, for example, since they are accessible from vq which
> we have already.
The main reason is that long term, we might want to
detach the arrays from vq - they are there simply to
avoid sticking too much data on stack - and the
fixed size becomes limiting for vhost block.
The advantage of passing them would be that
vhost won't have to change.
> Adding heads make it look less similar to the
> arg list for vhost_get_vq_desc(), but I'm more interested in
> getting the feature than particular arg lists, so your call.
Well, vhost_get_desc_n is just like vhost_get_desc but returns
multiple heads. In C you can return a single int by value
but many ints must be returned in an array, so I don't
think it is that dissimilar.
> >
> > > + int out, in = 0;
> >
> > Why is in initialized here?
>
> Needed in the previous version, but not here.
>
> >
> > > + int seg = 0; /* iov index */
> > > + int hc = 0; /* head count */
> > > +
> > > + while (datalen > 0) {
> >
> > Can't this simply call vhost_get_vq_desc in a loop somehow,
> > or use a common function that both this and vhost_get_vq_desc call?
>
> It is calling vhost_get_vq_desc() in this loop; that's how it
> gets the indiviual heads and iov entries. The rest of it is just
> massaging the offsets so vhost_get_vq_desc puts them all in the
> right places and tracks the heads and lengths correctly for add_used.
Sorry, will have to re-read.
> >
> > > + if (hc >= VHOST_NET_MAX_SG) {
> > > + vhost_discard_vq_desc(vq, hc);
> > > + return 0;
> > > + }
> > > + heads[hc].iov_base = (void
> *)vhost_get_vq_desc(vq->dev,
> > > vq,
> > > + vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out,
> &in,
> > > log,
> > > + log_num);
> > > + if (heads[hc].iov_base == (void *)vq->num) {
> > > + vhost_discard_vq_desc(vq, hc);
> > > + return 0;
> > > + }
> > > + if (out || in <= 0) {
> > > + vq_err(vq, "unexpected descriptor format for
> RX: "
> > > + "out %d, in %d\n", out, in);
> > > + vhost_discard_vq_desc(vq, hc);
> >
> > goto err above might help simplify cleanup.
>
> I generally would rather see the error cleanup explicitly where the
> error detection and message are, for code readability, but I can change
> it to a goto here if you think that's clearer.
This is the idiom I use all over this code.
> > > -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
>
> > > len)
> > > +int vhost_add_used(struct vhost_virtqueue *vq, struct iovec *heads,
> int
> > > count)
> >
> > count is always 0 for send, right?
> > I think it is better to have two APIs here as well:
> > vhost_add_used
> > vhost_add_used_multiple
> > we can use functions to avoid code duplication.
> >
> > > {
> > > - struct vring_used_elem *used;
> > > + struct vring_used_elem *used = 0;
> >
> > why is used initialized here?
>
> This is to remove a compiler warning that "used" may
> be used unitialized, for a case that can't happen (count <= 0).
>
I think there's some macro in kernel to shut the compiler up.
Or just ignore the warning. Let's not add extra initializers
just to shut up compiler, I think this might hide real bugs.
> > > + vq->last_used_idx++;
> >
> > I think we should update last_used_idx on success only,
> > at the end. Simply use last_used_idx + count
> > instead of last_used_idx + 1.
>
> OK.
>
> > > @@ -1023,22 +1071,35 @@ int vhost_add_used(struct vhost_virtqueu
> > > if (vq->log_ctx)
> > > eventfd_signal(vq->log_ctx, 1);
> > > }
> > > - vq->last_used_idx++;
> > > return 0;
> > > }
> > >
> > > +int vhost_available(struct vhost_virtqueue *vq)
> >
> > since this function is non-static, please document what it does.
>
> OK.
>
> > > +{
> > > + int avail;
> > > +
> > > + if (!vq->rxmaxheadcount) /* haven't got any yet */
> > > + return 1;
> >
> > since seems to make net - specific asumptions.
> > How about moving this check out to net.c?
>
> I can, but its only user is vhost_signal() in this file.
So why isn't it static?
> > > + avail = vq->avail_idx - vq->last_avail_idx;
> > > + if (avail < 0)
> > > + avail += 0x10000; /* wrapped */
> >
> > A branch that is likely non-taken. Also, rxmaxheadcount
> > is really unlikely to get as large as half the ring.
> > So this just wastes cycles?
>
> The indexes are free-running counters, so if
> avail_idx has overflowed but last_avail_idx has not
> then the result will be (incorrectly) negative. It
> has nothing to do with how many buffers are in use--
> consider avail_idx = 2 and last_avail_idx = 65534,
> then avail = will be -65532, but we want it to be 4.
Right. Sorry about the noise.
> >
> > > + return avail;
> > > +}
> > > +
> > > /* This actually signals the guest, using eventfd. */
> > > -void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > > +void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> bool
> > > recvr)
> >
> > This one is not elegant. receive is net.c concept, let's not
> > put it in vhost.c. Pass in headcount if you must.
>
> Mainly what I was after here is to not affect signal's
> for the transmit side. I thought about splitting it into a
> separate function entirely for the transmit, but came up with
> this, instead. I can't tell if it's a receiver from the vq,
> but I don't want to override NOTIFY_ON_EMPTY or the headcount
> check (which has nothing to do with transmitters) for the
> transmitter at all.
> The vq doesn't tell you if it is a receiver or not,
> so I either need a flag as argument to tell, or I need to
> split into a transmit vhost_signal and a receive vhost_signal.
You see, receive is really a net concept. virtio has in and out,
and a single descriptor can do both.
> I can do the available check before calling vhost_signal,
> but then I need to remove the NOTIFY check entirely so it'll
> still notify for the receiver case when available is not
> enough but still not 0.
My question is, guests can not rely on the 'available is not
enough but still not 0' to work because it relies on a heuristic
to figure out how much is 'not enough'. So why do we need it at all.
> So, I think the options are:
> 1) a flag
> 2) separate transmit or receive signal functions
> 3) move NOTIFY_ON_EMPTY out of vhost_signal and
> check that or available (for recvr) before
> calling vhost_signal().
There are ordering rules that make 3 impossible.
Let's try to rethink the concept of redefining NOTIFY_ON_EMPTY.
Is it really necessary?
> >
> > > {
> > > __u16 flags = 0;
> > > +
> > > if (get_user(flags, &vq->avail->flags)) {
> > > vq_err(vq, "Failed to get flags");
> > > return;
> > > }
> > >
> > > - /* If they don't want an interrupt, don't signal, unless
> empty. */
> > > + /* If they don't want an interrupt, don't signal, unless
> > > + * empty or receiver can't get a max-sized packet. */
> > > if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> > > - (vq->avail_idx != vq->last_avail_idx ||
> > > + (!recvr || vhost_available(vq) >= vq->rxmaxheadcount ||
> >
> > Is the above really worth the complexity?
> > Guests can't rely on this kind of fuzzy logic, can they?
> > Do you see this helping performance at all?
>
> It definitely hurts performance to allocate max-sized buffers
> and then free up the excess (the translate_desc is expensive).
Well, we do not need to rerun translate_desc, we can keep
the results around, however ...
> It is
> a heuristic, but it's one that keeps the notifies flowing without
> having to fail on a packet to reenable them.
I don't understand completely. We looked into the skb, so
we know the required length?
> The notify code has to change because there is no recovery
> if we need 5 buffers and have only 3, while notification is
> disabled unless we go all the way to 0. We can't honor NOTIFY_ON_EMPTY
> in that case because it will never proceed. We can remove the
> NOTIFY_ON_EMPTY check in vhost_signal and do it in the caller; then allow
> receiver to fail a buffer allocation and signal unconditionally,
> but we cannot wait for it to be 0 even if NOTIFY_ON_EMPTY is set--
> that's a deadlock.
>
Which guest kernel or spec are you looking at? As far as I know
F_NOTIFY_ON_EMPTY was only used for TX at some point.
I am not aware of this bit having the meaning you assign it.
I suspect that what you outline above simply means guest
can not combine F_NOTIFY_ON_EMPTY with mergeable buffers for
virtio net RX VQ.
> > > /* OK, now we need to know about added descriptors. */
> > > -bool vhost_enable_notify(struct vhost_virtqueue *vq)
> > > +bool vhost_enable_notify(struct vhost_virtqueue *vq, bool recvr)
> > > {
> > > u16 avail_idx;
> > > int r;
> > > @@ -1080,6 +1141,8 @@ bool vhost_enable_notify(struct vhost_vi
> > > return false;
> > > }
> > >
> > > + if (recvr && vq->rxmaxheadcount)
> > > + return (avail_idx - vq->last_avail_idx) >=
> > > vq->rxmaxheadcount;
> >
> > The fuzzy logic behind rxmaxheadcount might be a good
> > optimization, but I am not comfortable using it
> > for correctness. Maybe vhost_enable_notify should get
> > the last head so we can redo poll when another one
> > is added.
>
> I tried requiring at least 64K here always, but as
> I said before, the cost of doing the translate_desc and
> then throwing those away for small packets killed performance.
> I also considered using a highwater mark on the ring, but
> potentially differing buffer sizes makes that less useful,
> and a small ring may only have enough space for 1 max-sized
> packet.
> Maybe we should just remove NOTIFY_ON_EMPTY if we
> fail a packet allocation; I can try that out.
> Another alternative I considered was splitting out
> the translate_desc stuff in the hopes that we could found
> out how many buffers we need in a less-expensive way; then
> we could go for max-sized and free the excess without too
> much overhead, maybe.
> Anyway, I'll look harder at working around this.
I am kind of confused. We did a peek and have the skb length,
why do we need the maxheads?
Hmm, the iovec consumed is not just skb length, it is skb length + vnet
header. Is this it? Maybe we can just export this from socket somehow?
> > > struct iovec indirect[VHOST_NET_MAX_SG];
> > > - struct iovec iov[VHOST_NET_MAX_SG];
> > > - struct iovec hdr[VHOST_NET_MAX_SG];
> > > - size_t hdr_size;
> > > + struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr
> */
> >
> > VHOST_NET_MAX_SG should already include iovec vnet hdr.
>
> That's actually an artifact from the previous patch. I
> was arranging the iovec to have just the header needed by the
> guest and this simplified the bounds checking. It can be
> removed if we leave header size management to the socket side,
> but if vhost is going to be doing vnet_hdr manipulation for
> raw sockets, then it'll depend on what that new code needs
> to do to manage the header, I suppose.
>
> +-DLS
Note it doesn't manipluate the header, just skips it.
--
MST
^ permalink raw reply
* Re: [PATCH v2] Add Mergeable RX buffer feature to vhost_net
From: David Stevens @ 2010-04-01 18:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <20100401105415.GA3323@redhat.com>
kvm-owner@vger.kernel.org wrote on 04/01/2010 03:54:15 AM:
> On Wed, Mar 31, 2010 at 03:04:43PM -0700, David Stevens wrote:
> >
> > > > + head.iov_base = (void
*)vhost_get_vq_desc(&net->dev,
> > vq,
> > > > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in,
NULL,
> >
> > > > NULL);
> > >
> > > I this casting confusing.
> > > Is it really expensive to add an array of heads so that
> > > we do not need to cast?
> >
> > It needs the heads and the lengths, which looks a lot
> > like an iovec. I was trying to resist adding a new
> > struct XXX { unsigned head; unsigned len; } just for this,
> > but I could make these parallel arrays, one with head index and
> > the other with length.
Michael, on this one, if I add vq->heads as an argument to
vhost_get_heads (aka vhost_get_desc_n), I'd need the length too.
Would you rather this 1) remain an iovec (and a single arg added) but
cast still there, 2) 2 arrays (head and length) and 2 args added, or
3) a new struct type of {unsigned,int} to carry for the heads+len
instead of iovec?
My preference would be 1). I agree the casts are ugly, but
it is essentially an iovec the way we use it; it's just that the
base isn't a pointer but a descriptor index instead.
> >
> > EAGAIN is not possible after the change, because we don't
> > even enter the loop unless we have an skb on the read queue; the
> > other cases bomb out, so I figured the comment for future work is
> > now done. :-)
>
> Guest could be buggy so we'll get EFAULT.
> If skb is taken off the rx queue (as below), we might get EAGAIN.
We break on any error. If we get EAGAIN because someone read
on the socket, this code would break the loop, but EAGAIN is a more
serious problem if it changed since we peeked (because it means
someone else is reading the socket).
But I don't understand -- are you suggesting that the error
handling be different than that, or that the comment is still
relevant? My intention here is to do the "TODO" from the comment
so that it can be removed, by handling all error cases. I think
because of the peek, EAGAIN isn't something to be ignored anymore,
but the effect is the same whether we break out of the loop or
not, since we retry the packet next time around. Essentially, we
ignore every error since we will redo it with the same packet the
next time around. Maybe we should print something here, but since
we'll be retrying the packet that's still on the socket, a permanent
error would spew continuously. Maybe we should shut down entirely
if we get any negative return value here (including EAGAIN, since
that tells us someone messed with the socket when we don't want them
to).
If you want the comment still there, ok, but I do think EAGAIN
isn't a special case per the comment anymore, and is handled as all
other errors are: by exiting the loop and retrying next time.
+-DLS
^ permalink raw reply
* Re: [PATCH v2] Add Mergeable RX buffer feature to vhost_net
From: Michael S. Tsirkin @ 2010-04-04 8:55 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <OF9AF90021.62EF0EE4-ON882576F8.00618B3E-882576F8.0064F223@us.ibm.com>
On Thu, Apr 01, 2010 at 11:22:37AM -0700, David Stevens wrote:
> kvm-owner@vger.kernel.org wrote on 04/01/2010 03:54:15 AM:
>
> > On Wed, Mar 31, 2010 at 03:04:43PM -0700, David Stevens wrote:
>
> > >
> > > > > + head.iov_base = (void
> *)vhost_get_vq_desc(&net->dev,
> > > vq,
> > > > > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in,
> NULL,
> > >
> > > > > NULL);
> > > >
> > > > I this casting confusing.
> > > > Is it really expensive to add an array of heads so that
> > > > we do not need to cast?
> > >
> > > It needs the heads and the lengths, which looks a lot
> > > like an iovec. I was trying to resist adding a new
> > > struct XXX { unsigned head; unsigned len; } just for this,
> > > but I could make these parallel arrays, one with head index and
> > > the other with length.
>
> Michael, on this one, if I add vq->heads as an argument to
> vhost_get_heads (aka vhost_get_desc_n), I'd need the length too.
> Would you rather this 1) remain an iovec (and a single arg added) but
> cast still there, 2) 2 arrays (head and length) and 2 args added, or
> 3) a new struct type of {unsigned,int} to carry for the heads+len
> instead of iovec?
> My preference would be 1). I agree the casts are ugly, but
> it is essentially an iovec the way we use it; it's just that the
> base isn't a pointer but a descriptor index instead.
I prefer 2 or 3. If you prefer 1 strongly, I think we should
add a detailed comment near the iovec, and
a couple of inline wrappers to store/get data in the iovec.
> > >
> > > EAGAIN is not possible after the change, because we don't
> > > even enter the loop unless we have an skb on the read queue; the
> > > other cases bomb out, so I figured the comment for future work is
> > > now done. :-)
> >
> > Guest could be buggy so we'll get EFAULT.
> > If skb is taken off the rx queue (as below), we might get EAGAIN.
>
> We break on any error. If we get EAGAIN because someone read
> on the socket, this code would break the loop, but EAGAIN is a more
> serious problem if it changed since we peeked (because it means
> someone else is reading the socket).
> But I don't understand -- are you suggesting that the error
> handling be different than that, or that the comment is still
> relevant?
> My intention here is to do the "TODO" from the comment
> so that it can be removed, by handling all error cases. I think
> because of the peek, EAGAIN isn't something to be ignored anymore,
> but the effect is the same whether we break out of the loop or
> not, since we retry the packet next time around. Essentially, we
> ignore every error since we will redo it with the same packet the
> next time around. Maybe we should print something here, but since
> we'll be retrying the packet that's still on the socket, a permanent
> error would spew continuously. Maybe we should shut down entirely
> if we get any negative return value here (including EAGAIN, since
> that tells us someone messed with the socket when we don't want them
> to).
> If you want the comment still there, ok, but I do think EAGAIN
> isn't a special case per the comment anymore, and is handled as all
> other errors are: by exiting the loop and retrying next time.
>
> +-DLS
Yes, I just think some comment should stay, as you say, because
otherwise we simply retry continuously. Maybe we should trigger vq_err.
It needs to be given some thought which I have not given it yet.
Thinking aloud, EAGAIN means someone reads the socket
together with us, I prefer that this condition is made a fatal
error, we should make sure we are polling the socket
so we see packets if more appear.
--
MST
^ permalink raw reply
* [PATCH 00/10] (v5) virtio: console: Fixes, new way of discovering ports
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
Hello,
This patchset is still based on Linus' tree but I'll rebase when Rusty
sends out the disable multiport patches.
This series reworks the ABI to allow port discovery (only) via the
control queue.
In addition, it adds support for non-blocking write() support, which
means no spinning. This works fine with the recent patches that are on
qemu-devel.
Also included is removal of hvc_remove() as removing one such console
port causes other console ports (registered with hvc) to stall. This
has to be debugged in the hvc_console.c file, I'll do that later, but
we have a nice workaround for this: returning -EPIPE on any hvc
operations will make the hvc console core perform any cleanups for the
removed ports. Looks like we don't lose much by removing hvc_remove().
New in this version:
- Complete support for non-blocking write()
- Don't call hvc_remove()
- Return -EPIPE for hvc operations if a console port was
hot-unplugged.
Amit Shah (10):
virtio: console: Add a __send_control_msg() that can send messages
without a valid port
virtio: console: Let host know of port or device add failures
virtio: console: Return -EPIPE to hvc_console if we lost the
connection
virtio: console: Don't call hvc_remove() on unplugging console ports
virtio: console: Remove config work handler
virtio: console: Move code around for future patches
virtio: console: Use a control message to add ports
virtio: console: Don't always create a port 0 if using multiport
virtio: console: Rename wait_is_over() to will_read_block()
virtio: console: Add support for nonblocking write()s
drivers/char/virtio_console.c | 533 +++++++++++++++++++++-------------------
include/linux/virtio_console.h | 18 +-
2 files changed, 288 insertions(+), 263 deletions(-)
^ permalink raw reply
* [PATCH 01/10] virtio: console: Add a __send_control_msg() that can send messages without a valid port
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-1-git-send-email-amit.shah@redhat.com>
We will introduce control messages that operate on the device as a whole
rather than just ports. Make send_control_msg() a wrapper around
__send_control_msg() which does not need a valid port.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8f665bc..5d24015 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -373,22 +373,22 @@ out:
return ret;
}
-static ssize_t send_control_msg(struct port *port, unsigned int event,
- unsigned int value)
+static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
+ unsigned int event, unsigned int value)
{
struct scatterlist sg[1];
struct virtio_console_control cpkt;
struct virtqueue *vq;
unsigned int len;
- if (!use_multiport(port->portdev))
+ if (!use_multiport(portdev))
return 0;
- cpkt.id = port->id;
+ cpkt.id = port_id;
cpkt.event = event;
cpkt.value = value;
- vq = port->portdev->c_ovq;
+ vq = portdev->c_ovq;
sg_init_one(sg, &cpkt, sizeof(cpkt));
if (vq->vq_ops->add_buf(vq, sg, 1, 0, &cpkt) >= 0) {
@@ -399,6 +399,12 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
return 0;
}
+static ssize_t send_control_msg(struct port *port, unsigned int event,
+ unsigned int value)
+{
+ return __send_control_msg(port->portdev, port->id, event, value);
+}
+
static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
{
struct scatterlist sg[1];
--
1.6.2.5
^ permalink raw reply related
* [PATCH 02/10] virtio: console: Let host know of port or device add failures
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-2-git-send-email-amit.shah@redhat.com>
The host may want to know and let management apps notify of port or
device add failures. Send a control message saying the device or port is
not ready in this case.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 5 +++++
include/linux/virtio_console.h | 3 +++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 5d24015..1ae11f4 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1217,6 +1217,8 @@ free_cdev:
free_port:
kfree(port);
fail:
+ /* The host might want to notify management sw about port add failure */
+ send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 0);
return err;
}
@@ -1495,6 +1497,9 @@ free_chrdev:
free:
kfree(portdev);
fail:
+ /* The host might want to notify mgmt sw about device add failure */
+ __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+ VIRTIO_CONSOLE_DEVICE_READY, 0);
return err;
}
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ae4f039..0157361 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -14,6 +14,8 @@
#define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
+#define VIRTIO_CONSOLE_BAD_ID (~(u32)0)
+
struct virtio_console_config {
/* colums of the screens */
__u16 cols;
@@ -42,6 +44,7 @@ struct virtio_console_control {
#define VIRTIO_CONSOLE_PORT_OPEN 3
#define VIRTIO_CONSOLE_PORT_NAME 4
#define VIRTIO_CONSOLE_PORT_REMOVE 5
+#define VIRTIO_CONSOLE_DEVICE_READY 6
#ifdef __KERNEL__
int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
--
1.6.2.5
^ permalink raw reply related
* [PATCH 03/10] virtio: console: Return -EPIPE to hvc_console if we lost the connection
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-3-git-send-email-amit.shah@redhat.com>
hvc_console handles -EPIPE properly when the connection to the host is
lost.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1ae11f4..733276d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -660,7 +660,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
port = find_port_by_vtermno(vtermno);
if (!port)
- return 0;
+ return -EPIPE;
return send_buf(port, (void *)buf, count);
}
@@ -676,9 +676,13 @@ static int get_chars(u32 vtermno, char *buf, int count)
{
struct port *port;
+ /* If we've not set up the port yet, we have no input to give. */
+ if (unlikely(early_put_chars))
+ return 0;
+
port = find_port_by_vtermno(vtermno);
if (!port)
- return 0;
+ return -EPIPE;
/* If we don't have an input queue yet, we can't get input. */
BUG_ON(!port->in_vq);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 04/10] virtio: console: Don't call hvc_remove() on unplugging console ports
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-4-git-send-email-amit.shah@redhat.com>
hvc_remove() has some bug which freezes other active hvc ports when one
port is removed.
So disable calling of hvc_remove() which deregisters a port with the
hvc_console.
If the hvc_console code calls into our get_chars() routine as a result
of a poll operation, we will return -EPIPE and the hvc_console code will
then do the necessary cleanup.
This call will be restored when the bug in hvc_remove() is found and
fixed.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 733276d..b40b112 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -876,7 +876,18 @@ static int remove_port(struct port *port)
spin_lock_irq(&pdrvdata_lock);
list_del(&port->cons.list);
spin_unlock_irq(&pdrvdata_lock);
+#if 0
+ /*
+ * hvc_remove() not called as removing one hvc port
+ * results in other hvc ports getting frozen.
+ *
+ * Once this is resolved in hvc, this functionality
+ * will be enabled. Till that is done, the -EPIPE
+ * return from get_chars() above will help
+ * hvc_console.c to clean up on ports we remove here.
+ */
hvc_remove(port->cons.hvc);
+#endif
}
if (port->guest_connected)
send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 05/10] virtio: console: Remove config work handler
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-5-git-send-email-amit.shah@redhat.com>
We're going to switch to using control messages for port hot-plug and
initial port discovery. Remove the config work handler which handled
port hot-plug so far.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 64 +----------------------------------------
1 files changed, 1 insertions(+), 63 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b40b112..0473221 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -109,7 +109,6 @@ struct ports_device {
* notification
*/
struct work_struct control_work;
- struct work_struct config_work;
struct list_head ports;
@@ -1091,10 +1090,7 @@ static void config_intr(struct virtio_device *vdev)
struct ports_device *portdev;
portdev = vdev->priv;
- if (use_multiport(portdev)) {
- /* Handle port hot-add */
- schedule_work(&portdev->config_work);
- }
+
/*
* We'll use this way of resizing only for legacy support.
* For newer userspace (VIRTIO_CONSOLE_F_MULTPORT+), use
@@ -1237,62 +1233,6 @@ fail:
return err;
}
-/*
- * The workhandler for config-space updates.
- *
- * This is called when ports are hot-added.
- */
-static void config_work_handler(struct work_struct *work)
-{
- struct virtio_console_config virtconconf;
- struct ports_device *portdev;
- struct virtio_device *vdev;
- int err;
-
- portdev = container_of(work, struct ports_device, config_work);
-
- vdev = portdev->vdev;
- vdev->config->get(vdev,
- offsetof(struct virtio_console_config, nr_ports),
- &virtconconf.nr_ports,
- sizeof(virtconconf.nr_ports));
-
- if (portdev->config.nr_ports == virtconconf.nr_ports) {
- /*
- * Port 0 got hot-added. Since we already did all the
- * other initialisation for it, just tell the Host
- * that the port is ready if we find the port. In
- * case the port was hot-removed earlier, we call
- * add_port to add the port.
- */
- struct port *port;
-
- port = find_port_by_id(portdev, 0);
- if (!port)
- add_port(portdev, 0);
- else
- send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
- return;
- }
- if (virtconconf.nr_ports > portdev->config.max_nr_ports) {
- dev_warn(&vdev->dev,
- "More ports specified (%u) than allowed (%u)",
- portdev->config.nr_ports + 1,
- portdev->config.max_nr_ports);
- return;
- }
- if (virtconconf.nr_ports < portdev->config.nr_ports)
- return;
-
- /* Hot-add ports */
- while (virtconconf.nr_ports - portdev->config.nr_ports) {
- err = add_port(portdev, portdev->config.nr_ports);
- if (err)
- break;
- portdev->config.nr_ports++;
- }
-}
-
static int init_vqs(struct ports_device *portdev)
{
vq_callback_t **io_callbacks;
@@ -1485,7 +1425,6 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
spin_lock_init(&portdev->cvq_lock);
INIT_WORK(&portdev->control_work, &control_work_handler);
- INIT_WORK(&portdev->config_work, &config_work_handler);
nr_added_bufs = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
if (!nr_added_bufs) {
@@ -1528,7 +1467,6 @@ static void virtcons_remove(struct virtio_device *vdev)
portdev = vdev->priv;
cancel_work_sync(&portdev->control_work);
- cancel_work_sync(&portdev->config_work);
list_for_each_entry_safe(port, port2, &portdev->ports, list)
remove_port(port);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 06/10] virtio: console: Move code around for future patches
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-6-git-send-email-amit.shah@redhat.com>
We're going to use add_port() from handle_control_message() in the next
patch.
Move the add_port() and fill_queue(), which depends on it, above
handle_control_message() to avoid forward declarations.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 264 ++++++++++++++++++++---------------------
1 files changed, 131 insertions(+), 133 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0473221..fd98861 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -862,6 +862,137 @@ static const struct file_operations port_debugfs_ops = {
.read = debugfs_read,
};
+static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
+{
+ struct port_buffer *buf;
+ unsigned int nr_added_bufs;
+ int ret;
+
+ nr_added_bufs = 0;
+ do {
+ buf = alloc_buf(PAGE_SIZE);
+ if (!buf)
+ break;
+
+ spin_lock_irq(lock);
+ ret = add_inbuf(vq, buf);
+ if (ret < 0) {
+ spin_unlock_irq(lock);
+ free_buf(buf);
+ break;
+ }
+ nr_added_bufs++;
+ spin_unlock_irq(lock);
+ } while (ret > 0);
+
+ return nr_added_bufs;
+}
+
+static int add_port(struct ports_device *portdev, u32 id)
+{
+ char debugfs_name[16];
+ struct port *port;
+ struct port_buffer *buf;
+ dev_t devt;
+ unsigned int nr_added_bufs;
+ int err;
+
+ port = kmalloc(sizeof(*port), GFP_KERNEL);
+ if (!port) {
+ err = -ENOMEM;
+ goto fail;
+ }
+
+ port->portdev = portdev;
+ port->id = id;
+
+ port->name = NULL;
+ port->inbuf = NULL;
+ port->cons.hvc = NULL;
+
+ port->host_connected = port->guest_connected = false;
+
+ port->in_vq = portdev->in_vqs[port->id];
+ port->out_vq = portdev->out_vqs[port->id];
+
+ cdev_init(&port->cdev, &port_fops);
+
+ devt = MKDEV(portdev->chr_major, id);
+ err = cdev_add(&port->cdev, devt, 1);
+ if (err < 0) {
+ dev_err(&port->portdev->vdev->dev,
+ "Error %d adding cdev for port %u\n", err, id);
+ goto free_port;
+ }
+ port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
+ devt, port, "vport%up%u",
+ port->portdev->drv_index, id);
+ if (IS_ERR(port->dev)) {
+ err = PTR_ERR(port->dev);
+ dev_err(&port->portdev->vdev->dev,
+ "Error %d creating device for port %u\n",
+ err, id);
+ goto free_cdev;
+ }
+
+ spin_lock_init(&port->inbuf_lock);
+ init_waitqueue_head(&port->waitqueue);
+
+ /* Fill the in_vq with buffers so the host can send us data. */
+ nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
+ if (!nr_added_bufs) {
+ dev_err(port->dev, "Error allocating inbufs\n");
+ err = -ENOMEM;
+ goto free_device;
+ }
+
+ /*
+ * If we're not using multiport support, this has to be a console port
+ */
+ if (!use_multiport(port->portdev)) {
+ err = init_port_console(port);
+ if (err)
+ goto free_inbufs;
+ }
+
+ spin_lock_irq(&portdev->ports_lock);
+ list_add_tail(&port->list, &port->portdev->ports);
+ spin_unlock_irq(&portdev->ports_lock);
+
+ /*
+ * Tell the Host we're set so that it can send us various
+ * configuration parameters for this port (eg, port name,
+ * caching, whether this is a console port, etc.)
+ */
+ send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+
+ if (pdrvdata.debugfs_dir) {
+ /*
+ * Finally, create the debugfs file that we can use to
+ * inspect a port's state at any time
+ */
+ sprintf(debugfs_name, "vport%up%u",
+ port->portdev->drv_index, id);
+ port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
+ pdrvdata.debugfs_dir,
+ port,
+ &port_debugfs_ops);
+ }
+ return 0;
+
+free_inbufs:
+ while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+ free_buf(buf);
+free_device:
+ device_destroy(pdrvdata.class, port->dev->devt);
+free_cdev:
+ cdev_del(&port->cdev);
+free_port:
+ kfree(port);
+fail:
+ return err;
+}
+
/* Remove all port-specific data. */
static int remove_port(struct port *port)
{
@@ -1100,139 +1231,6 @@ static void config_intr(struct virtio_device *vdev)
resize_console(find_port_by_id(portdev, 0));
}
-static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
-{
- struct port_buffer *buf;
- unsigned int nr_added_bufs;
- int ret;
-
- nr_added_bufs = 0;
- do {
- buf = alloc_buf(PAGE_SIZE);
- if (!buf)
- break;
-
- spin_lock_irq(lock);
- ret = add_inbuf(vq, buf);
- if (ret < 0) {
- spin_unlock_irq(lock);
- free_buf(buf);
- break;
- }
- nr_added_bufs++;
- spin_unlock_irq(lock);
- } while (ret > 0);
-
- return nr_added_bufs;
-}
-
-static int add_port(struct ports_device *portdev, u32 id)
-{
- char debugfs_name[16];
- struct port *port;
- struct port_buffer *buf;
- dev_t devt;
- unsigned int nr_added_bufs;
- int err;
-
- port = kmalloc(sizeof(*port), GFP_KERNEL);
- if (!port) {
- err = -ENOMEM;
- goto fail;
- }
-
- port->portdev = portdev;
- port->id = id;
-
- port->name = NULL;
- port->inbuf = NULL;
- port->cons.hvc = NULL;
-
- port->host_connected = port->guest_connected = false;
-
- port->in_vq = portdev->in_vqs[port->id];
- port->out_vq = portdev->out_vqs[port->id];
-
- cdev_init(&port->cdev, &port_fops);
-
- devt = MKDEV(portdev->chr_major, id);
- err = cdev_add(&port->cdev, devt, 1);
- if (err < 0) {
- dev_err(&port->portdev->vdev->dev,
- "Error %d adding cdev for port %u\n", err, id);
- goto free_port;
- }
- port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
- devt, port, "vport%up%u",
- port->portdev->drv_index, id);
- if (IS_ERR(port->dev)) {
- err = PTR_ERR(port->dev);
- dev_err(&port->portdev->vdev->dev,
- "Error %d creating device for port %u\n",
- err, id);
- goto free_cdev;
- }
-
- spin_lock_init(&port->inbuf_lock);
- init_waitqueue_head(&port->waitqueue);
-
- /* Fill the in_vq with buffers so the host can send us data. */
- nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
- if (!nr_added_bufs) {
- dev_err(port->dev, "Error allocating inbufs\n");
- err = -ENOMEM;
- goto free_device;
- }
-
- /*
- * If we're not using multiport support, this has to be a console port
- */
- if (!use_multiport(port->portdev)) {
- err = init_port_console(port);
- if (err)
- goto free_inbufs;
- }
-
- spin_lock_irq(&portdev->ports_lock);
- list_add_tail(&port->list, &port->portdev->ports);
- spin_unlock_irq(&portdev->ports_lock);
-
- /*
- * Tell the Host we're set so that it can send us various
- * configuration parameters for this port (eg, port name,
- * caching, whether this is a console port, etc.)
- */
- send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
-
- if (pdrvdata.debugfs_dir) {
- /*
- * Finally, create the debugfs file that we can use to
- * inspect a port's state at any time
- */
- sprintf(debugfs_name, "vport%up%u",
- port->portdev->drv_index, id);
- port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
- pdrvdata.debugfs_dir,
- port,
- &port_debugfs_ops);
- }
- return 0;
-
-free_inbufs:
- while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
- free_buf(buf);
-free_device:
- device_destroy(pdrvdata.class, port->dev->devt);
-free_cdev:
- cdev_del(&port->cdev);
-free_port:
- kfree(port);
-fail:
- /* The host might want to notify management sw about port add failure */
- send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 0);
- return err;
-}
-
static int init_vqs(struct ports_device *portdev)
{
vq_callback_t **io_callbacks;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 07/10] virtio: console: Use a control message to add ports
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-7-git-send-email-amit.shah@redhat.com>
Instead of the host and guest independently enumerating ports, switch to
a control message to add ports where the host supplies the port number
so there's no ambiguity or a possibility of a race between the host and
the guest port numbers.
We now no longer need the 'nr_ports' config value. Since no kernel has
been released with the MULTIPORT changes yet, we have a chance to fiddle
with the config space without adding compatibility features.
This is beneficial for management software, which would now be able to
instantiate ports at known locations and avoid problems that arise with
implicit numbering in the host and the guest. This removes the 'guessing
game' part of it, and management software can now actually indicate
which id to spawn a particular port on.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 78 +++++++++++++++++----------------------
include/linux/virtio_console.h | 17 ++++-----
2 files changed, 42 insertions(+), 53 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fd98861..7b2dcdd 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1053,7 +1053,7 @@ static void handle_control_message(struct ports_device *portdev,
cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
port = find_port_by_id(portdev, cpkt->id);
- if (!port) {
+ if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
/* No valid header at start of buffer. Drop it. */
dev_dbg(&portdev->vdev->dev,
"Invalid index %u in control packet\n", cpkt->id);
@@ -1061,6 +1061,30 @@ static void handle_control_message(struct ports_device *portdev,
}
switch (cpkt->event) {
+ case VIRTIO_CONSOLE_PORT_ADD:
+ if (port) {
+ /*
+ * This can happen for port 0: we have to
+ * create a console port during probe() as was
+ * the behaviour before the MULTIPORT feature.
+ * On a newer host, when the host tells us
+ * that a port 0 is available, we should just
+ * say we have the port all set up.
+ */
+ send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+ break;
+ }
+ if (cpkt->id >= portdev->config.max_nr_ports) {
+ dev_warn(&portdev->vdev->dev,
+ "Request for adding port with out-of-bound id %u, max. supported id: %u\n",
+ cpkt->id, portdev->config.max_nr_ports - 1);
+ break;
+ }
+ add_port(portdev, cpkt->id);
+ break;
+ case VIRTIO_CONSOLE_PORT_REMOVE:
+ remove_port(port);
+ break;
case VIRTIO_CONSOLE_CONSOLE_PORT:
if (!cpkt->value)
break;
@@ -1119,32 +1143,6 @@ static void handle_control_message(struct ports_device *portdev,
kobject_uevent(&port->dev->kobj, KOBJ_CHANGE);
}
break;
- case VIRTIO_CONSOLE_PORT_REMOVE:
- /*
- * Hot unplug the port. We don't decrement nr_ports
- * since we don't want to deal with extra complexities
- * of using the lowest-available port id: We can just
- * pick up the nr_ports number as the id and not have
- * userspace send it to us. This helps us in two
- * ways:
- *
- * - We don't need to have a 'port_id' field in the
- * config space when a port is hot-added. This is a
- * good thing as we might queue up multiple hotplug
- * requests issued in our workqueue.
- *
- * - Another way to deal with this would have been to
- * use a bitmap of the active ports and select the
- * lowest non-active port from that map. That
- * bloats the already tight config space and we
- * would end up artificially limiting the
- * max. number of ports to sizeof(bitmap). Right
- * now we can support 2^32 ports (as the port id is
- * stored in a u32 type).
- *
- */
- remove_port(port);
- break;
}
}
@@ -1352,7 +1350,6 @@ static const struct file_operations portdev_fops = {
static int __devinit virtcons_probe(struct virtio_device *vdev)
{
struct ports_device *portdev;
- u32 i;
int err;
bool multiport;
@@ -1381,29 +1378,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
}
multiport = false;
- portdev->config.nr_ports = 1;
portdev->config.max_nr_ports = 1;
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
multiport = true;
vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
vdev->config->get(vdev, offsetof(struct virtio_console_config,
- nr_ports),
- &portdev->config.nr_ports,
- sizeof(portdev->config.nr_ports));
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
max_nr_ports),
&portdev->config.max_nr_ports,
sizeof(portdev->config.max_nr_ports));
- if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
- dev_warn(&vdev->dev,
- "More ports (%u) specified than allowed (%u). Will init %u ports.",
- portdev->config.nr_ports,
- portdev->config.max_nr_ports,
- portdev->config.max_nr_ports);
-
- portdev->config.nr_ports = portdev->config.max_nr_ports;
- }
}
/* Let the Host know we support multiple ports.*/
@@ -1431,13 +1414,20 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
err = -ENOMEM;
goto free_vqs;
}
+
}
- for (i = 0; i < portdev->config.nr_ports; i++)
- add_port(portdev, i);
+ /*
+ * For backward compatibility: if we're running on an older
+ * host, we always want to create a console port.
+ */
+ add_port(portdev, 0);
/* Start using the new console output. */
early_put_chars = NULL;
+
+ __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+ VIRTIO_CONSOLE_DEVICE_READY, 1);
return 0;
free_vqs:
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 0157361..a85064d 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -23,8 +23,6 @@ struct virtio_console_config {
__u16 rows;
/* max. number of ports this device can hold */
__u32 max_nr_ports;
- /* number of ports added so far */
- __u32 nr_ports;
} __attribute__((packed));
/*
@@ -38,13 +36,14 @@ struct virtio_console_control {
};
/* Some events for control messages */
-#define VIRTIO_CONSOLE_PORT_READY 0
-#define VIRTIO_CONSOLE_CONSOLE_PORT 1
-#define VIRTIO_CONSOLE_RESIZE 2
-#define VIRTIO_CONSOLE_PORT_OPEN 3
-#define VIRTIO_CONSOLE_PORT_NAME 4
-#define VIRTIO_CONSOLE_PORT_REMOVE 5
-#define VIRTIO_CONSOLE_DEVICE_READY 6
+#define VIRTIO_CONSOLE_DEVICE_READY 0
+#define VIRTIO_CONSOLE_PORT_ADD 1
+#define VIRTIO_CONSOLE_PORT_REMOVE 2
+#define VIRTIO_CONSOLE_PORT_READY 3
+#define VIRTIO_CONSOLE_CONSOLE_PORT 4
+#define VIRTIO_CONSOLE_RESIZE 5
+#define VIRTIO_CONSOLE_PORT_OPEN 6
+#define VIRTIO_CONSOLE_PORT_NAME 7
#ifdef __KERNEL__
int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
--
1.6.2.5
^ permalink raw reply related
* [PATCH 08/10] virtio: console: Don't always create a port 0 if using multiport
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-8-git-send-email-amit.shah@redhat.com>
If we're using multiport, there's no point in always creating a console
port. Create the console port only if the host doesn't support
multiport.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 33 +++++++++++++++------------------
1 files changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7b2dcdd..3d70729 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -791,6 +791,13 @@ int init_port_console(struct port *port)
spin_unlock_irq(&pdrvdata_lock);
port->guest_connected = true;
+ /*
+ * Start using the new console output if this is the first
+ * console to come up.
+ */
+ if (early_put_chars)
+ early_put_chars = NULL;
+
/* Notify host of port being opened */
send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
@@ -1063,14 +1070,8 @@ static void handle_control_message(struct ports_device *portdev,
switch (cpkt->event) {
case VIRTIO_CONSOLE_PORT_ADD:
if (port) {
- /*
- * This can happen for port 0: we have to
- * create a console port during probe() as was
- * the behaviour before the MULTIPORT feature.
- * On a newer host, when the host tells us
- * that a port 0 is available, we should just
- * say we have the port all set up.
- */
+ dev_dbg(&portdev->vdev->dev,
+ "Port %u already added\n", port->id);
send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
break;
}
@@ -1414,18 +1415,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
err = -ENOMEM;
goto free_vqs;
}
-
+ } else {
+ /*
+ * If we're running on an older host, we always want
+ * to create a console port.
+ */
+ add_port(portdev, 0);
}
- /*
- * For backward compatibility: if we're running on an older
- * host, we always want to create a console port.
- */
- add_port(portdev, 0);
-
- /* Start using the new console output. */
- early_put_chars = NULL;
-
__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
VIRTIO_CONSOLE_DEVICE_READY, 1);
return 0;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 09/10] virtio: console: Rename wait_is_over() to will_read_block()
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-9-git-send-email-amit.shah@redhat.com>
We'll introduce a function that checks if write will block. Have
function names that are similar for the two cases.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3d70729..680ea25 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -486,9 +486,9 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count,
}
/* The condition that must be true for polling to end */
-static bool wait_is_over(struct port *port)
+static bool will_read_block(struct port *port)
{
- return port_has_data(port) || !port->host_connected;
+ return !port_has_data(port) && port->host_connected;
}
static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
@@ -511,7 +511,7 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
return -EAGAIN;
ret = wait_event_interruptible(port->waitqueue,
- wait_is_over(port));
+ !will_read_block(port));
if (ret < 0)
return ret;
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 10/10] virtio: console: Add support for nonblocking write()s
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-10-git-send-email-amit.shah@redhat.com>
If the host port is not open, a write() should either just return if the
file is opened in non-blocking mode, or block till the host port is
opened.
Also, don't spin till host consumes data for nonblocking ports. For
non-blocking ports, we can do away with the spinning and reclaim the
buffers consumed by the host on the next write call or on the condition
that'll make poll return.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 82 +++++++++++++++++++++++++++++++++++++++--
1 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 680ea25..d70efda 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -183,6 +183,8 @@ struct port {
/* The 'id' to identify the port with the Host */
u32 id;
+ bool outvq_full;
+
/* Is the host device open */
bool host_connected;
@@ -404,7 +406,19 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
return __send_control_msg(port->portdev, port->id, event, value);
}
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
+static void reclaim_consumed_buffers(struct port *port)
+{
+ void *buf;
+ unsigned int len;
+
+ while ((buf = port->out_vq->vq_ops->get_buf(port->out_vq, &len))) {
+ kfree(buf);
+ port->outvq_full = false;
+ }
+}
+
+static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
+ bool nonblock)
{
struct scatterlist sg[1];
struct virtqueue *out_vq;
@@ -413,6 +427,8 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
out_vq = port->out_vq;
+ reclaim_consumed_buffers(port);
+
sg_init_one(sg, in_buf, in_count);
ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, in_buf);
@@ -424,6 +440,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
goto fail;
}
+ if (ret == 0)
+ port->outvq_full = true;
+
+ if (nonblock)
+ return in_count;
+
/*
* Wait till the host acknowledges it pushed out the data we
* sent. This is done for ports in blocking mode or for data
@@ -491,6 +513,20 @@ static bool will_read_block(struct port *port)
return !port_has_data(port) && port->host_connected;
}
+static bool will_write_block(struct port *port)
+{
+ if (!port->host_connected)
+ return true;
+
+ /*
+ * Check if the Host has consumed any buffers since we last
+ * sent data (this is only applicable for nonblocking ports).
+ */
+ reclaim_consumed_buffers(port);
+
+ return port->outvq_full;
+}
+
static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
size_t count, loff_t *offp)
{
@@ -537,9 +573,22 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
struct port *port;
char *buf;
ssize_t ret;
+ bool nonblock;
port = filp->private_data;
+ nonblock = filp->f_flags & O_NONBLOCK;
+
+ if (will_write_block(port)) {
+ if (nonblock)
+ return -EAGAIN;
+
+ ret = wait_event_interruptible(port->waitqueue,
+ !will_write_block(port));
+ if (ret < 0)
+ return ret;
+ }
+
count = min((size_t)(32 * 1024), count);
buf = kmalloc(count, GFP_KERNEL);
@@ -552,9 +601,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
goto free_buf;
}
- ret = send_buf(port, buf, count);
+ ret = send_buf(port, buf, count, nonblock);
+
+ if (nonblock && ret > 0)
+ goto out;
+
free_buf:
kfree(buf);
+out:
return ret;
}
@@ -569,7 +623,7 @@ static unsigned int port_fops_poll(struct file *filp, poll_table *wait)
ret = 0;
if (port->inbuf)
ret |= POLLIN | POLLRDNORM;
- if (port->host_connected)
+ if (!will_write_block(port))
ret |= POLLOUT;
if (!port->host_connected)
ret |= POLLHUP;
@@ -592,6 +646,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
discard_port_data(port);
spin_unlock_irq(&port->inbuf_lock);
+ reclaim_consumed_buffers(port);
return 0;
}
@@ -621,6 +676,13 @@ static int port_fops_open(struct inode *inode, struct file *filp)
port->guest_connected = true;
spin_unlock_irq(&port->inbuf_lock);
+ /*
+ * There might be a chance that we missed reclaiming a few
+ * buffers in the window of the port getting previously closed
+ * and opening now.
+ */
+ reclaim_consumed_buffers(port);
+
/* Notify host of port being opened */
send_control_msg(filp->private_data, VIRTIO_CONSOLE_PORT_OPEN, 1);
@@ -661,7 +723,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
if (!port)
return -EPIPE;
- return send_buf(port, (void *)buf, count);
+ return send_buf(port, (void *)buf, count, false);
}
/*
@@ -853,6 +915,8 @@ static ssize_t debugfs_read(struct file *filp, char __user *ubuf,
out_offset += snprintf(buf + out_offset, out_count - out_offset,
"host_connected: %d\n", port->host_connected);
out_offset += snprintf(buf + out_offset, out_count - out_offset,
+ "outvq_full: %d\n", port->outvq_full);
+ out_offset += snprintf(buf + out_offset, out_count - out_offset,
"is_console: %s\n",
is_console_port(port) ? "yes" : "no");
out_offset += snprintf(buf + out_offset, out_count - out_offset,
@@ -919,6 +983,8 @@ static int add_port(struct ports_device *portdev, u32 id)
port->host_connected = port->guest_connected = false;
+ port->outvq_full = false;
+
port->in_vq = portdev->in_vqs[port->id];
port->out_vq = portdev->out_vqs[port->id];
@@ -1036,6 +1102,8 @@ static int remove_port(struct port *port)
/* Remove unused data this port might have received. */
discard_port_data(port);
+ reclaim_consumed_buffers(port);
+
/* Remove buffers we queued up for the Host to send us data in. */
while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
free_buf(buf);
@@ -1107,6 +1175,12 @@ static void handle_control_message(struct ports_device *portdev,
case VIRTIO_CONSOLE_PORT_OPEN:
port->host_connected = cpkt->value;
wake_up_interruptible(&port->waitqueue);
+ /*
+ * If the host port got closed and the host had any
+ * unconsumed buffers, we'll be able to reclaim them
+ * now.
+ */
+ reclaim_unconsumed_buffers(port);
break;
case VIRTIO_CONSOLE_PORT_NAME:
/*
--
1.6.2.5
^ permalink raw reply related
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