Re: [Xen-devel] ARM: DOMU: Deadlock in xen-netfront and patch to solve it.

[Dmitry Piotrovsky <piotrovskydmitry@xxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
---
Â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 = "">     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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel