# HG changeset patch
# User Keir Fraser <keir.fraser@xxxxxxxxxx>
# Date 1238497203 -3600
# Node ID ab1d4fbbe4bf93f203e0c4d62c18bd248e4f1d4a
# Parent ad4d307bf9ced378d0b49d4559ae33ecbff3c1b7
netfront accel: Simplify, document, and fix a theoretical bug in use
of spin locks by netfront acceleration plugins
Signed-off-by: Kieran Mansley <kmansley@xxxxxxxxxxxxxx>
---
drivers/xen/netfront/accel.c | 226 +++++++++++++++++++---------------------
drivers/xen/netfront/netfront.c | 3
2 files changed, 109 insertions(+), 120 deletions(-)
diff -r ad4d307bf9ce -r ab1d4fbbe4bf drivers/xen/netfront/accel.c
--- a/drivers/xen/netfront/accel.c Tue Mar 31 11:59:10 2009 +0100
+++ b/drivers/xen/netfront/accel.c Tue Mar 31 12:00:03 2009 +0100
@@ -57,9 +57,6 @@ static int netfront_load_accelerator(str
*/
static struct list_head accelerators_list;
-/* Lock to protect access to accelerators_list */
-static spinlock_t accelerators_lock;
-
/* Workqueue to process acceleration configuration changes */
struct workqueue_struct *accel_watch_workqueue;
@@ -69,7 +66,6 @@ void netif_init_accel(void)
void netif_init_accel(void)
{
INIT_LIST_HEAD(&accelerators_list);
- spin_lock_init(&accelerators_lock);
accel_watch_workqueue = create_workqueue("net_accel");
}
@@ -77,13 +73,11 @@ void netif_exit_accel(void)
void netif_exit_accel(void)
{
struct netfront_accelerator *accelerator, *tmp;
- unsigned long flags;
flush_workqueue(accel_watch_workqueue);
destroy_workqueue(accel_watch_workqueue);
- spin_lock_irqsave(&accelerators_lock, flags);
-
+ /* No lock required as everything else should be quiet by now */
list_for_each_entry_safe(accelerator, tmp, &accelerators_list, link) {
BUG_ON(!list_empty(&accelerator->vif_states));
@@ -91,8 +85,6 @@ void netif_exit_accel(void)
kfree(accelerator->frontend);
kfree(accelerator);
}
-
- spin_unlock_irqrestore(&accelerators_lock, flags);
}
@@ -245,16 +237,12 @@ static int match_accelerator(const char
/*
* Add a frontend vif to the list of vifs that is using a netfront
- * accelerator plugin module.
+ * accelerator plugin module. Must be called with the accelerator
+ * mutex held.
*/
static void add_accelerator_vif(struct netfront_accelerator *accelerator,
struct netfront_info *np)
{
- unsigned long flags;
-
- /* Need lock to write list */
- spin_lock_irqsave(&accelerator->vif_states_lock, flags);
-
if (np->accelerator == NULL) {
np->accelerator = accelerator;
@@ -267,13 +255,13 @@ static void add_accelerator_vif(struct n
*/
BUG_ON(np->accelerator != accelerator);
}
-
- spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
-}
-
-
-/*
- * Initialise the state to track an accelerator plugin module.
+}
+
+
+/*
+ * Initialise the state to track an accelerator plugin module.
+ *
+ * Must be called with the accelerator mutex held.
*/
static int init_accelerator(const char *frontend,
struct netfront_accelerator **result,
@@ -281,7 +269,6 @@ static int init_accelerator(const char *
{
struct netfront_accelerator *accelerator =
kmalloc(sizeof(struct netfront_accelerator), GFP_KERNEL);
- unsigned long flags;
int frontend_len;
if (!accelerator) {
@@ -303,9 +290,7 @@ static int init_accelerator(const char *
accelerator->hooks = hooks;
- spin_lock_irqsave(&accelerators_lock, flags);
list_add(&accelerator->link, &accelerators_list);
- spin_unlock_irqrestore(&accelerators_lock, flags);
*result = accelerator;
@@ -316,11 +301,14 @@ static int init_accelerator(const char *
/*
* Modify the hooks stored in the per-vif state to match that in the
* netfront accelerator's state.
+ *
+ * Takes the vif_states_lock spinlock and may sleep.
*/
static void
accelerator_set_vif_state_hooks(struct netfront_accel_vif_state *vif_state)
{
- /* This function must be called with the vif_states_lock held */
+ struct netfront_accelerator *accelerator;
+ unsigned long flags;
DPRINTK("%p\n",vif_state);
@@ -328,19 +316,25 @@ accelerator_set_vif_state_hooks(struct n
netif_poll_disable(vif_state->np->netdev);
netif_tx_lock_bh(vif_state->np->netdev);
- vif_state->hooks = vif_state->np->accelerator->hooks;
+ accelerator = vif_state->np->accelerator;
+ spin_lock_irqsave(&accelerator->vif_states_lock, flags);
+ vif_state->hooks = accelerator->hooks;
+ spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
netif_tx_unlock_bh(vif_state->np->netdev);
netif_poll_enable(vif_state->np->netdev);
}
+/*
+ * Must be called with the accelerator mutex held. Takes the
+ * vif_states_lock spinlock.
+ */
static void accelerator_probe_new_vif(struct netfront_info *np,
struct xenbus_device *dev,
struct netfront_accelerator *accelerator)
{
struct netfront_accel_hooks *hooks;
- unsigned long flags;
DPRINTK("\n");
@@ -349,17 +343,8 @@ static void accelerator_probe_new_vif(st
hooks = accelerator->hooks;
- if (hooks) {
- if (hooks->new_device(np->netdev, dev) == 0) {
- spin_lock_irqsave
- (&accelerator->vif_states_lock, flags);
-
- accelerator_set_vif_state_hooks(&np->accel_vif_state);
-
- spin_unlock_irqrestore
- (&accelerator->vif_states_lock, flags);
- }
- }
+ if (hooks && hooks->new_device(np->netdev, dev) == 0)
+ accelerator_set_vif_state_hooks(&np->accel_vif_state);
return;
}
@@ -368,7 +353,10 @@ static void accelerator_probe_new_vif(st
/*
* Request that a particular netfront accelerator plugin is loaded.
* Usually called as a result of the vif configuration specifying
- * which one to use. Must be called with accelerator_mutex held
+ * which one to use.
+ *
+ * Must be called with accelerator_mutex held. Takes the
+ * vif_states_lock spinlock.
*/
static int netfront_load_accelerator(struct netfront_info *np,
struct xenbus_device *dev,
@@ -407,13 +395,15 @@ static int netfront_load_accelerator(str
* this accelerator. Notify the accelerator plugin of the relevant
* device if so. Called when an accelerator plugin module is first
* loaded and connects to netfront.
+ *
+ * Must be called with accelerator_mutex held. Takes the
+ * vif_states_lock spinlock.
*/
static void
accelerator_probe_vifs(struct netfront_accelerator *accelerator,
struct netfront_accel_hooks *hooks)
{
struct netfront_accel_vif_state *vif_state, *tmp;
- unsigned long flags;
DPRINTK("%p\n", accelerator);
@@ -425,29 +415,22 @@ accelerator_probe_vifs(struct netfront_a
BUG_ON(hooks == NULL);
accelerator->hooks = hooks;
- /*
- * currently hold accelerator_mutex, so don't need
- * vif_states_lock to read the list
- */
+ /* Holds accelerator_mutex to iterate list */
list_for_each_entry_safe(vif_state, tmp, &accelerator->vif_states,
link) {
struct netfront_info *np = vif_state->np;
- if (hooks->new_device(np->netdev, vif_state->dev) == 0) {
- spin_lock_irqsave
- (&accelerator->vif_states_lock, flags);
-
+ if (hooks->new_device(np->netdev, vif_state->dev) == 0)
accelerator_set_vif_state_hooks(vif_state);
-
- spin_unlock_irqrestore
- (&accelerator->vif_states_lock, flags);
- }
- }
-}
-
-
-/*
- * Called by the netfront accelerator plugin module when it has loaded
+ }
+}
+
+
+/*
+ * Called by the netfront accelerator plugin module when it has
+ * loaded.
+ *
+ * Takes the accelerator_mutex and vif_states_lock spinlock.
*/
int netfront_accelerator_loaded(int version, const char *frontend,
struct netfront_accel_hooks *hooks)
@@ -503,14 +486,20 @@ EXPORT_SYMBOL_GPL(netfront_accelerator_l
/*
* Remove the hooks from a single vif state.
+ *
+ * Takes the vif_states_lock spinlock and may sleep.
*/
static void
accelerator_remove_single_hook(struct netfront_accelerator *accelerator,
struct netfront_accel_vif_state *vif_state)
{
+ unsigned long flags;
+
/* Make sure there are no data path operations going on */
netif_poll_disable(vif_state->np->netdev);
netif_tx_lock_bh(vif_state->np->netdev);
+
+ spin_lock_irqsave(&accelerator->vif_states_lock, flags);
/*
* Remove the hooks, but leave the vif_state on the
@@ -520,6 +509,8 @@ accelerator_remove_single_hook(struct ne
*/
vif_state->hooks = NULL;
+ spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
+
netif_tx_unlock_bh(vif_state->np->netdev);
netif_poll_enable(vif_state->np->netdev);
}
@@ -527,25 +518,25 @@ accelerator_remove_single_hook(struct ne
/*
* Safely remove the accelerator function hooks from a netfront state.
+ *
+ * Must be called with the accelerator mutex held. Takes the
+ * vif_states_lock spinlock.
*/
static void accelerator_remove_hooks(struct netfront_accelerator *accelerator)
{
- struct netfront_accel_hooks *hooks;
struct netfront_accel_vif_state *vif_state, *tmp;
unsigned long flags;
- /* Mutex is held so don't need vif_states_lock to iterate list */
+ /* Mutex is held to iterate list */
list_for_each_entry_safe(vif_state, tmp,
&accelerator->vif_states,
link) {
- spin_lock_irqsave(&accelerator->vif_states_lock, flags);
-
if(vif_state->hooks) {
- hooks = vif_state->hooks;
-
+ spin_lock_irqsave(&accelerator->vif_states_lock, flags);
+
/* Last chance to get statistics from the accelerator */
- hooks->get_stats(vif_state->np->netdev,
- &vif_state->np->stats);
+ vif_state->hooks->get_stats(vif_state->np->netdev,
+ &vif_state->np->stats);
spin_unlock_irqrestore(&accelerator->vif_states_lock,
flags);
@@ -553,9 +544,6 @@ static void accelerator_remove_hooks(str
accelerator_remove_single_hook(accelerator, vif_state);
accelerator->hooks->remove(vif_state->dev);
- } else {
- spin_unlock_irqrestore(&accelerator->vif_states_lock,
- flags);
}
}
@@ -567,47 +555,47 @@ static void accelerator_remove_hooks(str
* Called by a netfront accelerator when it is unloaded. This safely
* removes the hooks into the plugin and blocks until all devices have
* finished using it, so on return it is safe to unload.
+ *
+ * Takes the accelerator mutex, and vif_states_lock spinlock.
*/
void netfront_accelerator_stop(const char *frontend)
{
struct netfront_accelerator *accelerator;
- unsigned long flags;
mutex_lock(&accelerator_mutex);
- spin_lock_irqsave(&accelerators_lock, flags);
list_for_each_entry(accelerator, &accelerators_list, link) {
if (match_accelerator(frontend, accelerator)) {
- spin_unlock_irqrestore(&accelerators_lock, flags);
-
accelerator_remove_hooks(accelerator);
-
goto out;
}
}
- spin_unlock_irqrestore(&accelerators_lock, flags);
out:
mutex_unlock(&accelerator_mutex);
}
EXPORT_SYMBOL_GPL(netfront_accelerator_stop);
-/* Helper for call_remove and do_suspend */
-static int do_remove(struct netfront_info *np, struct xenbus_device *dev,
- unsigned long *lock_flags)
+/*
+ * Helper for call_remove and do_suspend
+ *
+ * Must be called with the accelerator mutex held. Takes the
+ * vif_states_lock spinlock.
+ */
+static int do_remove(struct netfront_info *np, struct xenbus_device *dev)
{
struct netfront_accelerator *accelerator = np->accelerator;
- struct netfront_accel_hooks *hooks;
+ unsigned long flags;
int rc = 0;
if (np->accel_vif_state.hooks) {
- hooks = np->accel_vif_state.hooks;
+ spin_lock_irqsave(&accelerator->vif_states_lock, flags);
/* Last chance to get statistics from the accelerator */
- hooks->get_stats(np->netdev, &np->stats);
+ np->accel_vif_state.hooks->get_stats(np->netdev, &np->stats);
spin_unlock_irqrestore(&accelerator->vif_states_lock,
- *lock_flags);
+ flags);
/*
* Try and do the opposite of accelerator_probe_new_vif
@@ -618,20 +606,21 @@ static int do_remove(struct netfront_inf
&np->accel_vif_state);
rc = accelerator->hooks->remove(dev);
-
- spin_lock_irqsave(&accelerator->vif_states_lock, *lock_flags);
}
return rc;
}
+/*
+ * Must be called with the accelerator mutex held. Takes the
+ * vif_states_lock spinlock
+ */
static int netfront_remove_accelerator(struct netfront_info *np,
struct xenbus_device *dev)
{
struct netfront_accelerator *accelerator;
struct netfront_accel_vif_state *tmp_vif_state;
- unsigned long flags;
int rc = 0;
/* Check that we've got a device that was accelerated */
@@ -639,8 +628,6 @@ static int netfront_remove_accelerator(s
return rc;
accelerator = np->accelerator;
-
- spin_lock_irqsave(&accelerator->vif_states_lock, flags);
list_for_each_entry(tmp_vif_state, &accelerator->vif_states,
link) {
@@ -650,16 +637,18 @@ static int netfront_remove_accelerator(s
}
}
- rc = do_remove(np, dev, &flags);
+ rc = do_remove(np, dev);
np->accelerator = NULL;
- spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
-
return rc;
}
+/*
+ * No lock pre-requisites. Takes the accelerator mutex and the
+ * vif_states_lock spinlock.
+ */
int netfront_accelerator_call_remove(struct netfront_info *np,
struct xenbus_device *dev)
{
@@ -671,11 +660,14 @@ int netfront_accelerator_call_remove(str
return rc;
}
-
+
+/*
+ * No lock pre-requisites. Takes the accelerator mutex and the
+ * vif_states_lock spinlock.
+ */
int netfront_accelerator_suspend(struct netfront_info *np,
struct xenbus_device *dev)
{
- unsigned long flags;
int rc = 0;
netfront_accelerator_remove_watch(np);
@@ -690,11 +682,7 @@ int netfront_accelerator_suspend(struct
* Call the remove accelerator hook, but leave the vif_state
* on the accelerator's list in case there is a suspend_cancel.
*/
- spin_lock_irqsave(&np->accelerator->vif_states_lock, flags);
-
- rc = do_remove(np, dev, &flags);
-
- spin_unlock_irqrestore(&np->accelerator->vif_states_lock, flags);
+ rc = do_remove(np, dev);
out:
mutex_unlock(&accelerator_mutex);
return rc;
@@ -713,15 +701,16 @@ int netfront_accelerator_suspend_cancel(
netfront_accelerator_add_watch(np);
return 0;
}
-
-
+
+
+/*
+ * No lock pre-requisites. Takes the accelerator mutex
+ */
void netfront_accelerator_resume(struct netfront_info *np,
struct xenbus_device *dev)
{
struct netfront_accel_vif_state *accel_vif_state = NULL;
- spinlock_t *vif_states_lock;
- unsigned long flags;
-
+
mutex_lock(&accelerator_mutex);
/* Check that we've got a device that was accelerated */
@@ -733,9 +722,6 @@ void netfront_accelerator_resume(struct
link) {
if (accel_vif_state->dev == dev) {
BUG_ON(accel_vif_state != &np->accel_vif_state);
-
- vif_states_lock = &np->accelerator->vif_states_lock;
- spin_lock_irqsave(vif_states_lock, flags);
/*
* Remove it from the accelerator's list so
@@ -744,9 +730,7 @@ void netfront_accelerator_resume(struct
*/
list_del(&accel_vif_state->link);
np->accelerator = NULL;
-
- spin_unlock_irqrestore(vif_states_lock, flags);
-
+
break;
}
}
@@ -757,11 +741,13 @@ void netfront_accelerator_resume(struct
}
+/*
+ * No lock pre-requisites. Takes the vif_states_lock spinlock
+ */
int netfront_check_accelerator_queue_ready(struct net_device *dev,
struct netfront_info *np)
{
struct netfront_accelerator *accelerator;
- struct netfront_accel_hooks *hooks;
int rc = 1;
unsigned long flags;
@@ -770,8 +756,8 @@ int netfront_check_accelerator_queue_rea
/* Call the check_ready accelerator hook. */
if (np->accel_vif_state.hooks && accelerator) {
spin_lock_irqsave(&accelerator->vif_states_lock, flags);
- hooks = np->accel_vif_state.hooks;
- if (hooks && np->accelerator == accelerator)
+ if (np->accel_vif_state.hooks &&
+ np->accelerator == accelerator)
rc = np->accel_vif_state.hooks->check_ready(dev);
spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
}
@@ -780,11 +766,13 @@ int netfront_check_accelerator_queue_rea
}
+/*
+ * No lock pre-requisites. Takes the vif_states_lock spinlock
+ */
void netfront_accelerator_call_stop_napi_irq(struct netfront_info *np,
struct net_device *dev)
{
struct netfront_accelerator *accelerator;
- struct netfront_accel_hooks *hooks;
unsigned long flags;
accelerator = np->accelerator;
@@ -792,19 +780,21 @@ void netfront_accelerator_call_stop_napi
/* Call the stop_napi_interrupts accelerator hook. */
if (np->accel_vif_state.hooks && accelerator != NULL) {
spin_lock_irqsave(&accelerator->vif_states_lock, flags);
- hooks = np->accel_vif_state.hooks;
- if (hooks && np->accelerator == accelerator)
+ if (np->accel_vif_state.hooks &&
+ np->accelerator == accelerator)
np->accel_vif_state.hooks->stop_napi_irq(dev);
spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
}
}
+/*
+ * No lock pre-requisites. Takes the vif_states_lock spinlock
+ */
int netfront_accelerator_call_get_stats(struct netfront_info *np,
struct net_device *dev)
{
struct netfront_accelerator *accelerator;
- struct netfront_accel_hooks *hooks;
unsigned long flags;
int rc = 0;
@@ -813,8 +803,8 @@ int netfront_accelerator_call_get_stats(
/* Call the get_stats accelerator hook. */
if (np->accel_vif_state.hooks && accelerator != NULL) {
spin_lock_irqsave(&accelerator->vif_states_lock, flags);
- hooks = np->accel_vif_state.hooks;
- if (hooks && np->accelerator == accelerator)
+ if (np->accel_vif_state.hooks &&
+ np->accelerator == accelerator)
rc = np->accel_vif_state.hooks->get_stats(dev,
&np->stats);
spin_unlock_irqrestore(&accelerator->vif_states_lock, flags);
diff -r ad4d307bf9ce -r ab1d4fbbe4bf drivers/xen/netfront/netfront.c
--- a/drivers/xen/netfront/netfront.c Tue Mar 31 11:59:10 2009 +0100
+++ b/drivers/xen/netfront/netfront.c Tue Mar 31 12:00:03 2009 +0100
@@ -2238,10 +2238,9 @@ static void __exit netif_exit(void)
#ifdef CONFIG_INET
unregister_inetaddr_notifier(¬ifier_inetdev);
#endif
+ xenbus_unregister_driver(&netfront_driver);
netif_exit_accel();
-
- return xenbus_unregister_driver(&netfront_driver);
}
module_exit(netif_exit);
_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog
|
Home