* ARM: DOMU: Deadlock in xen-netfront and patch to solve it.
@ 2014-10-07 9:02 Dmitry Piotrovsky
2014-10-07 10:31 ` David Vrabel
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Piotrovsky @ 2014-10-07 9:02 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 13001 bytes --]
Hi,
I am using uEVM5432 (OMAP5432) board with xen 4.5.
(XEN) Xen version 4.5-unstable (dpiotrov@)
(arm-linux-gnueabihf-gcc (Ubuntu/Linaro 4.8.2-16ubuntu4) 4.8.2) debug=y
Linux version 3.16.2 (dpiotrov@ubuntu)
(gcc version 4.8.2 (Ubuntu/Linaro 4.8.2-16ubuntu4) )
Dom0 work with no issues,
domU - starting good, but when i
try to use networking - ping for example,
i got next message(ping continue to work):
~ #ping 192.168.0.3
PING 192.168.0.3 (192.168.0.3): 56 data bytes
=========================================================
[ INFO: possible irq lock inversion dependency detected ]
3.16.2 #16 Not tainted
---------------------------------------------------------
swapper/0/0 just changed the state of lock:
(&(&queue->tx_lock)->rlock){-.....}, at: [<c03adec8>]
xennet_tx_interrupt+0x14/0x34
but this lock took another, HARDIRQ-unsafe lock in the past:
(&stat->syncp.seq#2){+.-...}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&stat->syncp.seq#2);
local_irq_disable();
lock(&(&queue->tx_lock)->rlock);
lock(&stat->syncp.seq#2);
<Interrupt>
lock(&(&queue->tx_lock)->rlock);
*** DEADLOCK ***
... see all the log at end of the email ...
By my opinion, this possible happened because,
timing of domU machine is different (compared to dom0 or nonvirtual linux
machine).
Which cause changes in race results between irq and another lockers.
I made the following changes in drivers/net/xen-netfront.c. That solves
this problem:
--- a/drivers/net/xen-netfront.c 2014-09-28 14:29:07.000000000 -0700
+++ b/drivers/net/xen-netfront.c 2014-10-01 00:41:19.629581110 -0700
@@ -959,6 +959,7 @@
struct netfront_stats *stats = this_cpu_ptr(queue->info->stats);
int packets_dropped = 0;
struct sk_buff *skb;
+ unsigned long flags;
while ((skb = __skb_dequeue(rxq)) != NULL) {
int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
@@ -977,10 +978,12 @@
continue;
}
+ local_irq_save(flags);
u64_stats_update_begin(&stats->syncp);
stats->rx_packets++;
stats->rx_bytes += skb->len;
u64_stats_update_end(&stats->syncp);
+ local_irq_restore(flags);
/* Pass it up. */
napi_gro_receive(&queue->napi, skb);
@@ -1119,15 +1122,18 @@
struct netfront_stats *stats = per_cpu_ptr(np->stats, cpu);
u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
unsigned int start;
+ unsigned long flags;
+ local_irq_save(flags);
do {
- start = u64_stats_fetch_begin_irq(&stats->syncp);
+ start = u64_stats_fetch_begin(&stats->syncp);
rx_packets = stats->rx_packets;
tx_packets = stats->tx_packets;
rx_bytes = stats->rx_bytes;
tx_bytes = stats->tx_bytes;
- } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+ } while (u64_stats_fetch_retry(&stats->syncp, start));
+ local_irq_restore(flags);
tot->rx_packets += rx_packets;
tot->tx_packets += tx_packets;
Can you please review my patch?
If the patch solve actual problem –
please help me to deliver it -
i not so familiar with this sequence.
Thanks,
Dmitry.
--------------------------------------------------------
All the log of deadlock:
~ #ping 192.168.0.3
PING 192.168.0.3 (192.168.0.3): 56 data bytes
=========================================================
[ INFO: possible irq lock inversion dependency detected ]
3.16.2 #16 Not tainted
---------------------------------------------------------
swapper/0/0 just changed the state of lock:
(&(&queue->tx_lock)->rlock){-.....}, at: [<c03adec8>]
xennet_tx_interrupt+0x14/0x34
but this lock took another, HARDIRQ-unsafe lock in the past:
(&stat->syncp.seq#2){+.-...}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&stat->syncp.seq#2);
local_irq_disable();
lock(&(&queue->tx_lock)->rlock);
lock(&stat->syncp.seq#2);
<Interrupt>
lock(&(&queue->tx_lock)->rlock);
*** DEADLOCK ***
no locks held by swapper/0/0.
the shortest dependencies between 2nd lock and 1st lock:
-> (&stat->syncp.seq#2){+.-...} ops: 24 {
HARDIRQ-ON-W at:
[<c03b084c>] xennet_poll+0x908/0xb1c
[<c047db30>] net_rx_action+0xbc/0x18c
[<c003b638>] __do_softirq+0x154/0x2dc
[<c003babc>] irq_exit+0xc0/0x104
[<c000f240>] handle_IRQ+0x44/0x90
[<c000858c>] gic_handle_irq+0x2c/0x5c
[<c0012944>] __irq_svc+0x44/0x58
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c006c3f4>] cpu_startup_entry+0x104/0x15c
[<c072abac>] start_kernel+0x390/0x39c
IN-SOFTIRQ-W at:
[<c03b084c>] xennet_poll+0x908/0xb1c
[<c047db30>] net_rx_action+0xbc/0x18c
[<c003b638>] __do_softirq+0x154/0x2dc
[<c003babc>] irq_exit+0xc0/0x104
[<c000f240>] handle_IRQ+0x44/0x90
[<c000858c>] gic_handle_irq+0x2c/0x5c
[<c0012944>] __irq_svc+0x44/0x58
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c006c3f4>] cpu_startup_entry+0x104/0x15c
[<c072abac>] start_kernel+0x390/0x39c
INITIAL USE at:
[<c03aeabc>] xennet_get_stats64+0x13c/0x190
[<c047835c>] dev_get_stats+0x38/0xc8
[<c048c95c>] rtnl_fill_ifinfo+0x3b8/0x930
[<c048cf3c>] rtmsg_ifinfo+0x68/0xec
[<c048088c>] register_netdevice+0x484/0x4f4
[<c0480910>] register_netdev+0x14/0x24
[<c03aec4c>] netfront_probe+0x13c/0x244
[<c02cf944>] xenbus_dev_probe+0x80/0x134
[<c0316220>] driver_probe_device+0x118/0x250
[<c0316430>] __driver_attach+0x94/0x98
[<c0314858>] bus_for_each_dev+0x60/0x94
[<c0315a2c>] bus_add_driver+0x148/0x1f0
[<c03167f4>] driver_register+0x78/0xf8
[<c02d1154>] xenbus_register_frontend+0x20/0x38
[<c0008850>] do_one_initcall+0x8c/0x1c8
[<c072acf0>] kernel_init_freeable+0x138/0x1d8
[<c0539254>] kernel_init+0x8/0xf0
[<c000e9e8>] ret_from_fork+0x14/0x2c
}
... key at: [<c0fb00a4>] __key.45274+0x0/0x8
... acquired at:
[<c047ef3c>] dev_hard_start_xmit+0x2c8/0x44c
[<c04986dc>] sch_direct_xmit+0xc4/0x2e8
[<c047f2cc>] __dev_queue_xmit+0x20c/0x5f4
[<c04d60f8>] arp_solicit+0xf8/0x278
[<c0486634>] neigh_probe+0x50/0x78
[<c0487e04>] __neigh_event_send+0x98/0x328
[<c0488240>] neigh_resolve_output+0x1ac/0x268
[<c04aa464>] ip_finish_output+0x330/0xf74
[<c04ad4c8>] ip_send_skb+0x18/0xd4
[<c04cef20>] raw_sendmsg+0x504/0x93c
[<c0466858>] sock_sendmsg+0x88/0xac
[<c046898c>] SyS_sendto+0xb8/0xdc
[<c000e920>] ret_fast_syscall+0x0/0x48
-> (&(&queue->tx_lock)->rlock){-.....} ops: 6 {
IN-HARDIRQ-W at:
[<c0543294>] _raw_spin_lock_irqsave+0x40/0x54
[<c03adec8>] xennet_tx_interrupt+0x14/0x34
[<c007e960>] handle_irq_event_percpu+0x38/0x134
[<c007ea98>] handle_irq_event+0x3c/0x5c
[<c0081580>] handle_edge_irq+0xa4/0x184
[<c007e178>] generic_handle_irq+0x2c/0x3c
[<c02cc738>] evtchn_fifo_handle_events+0x18c/0x198
[<c02c9dd4>] __xen_evtchn_do_upcall+0x70/0xa4
[<c000e100>] xen_arm_callback+0x8/0x10
[<c0081cd4>] handle_percpu_devid_irq+0x6c/0x84
[<c007e178>] generic_handle_irq+0x2c/0x3c
[<c000f23c>] handle_IRQ+0x40/0x90
[<c000858c>] gic_handle_irq+0x2c/0x5c
[<c0012944>] __irq_svc+0x44/0x58
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c006c3f4>] cpu_startup_entry+0x104/0x15c
[<c072abac>] start_kernel+0x390/0x39c
INITIAL USE at:
[<c05432e4>] _raw_spin_lock_irq+0x3c/0x4c
[<c03afcfc>] netback_changed+0xc3c/0xe84
[<c02ce824>] xenwatch_thread+0x80/0x120
[<c0052bec>] kthread+0xe0/0xf4
[<c000e9e8>] ret_from_fork+0x14/0x2c
}
... key at: [<c0fb00ac>] __key.45357+0x0/0x8
... acquired at:
[<c0075a68>] __lock_acquire+0xae0/0x1d7c
[<c00774e0>] lock_acquire+0x68/0x7c
[<c0543294>] _raw_spin_lock_irqsave+0x40/0x54
[<c03adec8>] xennet_tx_interrupt+0x14/0x34
[<c007e960>] handle_irq_event_percpu+0x38/0x134
[<c007ea98>] handle_irq_event+0x3c/0x5c
[<c0081580>] handle_edge_irq+0xa4/0x184
[<c007e178>] generic_handle_irq+0x2c/0x3c
[<c02cc738>] evtchn_fifo_handle_events+0x18c/0x198
[<c02c9dd4>] __xen_evtchn_do_upcall+0x70/0xa4
[<c000e100>] xen_arm_callback+0x8/0x10
[<c0081cd4>] handle_percpu_devid_irq+0x6c/0x84
[<c007e178>] generic_handle_irq+0x2c/0x3c
[<c000f23c>] handle_IRQ+0x40/0x90
[<c000858c>] gic_handle_irq+0x2c/0x5c
[<c0012944>] __irq_svc+0x44/0x58
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c000f3a0>] arch_cpu_idle+0x24/0x30
[<c006c3f4>] cpu_startup_entry+0x104/0x15c
[<c072abac>] start_kernel+0x390/0x39c
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.16.2 #16
[<c0015aa8>] (unwind_backtrace) from [<c0011e4c>] (show_stack+0x10/0x14)
[<c0011e4c>] (show_stack) from [<c053d524>] (dump_stack+0x84/0x94)
[<c053d524>] (dump_stack) from [<c00737d4>]
(print_irq_inversion_bug+0x1c4/0x20c)
[<c00737d4>] (print_irq_inversion_bug) from [<c00738d4>]
(check_usage_forwards+0xb8/0x120)
[<c00738d4>] (check_usage_forwards) from [<c0074598>]
(mark_lock+0x370/0x7a0)
[<c0074598>] (mark_lock) from [<c0075a68>] (__lock_acquire+0xae0/0x1d7c)
[<c0075a68>] (__lock_acquire) from [<c00774e0>] (lock_acquire+0x68/0x7c)
[<c00774e0>] (lock_acquire) from [<c0543294>]
(_raw_spin_lock_irqsave+0x40/0x54)
[<c0543294>] (_raw_spin_lock_irqsave) from [<c03adec8>]
(xennet_tx_interrupt+0x14/0x34)
[<c03adec8>] (xennet_tx_interrupt) from [<c007e960>]
(handle_irq_event_percpu+0x38/0x134)
[<c007e960>] (handle_irq_event_percpu) from [<c007ea98>]
(handle_irq_event+0x3c/0x5c)
[<c007ea98>] (handle_irq_event) from [<c0081580>]
(handle_edge_irq+0xa4/0x184)
[<c0081580>] (handle_edge_irq) from [<c007e178>]
(generic_handle_irq+0x2c/0x3c)
[<c007e178>] (generic_handle_irq) from [<c02cc738>]
(evtchn_fifo_handle_events+0x18c/0x198)
[<c02cc738>] (evtchn_fifo_handle_events) from [<c02c9dd4>]
(__xen_evtchn_do_upcall+0x70/0xa4)
[<c02c9dd4>] (__xen_evtchn_do_upcall) from [<c000e100>]
(xen_arm_callback+0x8/0x10)
[<c000e100>] (xen_arm_callback) from [<c0081cd4>]
(handle_percpu_devid_irq+0x6c/0x84)
[<c0081cd4>] (handle_percpu_devid_irq) from [<c007e178>]
(generic_handle_irq+0x2c/0x3c)
[<c007e178>] (generic_handle_irq) from [<c000f23c>] (handle_IRQ+0x40/0x90)
[<c000f23c>] (handle_IRQ) from [<c000858c>] (gic_handle_irq+0x2c/0x5c)
[<c000858c>] (gic_handle_irq) from [<c0012944>] (__irq_svc+0x44/0x58)
Exception stack(0xc076df68 to 0xc076dfb0)
df60: 00000001 00000001 00000000 c077b3b0 c076c000
c077854c
df80: c07784e4 c07c061f c05472c0 00000001 c07c061f 00000000 00000000
c076dfb0
dfa0: c0074b5c c000f3a0 20070013 ffffffff
[<c0012944>] (__irq_svc) from [<c000f3a0>] (arch_cpu_idle+0x24/0x30)
[<c000f3a0>] (arch_cpu_idle) from [<c006c3f4>]
(cpu_startup_entry+0x104/0x15c)
[<c006c3f4>] (cpu_startup_entry) from [<c072abac>]
(start_kernel+0x390/0x39c)
64 bytes from 192.168.0.3: seq=0 ttl=64 time=328.585 ms
64 bytes from 192.168.0.3: seq=1 ttl=64 time=2.135 ms
64 bytes from 192.168.0.3: seq=2 ttl=64 time=2.328 ms
[-- Attachment #1.2: Type: text/html, Size: 21904 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: ARM: DOMU: Deadlock in xen-netfront and patch to solve it.
2014-10-07 9:02 ARM: DOMU: Deadlock in xen-netfront and patch to solve it Dmitry Piotrovsky
@ 2014-10-07 10:31 ` David Vrabel
2014-10-07 12:26 ` Dmitry Piotrovsky
0 siblings, 1 reply; 3+ messages in thread
From: David Vrabel @ 2014-10-07 10:31 UTC (permalink / raw)
To: Dmitry Piotrovsky, xen-devel
On 07/10/14 10:02, Dmitry Piotrovsky wrote:
> Hi,
>
> I am using uEVM5432 (OMAP5432) board with xen 4.5.
> (XEN) Xen version 4.5-unstable (dpiotrov@)
> (arm-linux-gnueabihf-gcc (Ubuntu/Linaro 4.8.2-16ubuntu4) 4.8.2) debug=y
>
> Linux version 3.16.2 (dpiotrov@ubuntu)
> (gcc version 4.8.2 (Ubuntu/Linaro 4.8.2-16ubuntu4) )
>
> Dom0 work with no issues,
> domU - starting good, but when i
> try to use networking - ping for example,
> i got next message(ping continue to work):
>
> ~ #ping 192.168.0.3
> PING 192.168.0.3 (192.168.0.3): 56 data bytes
>
> =========================================================
> [ INFO: possible irq lock inversion dependency detected ]
> 3.16.2 #16 Not tainted
> ---------------------------------------------------------
> swapper/0/0 just changed the state of lock:
> (&(&queue->tx_lock)->rlock){-.....}, at: [<c03adec8>]
> xennet_tx_interrupt+0x14/0x34
> but this lock took another, HARDIRQ-unsafe lock in the past:
> (&stat->syncp.seq#2){+.-...}
> and interrupts could create inverse lock ordering between them.
> other info that might help us debug this:
> Possible interrupt unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&stat->syncp.seq#2);
> local_irq_disable();
> lock(&(&queue->tx_lock)->rlock);
> lock(&stat->syncp.seq#2);
> <Interrupt>
> lock(&(&queue->tx_lock)->rlock);
> *** DEADLOCK ***
I think you need to separate the stats lock into a Rx and a Tx one.
Something like this?
8<---------------------
xen-netfront: use different locks for Rx and Tx stats
In netfront the Rx and Tx path are independent and use different
locks. The Tx lock is held with hard irqs disabled, but Rx lock is
held with only BH disabled. Since both sides use the same stats lock,
a deadlock may occur.
[ INFO: possible irq lock inversion dependency detected ]
3.16.2 #16 Not tainted
---------------------------------------------------------
swapper/0/0 just changed the state of lock:
(&(&queue->tx_lock)->rlock){-.....}, at: [<c03adec8>]
xennet_tx_interrupt+0x14/0x34
but this lock took another, HARDIRQ-unsafe lock in the past:
(&stat->syncp.seq#2){+.-...}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&stat->syncp.seq#2);
local_irq_disable();
lock(&(&queue->tx_lock)->rlock);
lock(&stat->syncp.seq#2);
<Interrupt>
lock(&(&queue->tx_lock)->rlock);
Using separate locks for the Rx and Tx stats fixes this deadlock.
Reported-by: Dmitry Piotrovsky <piotrovskydmitry@gmail.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
drivers/net/xen-netfront.c | 71 ++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 29 deletions(-)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index ca82f54..a56b59c 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -86,10 +86,8 @@ struct netfront_cb {
#define IRQ_NAME_SIZE (QUEUE_NAME_SIZE + 3)
struct netfront_stats {
- u64 rx_packets;
- u64 tx_packets;
- u64 rx_bytes;
- u64 tx_bytes;
+ u64 packets;
+ u64 bytes;
struct u64_stats_sync syncp;
};
@@ -165,7 +163,8 @@ struct netfront_info {
struct netfront_queue *queues;
/* Statistics */
- struct netfront_stats __percpu *stats;
+ struct netfront_stats __percpu *rx_stats;
+ struct netfront_stats __percpu *tx_stats;
atomic_t rx_gso_checksum_fixup;
};
@@ -593,7 +592,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
unsigned short id;
struct netfront_info *np = netdev_priv(dev);
- struct netfront_stats *stats = this_cpu_ptr(np->stats);
+ struct netfront_stats *tx_stats = this_cpu_ptr(np->tx_stats);
struct xen_netif_tx_request *tx;
char *data = skb->data;
RING_IDX i;
@@ -697,10 +696,10 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
if (notify)
notify_remote_via_irq(queue->tx_irq);
- u64_stats_update_begin(&stats->syncp);
- stats->tx_bytes += skb->len;
- stats->tx_packets++;
- u64_stats_update_end(&stats->syncp);
+ u64_stats_update_begin(&tx_stats->syncp);
+ tx_stats->bytes += skb->len;
+ tx_stats->packets++;
+ u64_stats_update_end(&tx_stats->syncp);
/* Note: It is not safe to access skb after xennet_tx_buf_gc()! */
xennet_tx_buf_gc(queue);
@@ -956,7 +955,7 @@ static int checksum_setup(struct net_device *dev, struct sk_buff *skb)
static int handle_incoming_queue(struct netfront_queue *queue,
struct sk_buff_head *rxq)
{
- struct netfront_stats *stats = this_cpu_ptr(queue->info->stats);
+ struct netfront_stats *rx_stats = this_cpu_ptr(queue->info->rx_stats);
int packets_dropped = 0;
struct sk_buff *skb;
@@ -977,10 +976,10 @@ static int handle_incoming_queue(struct netfront_queue *queue,
continue;
}
- u64_stats_update_begin(&stats->syncp);
- stats->rx_packets++;
- stats->rx_bytes += skb->len;
- u64_stats_update_end(&stats->syncp);
+ u64_stats_update_begin(&rx_stats->syncp);
+ rx_stats->packets++;
+ rx_stats->bytes += skb->len;
+ u64_stats_update_end(&rx_stats->syncp);
/* Pass it up. */
napi_gro_receive(&queue->napi, skb);
@@ -1116,18 +1115,22 @@ static struct rtnl_link_stats64 *xennet_get_stats64(struct net_device *dev,
int cpu;
for_each_possible_cpu(cpu) {
- struct netfront_stats *stats = per_cpu_ptr(np->stats, cpu);
+ struct netfront_stats *rx_stats = per_cpu_ptr(np->rx_stats, cpu);
+ struct netfront_stats *tx_stats = per_cpu_ptr(np->tx_stats, cpu);
u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
unsigned int start;
do {
- start = u64_stats_fetch_begin_irq(&stats->syncp);
+ start = u64_stats_fetch_begin_irq(&tx_stats->syncp);
+ tx_packets = tx_stats->packets;
+ tx_bytes = tx_stats->bytes;
+ } while (u64_stats_fetch_retry_irq(&tx_stats->syncp, start));
- rx_packets = stats->rx_packets;
- tx_packets = stats->tx_packets;
- rx_bytes = stats->rx_bytes;
- tx_bytes = stats->tx_bytes;
- } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+ do {
+ start = u64_stats_fetch_begin_irq(&rx_stats->syncp);
+ rx_packets = rx_stats->packets;
+ rx_bytes = rx_stats->bytes;
+ } while (u64_stats_fetch_retry_irq(&rx_stats->syncp, start));
tot->rx_packets += rx_packets;
tot->tx_packets += tx_packets;
@@ -1312,6 +1315,15 @@ static const struct net_device_ops xennet_netdev_ops = {
#endif
};
+static void xennet_free_netdev(struct net_device *netdev)
+{
+ struct netfront_info *np = netdev_priv(netdev);
+
+ free_percpu(np->rx_stats);
+ free_percpu(np->tx_stats);
+ free_netdev(netdev);
+}
+
static struct net_device *xennet_create_dev(struct xenbus_device *dev)
{
int err;
@@ -1332,8 +1344,11 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
np->queues = NULL;
err = -ENOMEM;
- np->stats = netdev_alloc_pcpu_stats(struct netfront_stats);
- if (np->stats == NULL)
+ np->rx_stats = netdev_alloc_pcpu_stats(struct netfront_stats);
+ if (np->rx_stats == NULL)
+ goto exit;
+ np->tx_stats = netdev_alloc_pcpu_stats(struct netfront_stats);
+ if (np->tx_stats == NULL)
goto exit;
netdev->netdev_ops = &xennet_netdev_ops;
@@ -1364,7 +1379,7 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
return netdev;
exit:
- free_netdev(netdev);
+ xennet_free_netdev(netdev);
return ERR_PTR(err);
}
@@ -1406,7 +1421,7 @@ static int netfront_probe(struct xenbus_device *dev,
return 0;
fail:
- free_netdev(netdev);
+ xennet_free_netdev(netdev);
dev_set_drvdata(&dev->dev, NULL);
return err;
}
@@ -2331,9 +2346,7 @@ static int xennet_remove(struct xenbus_device *dev)
info->queues = NULL;
}
- free_percpu(info->stats);
-
- free_netdev(info->netdev);
+ xennet_free_netdev(info->netdev);
return 0;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: ARM: DOMU: Deadlock in xen-netfront and patch to solve it.
2014-10-07 10:31 ` David Vrabel
@ 2014-10-07 12:26 ` Dmitry Piotrovsky
0 siblings, 0 replies; 3+ messages in thread
From: Dmitry Piotrovsky @ 2014-10-07 12:26 UTC (permalink / raw)
To: David Vrabel, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 9940 bytes --]
Hi David,
Thanks for fast answer!
I tried your patch - it solve the problem
and more correct that my :)
Regards,
Dmitry
On Tue, Oct 7, 2014 at 1:31 PM, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 07/10/14 10:02, Dmitry Piotrovsky wrote:
> > Hi,
> >
> > I am using uEVM5432 (OMAP5432) board with xen 4.5.
> > (XEN) Xen version 4.5-unstable (dpiotrov@)
> > (arm-linux-gnueabihf-gcc (Ubuntu/Linaro 4.8.2-16ubuntu4) 4.8.2) debug=y
> >
> > Linux version 3.16.2 (dpiotrov@ubuntu)
> > (gcc version 4.8.2 (Ubuntu/Linaro 4.8.2-16ubuntu4) )
> >
> > Dom0 work with no issues,
> > domU - starting good, but when i
> > try to use networking - ping for example,
> > i got next message(ping continue to work):
> >
> > ~ #ping 192.168.0.3
> > PING 192.168.0.3 (192.168.0.3): 56 data bytes
> >
> > =========================================================
> > [ INFO: possible irq lock inversion dependency detected ]
> > 3.16.2 #16 Not tainted
> > ---------------------------------------------------------
> > swapper/0/0 just changed the state of lock:
> > (&(&queue->tx_lock)->rlock){-.....}, at: [<c03adec8>]
> > xennet_tx_interrupt+0x14/0x34
> > but this lock took another, HARDIRQ-unsafe lock in the past:
> > (&stat->syncp.seq#2){+.-...}
> > and interrupts could create inverse lock ordering between them.
> > other info that might help us debug this:
> > Possible interrupt unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(&stat->syncp.seq#2);
> > local_irq_disable();
> > lock(&(&queue->tx_lock)->rlock);
> > lock(&stat->syncp.seq#2);
> > <Interrupt>
> > lock(&(&queue->tx_lock)->rlock);
> > *** DEADLOCK ***
>
> I think you need to separate the stats lock into a Rx and a Tx one.
>
> Something like this?
>
> 8<---------------------
> xen-netfront: use different locks for Rx and Tx stats
>
> In netfront the Rx and Tx path are independent and use different
> locks. The Tx lock is held with hard irqs disabled, but Rx lock is
> held with only BH disabled. Since both sides use the same stats lock,
> a deadlock may occur.
>
> [ INFO: possible irq lock inversion dependency detected ]
> 3.16.2 #16 Not tainted
> ---------------------------------------------------------
> swapper/0/0 just changed the state of lock:
> (&(&queue->tx_lock)->rlock){-.....}, at: [<c03adec8>]
> xennet_tx_interrupt+0x14/0x34
> but this lock took another, HARDIRQ-unsafe lock in the past:
> (&stat->syncp.seq#2){+.-...}
> and interrupts could create inverse lock ordering between them.
> other info that might help us debug this:
> Possible interrupt unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&stat->syncp.seq#2);
> local_irq_disable();
> lock(&(&queue->tx_lock)->rlock);
> lock(&stat->syncp.seq#2);
> <Interrupt>
> lock(&(&queue->tx_lock)->rlock);
>
> Using separate locks for the Rx and Tx stats fixes this deadlock.
>
> Reported-by: Dmitry Piotrovsky <piotrovskydmitry@gmail.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> drivers/net/xen-netfront.c | 71
> ++++++++++++++++++++++++++------------------
> 1 file changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index ca82f54..a56b59c 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -86,10 +86,8 @@ struct netfront_cb {
> #define IRQ_NAME_SIZE (QUEUE_NAME_SIZE + 3)
>
> struct netfront_stats {
> - u64 rx_packets;
> - u64 tx_packets;
> - u64 rx_bytes;
> - u64 tx_bytes;
> + u64 packets;
> + u64 bytes;
> struct u64_stats_sync syncp;
> };
>
> @@ -165,7 +163,8 @@ struct netfront_info {
> struct netfront_queue *queues;
>
> /* Statistics */
> - struct netfront_stats __percpu *stats;
> + struct netfront_stats __percpu *rx_stats;
> + struct netfront_stats __percpu *tx_stats;
>
> atomic_t rx_gso_checksum_fixup;
> };
> @@ -593,7 +592,7 @@ static int xennet_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> unsigned short id;
> struct netfront_info *np = netdev_priv(dev);
> - struct netfront_stats *stats = this_cpu_ptr(np->stats);
> + struct netfront_stats *tx_stats = this_cpu_ptr(np->tx_stats);
> struct xen_netif_tx_request *tx;
> char *data = skb->data;
> RING_IDX i;
> @@ -697,10 +696,10 @@ static int xennet_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> if (notify)
> notify_remote_via_irq(queue->tx_irq);
>
> - u64_stats_update_begin(&stats->syncp);
> - stats->tx_bytes += skb->len;
> - stats->tx_packets++;
> - u64_stats_update_end(&stats->syncp);
> + u64_stats_update_begin(&tx_stats->syncp);
> + tx_stats->bytes += skb->len;
> + tx_stats->packets++;
> + u64_stats_update_end(&tx_stats->syncp);
>
> /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */
> xennet_tx_buf_gc(queue);
> @@ -956,7 +955,7 @@ static int checksum_setup(struct net_device *dev,
> struct sk_buff *skb)
> static int handle_incoming_queue(struct netfront_queue *queue,
> struct sk_buff_head *rxq)
> {
> - struct netfront_stats *stats = this_cpu_ptr(queue->info->stats);
> + struct netfront_stats *rx_stats =
> this_cpu_ptr(queue->info->rx_stats);
> int packets_dropped = 0;
> struct sk_buff *skb;
>
> @@ -977,10 +976,10 @@ static int handle_incoming_queue(struct
> netfront_queue *queue,
> continue;
> }
>
> - u64_stats_update_begin(&stats->syncp);
> - stats->rx_packets++;
> - stats->rx_bytes += skb->len;
> - u64_stats_update_end(&stats->syncp);
> + u64_stats_update_begin(&rx_stats->syncp);
> + rx_stats->packets++;
> + rx_stats->bytes += skb->len;
> + u64_stats_update_end(&rx_stats->syncp);
>
> /* Pass it up. */
> napi_gro_receive(&queue->napi, skb);
> @@ -1116,18 +1115,22 @@ static struct rtnl_link_stats64
> *xennet_get_stats64(struct net_device *dev,
> int cpu;
>
> for_each_possible_cpu(cpu) {
> - struct netfront_stats *stats = per_cpu_ptr(np->stats, cpu);
> + struct netfront_stats *rx_stats =
> per_cpu_ptr(np->rx_stats, cpu);
> + struct netfront_stats *tx_stats =
> per_cpu_ptr(np->tx_stats, cpu);
> u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
> unsigned int start;
>
> do {
> - start = u64_stats_fetch_begin_irq(&stats->syncp);
> + start =
> u64_stats_fetch_begin_irq(&tx_stats->syncp);
> + tx_packets = tx_stats->packets;
> + tx_bytes = tx_stats->bytes;
> + } while (u64_stats_fetch_retry_irq(&tx_stats->syncp,
> start));
>
> - rx_packets = stats->rx_packets;
> - tx_packets = stats->tx_packets;
> - rx_bytes = stats->rx_bytes;
> - tx_bytes = stats->tx_bytes;
> - } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
> + do {
> + start =
> u64_stats_fetch_begin_irq(&rx_stats->syncp);
> + rx_packets = rx_stats->packets;
> + rx_bytes = rx_stats->bytes;
> + } while (u64_stats_fetch_retry_irq(&rx_stats->syncp,
> start));
>
> tot->rx_packets += rx_packets;
> tot->tx_packets += tx_packets;
> @@ -1312,6 +1315,15 @@ static const struct net_device_ops
> xennet_netdev_ops = {
> #endif
> };
>
> +static void xennet_free_netdev(struct net_device *netdev)
> +{
> + struct netfront_info *np = netdev_priv(netdev);
> +
> + free_percpu(np->rx_stats);
> + free_percpu(np->tx_stats);
> + free_netdev(netdev);
> +}
> +
> static struct net_device *xennet_create_dev(struct xenbus_device *dev)
> {
> int err;
> @@ -1332,8 +1344,11 @@ static struct net_device *xennet_create_dev(struct
> xenbus_device *dev)
> np->queues = NULL;
>
> err = -ENOMEM;
> - np->stats = netdev_alloc_pcpu_stats(struct netfront_stats);
> - if (np->stats == NULL)
> + np->rx_stats = netdev_alloc_pcpu_stats(struct netfront_stats);
> + if (np->rx_stats == NULL)
> + goto exit;
> + np->tx_stats = netdev_alloc_pcpu_stats(struct netfront_stats);
> + if (np->tx_stats == NULL)
> goto exit;
>
> netdev->netdev_ops = &xennet_netdev_ops;
> @@ -1364,7 +1379,7 @@ static struct net_device *xennet_create_dev(struct
> xenbus_device *dev)
> return netdev;
>
> exit:
> - free_netdev(netdev);
> + xennet_free_netdev(netdev);
> return ERR_PTR(err);
> }
>
> @@ -1406,7 +1421,7 @@ static int netfront_probe(struct xenbus_device *dev,
> return 0;
>
> fail:
> - free_netdev(netdev);
> + xennet_free_netdev(netdev);
> dev_set_drvdata(&dev->dev, NULL);
> return err;
> }
> @@ -2331,9 +2346,7 @@ static int xennet_remove(struct xenbus_device *dev)
> info->queues = NULL;
> }
>
> - free_percpu(info->stats);
> -
> - free_netdev(info->netdev);
> + xennet_free_netdev(info->netdev);
>
> return 0;
> }
> --
> 1.7.10.4
>
>
[-- Attachment #1.2: Type: text/html, Size: 13301 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-07 12:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-07 9:02 ARM: DOMU: Deadlock in xen-netfront and patch to solve it Dmitry Piotrovsky
2014-10-07 10:31 ` David Vrabel
2014-10-07 12:26 ` Dmitry Piotrovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.