sunvnet: vnet_start_xmit() must hold a refcnt on port.

PATCH In review Sowmini Varadhan 5 participants 12 messages Oct 1, 2014 – Oct 2, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Sowmini Varadhan

author

2014-10-01 18:56 UTC #0 lore raw

PATCH 2/2 sunvnet: vnet_start_xmit() must hold a refcnt on port.

A vnet_port_remove could be triggered as a result of an ldm-unbind operation by the peer, or a module unload, or other changes to the inter-vnet-link configuration. When this is concurrent with vnet_start_xmit(), the following sequence could occur

thread 1 thread 2
vnet_start_xmit
-> tx_port_find

    spin_lock_irqsave(&vp->lock..)
    ret = __tx_port_find(..)
    spin_lock_irqrestore(&vp->lock..)
                                           vio_remove -> ..
                                               ->vnet_port_remove
                                           spin_lock_irqsave(&vp->lock..)
                                           cleanup
                                           spin_lock_irqrestore(&vp->lock..)
                                           kfree(port)

/* attempt to use ret will bomb */

This patch avoids the problem by holding/releasing a refcnt on the vnet_port.

Signed-off-by Sowmini Varadhan <[email protected]>

Acked-by Raghuram Kothakota <[email protected]>

---
 drivers/net/ethernet/sun/sunvnet.c | 61 +++++++++++++++++++++++++++++++++-----
 drivers/net/ethernet/sun/sunvnet.h |  2 ++
 2 files changed, 55 insertions(+), 8 deletions(-)

drivers/net/ethernet/sun/sunvnet.c +53 -8

index e2aacf5..dca4397 100644
@@ -47,6 +47,42 @@ MODULE_VERSION(DRV_MODULE_VERSION);
 
 static int __vnet_tx_trigger(struct vnet_port *port, u32 start);
 
+static inline int vnet_refcnt_read(const struct vnet_port *port)
+{
+	return atomic_read(&port->refcnt);
+}
+
+static inline void vnet_hold(struct vnet_port *port)
+{
+	atomic_inc(&port->refcnt);
+	BUG_ON(vnet_refcnt_read(port) == 0);
+}
+
+static void vnet_put(struct vnet_port *port)
+{
+	atomic_dec(&port->refcnt);
+}
+
+static void vnet_wait_allrefs(struct vnet_port *port)
+{
+	unsigned long warning_time;
+	int refcnt = vnet_refcnt_read(port);
+
+	warning_time = jiffies;
+	while (refcnt != 0) {
+		msleep(250);
+		refcnt = vnet_refcnt_read(port);
+		if (time_after(jiffies, warning_time + 10 * HZ)) {
+			pr_emerg("vnet_wait_allrefs: waiting for port to "
+				 "%x:%x:%x:%x:%x:%x. Usage count = %d\n",
+				port->raddr[0], port->raddr[1],
+				port->raddr[2], port->raddr[3],
+				port->raddr[4], port->raddr[5], refcnt);
+			warning_time = jiffies;
+		}
+	}
+}
+
 /* Ordered from largest major to lowest */
 static struct vio_version vnet_versions[] = {
 	{ .major = 1, .minor = 6 },

@@ -773,14 +809,14 @@ struct vnet_port *__tx_port_find(struct vnet *vp, struct sk_buff *skb)
 	hlist_for_each_entry(port, hp, hash) {
 		if (!port_is_up(port))
 			continue;
-		if (ether_addr_equal(port->raddr, skb->data))
+		if (ether_addr_equal(port->raddr, skb->data)) {
+			vnet_hold(port);
 			return port;
+		}
 	}
-	list_for_each_entry(port, &vp->port_list, list) {
-		if (!port->switch_port)
-			continue;
-		if (!port_is_up(port))
-			continue;
+	port = vp->switch_port;
+	if (port != NULL && port_is_up(port)) {
+		vnet_hold(port);
 		return port;
 	}
 	return NULL;

@@ -974,6 +1010,7 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			dev->stats.tx_errors++;
 		}
 		spin_unlock_irqrestore(&port->vio.lock, flags);
+		vnet_put(port);
 		return NETDEV_TX_BUSY;
 	}

@@ -1071,6 +1108,7 @@ ldc_start_done:
 	vnet_free_skbs(freeskbs);
 
 	(void)mod_timer(&port->clean_timer, jiffies + VNET_CLEAN_TIMEOUT);
+	vnet_put(port);
 
 	return NETDEV_TX_OK;

@@ -1087,6 +1125,8 @@ out_dropped:
 	else
 		del_timer(&port->clean_timer);
 	dev->stats.tx_dropped++;
+	if (port)
+		vnet_put(port);
 	return NETDEV_TX_OK;
 }

@@ -1581,10 +1621,12 @@ static int vnet_port_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 	port->switch_port = switch_port;
 
 	spin_lock_irqsave(&vp->lock, flags);
-	if (switch_port)
+	if (switch_port) {
+		vp->switch_port = port;
 		list_add(&port->list, &vp->port_list);
-	else
+	} else {
 		list_add_tail(&port->list, &vp->port_list);
+	}
 	hlist_add_head(&port->hash, &vp->port_hash[vnet_hashfn(port->raddr)]);
 	spin_unlock_irqrestore(&vp->lock, flags);

@@ -1631,11 +1673,14 @@ static int vnet_port_remove(struct vio_dev *vdev)
 		 */
 		spin_lock_irqsave(&vp->lock, flags);
 		port->flags |= VNET_PORT_DEAD;
+		if (port->switch_port)
+			vp->switch_port = NULL;
 		list_del(&port->list);
 		hlist_del(&port->hash);
 		spin_unlock_irqrestore(&vp->lock, flags);
 
 		vnet_workq_disable(port);
+		vnet_wait_allrefs(port);
 
 		vnet_port_free_tx_bufs(port);
 		vio_ldc_free(&port->vio);

drivers/net/ethernet/sun/sunvnet.h +2 -0

index 1182ec6..6f62ea5 100644
@@ -53,6 +53,7 @@ struct vnet_port {
 	u32			stop_rx_idx;
 	bool			stop_rx;
 	bool			start_cons;
+	atomic_t		refcnt;
 
 	struct timer_list	clean_timer;

@@ -104,6 +105,7 @@ struct vnet {
 	u64			local_mac;
 
 	struct tasklet_struct	vnet_tx_wakeup;
+	struct vnet_port	*switch_port;
 };
 
 #endif /* _SUNVNET_H */

Signature · 1 line

1.8.4.2

Eric Dumazet

2014-10-01 19:05 UTC #1 lore raw

Quoted 17 lines · Sowmini Varadhan

A vnet_port_remove could be triggered as a result of an ldm-unbind operation by the peer, or a module unload, or other changes to the inter-vnet-link configuration. When this is concurrent with vnet_start_xmit(), the following sequence could occur
thread 1 thread 2
vnet_start_xmit
-> tx_port_find
    spin_lock_irqsave(&vp->lock..)
    ret = __tx_port_find(..)
    spin_lock_irqrestore(&vp->lock..)
                                           vio_remove -> ..
                                               ->vnet_port_remove
                                           spin_lock_irqsave(&vp->lock..)
                                           cleanup
                                           spin_lock_irqrestore(&vp->lock..)
                                           kfree(port)

Quoted 4 lines

/* attempt to use ret will bomb */
This patch avoids the problem by holding/releasing a refcnt on the vnet_port.

Hmpff... This calls for rcu protection here !

Sowmini Varadhan

author

2014-10-01 19:44 UTC #2 lore raw

Quoted 3 lines · Eric Dumazet

Hmpff... This calls for rcu protection here !

I did consider that, but given that the lists containing the ports are accessed in multiple contexts, some of which can sleep, and given that the vnet port is similar in spirit to the net_device, I followed the net_device model of dev_put etc.

--Sowmini

Eric Dumazet

2014-10-01 20:15 UTC #3 lore raw

Quoted 9 lines · Sowmini Varadhan

Quoted 3 lines · Eric Dumazet
Hmpff... This calls for rcu protection here !
I did consider that, but given that the lists containing the ports are accessed in multiple contexts, some of which can sleep, and given that the vnet port is similar in spirit to the net_device, I followed the net_device model of dev_put etc.

dev_put() uses per cpu variables, an order of magnitude faster than a atomic put/get :(

Sowmini Varadhan

author

2014-10-01 20:19 UTC #4 lore raw

Quoted 6 lines · Eric Dumazet

Quoted 5 lines · Sowmini Varadhan
Quoted 3 lines · Eric Dumazet
Hmpff... This calls for rcu protection here !

Quoted 7 lines

Quoted 4 lines
I did consider that, but given that the lists containing the ports are accessed in multiple contexts, some of which can sleep, and given that the vnet port is similar in spirit to the net_device, I followed the net_device model of dev_put etc.
dev_put() uses per cpu variables, an order of magnitude faster than a atomic put/get :(

I could make this a per-cpu variable, it would not be too hard to change vnet_put/hold etc. That's not a problem.

--Sowmini

David Miller

maintainer

2014-10-01 20:24 UTC #5 lore raw

From: Sowmini Varadhan <[email protected]>
Date: Wed, 01 Oct 2014 16:19:41 -0400

Quoted 12 lines · Eric Dumazet

Quoted 11 lines · Sowmini Varadhan
Quoted 9 lines · Eric Dumazet
Quoted 3 lines
Hmpff... This calls for rcu protection here !
I did consider that, but given that the lists containing the ports are accessed in multiple contexts, some of which can sleep, and given that the vnet port is similar in spirit to the net_device, I followed the net_device model of dev_put etc.

Quoted 6 lines

Quoted 2 lines
dev_put() uses per cpu variables, an order of magnitude faster than a atomic put/get :(
I could make this a per-cpu variable, it would not be too hard to change
vnet_put/hold etc. That's not a problem.

If you went with NAPI you could use RCU.

All of the negative and complicated aspects of your changes strictly have to do with not going with that implementation route.

David L Stevens

2014-10-01 19:06 UTC #6 lore raw

In reply to Sowmini Varadhan

drivers/net/ethernet/sun/sunvnet.c struct vnet_port *__tx_port_find(struct vnet *vp, struct sk_buff *skb) · line 822

Quoted 11 lines · Sowmini Varadhan

drivers/net/ethernet/sun/sunvnet.c +3 -5

-	list_for_each_entry(port, &vp->port_list, list) {
-		if (!port->switch_port)
-			continue;
-		if (!port_is_up(port))
-			continue;
+	port = vp->switch_port;
+	if (port != NULL && port_is_up(port)) {
+		vnet_hold(port);
 		return port;
 	}
 	return NULL;

This "vp->switch_port" addition doesn't appear to be related to the port refcnt change, and doesn't allow for multiple switch ports.

						+-DLS

drivers/net/ethernet/sun/sunvnet.c static int vnet_port_probe(struct vio_dev *vdev, const struct vio_device_id *id) · line 1626

Quoted 8 lines

drivers/net/ethernet/sun/sunvnet.c +2 -1

@@ -1581,10 +1621,12 @@ static int vnet_port_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 	port->switch_port = switch_port;
 
 	spin_lock_irqsave(&vp->lock, flags);
-	if (switch_port)
+	if (switch_port) {
+		vp->switch_port = port;
 		list_add(&port->list, &vp->port_list);

...and here.

Quoted 8 lines

drivers/net/ethernet/sun/sunvnet.c +2 -1

-	else
+	} else {
 		list_add_tail(&port->list, &vp->port_list);
+	}
 	hlist_add_head(&port->hash, &vp->port_hash[vnet_hashfn(port->raddr)]);
 	spin_unlock_irqrestore(&vp->lock, flags);

drivers/net/ethernet/sun/sunvnet.c static int vnet_port_remove(struct vio_dev *vdev) · line 1679

Quoted 7 lines

drivers/net/ethernet/sun/sunvnet.c +2 -0

@@ -1631,11 +1673,14 @@ static int vnet_port_remove(struct vio_dev *vdev)
 		 */
 		spin_lock_irqsave(&vp->lock, flags);
 		port->flags |= VNET_PORT_DEAD;
+		if (port->switch_port)
+			vp->switch_port = NULL;
 		list_del(&port->list);
 		hlist_del(&port->hash);

..and here.

Sowmini Varadhan

author

2014-10-01 19:23 UTC #7 lore raw

Quoted 3 lines · David L Stevens

This "vp->switch_port" addition doesn't appear to be related to the port refcnt change, and doesn't allow for multiple switch ports.

The switch_port is the connection to Dom0. Do you envision us having more than one switch_port? How?

--Sowmini

David L Stevens

2014-10-01 19:31 UTC #8 lore raw

Quoted 7 lines · Sowmini Varadhan

Quoted 3 lines · David L Stevens
This "vp->switch_port" addition doesn't appear to be related to the port refcnt change, and doesn't allow for multiple switch ports.
The switch_port is the connection to Dom0. Do you envision us having more than one switch_port? How?

While Dom0 might only create one port with the "switch" flag, the flag just means "I can reach anybody" and is not inherently unique. I don't think an attached VM should assume there is always only one; it prevents multipath load balancing kinds of things in the future.

Also, there is the broader point that this sort of change should be a separate patch. It isn't required for fixing the dangling reference -- it is an independent change.

							+-DLS

David Miller

maintainer

2014-10-01 19:52 UTC #9 lore raw

From: David L Stevens <[email protected]>
Date: Wed, 01 Oct 2014 15:31:49 -0400

Quoted 11 lines · Sowmini Varadhan

Quoted 7 lines · David L Stevens
Quoted 3 lines
This "vp->switch_port" addition doesn't appear to be related to the port refcnt change, and doesn't allow for multiple switch ports.
The switch_port is the connection to Dom0. Do you envision us having more than one switch_port? How?

Quoted 7 lines

While Dom0 might only create one port with the "switch" flag, the flag just means "I can reach anybody" and is not inherently unique. I don't think an attached VM should assume there is always only one; it prevents multipath load balancing kinds of things in the future.
Also, there is the broader point that this sort of change should be a separate patch. It isn't required for fixing the dangling reference -- it is an independent change.

Multiple switch ports are absolutely allowed by the protocol spec and can provide the suggested facilities David mentioned, don't prevent them from being used.

Raghuram Kothakota

2014-10-02 03:36 UTC #10 lore raw

Quoted 19 lines · David Miller

From: David L Stevens <[email protected]>
Date: Wed, 01 Oct 2014 15:31:49 -0400
Quoted 16 lines · Sowmini Varadhan
Quoted 7 lines · David L Stevens
Quoted 3 lines
This "vp->switch_port" addition doesn't appear to be related to the port refcnt change, and doesn't allow for multiple switch ports.
The switch_port is the connection to Dom0. Do you envision us having more than one switch_port? How?
While Dom0 might only create one port with the "switch" flag, the flag just means "I can reach anybody" and is not inherently unique. I don't think an attached VM should assume there is always only one; it prevents multipath load balancing kinds of things in the future.

Quoted 6 lines

Quoted 2 lines
Also, there is the broader point that this sort of change should be a separate patch. It isn't required for fixing the dangling reference -- it is an independent change.
Multiple switch ports are absolutely allowed by the protocol spec and can provide the suggested facilities David mentioned, don't prevent them from being used.

In reality, introducing multiple switch-ports will need Guest driver change as well. The existing sunvnet driver will not automatically utilize all switch-ports and requires changes. When we add the full support to use multiple switch-ports, I am sure we can change the current optimization for switch-port lookup with a different method probably with another optimized method than what it is today.

-Raghuram

Raghuram Kothakota

2014-10-02 03:24 UTC #11 lore raw

In reply to David L Stevens

Quoted 8 lines · David L Stevens

Quoted 5 lines · Sowmini Varadhan
Quoted 3 lines · David L Stevens
This "vp->switch_port" addition doesn't appear to be related to the port refcnt change, and doesn't allow for multiple switch ports.

Quoted 7 lines

Quoted 2 lines
The switch_port is the connection to Dom0. Do you envision us having more than one switch_port? How?
While Dom0 might only create one port with the "switch" flag, the flag just means "I can reach anybody" and is not inherently unique. I don't think an attached VM should assume there is always only one; it prevents multipath load balancing kinds of things in the future.

At the moment our architecture defines only one switch-port. We certainly toyed with the idea of multiple paths(actually LDCs) mainly for the performance but we were able to achieve our goal with one LDC so that was not introduced. Our original design included the idea of multiple LDCs in a port, but not multiple ports for performance purpose. We also explored adding paths to different virtual switches, mainly intended for path failover when one of them failed due to service domain crash/panic. We have such feature in our virtual disk, there we realized it is very proprietary implementation and cost of maintaining and improving those features is high. We abandoned these proprietary solutions but rely on the rich features that already exists in the Guest OS operating systems with multiple network devices. In summary, currently we do not have any plans to multiple paths.

Even if we implement multiple paths, we expect it is the choice of the Guest on how it utilizes those paths as it need to have the knowledge of these multiple paths and use them as designed. That is, the driver implementation has to change. For now, I do not see any issues with this driver assuming one switch-port. If we ever support multiple paths, I would expect this driver to change, at that time the code can implement appropriate method of load balancing or failover.

-Raghuram

Quoted 10 lines

Also, there is the broader point that this sort of change should be a separate patch. It isn't required for fixing the dangling reference -- it is an independent change.
							+-DLS
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html