* inconsistent lock state
From: Sergey Senozhatsky @ 2010-03-07 10:21 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, Ingo Molnar
[-- Attachment #1: Type: text/plain, Size: 5293 bytes --]
Hello,
Hardly reproducible.
/*
* 2.6.33. x86. ASUS f3jc
*/
[329645.010697] =================================
[329645.010699] [ INFO: inconsistent lock state ]
[329645.010703] 2.6.33-33-0-dbg #31
[329645.010705] ---------------------------------
[329645.010708] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[329645.010712] events/0/9 [HC0[0]:SC0[0]:HE1:SE1] takes:
[329645.010715] (&(&table->hash[i].lock)->rlock){+.?.-.}, at: [<c12c1892>] spin_lock+0x8/0xa
[329645.010729] {IN-SOFTIRQ-W} state was registered at:
[329645.010732] [<c105878f>] __lock_acquire+0x27e/0xb86
[329645.010739] [<c1059988>] lock_acquire+0xa1/0xb8
[329645.010744] [<c12f39b2>] _raw_spin_lock+0x28/0x58
[329645.010750] [<c12c1892>] spin_lock+0x8/0xa
[329645.010755] [<c12c2c8f>] T.958+0x3c/0x141
[329645.010759] [<c12c2f7a>] __udp4_lib_rcv+0x1e6/0x3cd
[329645.010764] [<c12c3173>] udp_rcv+0x12/0x14
[329645.010769] [<c12a559b>] ip_local_deliver_finish+0xc9/0x130
[329645.010774] [<c12a5663>] ip_local_deliver+0x61/0x66
[329645.010778] [<c12a51a5>] ip_rcv_finish+0x275/0x29d
[329645.010783] [<c12a539a>] ip_rcv+0x1cd/0x1ed
[329645.010786] [<c128858d>] netif_receive_skb+0x340/0x360
[329645.010791] [<fd1ab6f4>] rtl8169_rx_interrupt+0x2bf/0x37e [r8169]
[329645.010801] [<fd1adab8>] rtl8169_poll+0x29/0x15a [r8169]
[329645.010808] [<c1288b8c>] net_rx_action+0x95/0x1af
[329645.010812] [<c1037243>] __do_softirq+0xc6/0x187
[329645.010819] [<c103732f>] do_softirq+0x2b/0x43
[329645.010823] [<c10374bf>] irq_exit+0x38/0x75
[329645.010828] [<c1004116>] do_IRQ+0x88/0x9c
[329645.010833] [<c1002f35>] common_interrupt+0x35/0x3c
[329645.010837] [<c1275d0b>] cpuidle_idle_call+0x72/0xd3
[329645.010844] [<c1001c1b>] cpu_idle+0x92/0xbf
[329645.010848] [<c12e3f16>] rest_init+0x76/0x78
[329645.010853] [<c14c2868>] start_kernel+0x33c/0x341
[329645.010859] [<c14c2092>] i386_start_kernel+0x92/0x99
[329645.010864] irq event stamp: 157782307
[329645.010866] hardirqs last enabled at (157782307): [<c10b74b8>] kmem_cache_free+0x97/0xd6
[329645.010873] hardirqs last disabled at (157782306): [<c10b745a>] kmem_cache_free+0x39/0xd6
[329645.010878] softirqs last enabled at (157782304): [<c12928ef>] rcu_read_unlock_bh+0x1c/0x1e
[329645.010885] softirqs last disabled at (157782302): [<c129289b>] rcu_read_lock_bh+0x8/0x26
[329645.010892]
[329645.010893] other info that might help us debug this:
[329645.010896] 5 locks held by events/0/9:
[329645.010898] #0: (events){+.+.+.}, at: [<c1044f2f>] worker_thread+0x16a/0x27c
[329645.010908] #1: ((&(&tp->task)->work)){+.+...}, at: [<c1044f2f>] worker_thread+0x16a/0x27c
[329645.010915] #2: (rtnl_mutex){+.+.+.}, at: [<c12911dd>] rtnl_lock+0xf/0x11
[329645.010922] #3: (rcu_read_lock){.+.+..}, at: [<c1286a01>] rcu_read_lock+0x0/0x2b
[329645.010931] #4: (rcu_read_lock){.+.+..}, at: [<c12a4ed0>] rcu_read_lock+0x0/0x2b
[329645.010938]
[329645.010939] stack backtrace:
[329645.010942] Pid: 9, comm: events/0 Not tainted 2.6.33-33-0-dbg #31
[329645.010945] Call Trace:
[329645.010950] [<c12f18ec>] ? printk+0xf/0x11
[329645.010955] [<c1057800>] valid_state+0x12a/0x13d
[329645.010960] [<c1057904>] mark_lock+0xf1/0x1e2
[329645.010965] [<c1057ffa>] ? check_usage_backwards+0x0/0x6f
[329645.010970] [<c10587fd>] __lock_acquire+0x2ec/0xb86
[329645.010976] [<c100864f>] ? native_sched_clock+0x48/0x8d
[329645.010982] [<c104d24b>] ? sched_clock_local+0x17/0x11e
[329645.010987] [<c12c1892>] ? spin_lock+0x8/0xa
[329645.010992] [<c1059988>] lock_acquire+0xa1/0xb8
[329645.010997] [<c12c1892>] ? spin_lock+0x8/0xa
[329645.011002] [<c12f39b2>] _raw_spin_lock+0x28/0x58
[329645.011006] [<c12c1892>] ? spin_lock+0x8/0xa
[329645.011010] [<c12c1892>] spin_lock+0x8/0xa
[329645.011015] [<c12c2c8f>] T.958+0x3c/0x141
[329645.011020] [<c104d472>] ? sched_clock_cpu+0x120/0x128
[329645.011025] [<c100864f>] ? native_sched_clock+0x48/0x8d
[329645.011031] [<c1059088>] ? __lock_acquire+0xb77/0xb86
[329645.011037] [<c12a1345>] ? rcu_read_unlock+0x0/0x35
[329645.011042] [<c12a38c0>] ? ip_route_input+0x102/0xacb
[329645.011046] [<c1057831>] ? mark_lock+0x1e/0x1e2
[329645.011051] [<c12a4ed0>] ? rcu_read_lock+0x0/0x2b
[329645.011056] [<c12c2f7a>] __udp4_lib_rcv+0x1e6/0x3cd
[329645.011061] [<c12c3173>] udp_rcv+0x12/0x14
[329645.011065] [<c12a559b>] ip_local_deliver_finish+0xc9/0x130
[329645.011070] [<c12a5663>] ip_local_deliver+0x61/0x66
[329645.011074] [<c12a51a5>] ip_rcv_finish+0x275/0x29d
[329645.011078] [<c12a539a>] ip_rcv+0x1cd/0x1ed
[329645.011083] [<c128858d>] netif_receive_skb+0x340/0x360
[329645.011093] [<fd1ab6f4>] rtl8169_rx_interrupt+0x2bf/0x37e [r8169]
[329645.011100] [<fd1aba02>] rtl8169_reset_task+0x38/0xcd [r8169]
[329645.011105] [<c1044f71>] worker_thread+0x1ac/0x27c
[329645.011110] [<c1044f2f>] ? worker_thread+0x16a/0x27c
[329645.011116] [<fd1ab9ca>] ? rtl8169_reset_task+0x0/0xcd [r8169]
[329645.011123] [<c1048725>] ? autoremove_wake_function+0x0/0x2f
[329645.011128] [<c1044dc5>] ? worker_thread+0x0/0x27c
[329645.011132] [<c104838a>] kthread+0x6a/0x6f
[329645.011137] [<c1048320>] ? kthread+0x0/0x6f
[329645.011142] [<c1002f42>] kernel_thread_helper+0x6/0x1a
Sergey
[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]
^ permalink raw reply
* Re: [PATCH v1 0/3] Provide a zero-copy method on KVM virtio-net.
From: Michael S. Tsirkin @ 2010-03-07 10:50 UTC (permalink / raw)
To: xiaohui.xin; +Cc: netdev, kvm, linux-kernel, mingo, jdike
In-Reply-To: <1267868318-19268-1-git-send-email-xiaohui.xin@intel.com>
On Sat, Mar 06, 2010 at 05:38:35PM +0800, xiaohui.xin@intel.com wrote:
> The idea is simple, just to pin the guest VM user space and then
> let host NIC driver has the chance to directly DMA to it.
> The patches are based on vhost-net backend driver. We add a device
> which provides proto_ops as sendmsg/recvmsg to vhost-net to
> send/recv directly to/from the NIC driver. KVM guest who use the
> vhost-net backend may bind any ethX interface in the host side to
> get copyless data transfer thru guest virtio-net frontend.
>
> We provide multiple submits and asynchronous notifiicaton to
> vhost-net too.
>
> Our goal is to improve the bandwidth and reduce the CPU usage.
> Exact performance data will be provided later. But for simple
> test with netperf, we found bindwidth up and CPU % up too,
> but the bindwidth up ratio is much more than CPU % up ratio.
>
> What we have not done yet:
> packet split support
> To support GRO
> Performance tuning
Am I right to say that nic driver needs changes for these patches
to work? If so, please publish nic driver patches as well.
> what we have done in v1:
> polish the RCU usage
> deal with write logging in asynchroush mode in vhost
> add notifier block for mp device
> rename page_ctor to mp_port in netdevice.h to make it looks generic
> add mp_dev_change_flags() for mp device to change NIC state
> add CONIFG_VHOST_MPASSTHRU to limit the usage when module is not load
> a small fix for missing dev_put when fail
> using dynamic minor instead of static minor number
> a __KERNEL__ protect to mp_get_sock()
>
> performance:
> using netperf with GSO/TSO disabled, 10G NIC,
> disabled packet split mode, with raw socket case compared to vhost.
>
> bindwidth will be from 1.1Gbps to 1.7Gbps
> CPU % from 120%-140% to 140%-160%
That's pretty low for a 10Gb nic. Are you hitting some other bottleneck,
like high interrupt rate? Also, GSO support and performance tuning
for raw are incomplete. Try comparing with e.g. tap with GSO.
--
MST
^ permalink raw reply
* [PATCH 0/3] cpmac fixes
From: Florian Fainelli @ 2010-03-07 10:55 UTC (permalink / raw)
To: netdev, David S. Miller
Hi David,
Here are 2 small cpmac fixes and a driver version bumping. Thanks!
Florian Fainelli (3):
cpmac: fix the receiving of 802.1q frames
cpmac: fallback to switch mode if no PHY chip found
cpmac: bump version to 0.5.2
^ permalink raw reply
* [PATCH 1/3] cpmac: fix the receiving of 802.1q frames
From: Florian Fainelli @ 2010-03-07 10:55 UTC (permalink / raw)
To: netdev, David S. Miller
Despite what the comment above CPMAC_SKB_SIZE says, the hardware also
needs to account for the FCS length in a received frame. This patch fix
the receiving of 802.1q frames which have 4 more bytes. While at it
unhardcode the definition and use the one from if_vlan.h.
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/cpmac.c b/drivers/net/cpmac.c
index b85c81f..722a196 100644
--- a/drivers/net/cpmac.c
+++ b/drivers/net/cpmac.c
@@ -28,6 +28,7 @@
#include <linux/delay.h>
#include <linux/netdevice.h>
+#include <linux/if_vlan.h>
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
#include <linux/skbuff.h>
@@ -56,8 +57,8 @@ MODULE_PARM_DESC(debug_level, "Number of NETIF_MSG bits to enable");
MODULE_PARM_DESC(dumb_switch, "Assume switch is not connected to MDIO bus");
#define CPMAC_VERSION "0.5.1"
-/* frame size + 802.1q tag */
-#define CPMAC_SKB_SIZE (ETH_FRAME_LEN + 4)
+/* frame size + 802.1q tag + FCS size */
+#define CPMAC_SKB_SIZE (ETH_FRAME_LEN + ETH_FCS_LEN + VLAN_HLEN)
#define CPMAC_QUEUES 8
/* Ethernet registers */
^ permalink raw reply related
* [PATCH 2/3] cpmac: fallback to switch mode if no PHY chip found
From: Florian Fainelli @ 2010-03-07 10:55 UTC (permalink / raw)
To: netdev, David S. Miller
If we were unable to detect a PHY on any of the MDIO bus id we tried instead of
bailing out with -ENODEV, assume the MAC is connected to a switch and use MDIO
bus 0. This unbreaks quite a lot of devices out there whose switch cannot be
detected.
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/cpmac.c b/drivers/net/cpmac.c
index 722a196..4051e81 100644
--- a/drivers/net/cpmac.c
+++ b/drivers/net/cpmac.c
@@ -1137,8 +1137,9 @@ static int __devinit cpmac_probe(struct platform_device *pdev)
}
if (phy_id == PHY_MAX_ADDR) {
- dev_err(&pdev->dev, "no PHY present\n");
- return -ENODEV;
+ dev_err(&pdev->dev, "no PHY present, falling back to switch on MDIO bus 0\n");
+ strncpy(mdio_bus_id, "0", MII_BUS_ID_SIZE); /* fixed phys bus */
+ phy_id = pdev->id;
}
dev = alloc_etherdev_mq(sizeof(*priv), CPMAC_QUEUES);
^ permalink raw reply related
* [PATCH 3/3] cpmac: bump version to 0.5.2
From: Florian Fainelli @ 2010-03-07 10:55 UTC (permalink / raw)
To: netdev, David S. Miller
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/cpmac.c b/drivers/net/cpmac.c
index 4051e81..97b8890 100644
--- a/drivers/net/cpmac.c
+++ b/drivers/net/cpmac.c
@@ -56,7 +56,7 @@ module_param(dumb_switch, int, 0444);
MODULE_PARM_DESC(debug_level, "Number of NETIF_MSG bits to enable");
MODULE_PARM_DESC(dumb_switch, "Assume switch is not connected to MDIO bus");
-#define CPMAC_VERSION "0.5.1"
+#define CPMAC_VERSION "0.5.2"
/* frame size + 802.1q tag + FCS size */
#define CPMAC_SKB_SIZE (ETH_FRAME_LEN + ETH_FCS_LEN + VLAN_HLEN)
#define CPMAC_QUEUES 8
^ permalink raw reply related
* Re: [PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
From: Michael S. Tsirkin @ 2010-03-07 11:18 UTC (permalink / raw)
To: xiaohui.xin; +Cc: netdev, kvm, linux-kernel, mingo, jdike
In-Reply-To: <1267868318-19268-3-git-send-email-xiaohui.xin@intel.com>
> +/* The structure to notify the virtqueue for async socket */
> +struct vhost_notifier {
> + struct list_head list;
> + struct vhost_virtqueue *vq;
> + int head;
> + int size;
> + int log;
> + void *ctrl;
> + void (*dtor)(struct vhost_notifier *);
> +};
> +
So IMO, this is not the best interface between vhost
and your driver, exposing them to each other unnecessarily.
If you think about it, your driver should not care about this structure.
It could get e.g. a kiocb (sendmsg already gets one), and call ki_dtor
on completion. vhost could save it's state in ki_user_data. If your
driver needs to add more data to do more tracking, I think it can put
skb pointer in the private pointer.
--
MST
^ permalink raw reply
* [patch] irda-usb: add error handling and fix leak
From: Dan Carpenter @ 2010-03-07 12:35 UTC (permalink / raw)
To: Samuel Ortiz
Cc: David S. Miller, Stephen Hemminger, Patrick McHardy, Tadashi Abe,
Julia Lawall, netdev, kernel-janitors
If the call to kcalloc() fails then we should return -ENOMEM.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/net/irda/irda-usb.c b/drivers/net/irda/irda-usb.c
index e8e33bb..2c9b3af 100644
--- a/drivers/net/irda/irda-usb.c
+++ b/drivers/net/irda/irda-usb.c
@@ -1651,6 +1651,8 @@ static int irda_usb_probe(struct usb_interface *intf,
self->rx_urb = kcalloc(self->max_rx_urb, sizeof(struct urb *),
GFP_KERNEL);
+ if (!self->rx_urb)
+ goto err_free_net;
for (i = 0; i < self->max_rx_urb; i++) {
self->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
@@ -1783,6 +1785,8 @@ err_out_2:
err_out_1:
for (i = 0; i < self->max_rx_urb; i++)
usb_free_urb(self->rx_urb[i]);
+ kfree(self->rx_urb);
+err_free_net:
free_netdev(net);
err_out:
return ret;
^ permalink raw reply related
* Re: [RFC][ PATCH 1/3] vhost-net: support multiple buffer heads in receiver
From: Michael S. Tsirkin @ 2010-03-07 15:31 UTC (permalink / raw)
To: David Stevens; +Cc: rusty, netdev, kvm, virtualization
In-Reply-To: <OF201C1B9E.C89ADF9F-ON882576DB.00006AD7-882576DB.0001DA4E@us.ibm.com>
On Tue, Mar 02, 2010 at 05:20:15PM -0700, David Stevens wrote:
> This patch generalizes buffer handling functions to
> support multiple buffer heads.
>
> In-line for viewing, attached for applying.
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
> diff -ruN net-next-p0/drivers/vhost/net.c net-next-p1/drivers/vhost/net.c
> --- net-next-p0/drivers/vhost/net.c 2010-02-24 12:59:24.000000000
> -0800
> +++ net-next-p1/drivers/vhost/net.c 2010-03-01 11:44:22.000000000
> -0800
> @@ -97,7 +97,8 @@
> 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, s;
> + struct iovec head;
> struct msghdr msg = {
> .msg_name = NULL,
> .msg_namelen = 0,
> @@ -126,12 +127,10 @@
> 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);
Should type for head be changed so that we do not need to cast?
I also prefer aligning descendants on the opening (.
> /* 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);
> @@ -152,7 +151,7 @@
> /* 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: "
> @@ -163,14 +162,14 @@
> /* 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);
> + vhost_discard(vq, 1);
Isn't the original name a bit more descriptive?
> tx_poll_start(net, sock);
> 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);
> + vhost_add_used_and_signal(&net->dev, vq, &head, 1);
> total_len += len;
> if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> vhost_poll_queue(&vq->poll);
> @@ -182,12 +181,22 @@
> unuse_mm(net->dev.mm);
> }
>
> +static int skb_head_len(struct sk_buff_head *skq)
> +{
> + struct sk_buff *head;
> +
> + head = skb_peek(skq);
> + if (head)
> + return head->len;
> + return 0;
> +}
> +
This is done without locking, which I think can crash
if skb is consumed after we peek it but before we read the
length.
> /* 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, s;
> struct vhost_log *vq_log;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -204,10 +213,11 @@
> };
>
> size_t len, total_len = 0;
> - int err;
> + int err, headcount, datalen;
> size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
> - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> +
> + if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> return;
>
Isn't this equivalent? Do you expect zero len skbs in socket?
If yes, maybe we should discard these, not stop processing ...
> use_mm(net->dev.mm);
> @@ -218,13 +228,10 @@
> 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 = skb_head_len(&sock->sk->sk_receive_queue))) {
This peeks in the queue to figure out how much data we have.
It's a neat trick, but it does introduce new failure modes
where an skb is consumed after we did the peek:
- new skb could be shorter, we should return the unconsumed part
- new skb could be longer, this will drop a packet.
maybe this last is not an issue as the race should be rare in practice
> + headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> &log);
> /* OK, now we need to know about added descriptors. */
> - if (head == vq->num) {
> + if (!headcount) {
> if (unlikely(vhost_enable_notify(vq))) {
> /* They have slipped one in as we were
> * doing that: check again. */
> @@ -235,13 +242,6 @@
> * they refilled. */
> break;
> }
> - /* We don't need to be notified again. */
you find this comment unhelpful?
> - if (out) {
> - vq_err(vq, "Unexpected descriptor format for RX: "
> - "out %d, int %d\n",
> - out, in);
> - break;
> - }
we still need this check, don't we?
> /* Skip header. TODO: support TSO/mergeable rx buffers. */
> s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> msg.msg_iovlen = in;
> @@ -257,14 +257,14 @@
> 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, 1);
> break;
> }
> /* TODO: Should check and handle checksum. */
> if (err > len) {
> pr_err("Discarded truncated rx packet: "
> " len %d > %zd\n", err, len);
> - vhost_discard_vq_desc(vq);
> + vhost_discard(vq, 1);
> continue;
> }
> len = err;
> @@ -275,7 +275,7 @@
> 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);
> if (unlikely(vq_log))
> vhost_log_write(vq, vq_log, log, len);
> total_len += len;
> diff -ruN net-next-p0/drivers/vhost/vhost.c
> net-next-p1/drivers/vhost/vhost.c
> --- net-next-p0/drivers/vhost/vhost.c 2010-02-15 20:08:35.000000000
> -0800
> +++ net-next-p1/drivers/vhost/vhost.c 2010-03-01 11:44:06.000000000
> -0800
> @@ -848,6 +848,38 @@
> return 0;
> }
>
> +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> *iovcount,
> + struct vhost_log *log, unsigned int *log_num)
Could you please document this function's parameters? It's not obvious
what iovcount, log, log_num are.
> +{
> + struct iovec *heads = vq->heads;
> + int out, in;
> + int hc = 0;
> +
> + while (datalen > 0) {
> + if (hc >= VHOST_NET_MAX_SG) {
> + vhost_discard(vq, hc);
> + return 0;
> + }
> + heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev,
> vq,
> + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> log_num);
> + if (heads[hc].iov_base == (void *)vq->num) {
> + vhost_discard(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, hc);
> + return 0;
> + }
> + heads[hc].iov_len = iov_length(vq->iov, in);
> + hc++;
> + datalen -= heads[hc].iov_len;
> + }
> + *iovcount = in;
Only the last value?
> + 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
> @@ -973,31 +1005,32 @@
> }
>
> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
> -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> +void vhost_discard(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;
> + 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;
> + for (i=0; i<count; i++, vq->last_used_idx++) {
whitespace damage: I prefer space around =, <.
I also use ++i, etc in this driver, better be consistent?
Also for clarity, I prefer to put vq->last_used_idx inside the loop.
> + 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;
> + }
If this fails, last_used_idx will still be incremented, which I think is wrong.
> }
> /* 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)) {
here, too
> vq_err(vq, "Failed to increment used idx");
> return -EFAULT;
> }
> @@ -1011,7 +1044,6 @@
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> - vq->last_used_idx++;
> return 0;
> }
>
> @@ -1038,9 +1070,9 @@
> /* 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)
> {
> - vhost_add_used(vq, head, len);
> + vhost_add_used(vq, heads, count);
> vhost_signal(dev, vq);
> }
>
> diff -ruN net-next-p0/drivers/vhost/vhost.h
> net-next-p1/drivers/vhost/vhost.h
> --- net-next-p0/drivers/vhost/vhost.h 2010-02-15 20:08:35.000000000
> -0800
> +++ net-next-p1/drivers/vhost/vhost.h 2010-03-01 11:42:18.000000000
> -0800
> @@ -84,6 +84,7 @@
> struct iovec indirect[VHOST_NET_MAX_SG];
> struct iovec iov[VHOST_NET_MAX_SG];
> struct iovec hdr[VHOST_NET_MAX_SG];
> + struct iovec heads[VHOST_NET_MAX_SG];
> size_t hdr_size;
> /* We use a kind of RCU to access private pointer.
> * All readers access it from workqueue, which makes it possible
> to
> @@ -120,16 +121,18 @@
> 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(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);
> +void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> void vhost_disable_notify(struct vhost_virtqueue *);
> bool vhost_enable_notify(struct vhost_virtqueue *);
>
^ permalink raw reply
* Re: 2.6.33 dies on modprobe
From: Américo Wang @ 2010-03-07 15:41 UTC (permalink / raw)
To: M G Berberich
Cc: Américo Wang, linux-kernel, Linux Kernel Network Developers,
Greg Kroah-Hartman
In-Reply-To: <20100304203803.GA3267@invalid>
On Thu, Mar 04, 2010 at 09:38:03PM +0100, M G Berberich wrote:
>Am Donnerstag, den 04. März schrieb Américo Wang:
>> On Thu, Mar 04, 2010 at 12:04:38AM +0100, M G Berberich wrote:
>> >Am Mittwoch, den 03. März schrieb Américo Wang:
>> >> On Wed, Mar 3, 2010 at 5:08 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> >> > On Wed, Mar 3, 2010 at 4:58 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> >> >> Ok, below is my patch, I am not sure it could fix the BUG for you,
>> >> >> but it could fix the WARNING. But perhaps they are related.
>> >> >>
>> >> >> Please give it a try.
>> >> >>
>> >> >
>> >> > Oops! Ignore the patch, it should not be correct, I will send a
>> >> > correct version soon.
>> >> > Sorry.
>> >> >
>> >>
>> >> Here we go:
>> >
>> >Seems to fix the warning, but not the modprobe-Oops.
>> >
>>
>> Thanks much for your testing!
>>
>> Could you please try to turn on DEBUG_KOBJECT and boot your kernel
>> with "initcall_debug"? And then show us your dmesg.
>
>If I'm doing this (enabled CONFIG_DEBUG_KERNEL and
>CONFIG_DEBUG_KOBJECT), the kernel dies immediately - no output.
>
Ouch!
CONFIG_DEBUG_KOBJECT will only enable some printk's, so I think
probably it's CONFIG_DEBUG_KERNEL which breaks your booting.
Could you please provide your .config?
Thanks!
^ permalink raw reply
* Re: [RFC][ PATCH 2/3] vhost-net: handle vnet_hdr processing for MRG_RX_BUF
From: Michael S. Tsirkin @ 2010-03-07 16:12 UTC (permalink / raw)
To: David Stevens; +Cc: rusty, netdev, kvm, virtualization
In-Reply-To: <OF9CBCD776.645508E4-ON882576DB.0000CB6E-882576DB.0001DED1@us.ibm.com>
On Tue, Mar 02, 2010 at 05:20:26PM -0700, David Stevens wrote:
> This patch adds vnet_hdr processing for mergeable buffer
> support to vhost-net.
>
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
>
> diff -ruN net-next-p1/drivers/vhost/net.c net-next-p2/drivers/vhost/net.c
Could you please add -p to diff flags so that it's easier
to figure out which function is changes?
> --- net-next-p1/drivers/vhost/net.c 2010-03-01 11:44:22.000000000
> -0800
> +++ net-next-p2/drivers/vhost/net.c 2010-03-02 13:01:34.000000000
> -0800
> @@ -109,7 +109,6 @@
> };
> size_t len, total_len = 0;
> int err, wmem;
> - size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
> if (!sock)
> return;
> @@ -124,7 +123,6 @@
>
> if (wmem < sock->sk->sk_sndbuf * 2)
> tx_poll_stop(net);
> - hdr_size = vq->hdr_size;
>
> for (;;) {
> head.iov_base = (void *)vhost_get_vq_desc(&net->dev, vq,
> @@ -148,25 +146,45 @@
> "out %d, int %d\n", out, in);
> break;
> }
> + if (vq->guest_hlen > vq->sock_hlen) {
> + if (msg.msg_iov[0].iov_len == vq->guest_hlen)
> + msg.msg_iov[0].iov_len = vq->sock_hlen;
> + else if (out == ARRAY_SIZE(vq->iov))
> + vq_err(vq, "handle_tx iov overflow!");
> + else {
> + int i;
> +
> + /* give header its own iov */
> + for (i=out; i>0; ++i)
> + msg.msg_iov[i+1] = msg.msg_iov[i];
> + msg.msg_iov[0].iov_len = vq->sock_hlen;
> + msg.msg_iov[1].iov_base += vq->guest_hlen;
> + msg.msg_iov[1].iov_len -= vq->guest_hlen;
> + out++;
We seem to spend a fair bit of code here and elsewhere trying to cover
the diff between header size in guest and host. In hindsight, it was
not a good idea to put new padding between data and the header:
virtio-net should have added it *before*. But it is what it is.
Wouldn't it be easier to just add an ioctl to tap so that
vnet header size is made bigger by 4 bytes?
> + }
> + }
> /* Skip header. TODO: support TSO. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
> msg.msg_iovlen = 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);
> + len, vq->guest_hlen);
> 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, 1);
> - tx_poll_start(net, sock);
> + if (err == -EAGAIN) {
The comment mentions ENOBUFS. Are you sure it's EAGAIN?
> + tx_poll_start(net, sock);
Don't we need to call discard to move the last avail header back?
> + } 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);
> + }
> break;
> - }
> - if (err != len)
> + } else if (err != len)
Previous if ends with break so no need for else here.
> pr_err("Truncated TX packet: "
> " len %d != %zd\n", err, len);
> vhost_add_used_and_signal(&net->dev, vq, &head, 1);
> @@ -207,14 +225,8 @@
> .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, headcount, datalen;
> - size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
>
> if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> @@ -223,7 +235,6 @@
> 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;
> @@ -232,25 +243,18 @@
> headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> &log);
> /* OK, now we need to know about added descriptors. */
> if (!headcount) {
> - if (unlikely(vhost_enable_notify(vq))) {
> - /* They have slipped one in as we were
> - * doing that: check again. */
> - vhost_disable_notify(vq);
> - continue;
> - }
> - /* Nothing new? Wait for eventfd to tell us
> - * they refilled. */
> + vhost_enable_notify(vq);
You don't think this race can happen?
> break;
> }
> /* Skip header. TODO: support TSO/mergeable rx buffers. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> 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);
> + len, vq->guest_hlen);
> break;
> }
> err = sock->ops->recvmsg(NULL, sock, &msg,
> @@ -268,13 +272,7 @@
> 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;
> + len += vq->guest_hlen - vq->sock_hlen;
> vhost_add_used_and_signal(&net->dev, vq, vq->heads,
> headcount);
> if (unlikely(vq_log))
> vhost_log_write(vq, vq_log, log, len);
> @@ -483,6 +481,13 @@
> return ERR_PTR(-ENOTSOCK);
> }
>
> +static int vhost_sock_is_raw(struct socket *sock)
> +{
> + if (!sock || !sock->sk)
> + return 0;
> + return sock->sk->sk_type == SOCK_RAW;
> +}
> +
> static long vhost_net_set_backend(struct vhost_net *n, unsigned index,
> int fd)
> {
> struct socket *sock, *oldsock;
> @@ -519,6 +524,20 @@
>
> vhost_net_disable_vq(n, vq);
> rcu_assign_pointer(vq->private_data, sock);
> +
> + if (sock && sock->sk) {
> + if (!vhost_sock_is_raw(sock) ||
I dislike this backend-specific code, it ties us with specifics of
backend implementations, which change without notice. Ideally all
backend-specific stuff should live in userspace, userspace programs
vhost device appropriately.
> + vhost_has_feature(&n->dev,
> VHOST_NET_F_VIRTIO_NET_HDR)) {
> + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> + if (vhost_has_feature(&n->dev,
> VIRTIO_NET_F_MRG_RXBUF))
> + vq->guest_hlen =
> + sizeof(struct
> virtio_net_hdr_mrg_rxbuf);
> + else
> + vq->guest_hlen = sizeof(struct
> virtio_net_hdr);
> + } else
> + vq->guest_hlen = vq->sock_hlen = 0;
> + } else
> + vq_err(vq, "vhost_net_set_backend: sock->sk is NULL");
As proposed above, IMO it would be nicer to add an ioctl in tap
that let us program header size.
> vhost_net_enable_vq(n, vq);
> mutex_unlock(&vq->mutex);
> done:
> @@ -566,8 +585,17 @@
> 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;
> + struct vhost_virtqueue *vq = n->vqs + i;
> + struct socket *sock = vq->private_data;
> +
> + mutex_lock(&vq->mutex);
> + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
> + vq->sock_hlen = sizeof(struct
> virtio_net_hdr_mrg_rxbuf);
> + else if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ||
> + !vhost_sock_is_raw(sock))
> + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> + else
> + vq->sock_hlen = 0;
> mutex_unlock(&n->vqs[i].mutex);
> }
> vhost_net_flush(n);
> diff -ruN net-next-p1/drivers/vhost/vhost.c
> net-next-p2/drivers/vhost/vhost.c
> --- net-next-p1/drivers/vhost/vhost.c 2010-03-01 11:44:06.000000000
> -0800
> +++ net-next-p2/drivers/vhost/vhost.c 2010-03-02 12:53:02.000000000
> -0800
> @@ -113,7 +113,8 @@
> vq->used_flags = 0;
> vq->log_used = false;
> vq->log_addr = -1ull;
> - vq->hdr_size = 0;
> + vq->guest_hlen = 0;
> + vq->sock_hlen = 0;
> vq->private_data = NULL;
> vq->log_base = NULL;
> vq->error_ctx = NULL;
> @@ -848,20 +849,85 @@
> return 0;
> }
>
> +static int
> +vhost_get_hdr(struct vhost_virtqueue *vq, int *in, struct vhost_log *log,
> + int *log_num)
> +{
> + struct iovec *heads = vq->heads;
> + struct iovec *iov = vq->iov;
> + int out;
> +
> + *in = 0;
> + iov[0].iov_len = 0;
> +
> + /* get buffer, starting from iov[1] */
> + heads[0].iov_base = (void *)vhost_get_vq_desc(vq->dev, vq,
> + vq->iov+1, ARRAY_SIZE(vq->iov)-1, &out, in, log, log_num);
> + if (out || *in <= 0) {
> + vq_err(vq, "unexpected descriptor format for RX: out %d, "
> + "in %d\n", out, *in);
> + return 0;
> + }
> + if (heads[0].iov_base == (void *)vq->num)
> + return 0;
> +
> + /* make iov[0] the header */
> + if (!vq->guest_hlen) {
> + if (vq->sock_hlen) {
> + static struct virtio_net_hdr junk; /* bit bucket
> */
> +
> + iov[0].iov_base = &junk;
> + iov[0].iov_len = sizeof(junk);
> + } else
> + iov[0].iov_len = 0;
> + }
> + if (vq->sock_hlen < vq->guest_hlen) {
> + iov[0].iov_base = iov[1].iov_base;
> + iov[0].iov_len = vq->sock_hlen;
> +
> + if (iov[1].iov_len < vq->sock_hlen) {
> + vq_err(vq, "can't fit header in one buffer!");
> + vhost_discard(vq, 1);
> + return 0;
> + }
> + if (!vq->sock_hlen) {
> + static const struct virtio_net_hdr_mrg_rxbuf hdr =
> {
> + .hdr.flags = 0,
> + .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
> + };
> + memcpy(iov[0].iov_base, &hdr, vq->guest_hlen);
> + }
> + iov[1].iov_base += vq->guest_hlen;
> + iov[1].iov_len -= vq->guest_hlen;
> + }
> + return 1;
The above looks kind of scary, lots of special-casing. I'll send a
patch for tap to set vnet header size, let's see if it makes life easier
for us.
> +}
> +
> 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;
> + int out, in = 0;
> + int seg = 0;
> int hc = 0;
I think it's better to stick to simple names like i
for index variables, alternatively give it a meaningful
name like "count".
>
> + if (vq->guest_hlen != vq->sock_hlen) {
Sticking guest_hlen/sock_hlen in vhost is somewhat ugly.
> + seg = vhost_get_hdr(vq, &in, log, log_num);
> + if (!seg)
> + return 0;
> + hc++;
> + datalen -= iov_length(vq->iov+seg, in);
> + seg += in;
> + }
> +
> while (datalen > 0) {
> if (hc >= VHOST_NET_MAX_SG) {
> vhost_discard(vq, hc);
> return 0;
> }
> heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev,
> vq,
> - vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> log_num);
> + vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out, &in,
> + log, log_num);
> if (heads[hc].iov_base == (void *)vq->num) {
> vhost_discard(vq, hc);
> return 0;
> @@ -872,11 +938,12 @@
> vhost_discard(vq, hc);
> return 0;
> }
> - heads[hc].iov_len = iov_length(vq->iov, in);
> - hc++;
> + heads[hc].iov_len = iov_length(vq->iov+seg, in);
> datalen -= heads[hc].iov_len;
> + hc++;
> + seg += in;
> }
> - *iovcount = in;
> + *iovcount = seg;
> return hc;
> }
>
> diff -ruN net-next-p1/drivers/vhost/vhost.h
> net-next-p2/drivers/vhost/vhost.h
> --- net-next-p1/drivers/vhost/vhost.h 2010-03-01 11:42:18.000000000
> -0800
> +++ net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> -0800
> @@ -82,10 +82,9 @@
> u64 log_addr;
>
> struct iovec indirect[VHOST_NET_MAX_SG];
> - struct iovec iov[VHOST_NET_MAX_SG];
> - struct iovec hdr[VHOST_NET_MAX_SG];
> + struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
> struct iovec heads[VHOST_NET_MAX_SG];
> - size_t hdr_size;
> + size_t guest_hlen, sock_hlen;
> /* 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
>
^ permalink raw reply
* Re: [RFC][ PATCH 3/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: Michael S. Tsirkin @ 2010-03-07 16:26 UTC (permalink / raw)
To: David Stevens; +Cc: rusty, netdev, kvm, virtualization
In-Reply-To: <OFD112C680.DB48ADA2-ON882576DB.0000CE4F-882576DB.0001E192@us.ibm.com>
On Tue, Mar 02, 2010 at 05:20:34PM -0700, David Stevens wrote:
> This patch glues them all together and makes sure we
> notify whenever we don't have enough buffers to receive
> a max-sized packet, and adds the feature bit.
>
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
Maybe split this up?
> diff -ruN net-next-p2/drivers/vhost/net.c net-next-p3/drivers/vhost/net.c
> --- net-next-p2/drivers/vhost/net.c 2010-03-02 13:01:34.000000000
> -0800
> +++ net-next-p3/drivers/vhost/net.c 2010-03-02 15:25:15.000000000
> -0800
> @@ -54,26 +54,6 @@
> 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,7 @@
> static void handle_tx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> - unsigned out, in, s;
> + unsigned out, in;
> struct iovec head;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -110,6 +90,7 @@
> size_t len, total_len = 0;
> int err, wmem;
> struct socket *sock = rcu_dereference(vq->private_data);
> +
I tend not to add empty lines if line below it is already short.
> if (!sock)
> return;
>
> @@ -166,11 +147,11 @@
> /* Skip header. TODO: support TSO. */
> msg.msg_iovlen = out;
> head.iov_len = len = iov_length(vq->iov, out);
> +
I tend not to add empty lines if line below it is a comment.
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for TX: "
> - "%zd expected %zd\n",
> - len, vq->guest_hlen);
> + "%zd expected %zd\n", len, vq->guest_hlen);
> break;
> }
> /* TODO: Check specific error and bomb out unless ENOBUFS?
> */
> @@ -214,7 +195,7 @@
> static void handle_rx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> - unsigned in, log, s;
> + unsigned in, log;
> struct vhost_log *vq_log;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -245,30 +226,36 @@
> if (!headcount) {
> vhost_enable_notify(vq);
> break;
> - }
> + } else if (vq->maxheadcount < headcount)
> + vq->maxheadcount = headcount;
> /* Skip header. TODO: support TSO/mergeable rx buffers. */
> 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",
> - len, vq->guest_hlen);
> + "%zd expected %zd\n", len, vq->guest_hlen);
> 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, 1);
> + vhost_discard(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 */
> + vq->iov[0].iov_len = vq->guest_hlen;
> + vhdr->num_buffers = headcount;
I don't understand this. iov_base is a userspace pointer, isn't it.
How can you assign values to it like that?
Rusty also commented earlier that it's not a good idea to assume
specific layout, such as first chunk being large enough to
include virtio_net_hdr_mrg_rxbuf.
I think we need to use memcpy to/from iovec etc.
> + }
> if (err > len) {
> pr_err("Discarded truncated rx packet: "
> " len %d > %zd\n", err, len);
> - vhost_discard(vq, 1);
> + vhost_discard(vq, headcount);
> continue;
> }
> len = err;
> @@ -573,8 +560,6 @@
>
> 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)) &&
> diff -ruN net-next-p2/drivers/vhost/vhost.c
> net-next-p3/drivers/vhost/vhost.c
> --- net-next-p2/drivers/vhost/vhost.c 2010-03-02 12:53:02.000000000
> -0800
> +++ net-next-p3/drivers/vhost/vhost.c 2010-03-02 15:24:50.000000000
> -0800
> @@ -115,6 +115,7 @@
> vq->log_addr = -1ull;
> vq->guest_hlen = 0;
> vq->sock_hlen = 0;
> + vq->maxheadcount = 0;
> vq->private_data = NULL;
> vq->log_base = NULL;
> vq->error_ctx = NULL;
> @@ -410,6 +411,7 @@
> vq->last_avail_idx = s.num;
> /* Forget the cached index value. */
> vq->avail_idx = vq->last_avail_idx;
> + vq->maxheadcount = 0;
> break;
> case VHOST_GET_VRING_BASE:
> s.index = idx;
> @@ -1114,10 +1116,23 @@
> return 0;
> }
>
> +int vhost_available(struct vhost_virtqueue *vq)
> +{
> + int avail;
> +
> + if (!vq->maxheadcount) /* 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)
> {
> __u16 flags = 0;
> +
I tend not to add empty lines if a line above it is already short.
> if (get_user(flags, &vq->avail->flags)) {
> vq_err(vq, "Failed to get flags");
> return;
> @@ -1125,7 +1140,7 @@
>
> /* If they don't want an interrupt, don't signal, unless empty. */
> if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> - (vq->avail_idx != vq->last_avail_idx ||
> + (vhost_available(vq) > vq->maxheadcount ||
I don't understand this change. It seems to make
code not match the comments.
> !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
> return;
>
> diff -ruN net-next-p2/drivers/vhost/vhost.h
> net-next-p3/drivers/vhost/vhost.h
> --- net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> -0800
> +++ net-next-p3/drivers/vhost/vhost.h 2010-03-02 14:29:44.000000000
> -0800
> @@ -85,6 +85,7 @@
> struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
> struct iovec heads[VHOST_NET_MAX_SG];
> size_t guest_hlen, sock_hlen;
> + int maxheadcount;
I don't completely understand what does this field does.
It seems to be only set on rx? Maybe name should reflect this?
> /* 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
> @@ -151,7 +152,8 @@
> 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
* [patch] another pegasus usb net device
From: Petko Manolov @ 2010-03-07 16:10 UTC (permalink / raw)
To: netdev; +Cc: davem, julian, petkan
[-- Attachment #1: Type: TEXT/PLAIN, Size: 214 bytes --]
This one removes trailing whitespace in pegasus.h and more importantly
adds new Pegasus compatible device.
Signed-off-by: Julian Brown <julian@codesourcery.com>
Signed-off-by: Petko Manolov <petkan@nucleusys.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/X-DIFF; NAME=pegasus-belkin-f5u122-1.diff, Size: 1634 bytes --]
diff --git a/drivers/net/usb/pegasus.h b/drivers/net/usb/pegasus.h
index 5d02f02..b90d876 100644
--- a/drivers/net/usb/pegasus.h
+++ b/drivers/net/usb/pegasus.h
@@ -177,7 +177,7 @@ PEGASUS_DEV( "USB 10/100 Fast Ethernet", VENDOR_ABOCOM, 0x400c,
PEGASUS_DEV( "USB 10/100 Fast Ethernet", VENDOR_ABOCOM, 0xabc1,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "USB 10/100 Fast Ethernet", VENDOR_ABOCOM, 0x200c,
- DEFAULT_GPIO_RESET | PEGASUS_II )
+ DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "Accton USB 10/100 Ethernet Adapter", VENDOR_ACCTON, 0x1046,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "SpeedStream USB 10/100 Ethernet", VENDOR_ACCTON, 0x5046,
@@ -208,6 +208,8 @@ PEGASUS_DEV( "Allied Telesyn Int. AT-USB100", VENDOR_ALLIEDTEL, 0xb100,
*/
PEGASUS_DEV_CLASS( "Belkin F5D5050 USB Ethernet", VENDOR_BELKIN, 0x0121, 0x00,
DEFAULT_GPIO_RESET | PEGASUS_II )
+PEGASUS_DEV( "Belkin F5U122 10/100 USB Ethernet", VENDOR_BELKIN, 0x0122,
+ DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "Billionton USB-100", VENDOR_BILLIONTON, 0x0986,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "Billionton USBLP-100", VENDOR_BILLIONTON, 0x0987,
@@ -249,7 +251,7 @@ PEGASUS_DEV( "GIGABYTE GN-BR402W Wireless Router", VENDOR_GIGABYTE, 0x8002,
PEGASUS_DEV( "Hawking UF100 10/100 Ethernet", VENDOR_HAWKING, 0x400c,
DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "HP hn210c Ethernet USB", VENDOR_HP, 0x811c,
- DEFAULT_GPIO_RESET | PEGASUS_II )
+ DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "IO DATA USB ET/TX", VENDOR_IODATA, 0x0904,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "IO DATA USB ET/TX-S", VENDOR_IODATA, 0x0913,
^ permalink raw reply related
* Re: [kernel,2.6.33-git11] lib8390: use spin_lock_irqsave for locking
From: David Miller @ 2010-03-07 23:22 UTC (permalink / raw)
To: ken_kawasaki; +Cc: netdev
This change is not correct.
disable_irq is being intentionally used because the reset
sequence can take a very long time and we don't want to
have cpu interrupts disabled during the entire sequence.
Otherwise slow serial devices will drop characters and
stuff like that.
^ permalink raw reply
* Re: [PATCH 0/3] cpmac fixes
From: David Miller @ 2010-03-07 23:26 UTC (permalink / raw)
To: florian; +Cc: netdev
In-Reply-To: <201003071155.17553.florian@openwrt.org>
From: Florian Fainelli <florian@openwrt.org>
Date: Sun, 7 Mar 2010 11:55:16 +0100
> Here are 2 small cpmac fixes and a driver version bumping. Thanks!
>
> Florian Fainelli (3):
> cpmac: fix the receiving of 802.1q frames
> cpmac: fallback to switch mode if no PHY chip found
> cpmac: bump version to 0.5.2
All applied, thank you.
^ permalink raw reply
* Re: [patch] irda-usb: add error handling and fix leak
From: David Miller @ 2010-03-07 23:26 UTC (permalink / raw)
To: error27; +Cc: samuel, shemminger, kaber, tabe, julia, netdev, kernel-janitors
In-Reply-To: <20100307123542.GA6469@bicker>
From: Dan Carpenter <error27@gmail.com>
Date: Sun, 7 Mar 2010 15:35:42 +0300
> If the call to kcalloc() fails then we should return -ENOMEM.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: Optmize translation between IPV6_PREFER_SRC_xxx and RT6_LOOKUP_F_xxx.
From: David Miller @ 2010-03-07 23:26 UTC (permalink / raw)
To: yoshfuji; +Cc: netdev
In-Reply-To: <201003071014.o27AEio4001703@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Sun, 7 Mar 2010 19:14:44 +0900
> IPV6_PREFER_SRC_xxx definitions:
> | #define IPV6_PREFER_SRC_TMP 0x0001
> | #define IPV6_PREFER_SRC_PUBLIC 0x0002
> | #define IPV6_PREFER_SRC_COA 0x0004
>
> RT6_LOOKUP_F_xxx definitions:
> | #define RT6_LOOKUP_F_SRCPREF_TMP 0x00000008
> | #define RT6_LOOKUP_F_SRCPREF_PUBLIC 0x00000010
> | #define RT6_LOOKUP_F_SRCPREF_COA 0x00000020
>
> So, we can translate between these two groups by shift operation
> instead of multiple 'if's.
>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-2.6] netfilter: ebt_ip6: Use ipv6_masked_addr_cmp().
From: David Miller @ 2010-03-07 23:27 UTC (permalink / raw)
To: yoshfuji; +Cc: bart.de.schuymer, kaber, ebtables-devel, netfilter-devel, netdev
In-Reply-To: <201003070918.o279IXtq029570@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Sun, 7 Mar 2010 17:39:10 +0900
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Patrick, I assume you'll get this.
^ permalink raw reply
* Re: [PATCH net-2.6] netfilter: Remove stale declaration for ip6_masked_addrcmp().
From: David Miller @ 2010-03-07 23:27 UTC (permalink / raw)
To: yoshfuji; +Cc: kaber, netfilter-devel, netdev
In-Reply-To: <201003070918.o279IfCk029612@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Sun, 7 Mar 2010 17:39:13 +0900
> Commit f2ffd9ee... ("[NETFILTER]: Move ip6_masked_addrcmp to
> include/net/ipv6.h") replaced ip6_masked_addrcmp() with
> ipv6_masked_addr_cmp(). Function definition went away.
>
> Let's remove its declaration as well in header file.
>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Patrick, I assume you'll get this one too.
Thanks.
^ permalink raw reply
* Re: [patch] bridge: cleanup: remove unneed check
From: David Miller @ 2010-03-07 23:27 UTC (permalink / raw)
To: herbert; +Cc: error27, shemminger, bridge, netdev, kernel-janitors
In-Reply-To: <20100306122031.GA15864@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 6 Mar 2010 20:20:32 +0800
> On Sat, Mar 06, 2010 at 02:14:09PM +0300, Dan Carpenter wrote:
>> We dereference "port" on the lines immediately before and immediately
>> after the test so port should hopefully never be null here.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Yes this looks correct to me.
Applied.
^ permalink raw reply
* Re: [patch] ems_usb: cleanup: remove uneeded check
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: error27
Cc: urs.thuermann, oliver.hartkopp, wg, haas, kurt.van.dijck, netdev,
kernel-janitors
In-Reply-To: <20100306111138.GH4958@bicker>
From: Dan Carpenter <error27@gmail.com>
Date: Sat, 6 Mar 2010 14:11:38 +0300
> "skb" is alway non-null here, but even if it were null the check isn't
> needed because dev_kfree_skb() can handle it.
>
> This eliminates a smatch warning about dereferencing a variable before
> checking that it is non-null.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied.
^ permalink raw reply
* Re: [patch] sock.c: potential null dereference
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: error27
Cc: netdev, eric.dumazet, jengelh, mingo, akpm, linux-kernel,
kernel-janitors
In-Reply-To: <20100306110445.GC4958@bicker>
From: Dan Carpenter <error27@gmail.com>
Date: Sat, 6 Mar 2010 14:04:45 +0300
> We test that "prot->rsk_prot" is non-null right before we dereference it
> on this line.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] MAINTAINER: Correct CAN Maintainer responsibilities and paths
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: wg; +Cc: socketcan, oliver.hartkopp, urs.thuermann, netdev, socketcan-core
In-Reply-To: <4B929551.3000901@grandegger.com>
From: Wolfgang Grandegger <wg@grandegger.com>
Date: Sat, 06 Mar 2010 18:48:01 +0100
> Oliver Hartkopp wrote:
>> Update the CAN Maintainer responsibilities and add source paths.
>> Additional the SocketCAN core ML is not subscribers-only anymore.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
...
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Applied.
^ permalink raw reply
* Re: [PATCH] fix a race in ks8695_poll
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: figo1802; +Cc: netdev, shemminger, yegorslists
In-Reply-To: <1267842962.2669.2.camel@myhost>
From: "Figo.zhang" <figo1802@gmail.com>
Date: Sat, 06 Mar 2010 10:36:02 +0800
> fix a race at the end of NAPI processing in ks8695_poll() function.
>
> Signed-off-by:Figo.zhang <figo1802@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: David Miller @ 2010-03-07 23:31 UTC (permalink / raw)
To: herbert; +Cc: paulmck, netdev, shemminger
In-Reply-To: <20100306070335.GA14470@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 6 Mar 2010 15:03:35 +0800
> On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
>>
>> > In contrast, rcu_barrier_bh() is guaranteed not to return until all
>> > pending RCU-bh callbacks have executed.
>>
>> You're absolutely right. I'll send a patch to fix this.
>
> bridge: Fix RCU race in br_multicast_stop
>
> Thanks to Paul McKenny for pointing out that it is incorrect to use
> synchronize_rcu_bh to ensure that pending callbacks have completed.
> Instead we should use rcu_barrier_bh.
>
> Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox