* Re: 3.11-rc7:BUG: soft lockup
From: Baoquan He @ 2013-09-02 6:06 UTC (permalink / raw)
To: Hillf Danton; +Cc: Cong Wang, LKML, Linux Kernel Network Developers
In-Reply-To: <CAJd=RBCT6dbLwX6z+1LxCvJ9-NOYQRe0zzBkEQosFgLvNgg4bg@mail.gmail.com>
Hi both,
Thanks for your patches. I tried to test your patches, first the 2nd
one, namely Hillf's patch, it's OK. Then when I wanted to reproduce and
test Cong's patch, it failed to happen again.
I remember this bug happened randomly at the very beginning,
just after kernel compiling it always happened one day.
So maybe when it happened again, I will test your patch separately.
Baoquan
Thanks
On 08/31/2013 11:25 AM, Hillf Danton wrote:
> On Fri, Aug 30, 2013 at 8:18 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> Cc'ing netdev
>>
>> On Fri, Aug 30, 2013 at 4:20 PM, Baoquan He <baoquan.he@gmail.com> wrote:
>>> Hi,
>>>
>>> I tried the 3.11.0-rc7+ on x86_64, and after bootup, the soft lockup bug
>>> happened.
>>>
>>> [ 48.895000] BUG: soft lockup - CPU#1 stuck for 22s! [ebtables:444]
>>> [ 48.901191] Modules linked in: bnep(F) bluetooth(F) ebtables(F)
>>> ip6table_filter(F) ip6_tables(F) rfkill(F) snd_hda_intel(F+)
>>> snd_hda_codec(F) snd_hwdep(F) snd_seq(F) sn)
>>> [ 48.950034] CPU: 1 PID: 444 Comm: ebtables Tainted: GF D
>>> 3.11.0-rc7+ #1
>>> [ 48.957433] Hardware name: Hewlett-Packard HP Z420 Workstation/1589,
>>> BIOS J61 v01.02 03/09/2012
>>> [ 48.966131] task: ffff88040c2dc650 ti: ffff8804187d2000 task.ti:
>>> ffff8804187d2000
>>> [ 48.973610] RIP: 0010:[<ffffffff812e57a7>] [<ffffffff812e57a7>]
>>> strcmp+0x27/0x40
>>> [ 48.981119] RSP: 0018:ffff8804187d3db8 EFLAGS: 00000246
>>> [ 48.986430] RAX: 0000000000000000 RBX: 00007fffda942730 RCX:
>>> ffff8804187d3fd8
>>> [ 48.993566] RDX: 0000000000000000 RSI: ffff8804187d3e01 RDI:
>>> ffffffff81cb8a39
>>> [ 49.000707] RBP: ffff8804187d3db8 R08: 00000000fffffff2 R09:
>>> 0000000000000000
>>> [ 49.007841] R10: 0000000000000163 R11: 0000000000000000 R12:
>>> ffffffff8128300c
>>> [ 49.014972] R13: ffff8804187d3d98 R14: ffff8804187d3ef4 R15:
>>> 0000000000000004
>>> [ 49.022112] FS: 00007faab6589740(0000) GS:ffff88042fc80000(0000)
>>> knlGS:0000000000000000
>>> [ 49.030194] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [ 49.035942] CR2: 0000003f0d810414 CR3: 000000040d2cc000 CR4:
>>> 00000000000407e0
>>> [ 49.043077] Stack:
>>> [ 49.045096] ffff8804187d3de8 ffffffffa0249674 0000000000000080
>>> ffffffff81cb8180
>>> [ 49.052559] 00007fffda942730 ffff8804187d3ef4 ffff8804187d3ea0
>>> ffffffffa02497a9
>>> [ 49.060020] 0000000000000000 00007265746c6966 0000003f0d7b92c0
>>> 00007fffda942850
>>> [ 49.067487] Call Trace:
>>> [ 49.069949] [<ffffffffa0249674>]
>>> find_inlist_lock.constprop.16+0x54/0x100 [ebtables]
>>> [ 49.077779] [<ffffffffa02497a9>] do_ebt_get_ctl+0x89/0x1d0 [ebtables]
>>> [ 49.084306] [<ffffffff81551ca8>] nf_getsockopt+0x68/0x90
>>> [ 49.089717] [<ffffffff81560d40>] ip_getsockopt+0x80/0xa0
>>> [ 49.095113] [<ffffffff815835c5>] raw_getsockopt+0x25/0x50
>>> [ 49.100588] [<ffffffff8150ddd4>] sock_common_getsockopt+0x14/0x20
>>> [ 49.106766] [<ffffffff8150d208>] SyS_getsockopt+0x68/0xd0
>>> [ 49.112257] [<ffffffff8162c682>] system_call_fastpath+0x16/0x1b
>>> [ 49.118260] Code: 00 00 00 00 55 48 89 e5 eb 0e 66 2e 0f 1f 84 00 00
>>> 00 00 00 84 c0 74 1c 48 83 c7 01 0f b6 47 ff 48 83 c6 01 3a 46 ff 74 eb
>>> 19 c0 <83> c8 01 5d c3 0f 1
>>> [ 76.925880] BUG: soft lockup - CPU#1 stuck for 22s! [ebtables:444]
>>> [ 76.932069] Modules linked in: bnep(F) bluetooth(F) ebtables(F)
>>> ip6table_filter(F) ip6_tables(F) rfkill(F) snd_hda_intel(F+)
>>> snd_hda_codec(F) snd_hwdep(F) snd_seq(F) sn)
>>> [ 76.980847] CPU: 1 PID: 444 Comm: ebtables Tainted: GF D
>>> 3.11.0-rc7+ #1
>>> [ 76.988245] Hardware name: Hewlett-Packard HP Z420 Workstation/1589,
>>> BIOS J61 v01.02 03/09/2012
>>> [ 76.996940] task: ffff88040c2dc650 ti: ffff8804187d2000 task.ti:
>>> ffff8804187d2000
>>> [ 77.004426] RIP: 0010:[<ffffffff812e5784>] [<ffffffff812e5784>]
>>> strcmp+0x4/0x40
>>> [ 77.011849] RSP: 0018:ffff8804187d3db8 EFLAGS: 00000212
>>> [ 77.017163] RAX: 0000000000000001 RBX: 00007fffda942730 RCX:
>>> ffff8804187d3fd8
>>> [ 77.024304] RDX: 0000000000000000 RSI: ffff8804187d3e00 RDI:
>>> ffffffff81cb8a38
>>> [ 77.031434] RBP: ffff8804187d3db8 R08: 00000000fffffff2 R09:
>>> 0000000000000000
>>> [ 77.038566] R10: 0000000000000163 R11: 0000000000000000 R12:
>>> ffffffff8128300c
>>> [ 77.045699] R13: ffff8804187d3d98 R14: ffff8804187d3ef4 R15:
>>> 0000000000000004
>>> [ 77.052842] FS: 00007faab6589740(0000) GS:ffff88042fc80000(0000)
>>> knlGS:0000000000000000
>>> [ 77.060934] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [ 77.066668] CR2: 0000003f0d810414 CR3: 000000040d2cc000 CR4:
>>> 00000000000407e0
>>> [ 77.073799] Stack:
>>> [ 77.075818] ffff8804187d3de8 ffffffffa0249674 0000000000000080
>>> ffffffff81cb8180
>>> [ 77.083287] 00007fffda942730 ffff8804187d3ef4 ffff8804187d3ea0
>>> ffffffffa02497a9
>>> [ 77.090749] 0000000000000000 00007265746c6966 0000003f0d7b92c0
>>> 00007fffda942850
>>> [ 77.098215] Call Trace:
>>> [ 77.100668] [<ffffffffa0249674>]
>>> find_inlist_lock.constprop.16+0x54/0x100 [ebtables]
>>> [ 77.108500] [<ffffffffa02497a9>] do_ebt_get_ctl+0x89/0x1d0 [ebtables]
>>> [ 77.115035] [<ffffffff81551ca8>] nf_getsockopt+0x68/0x90
>>> [ 77.120438] [<ffffffff81560d40>] ip_getsockopt+0x80/0xa0
>>> [ 77.125845] [<ffffffff815835c5>] raw_getsockopt+0x25/0x50
>>> [ 77.131328] [<ffffffff8150ddd4>] sock_common_getsockopt+0x14/0x20
>>> [ 77.137515] [<ffffffff8150d208>] SyS_getsockopt+0x68/0xd0
>>> [ 77.143011] [<ffffffff8162c682>] system_call_fastpath+0x16/0x1b
>>> [ 77.149019] Code: 0f 1f 80 00 00 00 00 48 83 c6 01 0f b6 4e ff 48 83
>>> c2 01 84 c9 88 4a ff 75 ed 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 55 48
>>> 89 e5 <eb> 0e 66 2e 0f 1f 8
>>
>> Does the following patch help?
>>
>>
>> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
>> index ac78024..4a0ec8f 100644
>> --- a/net/bridge/netfilter/ebtables.c
>> +++ b/net/bridge/netfilter/ebtables.c
>> @@ -1503,6 +1503,10 @@ static int do_ebt_get_ctl(struct sock *sk, int
>> cmd, void __user *user, int *len)
>> if (copy_from_user(&tmp, user, sizeof(tmp)))
>> return -EFAULT;
>>
>> + if (memscan(tmp.name, '\0', EBT_TABLE_MAXNAMELEN) ==
>> + (tmp.name + EBT_TABLE_MAXNAMELEN))
>> + return -EINVAL;
>> +
>> t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
>> if (!t)
>> return ret;
>> --
>>
> release lock!!
>
> --- a/net/bridge/netfilter/ebtables.c Sat Aug 31 11:12:54 2013
> +++ b/net/bridge/netfilter/ebtables.c Sat Aug 31 11:15:24 2013
> @@ -332,8 +332,10 @@ find_inlist_lock_noload(struct list_head
> return NULL;
>
> list_for_each_entry(e, head, list) {
> - if (strcmp(e->name, name) == 0)
> + if (strcmp(e->name, name) == 0) {
> + mutex_unlock(mutex);
> return e;
> + }
> }
> *error = -ENOENT;
> mutex_unlock(mutex);
> --
^ permalink raw reply
* Re: [PATCH v3] ipv6:introduce function to find route for redirect
From: Duan Jiong @ 2013-09-02 6:07 UTC (permalink / raw)
To: hannes; +Cc: davem, netdev
In-Reply-To: <5223FA1C.5020501@cn.fujitsu.com>
于 2013年09月02日 10:38, Duan Jiong 写道:
>>> void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
>>
>> Maybe we should rename oif to iif now?
> Yes, that looks no ambiguity.
I'm sorry about that.
I finally think that there is no need to rename oif to iif, because
the argument oif will be assigned to fl6.flowi6_oif, and if we
rename it, others will be confused by this.
Thanks,
Duan
^ permalink raw reply
* [PATCH v4] ipv6:introduce function to find route for redirect
From: Duan Jiong @ 2013-09-02 6:14 UTC (permalink / raw)
To: davem; +Cc: duanj.fnst, netdev, hannes
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
RFC 4861 says that the IP source address of the Redirect is the
same as the current first-hop router for the specified ICMP
Destination Address, so the gateway should be taken into
consideration when we find the route for redirect.
There was once a check in commit
a6279458c534d01ccc39498aba61c93083ee0372 ("NDISC: Search over
all possible rules on receipt of redirect.") and the check
went away in commit b94f1c0904da9b8bf031667afc48080ba7c3e8c9
("ipv6: Use icmpv6_notify() to propagate redirect, instead of
rt6_redirect()").
The bug is only "exploitable" on layer-2 because the source
address of the redirect is checked to be a valid link-local
address but it makes spoofing a lot easier in the same L2
domain nonetheless.
Thanks very much for Hannes's help.
Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
Changes for v4:
1.Fix the intended problems
2.Change the ordering of the arguments in function
ip6_route_redirect
3.Drop the unnecessary flag RT6_LOOKUP_F_IFACE
4.Check the dst.error when look up route for redirect
net/ipv6/ah6.c | 2 +-
net/ipv6/esp6.c | 2 +-
net/ipv6/icmp.c | 2 +-
net/ipv6/ipcomp6.c | 2 +-
net/ipv6/ndisc.c | 3 ++-
net/ipv6/route.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++-----
6 files changed, 77 insertions(+), 11 deletions(-)
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index bb02e17..73784c3 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -628,7 +628,7 @@ static void ah6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
return;
if (type == NDISC_REDIRECT)
- ip6_redirect(skb, net, 0, 0);
+ ip6_redirect(skb, net, skb->dev->ifindex, 0);
else
ip6_update_pmtu(skb, net, info, 0, 0);
xfrm_state_put(x);
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index aeac0dc..d3618a7 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -447,7 +447,7 @@ static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
return;
if (type == NDISC_REDIRECT)
- ip6_redirect(skb, net, 0, 0);
+ ip6_redirect(skb, net, skb->dev->ifindex, 0);
else
ip6_update_pmtu(skb, net, info, 0, 0);
xfrm_state_put(x);
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 7cfc8d2..73681c2 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -92,7 +92,7 @@ static void icmpv6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
if (type == ICMPV6_PKT_TOOBIG)
ip6_update_pmtu(skb, net, info, 0, 0);
else if (type == NDISC_REDIRECT)
- ip6_redirect(skb, net, 0, 0);
+ ip6_redirect(skb, net, skb->dev->ifindex, 0);
if (!(type & ICMPV6_INFOMSG_MASK))
if (icmp6->icmp6_type == ICMPV6_ECHO_REQUEST)
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 7af5aee..5636a91 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -76,7 +76,7 @@ static void ipcomp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
return;
if (type == NDISC_REDIRECT)
- ip6_redirect(skb, net, 0, 0);
+ ip6_redirect(skb, net, skb->dev->ifindex, 0);
else
ip6_update_pmtu(skb, net, info, 0, 0);
xfrm_state_put(x);
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 04d31c2..70abece 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1370,7 +1370,8 @@ static void ndisc_redirect_rcv(struct sk_buff *skb)
return;
if (!ndopts.nd_opts_rh) {
- ip6_redirect_no_header(skb, dev_net(skb->dev), 0, 0);
+ ip6_redirect_no_header(skb, dev_net(skb->dev),
+ skb->dev->ifindex, 0);
return;
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 8d9a93e..a01337f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1157,6 +1157,73 @@ void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
}
EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
+/* Handle redirects */
+struct ip6rd_flowi {
+ struct flowi6 fl6;
+ struct in6_addr gateway;
+};
+
+static struct rt6_info *__ip6_route_redirect(struct net *net,
+ struct fib6_table *table,
+ struct flowi6 *fl6,
+ int flags)
+{
+ struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
+ struct rt6_info *rt;
+ struct fib6_node *fn;
+
+ /* Get the "current" route for this destination and
+ * check if the redirect has come from approriate router.
+ *
+ * RFC 4861 specifies that redirects should only be
+ * accepted if they come from the nexthop to the target.
+ * Due to the way the routes are chosen, this notion
+ * is a bit fuzzy and one might need to check all possible
+ * routes.
+ */
+
+ read_lock_bh(&table->tb6_lock);
+ fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
+restart:
+ for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) {
+ if (rt6_check_expired(rt))
+ continue;
+ if (rt->dst.error)
+ continue;
+ if (!(rt->rt6i_flags & RTF_GATEWAY))
+ continue;
+ if (fl6->flowi6_oif != rt->dst.dev->ifindex)
+ continue;
+ if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway))
+ continue;
+ break;
+ }
+
+ if (!rt)
+ rt = net->ipv6.ip6_null_entry;
+ BACKTRACK(net, &fl6->saddr);
+out:
+ dst_hold(&rt->dst);
+
+ read_unlock_bh(&table->tb6_lock);
+
+ return rt;
+};
+
+static struct dst_entry *ip6_route_redirect(struct net *net,
+ const struct flowi6 *fl6,
+ const struct in6_addr *gateway)
+{
+ int flags = RT6_LOOKUP_F_HAS_SADDR;
+ struct ip6rd_flowi rdfl;
+
+ rdfl.fl6 = *fl6;
+ rdfl.gateway = *gateway;
+
+ return fib6_rule_lookup(net, &rdfl.fl6,
+ flags, __ip6_route_redirect);
+}
+
void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
{
const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
@@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
fl6.saddr = iph->saddr;
fl6.flowlabel = ip6_flowinfo(iph);
- dst = ip6_route_output(net, NULL, &fl6);
- if (!dst->error)
- rt6_do_redirect(dst, NULL, skb);
+ dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
+ rt6_do_redirect(dst, NULL, skb);
dst_release(dst);
}
EXPORT_SYMBOL_GPL(ip6_redirect);
@@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
fl6.daddr = msg->dest;
fl6.saddr = iph->daddr;
- dst = ip6_route_output(net, NULL, &fl6);
- if (!dst->error)
- rt6_do_redirect(dst, NULL, skb);
+ dst = ip6_route_redirect(net, &fl6, &iph->saddr);
+ rt6_do_redirect(dst, NULL, skb);
dst_release(dst);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH V2 2/6] vhost_net: use vhost_add_used_and_signal_n() in vhost_zerocopy_signal_used()
From: Jason Wang @ 2013-09-02 6:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20130902055048.GA19838@redhat.com>
On 09/02/2013 01:50 PM, Michael S. Tsirkin wrote:
> On Fri, Aug 30, 2013 at 12:29:18PM +0800, Jason Wang wrote:
>> > We tend to batch the used adding and signaling in vhost_zerocopy_callback()
>> > which may result more than 100 used buffers to be updated in
>> > vhost_zerocopy_signal_used() in some cases. So wwitch to use
> switch
Ok.
>> > vhost_add_used_and_signal_n() to avoid multiple calls to
>> > vhost_add_used_and_signal(). Which means much more less times of used index
>> > updating and memory barriers.
> pls put info on perf gain in commit log too
>
Sure.
^ permalink raw reply
* Re: [PATCH V2 1/6] vhost_net: make vhost_zerocopy_signal_used() returns void
From: Jason Wang @ 2013-09-02 6:29 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20130902055118.GB19838@redhat.com>
On 09/02/2013 01:51 PM, Michael S. Tsirkin wrote:
> tweak subj s/returns/return/
>
> On Fri, Aug 30, 2013 at 12:29:17PM +0800, Jason Wang wrote:
>> > None of its caller use its return value, so let it return void.
>> >
>> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>> > ---
Will correct it in v3.
^ permalink raw reply
* Re: [PATCH V2 6/6] vhost_net: correctly limit the max pending buffers
From: Jason Wang @ 2013-09-02 6:30 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20130902055609.GC19838@redhat.com>
On 09/02/2013 01:56 PM, Michael S. Tsirkin wrote:
> On Fri, Aug 30, 2013 at 12:29:22PM +0800, Jason Wang wrote:
>> As Michael point out, We used to limit the max pending DMAs to get better cache
>> utilization. But it was not done correctly since it was one done when there's no
>> new buffers submitted from guest. Guest can easily exceeds the limitation by
>> keeping sending packets.
>>
>> So this patch moves the check into main loop. Tests shows about 5%-10%
>> improvement on per cpu throughput for guest tx. But a 5% drop on per cpu
>> transaction rate for a single session TCP_RR.
> Any explanation for the drop? single session TCP_RR is unlikely to
> exceed VHOST_MAX_PEND, correct?
Unlikely to exceed. Recheck the result, looks like it was not stable
enough. Will re-test and report.
>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/vhost/net.c | 15 ++++-----------
>> 1 files changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index d09c17c..592e1f2 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -363,6 +363,10 @@ static void handle_tx(struct vhost_net *net)
>> if (zcopy)
>> vhost_zerocopy_signal_used(net, vq);
>>
>> + if ((nvq->upend_idx + vq->num - VHOST_MAX_PEND) % UIO_MAXIOV ==
>> + nvq->done_idx)
>> + break;
>> +
>> head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
>> ARRAY_SIZE(vq->iov),
>> &out, &in,
>> @@ -372,17 +376,6 @@ static void handle_tx(struct vhost_net *net)
>> break;
>> /* Nothing new? Wait for eventfd to tell us they refilled. */
>> if (head == vq->num) {
>> - int num_pends;
>> -
>> - /* If more outstanding DMAs, queue the work.
>> - * Handle upend_idx wrap around
>> - */
>> - num_pends = likely(nvq->upend_idx >= nvq->done_idx) ?
>> - (nvq->upend_idx - nvq->done_idx) :
>> - (nvq->upend_idx + UIO_MAXIOV -
>> - nvq->done_idx);
>> - if (unlikely(num_pends > VHOST_MAX_PEND))
>> - break;
>> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>> vhost_disable_notify(&net->dev, vq);
>> continue;
>> --
>> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] rtl8187: fix use after free on failure path in rtl8187_init_urbs()
From: Hin-Tak Leung @ 2013-09-02 6:34 UTC (permalink / raw)
To: khoroshilov-ufN2psIa012HXe+LvDLADg
Cc: larry.finger-tQ5ms3gMjBLk1uMJSBkQmQ,
linville-2XuSBdqkA4R54TAoqtyWWQ,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ldv-project-tpLiQldItUH5n4uC9ZG1Ww,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
------------------------------
On Mon, Sep 2, 2013 05:06 BST Alexey Khoroshilov wrote:
>On 01.09.2013 10:51, Hin-Tak Leung wrote:
>> ------------------------------
>> On Sat, Aug 31, 2013 22:18 BST Alexey Khoroshilov wrote:
>>
>> In case of __dev_alloc_skb() failure rtl8187_init_urbs()
>> calls usb_free_urb(entry) where 'entry' can points to urb
>> allocated at the previous iteration. That means refcnt will be
>> decremented incorrectly and the urb can be used after memory
>> deallocation.
>>
>> The patch fixes the issue and implements error handling of init_urbs
>> in rtl8187_start().
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov-ufN2psIa012HXe+LvDLADg@public.gmane.org>
>> ---
>> drivers/net/wireless/rtl818x/rtl8187/dev.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> index f49220e..e83d53c 100644
>> --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> @@ -438,17 +438,16 @@ static int rtl8187_init_urbs(struct ieee80211_hw *dev)
>> skb_queue_tail(&priv->rx_queue, skb);
>> usb_anchor_urb(entry, &priv->anchored);
>> ret = usb_submit_urb(entry, GFP_KERNEL);
>> + usb_free_urb(entry);
>> if (ret) {
>> skb_unlink(skb, &priv->rx_queue);
>> usb_unanchor_urb(entry);
>> goto err;
>> }
>> - usb_free_urb(entry);
>> }
>> return ret;
>>
>> err:
>> - usb_free_urb(entry);
>> kfree_skb(skb);
>> usb_kill_anchored_urbs(&priv->anchored);
>> return ret;
>> This part looks wrong - you free_urb(entry) then unanchor_urb(entry).
>I do not see any problems here.
>usb_free_urb() just decrements refcnt of the urb.
>While usb_anchor_urb() and usb_unanchor_urb() increment and decrement it
>as well.
>So actual memory deallocation will happen in usb_unanchor_urb().
If the routines work as you say, they probably are misnamed, and/or prototyped wrongly?
Also, you are making assumptions about how they are implemented, and relying
on the implementation details to be fixed for eternity.
I am just saying,
XXX_free(some_entity);
if(condtion)
do_stuff(some_entity);
looks wrong, and if that's intentional, those routines really shouldn't be named as such.
Hin-Tak
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: 3.11-rc7:BUG: soft lockup
From: Ding Tianhong @ 2013-09-02 6:57 UTC (permalink / raw)
To: Baoquan He; +Cc: Hillf Danton, Cong Wang, LKML, Linux Kernel Network Developers
In-Reply-To: <52242AEA.9020502@gmail.com>
On 2013/9/2 14:06, Baoquan He wrote:
> Hi both,
>
> Thanks for your patches. I tried to test your patches, first the 2nd
> one, namely Hillf's patch, it's OK. Then when I wanted to reproduce and
> test Cong's patch, it failed to happen again.
>
> I remember this bug happened randomly at the very beginning,
> just after kernel compiling it always happened one day.
>
> So maybe when it happened again, I will test your patch separately.
>
> Baoquan
> Thanks
>
> On 08/31/2013 11:25 AM, Hillf Danton wrote:
>> On Fri, Aug 30, 2013 at 8:18 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> Cc'ing netdev
>>>
>>> On Fri, Aug 30, 2013 at 4:20 PM, Baoquan He <baoquan.he@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> I tried the 3.11.0-rc7+ on x86_64, and after bootup, the soft lockup bug
>>>> happened.
>>>>
>>>> [ 48.895000] BUG: soft lockup - CPU#1 stuck for 22s! [ebtables:444]
>>>> [ 48.901191] Modules linked in: bnep(F) bluetooth(F) ebtables(F)
>>>> ip6table_filter(F) ip6_tables(F) rfkill(F) snd_hda_intel(F+)
>>>> snd_hda_codec(F) snd_hwdep(F) snd_seq(F) sn)
>>>> [ 48.950034] CPU: 1 PID: 444 Comm: ebtables Tainted: GF D
>>>> 3.11.0-rc7+ #1
>>>> [ 48.957433] Hardware name: Hewlett-Packard HP Z420 Workstation/1589,
>>>> BIOS J61 v01.02 03/09/2012
>>>> [ 48.966131] task: ffff88040c2dc650 ti: ffff8804187d2000 task.ti:
>>>> ffff8804187d2000
>>>> [ 48.973610] RIP: 0010:[<ffffffff812e57a7>] [<ffffffff812e57a7>]
>>>> strcmp+0x27/0x40
>>>> [ 48.981119] RSP: 0018:ffff8804187d3db8 EFLAGS: 00000246
>>>> [ 48.986430] RAX: 0000000000000000 RBX: 00007fffda942730 RCX:
>>>> ffff8804187d3fd8
>>>> [ 48.993566] RDX: 0000000000000000 RSI: ffff8804187d3e01 RDI:
>>>> ffffffff81cb8a39
>>>> [ 49.000707] RBP: ffff8804187d3db8 R08: 00000000fffffff2 R09:
>>>> 0000000000000000
>>>> [ 49.007841] R10: 0000000000000163 R11: 0000000000000000 R12:
>>>> ffffffff8128300c
>>>> [ 49.014972] R13: ffff8804187d3d98 R14: ffff8804187d3ef4 R15:
>>>> 0000000000000004
>>>> [ 49.022112] FS: 00007faab6589740(0000) GS:ffff88042fc80000(0000)
>>>> knlGS:0000000000000000
>>>> [ 49.030194] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 49.035942] CR2: 0000003f0d810414 CR3: 000000040d2cc000 CR4:
>>>> 00000000000407e0
>>>> [ 49.043077] Stack:
>>>> [ 49.045096] ffff8804187d3de8 ffffffffa0249674 0000000000000080
>>>> ffffffff81cb8180
>>>> [ 49.052559] 00007fffda942730 ffff8804187d3ef4 ffff8804187d3ea0
>>>> ffffffffa02497a9
>>>> [ 49.060020] 0000000000000000 00007265746c6966 0000003f0d7b92c0
>>>> 00007fffda942850
>>>> [ 49.067487] Call Trace:
>>>> [ 49.069949] [<ffffffffa0249674>]
>>>> find_inlist_lock.constprop.16+0x54/0x100 [ebtables]
>>>> [ 49.077779] [<ffffffffa02497a9>] do_ebt_get_ctl+0x89/0x1d0 [ebtables]
>>>> [ 49.084306] [<ffffffff81551ca8>] nf_getsockopt+0x68/0x90
>>>> [ 49.089717] [<ffffffff81560d40>] ip_getsockopt+0x80/0xa0
>>>> [ 49.095113] [<ffffffff815835c5>] raw_getsockopt+0x25/0x50
>>>> [ 49.100588] [<ffffffff8150ddd4>] sock_common_getsockopt+0x14/0x20
>>>> [ 49.106766] [<ffffffff8150d208>] SyS_getsockopt+0x68/0xd0
>>>> [ 49.112257] [<ffffffff8162c682>] system_call_fastpath+0x16/0x1b
>>>> [ 49.118260] Code: 00 00 00 00 55 48 89 e5 eb 0e 66 2e 0f 1f 84 00 00
>>>> 00 00 00 84 c0 74 1c 48 83 c7 01 0f b6 47 ff 48 83 c6 01 3a 46 ff 74 eb
>>>> 19 c0 <83> c8 01 5d c3 0f 1
>>>> [ 76.925880] BUG: soft lockup - CPU#1 stuck for 22s! [ebtables:444]
>>>> [ 76.932069] Modules linked in: bnep(F) bluetooth(F) ebtables(F)
>>>> ip6table_filter(F) ip6_tables(F) rfkill(F) snd_hda_intel(F+)
>>>> snd_hda_codec(F) snd_hwdep(F) snd_seq(F) sn)
>>>> [ 76.980847] CPU: 1 PID: 444 Comm: ebtables Tainted: GF D
>>>> 3.11.0-rc7+ #1
>>>> [ 76.988245] Hardware name: Hewlett-Packard HP Z420 Workstation/1589,
>>>> BIOS J61 v01.02 03/09/2012
>>>> [ 76.996940] task: ffff88040c2dc650 ti: ffff8804187d2000 task.ti:
>>>> ffff8804187d2000
>>>> [ 77.004426] RIP: 0010:[<ffffffff812e5784>] [<ffffffff812e5784>]
>>>> strcmp+0x4/0x40
>>>> [ 77.011849] RSP: 0018:ffff8804187d3db8 EFLAGS: 00000212
>>>> [ 77.017163] RAX: 0000000000000001 RBX: 00007fffda942730 RCX:
>>>> ffff8804187d3fd8
>>>> [ 77.024304] RDX: 0000000000000000 RSI: ffff8804187d3e00 RDI:
>>>> ffffffff81cb8a38
>>>> [ 77.031434] RBP: ffff8804187d3db8 R08: 00000000fffffff2 R09:
>>>> 0000000000000000
>>>> [ 77.038566] R10: 0000000000000163 R11: 0000000000000000 R12:
>>>> ffffffff8128300c
>>>> [ 77.045699] R13: ffff8804187d3d98 R14: ffff8804187d3ef4 R15:
>>>> 0000000000000004
>>>> [ 77.052842] FS: 00007faab6589740(0000) GS:ffff88042fc80000(0000)
>>>> knlGS:0000000000000000
>>>> [ 77.060934] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 77.066668] CR2: 0000003f0d810414 CR3: 000000040d2cc000 CR4:
>>>> 00000000000407e0
>>>> [ 77.073799] Stack:
>>>> [ 77.075818] ffff8804187d3de8 ffffffffa0249674 0000000000000080
>>>> ffffffff81cb8180
>>>> [ 77.083287] 00007fffda942730 ffff8804187d3ef4 ffff8804187d3ea0
>>>> ffffffffa02497a9
>>>> [ 77.090749] 0000000000000000 00007265746c6966 0000003f0d7b92c0
>>>> 00007fffda942850
>>>> [ 77.098215] Call Trace:
>>>> [ 77.100668] [<ffffffffa0249674>]
>>>> find_inlist_lock.constprop.16+0x54/0x100 [ebtables]
>>>> [ 77.108500] [<ffffffffa02497a9>] do_ebt_get_ctl+0x89/0x1d0 [ebtables]
>>>> [ 77.115035] [<ffffffff81551ca8>] nf_getsockopt+0x68/0x90
>>>> [ 77.120438] [<ffffffff81560d40>] ip_getsockopt+0x80/0xa0
>>>> [ 77.125845] [<ffffffff815835c5>] raw_getsockopt+0x25/0x50
>>>> [ 77.131328] [<ffffffff8150ddd4>] sock_common_getsockopt+0x14/0x20
>>>> [ 77.137515] [<ffffffff8150d208>] SyS_getsockopt+0x68/0xd0
>>>> [ 77.143011] [<ffffffff8162c682>] system_call_fastpath+0x16/0x1b
>>>> [ 77.149019] Code: 0f 1f 80 00 00 00 00 48 83 c6 01 0f b6 4e ff 48 83
>>>> c2 01 84 c9 88 4a ff 75 ed 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 55 48
>>>> 89 e5 <eb> 0e 66 2e 0f 1f 8
>>>
>>> Does the following patch help?
>>>
>>>
>>> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
>>> index ac78024..4a0ec8f 100644
>>> --- a/net/bridge/netfilter/ebtables.c
>>> +++ b/net/bridge/netfilter/ebtables.c
>>> @@ -1503,6 +1503,10 @@ static int do_ebt_get_ctl(struct sock *sk, int
>>> cmd, void __user *user, int *len)
>>> if (copy_from_user(&tmp, user, sizeof(tmp)))
>>> return -EFAULT;
>>>
>>> + if (memscan(tmp.name, '\0', EBT_TABLE_MAXNAMELEN) ==
>>> + (tmp.name + EBT_TABLE_MAXNAMELEN))
>>> + return -EINVAL;
>>> +
>>> t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
>>> if (!t)
>>> return ret;
>>> --
>>>
>> release lock!!
>>
>> --- a/net/bridge/netfilter/ebtables.c Sat Aug 31 11:12:54 2013
>> +++ b/net/bridge/netfilter/ebtables.c Sat Aug 31 11:15:24 2013
>> @@ -332,8 +332,10 @@ find_inlist_lock_noload(struct list_head
>> return NULL;
>>
>> list_for_each_entry(e, head, list) {
>> - if (strcmp(e->name, name) == 0)
>> + if (strcmp(e->name, name) == 0) {
>> + mutex_unlock(mutex);
>> return e;
>> + }
>> }
>> *error = -ENOENT;
>> mutex_unlock(mutex);
>> --
could not release lock here, otherwise there will unlock twice. I did not meet any problem about the drivers, maybe
it is hard to occur.
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
^ permalink raw reply
* Re: 3.11-rc7:BUG: soft lockup
From: Ding Tianhong @ 2013-09-02 7:44 UTC (permalink / raw)
To: Baoquan He; +Cc: Hillf Danton, Cong Wang, LKML, Linux Kernel Network Developers
In-Reply-To: <52242AEA.9020502@gmail.com>
On 2013/9/2 14:06, Baoquan He wrote:
> Hi both,
>
> Thanks for your patches. I tried to test your patches, first the 2nd
> one, namely Hillf's patch, it's OK. Then when I wanted to reproduce and
> test Cong's patch, it failed to happen again.
>
> I remember this bug happened randomly at the very beginning,
> just after kernel compiling it always happened one day.
>
> So maybe when it happened again, I will test your patch separately.
>
> Baoquan
> Thanks
>
> On 08/31/2013 11:25 AM, Hillf Danton wrote:
>> On Fri, Aug 30, 2013 at 8:18 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> Cc'ing netdev
>>>
>>> On Fri, Aug 30, 2013 at 4:20 PM, Baoquan He <baoquan.he@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> I tried the 3.11.0-rc7+ on x86_64, and after bootup, the soft lockup bug
>>>> happened.
>>>>
>>>> [ 48.895000] BUG: soft lockup - CPU#1 stuck for 22s! [ebtables:444]
>>>> [ 48.901191] Modules linked in: bnep(F) bluetooth(F) ebtables(F)
>>>> ip6table_filter(F) ip6_tables(F) rfkill(F) snd_hda_intel(F+)
>>>> snd_hda_codec(F) snd_hwdep(F) snd_seq(F) sn)
>>>> [ 48.950034] CPU: 1 PID: 444 Comm: ebtables Tainted: GF D
>>>> 3.11.0-rc7+ #1
>>>> [ 48.957433] Hardware name: Hewlett-Packard HP Z420 Workstation/1589,
>>>> BIOS J61 v01.02 03/09/2012
>>>> [ 48.966131] task: ffff88040c2dc650 ti: ffff8804187d2000 task.ti:
>>>> ffff8804187d2000
>>>> [ 48.973610] RIP: 0010:[<ffffffff812e57a7>] [<ffffffff812e57a7>]
>>>> strcmp+0x27/0x40
>>>> [ 48.981119] RSP: 0018:ffff8804187d3db8 EFLAGS: 00000246
>>>> [ 48.986430] RAX: 0000000000000000 RBX: 00007fffda942730 RCX:
>>>> ffff8804187d3fd8
>>>> [ 48.993566] RDX: 0000000000000000 RSI: ffff8804187d3e01 RDI:
>>>> ffffffff81cb8a39
>>>> [ 49.000707] RBP: ffff8804187d3db8 R08: 00000000fffffff2 R09:
>>>> 0000000000000000
>>>> [ 49.007841] R10: 0000000000000163 R11: 0000000000000000 R12:
>>>> ffffffff8128300c
>>>> [ 49.014972] R13: ffff8804187d3d98 R14: ffff8804187d3ef4 R15:
>>>> 0000000000000004
>>>> [ 49.022112] FS: 00007faab6589740(0000) GS:ffff88042fc80000(0000)
>>>> knlGS:0000000000000000
>>>> [ 49.030194] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 49.035942] CR2: 0000003f0d810414 CR3: 000000040d2cc000 CR4:
>>>> 00000000000407e0
>>>> [ 49.043077] Stack:
>>>> [ 49.045096] ffff8804187d3de8 ffffffffa0249674 0000000000000080
>>>> ffffffff81cb8180
>>>> [ 49.052559] 00007fffda942730 ffff8804187d3ef4 ffff8804187d3ea0
>>>> ffffffffa02497a9
>>>> [ 49.060020] 0000000000000000 00007265746c6966 0000003f0d7b92c0
>>>> 00007fffda942850
>>>> [ 49.067487] Call Trace:
>>>> [ 49.069949] [<ffffffffa0249674>]
>>>> find_inlist_lock.constprop.16+0x54/0x100 [ebtables]
>>>> [ 49.077779] [<ffffffffa02497a9>] do_ebt_get_ctl+0x89/0x1d0 [ebtables]
>>>> [ 49.084306] [<ffffffff81551ca8>] nf_getsockopt+0x68/0x90
>>>> [ 49.089717] [<ffffffff81560d40>] ip_getsockopt+0x80/0xa0
>>>> [ 49.095113] [<ffffffff815835c5>] raw_getsockopt+0x25/0x50
>>>> [ 49.100588] [<ffffffff8150ddd4>] sock_common_getsockopt+0x14/0x20
>>>> [ 49.106766] [<ffffffff8150d208>] SyS_getsockopt+0x68/0xd0
>>>> [ 49.112257] [<ffffffff8162c682>] system_call_fastpath+0x16/0x1b
>>>> [ 49.118260] Code: 00 00 00 00 55 48 89 e5 eb 0e 66 2e 0f 1f 84 00 00
>>>> 00 00 00 84 c0 74 1c 48 83 c7 01 0f b6 47 ff 48 83 c6 01 3a 46 ff 74 eb
>>>> 19 c0 <83> c8 01 5d c3 0f 1
>>>> [ 76.925880] BUG: soft lockup - CPU#1 stuck for 22s! [ebtables:444]
>>>> [ 76.932069] Modules linked in: bnep(F) bluetooth(F) ebtables(F)
>>>> ip6table_filter(F) ip6_tables(F) rfkill(F) snd_hda_intel(F+)
>>>> snd_hda_codec(F) snd_hwdep(F) snd_seq(F) sn)
>>>> [ 76.980847] CPU: 1 PID: 444 Comm: ebtables Tainted: GF D
>>>> 3.11.0-rc7+ #1
>>>> [ 76.988245] Hardware name: Hewlett-Packard HP Z420 Workstation/1589,
>>>> BIOS J61 v01.02 03/09/2012
>>>> [ 76.996940] task: ffff88040c2dc650 ti: ffff8804187d2000 task.ti:
>>>> ffff8804187d2000
>>>> [ 77.004426] RIP: 0010:[<ffffffff812e5784>] [<ffffffff812e5784>]
>>>> strcmp+0x4/0x40
>>>> [ 77.011849] RSP: 0018:ffff8804187d3db8 EFLAGS: 00000212
>>>> [ 77.017163] RAX: 0000000000000001 RBX: 00007fffda942730 RCX:
>>>> ffff8804187d3fd8
>>>> [ 77.024304] RDX: 0000000000000000 RSI: ffff8804187d3e00 RDI:
>>>> ffffffff81cb8a38
>>>> [ 77.031434] RBP: ffff8804187d3db8 R08: 00000000fffffff2 R09:
>>>> 0000000000000000
>>>> [ 77.038566] R10: 0000000000000163 R11: 0000000000000000 R12:
>>>> ffffffff8128300c
>>>> [ 77.045699] R13: ffff8804187d3d98 R14: ffff8804187d3ef4 R15:
>>>> 0000000000000004
>>>> [ 77.052842] FS: 00007faab6589740(0000) GS:ffff88042fc80000(0000)
>>>> knlGS:0000000000000000
>>>> [ 77.060934] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 77.066668] CR2: 0000003f0d810414 CR3: 000000040d2cc000 CR4:
>>>> 00000000000407e0
>>>> [ 77.073799] Stack:
>>>> [ 77.075818] ffff8804187d3de8 ffffffffa0249674 0000000000000080
>>>> ffffffff81cb8180
>>>> [ 77.083287] 00007fffda942730 ffff8804187d3ef4 ffff8804187d3ea0
>>>> ffffffffa02497a9
>>>> [ 77.090749] 0000000000000000 00007265746c6966 0000003f0d7b92c0
>>>> 00007fffda942850
>>>> [ 77.098215] Call Trace:
>>>> [ 77.100668] [<ffffffffa0249674>]
>>>> find_inlist_lock.constprop.16+0x54/0x100 [ebtables]
>>>> [ 77.108500] [<ffffffffa02497a9>] do_ebt_get_ctl+0x89/0x1d0 [ebtables]
>>>> [ 77.115035] [<ffffffff81551ca8>] nf_getsockopt+0x68/0x90
>>>> [ 77.120438] [<ffffffff81560d40>] ip_getsockopt+0x80/0xa0
>>>> [ 77.125845] [<ffffffff815835c5>] raw_getsockopt+0x25/0x50
>>>> [ 77.131328] [<ffffffff8150ddd4>] sock_common_getsockopt+0x14/0x20
>>>> [ 77.137515] [<ffffffff8150d208>] SyS_getsockopt+0x68/0xd0
>>>> [ 77.143011] [<ffffffff8162c682>] system_call_fastpath+0x16/0x1b
>>>> [ 77.149019] Code: 0f 1f 80 00 00 00 00 48 83 c6 01 0f b6 4e ff 48 83
>>>> c2 01 84 c9 88 4a ff 75 ed 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 55 48
>>>> 89 e5 <eb> 0e 66 2e 0f 1f 8
>>>
>>> Does the following patch help?
>>>
>>>
>>> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
>>> index ac78024..4a0ec8f 100644
>>> --- a/net/bridge/netfilter/ebtables.c
>>> +++ b/net/bridge/netfilter/ebtables.c
>>> @@ -1503,6 +1503,10 @@ static int do_ebt_get_ctl(struct sock *sk, int
>>> cmd, void __user *user, int *len)
>>> if (copy_from_user(&tmp, user, sizeof(tmp)))
>>> return -EFAULT;
>>>
>>> + if (memscan(tmp.name, '\0', EBT_TABLE_MAXNAMELEN) ==
>>> + (tmp.name + EBT_TABLE_MAXNAMELEN))
>>> + return -EINVAL;
>>> +
>>> t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
>>> if (!t)
>>> return ret;
>>> --
>>>
>> release lock!!
>>
>> --- a/net/bridge/netfilter/ebtables.c Sat Aug 31 11:12:54 2013
>> +++ b/net/bridge/netfilter/ebtables.c Sat Aug 31 11:15:24 2013
>> @@ -332,8 +332,10 @@ find_inlist_lock_noload(struct list_head
>> return NULL;
>>
>> list_for_each_entry(e, head, list) {
>> - if (strcmp(e->name, name) == 0)
>> + if (strcmp(e->name, name) == 0) {
>> + mutex_unlock(mutex);
>> return e;
>> + }
>> }
>> *error = -ENOENT;
>> mutex_unlock(mutex);
>> --
>
please try this patch and give me the result, thanks.
Return the correct value if mutex_lock_interruptible() failed, avoid
confusion with that the modules is not exist, and deal with the return
value in right way.
if mutex_lock_interrupt() failed, sh
Signed-off-by: root <root@linux-yocto.site>
---
net/bridge/netfilter/ebtables.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index ac78024..e7fe9f8 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -322,17 +322,14 @@ static inline void *
find_inlist_lock_noload(struct list_head *head, const char *name, int *error,
struct mutex *mutex)
{
- struct {
- struct list_head list;
- char name[EBT_FUNCTION_MAXNAMELEN];
- } *e;
+ struct ebt_table *e;
*error = mutex_lock_interruptible(mutex);
if (*error != 0)
- return NULL;
+ return ERR_PTR(-EINTR);
list_for_each_entry(e, head, list) {
- if (strcmp(e->name, name) == 0)
+ if (strcmp(e->name, name) == 0 && try_module_get(e->me))
return e;
}
*error = -ENOENT;
@@ -1005,7 +1002,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
goto free_counterstmp;
t = find_table_lock(net, repl->name, &ret, &ebt_mutex);
- if (!t) {
+ if (IS_ERR_OR_NULL(t)) {
ret = -ENOENT;
goto free_iterate;
}
@@ -1284,7 +1281,7 @@ static int do_update_counters(struct net *net, const char *name,
return -ENOMEM;
t = find_table_lock(net, name, &ret, &ebt_mutex);
- if (!t)
+ if (IS_ERR_OR_NULL(t))
goto free_tmp;
if (num_counters != t->private->nentries) {
@@ -1504,7 +1501,7 @@ static int do_ebt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
return -EFAULT;
t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
- if (!t)
+ if (IS_ERR_OR_NULL(t))
return ret;
switch(cmd) {
@@ -2319,7 +2316,7 @@ static int compat_do_ebt_get_ctl(struct sock *sk, int cmd,
return -EFAULT;
t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
- if (!t)
+ if (IS_ERR_OR_NULL(t))
return ret;
xt_compat_lock(NFPROTO_BRIDGE);
--
1.8.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
^ permalink raw reply related
* Re: Is fallback vhost_net to qemu for live migrate available？
From: Wei Liu @ 2013-09-02 7:57 UTC (permalink / raw)
To: Qin Chuanyu
Cc: Anthony Liguori, Michael S. Tsirkin, jasowang, KVM list, netdev,
qianhuibin, xen-devel@lists.xen.org, wangfuhai, likunyun,
liuyongan, liuyingdong, wei.liu2
In-Reply-To: <522174D7.6080903@huawei.com>
On Sat, Aug 31, 2013 at 12:45:11PM +0800, Qin Chuanyu wrote:
> On 2013/8/30 0:08, Anthony Liguori wrote:
> >Hi Qin,
>
> >>By change the memory copy and notify mechanism ，currently virtio-net with
> >>vhost_net could run on Xen with good performance。
> >
> >I think the key in doing this would be to implement a property
> >ioeventfd and irqfd interface in the driver domain kernel. Just
> >hacking vhost_net with Xen specific knowledge would be pretty nasty
> >IMHO.
> >
> Yes, I add a kernel module which persist virtio-net pio_addr and
> msix address as what kvm module did. Guest wake up vhost thread by
> adding a hook func in evtchn_interrupt.
>
> >Did you modify the front end driver to do grant table mapping or is
> >this all being done by mapping the domain's memory?
> >
> There is nothing changed in front end driver. Currently I use
> alloc_vm_area to get address space， and map the domain's memory as
> what what qemu did.
>
You mean you're using xc_map_foreign_range and friends in the backend to
map guest memory? That's not very desirable as it violates Xen's
security model. It would not be too hard to pass grant references
instead of guest physical memory address IMHO.
Wei.
^ permalink raw reply
* [PATCH 1/5] net: emac: use platform_{get,set}_drvdata()
From: Jingoo Han @ 2013-09-02 8:06 UTC (permalink / raw)
To: 'David S. Miller'
Cc: netdev, 'Petri Gynther', 'Jingoo Han'
Use the wrapper functions for getting and setting the driver data
using platform_device instead of using dev_{get,set}_drvdata()
with &pdev->dev, so we can directly pass a struct platform_device.
This is a purely cosmetic change.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/net/ethernet/ibm/emac/core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 2d3b064..6b5c722 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -2312,7 +2312,7 @@ static int emac_check_deps(struct emac_instance *dev,
if (deps[i].ofdev == NULL)
continue;
if (deps[i].drvdata == NULL)
- deps[i].drvdata = dev_get_drvdata(&deps[i].ofdev->dev);
+ deps[i].drvdata = platform_get_drvdata(deps[i].ofdev);
if (deps[i].drvdata != NULL)
there++;
}
@@ -2799,9 +2799,9 @@ static int emac_probe(struct platform_device *ofdev)
/* display more info about what's missing ? */
goto err_reg_unmap;
}
- dev->mal = dev_get_drvdata(&dev->mal_dev->dev);
+ dev->mal = platform_get_drvdata(dev->mal_dev);
if (dev->mdio_dev != NULL)
- dev->mdio_instance = dev_get_drvdata(&dev->mdio_dev->dev);
+ dev->mdio_instance = platform_get_drvdata(dev->mdio_dev);
/* Register with MAL */
dev->commac.ops = &emac_commac_ops;
@@ -2892,7 +2892,7 @@ static int emac_probe(struct platform_device *ofdev)
* fully initialized
*/
wmb();
- dev_set_drvdata(&ofdev->dev, dev);
+ platform_set_drvdata(ofdev, dev);
/* There's a new kid in town ! Let's tell everybody */
wake_up_all(&emac_probe_wait);
@@ -2951,7 +2951,7 @@ static int emac_probe(struct platform_device *ofdev)
static int emac_remove(struct platform_device *ofdev)
{
- struct emac_instance *dev = dev_get_drvdata(&ofdev->dev);
+ struct emac_instance *dev = platform_get_drvdata(ofdev);
DBG(dev, "remove" NL);
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/5] net: sunhme: use platform_{get,set}_drvdata()
From: Jingoo Han @ 2013-09-02 8:08 UTC (permalink / raw)
To: 'David S. Miller'
Cc: netdev, 'Jon Mason', 'Francois Romieu',
'Jingoo Han'
In-Reply-To: <000f01cea7b3$6226fd30$2674f790$%han@samsung.com>
Use the wrapper functions for getting and setting the driver data
using platform_device instead of using dev_{get,set}_drvdata()
with &pdev->dev, so we can directly pass a struct platform_device.
This is a purely cosmetic change.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/net/ethernet/sun/sunhme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index c67e683..227c499 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2798,7 +2798,7 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
goto err_out_free_coherent;
}
- dev_set_drvdata(&op->dev, hp);
+ platform_set_drvdata(op, hp);
if (qfe_slot != -1)
printk(KERN_INFO "%s: Quattro HME slot %d (SBUS) 10/100baseT Ethernet ",
--
1.7.10.4
^ permalink raw reply related
* [PATCH 3/5] net: mdio-octeon: use platform_{get,set}_drvdata()
From: Jingoo Han @ 2013-09-02 8:10 UTC (permalink / raw)
To: 'David S. Miller'
Cc: netdev, 'David Daney', 'Jingoo Han'
In-Reply-To: <000f01cea7b3$6226fd30$2674f790$%han@samsung.com>
Use the wrapper functions for getting and setting the driver data
using platform_device instead of using dev_{get,set}_drvdata()
with &pdev->dev, so we can directly pass a struct platform_device.
This is a purely cosmetic change.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/net/phy/mdio-octeon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio-octeon.c b/drivers/net/phy/mdio-octeon.c
index 7f18f80..6aee02e 100644
--- a/drivers/net/phy/mdio-octeon.c
+++ b/drivers/net/phy/mdio-octeon.c
@@ -244,7 +244,7 @@ static int octeon_mdiobus_remove(struct platform_device *pdev)
struct octeon_mdiobus *bus;
union cvmx_smix_en smi_en;
- bus = dev_get_drvdata(&pdev->dev);
+ bus = platform_get_drvdata(pdev);
mdiobus_unregister(bus->mii_bus);
mdiobus_free(bus->mii_bus);
--
1.7.10.4
^ permalink raw reply related
* Re: Is fallback vhost_net to qemu for live migrate available？
From: Michael S. Tsirkin @ 2013-09-02 8:11 UTC (permalink / raw)
To: Wei Liu
Cc: Qin Chuanyu, Anthony Liguori, jasowang, KVM list, netdev,
qianhuibin, xen-devel@lists.xen.org, wangfuhai, likunyun,
liuyongan, liuyingdong
In-Reply-To: <20130902075722.GZ15729@zion.uk.xensource.com>
On Mon, Sep 02, 2013 at 08:57:22AM +0100, Wei Liu wrote:
> On Sat, Aug 31, 2013 at 12:45:11PM +0800, Qin Chuanyu wrote:
> > On 2013/8/30 0:08, Anthony Liguori wrote:
> > >Hi Qin,
> >
> > >>By change the memory copy and notify mechanism ，currently virtio-net with
> > >>vhost_net could run on Xen with good performance。
> > >
> > >I think the key in doing this would be to implement a property
> > >ioeventfd and irqfd interface in the driver domain kernel. Just
> > >hacking vhost_net with Xen specific knowledge would be pretty nasty
> > >IMHO.
> > >
> > Yes, I add a kernel module which persist virtio-net pio_addr and
> > msix address as what kvm module did. Guest wake up vhost thread by
> > adding a hook func in evtchn_interrupt.
> >
> > >Did you modify the front end driver to do grant table mapping or is
> > >this all being done by mapping the domain's memory?
> > >
> > There is nothing changed in front end driver. Currently I use
> > alloc_vm_area to get address space， and map the domain's memory as
> > what what qemu did.
> >
>
> You mean you're using xc_map_foreign_range and friends in the backend to
> map guest memory? That's not very desirable as it violates Xen's
> security model. It would not be too hard to pass grant references
> instead of guest physical memory address IMHO.
>
> Wei.
It's a start and it should make it fast and work with existing
infrastructure in the host, though.
--
MST
^ permalink raw reply
* [PATCH 4/5] net: tulip: use pci_{get,set}_drvdata()
From: Jingoo Han @ 2013-09-02 8:11 UTC (permalink / raw)
To: 'David S. Miller'
Cc: netdev, 'Danny Kukawka', 'Jeff Kirsher',
'Jingoo Han'
In-Reply-To: <000f01cea7b3$6226fd30$2674f790$%han@samsung.com>
Use the wrapper functions for getting and setting the driver data
using pci_dev instead of using dev_{get,set}_drvdata() with
&pdev->dev, so we can directly pass a struct pci_dev. This is
a purely cosmetic change.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/net/ethernet/dec/tulip/de4x5.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/dec/tulip/de4x5.c b/drivers/net/ethernet/dec/tulip/de4x5.c
index 4c83003..2db6c57 100644
--- a/drivers/net/ethernet/dec/tulip/de4x5.c
+++ b/drivers/net/ethernet/dec/tulip/de4x5.c
@@ -2319,7 +2319,7 @@ static void de4x5_pci_remove(struct pci_dev *pdev)
struct net_device *dev;
u_long iobase;
- dev = dev_get_drvdata(&pdev->dev);
+ dev = pci_get_drvdata(pdev);
iobase = dev->base_addr;
unregister_netdev (dev);
--
1.7.10.4
^ permalink raw reply related
* [PATCH 5/5] net: sunhme: use pci_{get,set}_drvdata()
From: Jingoo Han @ 2013-09-02 8:12 UTC (permalink / raw)
To: 'David S. Miller'
Cc: netdev, 'Jon Mason', 'Francois Romieu',
'Jingoo Han'
In-Reply-To: <000f01cea7b3$6226fd30$2674f790$%han@samsung.com>
Use the wrapper functions for getting and setting the driver data
using pci_dev instead of using dev_{get,set}_drvdata() with
&pdev->dev, so we can directly pass a struct pci_dev. This is
a purely cosmetic change.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/net/ethernet/sun/sunhme.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index 227c499..e37b587 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -3111,7 +3111,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
goto err_out_iounmap;
}
- dev_set_drvdata(&pdev->dev, hp);
+ pci_set_drvdata(pdev, hp);
if (!qfe_slot) {
struct pci_dev *qpdev = qp->quattro_dev;
@@ -3159,7 +3159,7 @@ err_out:
static void happy_meal_pci_remove(struct pci_dev *pdev)
{
- struct happy_meal *hp = dev_get_drvdata(&pdev->dev);
+ struct happy_meal *hp = pci_get_drvdata(pdev);
struct net_device *net_dev = hp->dev;
unregister_netdev(net_dev);
@@ -3171,7 +3171,7 @@ static void happy_meal_pci_remove(struct pci_dev *pdev)
free_netdev(net_dev);
- dev_set_drvdata(&pdev->dev, NULL);
+ pci_set_drvdata(pdev, NULL);
}
static DEFINE_PCI_DEVICE_TABLE(happymeal_pci_ids) = {
--
1.7.10.4
^ permalink raw reply related
* Re: 3.11-rc7:BUG: soft lockup
From: Baoquan He @ 2013-09-02 8:24 UTC (permalink / raw)
To: Ding Tianhong
Cc: Hillf Danton, Cong Wang, LKML, Linux Kernel Network Developers
In-Reply-To: <522441E9.8040809@huawei.com>
Hi Tianhong,
I applied your patch and execute below cmd.
Then keyboard inputting problems happened,
I can't enter user/password correctly, then reboot again, it's OK now.
The original bug still can't be reproduced.
make SUBDIRS=net/bridge/netfilter/
cp /net/bridge/netfilter/ebtables.ko
/lib/modules/3.11.0-rc7+/kernel/net/bridge/netfilter/ebtables.ko
depmod
modprobe ebtables.ko
reboot
Baoquan
Thanks
> please try this patch and give me the result, thanks.
>
> Return the correct value if mutex_lock_interruptible() failed, avoid
> confusion with that the modules is not exist, and deal with the return
> value in right way.
>
> if mutex_lock_interrupt() failed, sh
>
> Signed-off-by: root <root@linux-yocto.site>
> ---
> net/bridge/netfilter/ebtables.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index ac78024..e7fe9f8 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -322,17 +322,14 @@ static inline void *
> find_inlist_lock_noload(struct list_head *head, const char *name, int *error,
> struct mutex *mutex)
> {
> - struct {
> - struct list_head list;
> - char name[EBT_FUNCTION_MAXNAMELEN];
> - } *e;
> + struct ebt_table *e;
>
> *error = mutex_lock_interruptible(mutex);
> if (*error != 0)
> - return NULL;
> + return ERR_PTR(-EINTR);
>
> list_for_each_entry(e, head, list) {
> - if (strcmp(e->name, name) == 0)
> + if (strcmp(e->name, name) == 0 && try_module_get(e->me))
> return e;
> }
> *error = -ENOENT;
> @@ -1005,7 +1002,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
> goto free_counterstmp;
>
> t = find_table_lock(net, repl->name, &ret, &ebt_mutex);
> - if (!t) {
> + if (IS_ERR_OR_NULL(t)) {
> ret = -ENOENT;
> goto free_iterate;
> }
> @@ -1284,7 +1281,7 @@ static int do_update_counters(struct net *net, const char *name,
> return -ENOMEM;
>
> t = find_table_lock(net, name, &ret, &ebt_mutex);
> - if (!t)
> + if (IS_ERR_OR_NULL(t))
> goto free_tmp;
>
> if (num_counters != t->private->nentries) {
> @@ -1504,7 +1501,7 @@ static int do_ebt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
> return -EFAULT;
>
> t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
> - if (!t)
> + if (IS_ERR_OR_NULL(t))
> return ret;
>
> switch(cmd) {
> @@ -2319,7 +2316,7 @@ static int compat_do_ebt_get_ctl(struct sock *sk, int cmd,
> return -EFAULT;
>
> t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
> - if (!t)
> + if (IS_ERR_OR_NULL(t))
> return ret;
>
> xt_compat_lock(NFPROTO_BRIDGE);
^ permalink raw reply
* Re: [PATCH V2 6/6] vhost_net: correctly limit the max pending buffers
From: Jason Wang @ 2013-09-02 8:37 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <52243071.5090303@redhat.com>
On 09/02/2013 02:30 PM, Jason Wang wrote:
> On 09/02/2013 01:56 PM, Michael S. Tsirkin wrote:
>> > On Fri, Aug 30, 2013 at 12:29:22PM +0800, Jason Wang wrote:
>>> >> As Michael point out, We used to limit the max pending DMAs to get better cache
>>> >> utilization. But it was not done correctly since it was one done when there's no
>>> >> new buffers submitted from guest. Guest can easily exceeds the limitation by
>>> >> keeping sending packets.
>>> >>
>>> >> So this patch moves the check into main loop. Tests shows about 5%-10%
>>> >> improvement on per cpu throughput for guest tx. But a 5% drop on per cpu
>>> >> transaction rate for a single session TCP_RR.
>> > Any explanation for the drop? single session TCP_RR is unlikely to
>> > exceed VHOST_MAX_PEND, correct?
> Unlikely to exceed. Recheck the result, looks like it was not stable
> enough. Will re-test and report.
Re-tested with more iterations, result shows no difference on TCP_RR
test and 5%-10% improvement on per cpu throughput for guest tx.
Will post V3 soon.
^ permalink raw reply
* [PATCH V3 0/6] vhost code cleanup and minor enhancement
From: Jason Wang @ 2013-09-02 8:40 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel
This series tries to unify and simplify vhost codes especially for
zerocopy. With this series, 5% - 10% improvement for per cpu throughput were
seen during netperf guest sending test.
Plase review.
Changes from V2:
- Typo fixes and code style fix
- Add performance gain in the commit log of patch 2/6
- Retest the update the result in patch 6/6
Changes from V1:
- Fix the zerocopy enabling check by changing the check of upend_idx != done_idx
to (upend_idx + 1) % UIO_MAXIOV == done_idx.
- Switch to use put_user() in __vhost_add_used_n() if there's only one used
- Keep the max pending check based on Michael's suggestion.
Jason Wang (6):
vhost_net: make vhost_zerocopy_signal_used() return void
vhost_net: use vhost_add_used_and_signal_n() in
vhost_zerocopy_signal_used()
vhost: switch to use vhost_add_used_n()
vhost_net: determine whether or not to use zerocopy at one time
vhost_net: poll vhost queue after marking DMA is done
vhost_net: correctly limit the max pending buffers
drivers/vhost/net.c | 92 ++++++++++++++++++++++--------------------------
drivers/vhost/vhost.c | 54 ++++++----------------------
2 files changed, 54 insertions(+), 92 deletions(-)
^ permalink raw reply
* [PATCH V3 1/6] vhost_net: make vhost_zerocopy_signal_used() return void
From: Jason Wang @ 2013-09-02 8:40 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1378111261-14826-1-git-send-email-jasowang@redhat.com>
None of its caller use its return value, so let it return void.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 969a859..280ee66 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -276,8 +276,8 @@ static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
* of used idx. Once lower device DMA done contiguously, we will signal KVM
* guest used idx.
*/
-static int vhost_zerocopy_signal_used(struct vhost_net *net,
- struct vhost_virtqueue *vq)
+static void vhost_zerocopy_signal_used(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
{
struct vhost_net_virtqueue *nvq =
container_of(vq, struct vhost_net_virtqueue, vq);
@@ -297,7 +297,6 @@ static int vhost_zerocopy_signal_used(struct vhost_net *net,
}
if (j)
nvq->done_idx = i;
- return j;
}
static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
--
1.7.1
^ permalink raw reply related
* [PATCH V3 2/6] vhost_net: use vhost_add_used_and_signal_n() in vhost_zerocopy_signal_used()
From: Jason Wang @ 2013-09-02 8:40 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1378111261-14826-1-git-send-email-jasowang@redhat.com>
We tend to batch the used adding and signaling in vhost_zerocopy_callback()
which may result more than 100 used buffers to be updated in
vhost_zerocopy_signal_used() in some cases. So switch to use
vhost_add_used_and_signal_n() to avoid multiple calls to
vhost_add_used_and_signal(). Which means much less times of used index
updating and memory barriers.
2% performance improvement were seen on netperf TCP_RR test.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 280ee66..8a6dd0d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -281,7 +281,7 @@ static void vhost_zerocopy_signal_used(struct vhost_net *net,
{
struct vhost_net_virtqueue *nvq =
container_of(vq, struct vhost_net_virtqueue, vq);
- int i;
+ int i, add;
int j = 0;
for (i = nvq->done_idx; i != nvq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
@@ -289,14 +289,17 @@ static void vhost_zerocopy_signal_used(struct vhost_net *net,
vhost_net_tx_err(net);
if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
- vhost_add_used_and_signal(vq->dev, vq,
- vq->heads[i].id, 0);
++j;
} else
break;
}
- if (j)
- nvq->done_idx = i;
+ while (j) {
+ add = min(UIO_MAXIOV - nvq->done_idx, j);
+ vhost_add_used_and_signal_n(vq->dev, vq,
+ &vq->heads[nvq->done_idx], add);
+ nvq->done_idx = (nvq->done_idx + add) % UIO_MAXIOV;
+ j -= add;
+ }
}
static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
--
1.7.1
^ permalink raw reply related
* [PATCH V3 3/6] vhost: switch to use vhost_add_used_n()
From: Jason Wang @ 2013-09-02 8:40 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1378111261-14826-1-git-send-email-jasowang@redhat.com>
Let vhost_add_used() to use vhost_add_used_n() to reduce the code
duplication. To avoid the overhead brought by __copy_to_user(). We will use
put_user() when one used need to be added.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 54 ++++++++++--------------------------------------
1 files changed, 12 insertions(+), 42 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 448efe0..9a9502a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1332,48 +1332,9 @@ EXPORT_SYMBOL_GPL(vhost_discard_vq_desc);
* want to notify the guest, using eventfd. */
int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
{
- struct vring_used_elem __user *used;
+ struct vring_used_elem heads = { head, len };
- /* 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;
- }
- /* Make sure buffer is written before we update index. */
- smp_wmb();
- if (__put_user(vq->last_used_idx + 1, &vq->used->idx)) {
- vq_err(vq, "Failed to increment used idx");
- return -EFAULT;
- }
- if (unlikely(vq->log_used)) {
- /* Make sure data is seen before log. */
- smp_wmb();
- /* Log used ring entry write. */
- log_write(vq->log_base,
- vq->log_addr +
- ((void __user *)used - (void __user *)vq->used),
- sizeof *used);
- /* Log used index update. */
- log_write(vq->log_base,
- vq->log_addr + offsetof(struct vring_used, idx),
- sizeof vq->used->idx);
- if (vq->log_ctx)
- eventfd_signal(vq->log_ctx, 1);
- }
- vq->last_used_idx++;
- /* If the driver never bothers to signal in a very long while,
- * used index might wrap around. If that happens, invalidate
- * signalled_used index we stored. TODO: make sure driver
- * signals at least once in 2^16 and remove this. */
- if (unlikely(vq->last_used_idx == vq->signalled_used))
- vq->signalled_used_valid = false;
- return 0;
+ return vhost_add_used_n(vq, &heads, 1);
}
EXPORT_SYMBOL_GPL(vhost_add_used);
@@ -1387,7 +1348,16 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx % vq->num;
used = vq->used->ring + start;
- if (__copy_to_user(used, heads, count * sizeof *used)) {
+ if (count == 1) {
+ if (__put_user(heads[0].id, &used->id)) {
+ vq_err(vq, "Failed to write used id");
+ return -EFAULT;
+ }
+ if (__put_user(heads[0].len, &used->len)) {
+ vq_err(vq, "Failed to write used len");
+ return -EFAULT;
+ }
+ } else if (__copy_to_user(used, heads, count * sizeof *used)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
--
1.7.1
^ permalink raw reply related
* [PATCH V3 4/6] vhost_net: determine whether or not to use zerocopy at one time
From: Jason Wang @ 2013-09-02 8:40 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1378111261-14826-1-git-send-email-jasowang@redhat.com>
Currently, even if the packet length is smaller than VHOST_GOODCOPY_LEN, if
upend_idx != done_idx we still set zcopy_used to true and rollback this choice
later. This could be avoided by determining zerocopy once by checking all
conditions at one time before.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 47 ++++++++++++++++++++---------------------------
1 files changed, 20 insertions(+), 27 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8a6dd0d..3f89dea 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -404,43 +404,36 @@ static void handle_tx(struct vhost_net *net)
iov_length(nvq->hdr, s), hdr_size);
break;
}
- zcopy_used = zcopy && (len >= VHOST_GOODCOPY_LEN ||
- nvq->upend_idx != nvq->done_idx);
+
+ zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
+ && (nvq->upend_idx + 1) % UIO_MAXIOV !=
+ nvq->done_idx
+ && vhost_net_tx_select_zcopy(net);
/* use msg_control to pass vhost zerocopy ubuf info to skb */
if (zcopy_used) {
+ struct ubuf_info *ubuf;
+ ubuf = nvq->ubuf_info + nvq->upend_idx;
+
vq->heads[nvq->upend_idx].id = head;
- if (!vhost_net_tx_select_zcopy(net) ||
- len < VHOST_GOODCOPY_LEN) {
- /* copy don't need to wait for DMA done */
- vq->heads[nvq->upend_idx].len =
- VHOST_DMA_DONE_LEN;
- msg.msg_control = NULL;
- msg.msg_controllen = 0;
- ubufs = NULL;
- } else {
- struct ubuf_info *ubuf;
- ubuf = nvq->ubuf_info + nvq->upend_idx;
-
- vq->heads[nvq->upend_idx].len =
- VHOST_DMA_IN_PROGRESS;
- ubuf->callback = vhost_zerocopy_callback;
- ubuf->ctx = nvq->ubufs;
- ubuf->desc = nvq->upend_idx;
- msg.msg_control = ubuf;
- msg.msg_controllen = sizeof(ubuf);
- ubufs = nvq->ubufs;
- kref_get(&ubufs->kref);
- }
+ vq->heads[nvq->upend_idx].len = VHOST_DMA_IN_PROGRESS;
+ ubuf->callback = vhost_zerocopy_callback;
+ ubuf->ctx = nvq->ubufs;
+ ubuf->desc = nvq->upend_idx;
+ msg.msg_control = ubuf;
+ msg.msg_controllen = sizeof(ubuf);
+ ubufs = nvq->ubufs;
+ kref_get(&ubufs->kref);
nvq->upend_idx = (nvq->upend_idx + 1) % UIO_MAXIOV;
- } else
+ } else {
msg.msg_control = NULL;
+ ubufs = NULL;
+ }
/* TODO: Check specific error and bomb out unless ENOBUFS? */
err = sock->ops->sendmsg(NULL, sock, &msg, len);
if (unlikely(err < 0)) {
if (zcopy_used) {
- if (ubufs)
- vhost_net_ubuf_put(ubufs);
+ vhost_net_ubuf_put(ubufs);
nvq->upend_idx = ((unsigned)nvq->upend_idx - 1)
% UIO_MAXIOV;
}
--
1.7.1
^ permalink raw reply related
* [PATCH V3 5/6] vhost_net: poll vhost queue after marking DMA is done
From: Jason Wang @ 2013-09-02 8:41 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel; +Cc: Jason Wang
In-Reply-To: <1378111261-14826-1-git-send-email-jasowang@redhat.com>
We used to poll vhost queue before making DMA is done, this is racy if vhost
thread were waked up before marking DMA is done which can result the signal to
be missed. Fix this by always polling the vhost thread before DMA is done.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
- The patch is needed for stable 3.4+
---
drivers/vhost/net.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 3f89dea..8e9dc55 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -308,6 +308,11 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
struct vhost_virtqueue *vq = ubufs->vq;
int cnt = atomic_read(&ubufs->kref.refcount);
+ /* set len to mark this desc buffers done DMA */
+ vq->heads[ubuf->desc].len = success ?
+ VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
+ vhost_net_ubuf_put(ubufs);
+
/*
* Trigger polling thread if guest stopped submitting new buffers:
* in this case, the refcount after decrement will eventually reach 1
@@ -318,10 +323,6 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
*/
if (cnt <= 2 || !(cnt % 16))
vhost_poll_queue(&vq->poll);
- /* set len to mark this desc buffers done DMA */
- vq->heads[ubuf->desc].len = success ?
- VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN;
- vhost_net_ubuf_put(ubufs);
}
/* Expects to be always run from workqueue - which acts as
--
1.7.1
^ permalink raw reply related
* [PATCH V3 6/6] vhost_net: correctly limit the max pending buffers
From: Jason Wang @ 2013-09-02 8:41 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1378111261-14826-1-git-send-email-jasowang@redhat.com>
As Michael point out, We used to limit the max pending DMAs to get better cache
utilization. But it was not done correctly since it was one done when there's no
new buffers submitted from guest. Guest can easily exceeds the limitation by
keeping sending packets.
So this patch moves the check into main loop. Tests shows about 5%-10%
improvement on per cpu throughput for guest tx.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 18 +++++++-----------
1 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8e9dc55..831eb4f 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -363,6 +363,13 @@ static void handle_tx(struct vhost_net *net)
if (zcopy)
vhost_zerocopy_signal_used(net, vq);
+ /* If more outstanding DMAs, queue the work.
+ * Handle upend_idx wrap around
+ */
+ if (unlikely((nvq->upend_idx + vq->num - VHOST_MAX_PEND)
+ % UIO_MAXIOV == nvq->done_idx))
+ break;
+
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
&out, &in,
@@ -372,17 +379,6 @@ static void handle_tx(struct vhost_net *net)
break;
/* Nothing new? Wait for eventfd to tell us they refilled. */
if (head == vq->num) {
- int num_pends;
-
- /* If more outstanding DMAs, queue the work.
- * Handle upend_idx wrap around
- */
- num_pends = likely(nvq->upend_idx >= nvq->done_idx) ?
- (nvq->upend_idx - nvq->done_idx) :
- (nvq->upend_idx + UIO_MAXIOV -
- nvq->done_idx);
- if (unlikely(num_pends > VHOST_MAX_PEND))
- break;
if (unlikely(vhost_enable_notify(&net->dev, vq))) {
vhost_disable_notify(&net->dev, vq);
continue;
--
1.7.1
^ 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