Kieran Mansley
2007-Dec-18 15:07 UTC
[Xen-devel] [PATCH] hotplug/unplug support and bug fixes for network accelerators
Adding support to the network accelerators for hot plugging and unplugging the NIC revealed some new and exciting ways in which the network acceleration plugins could be loaded and unloaded. This patch makes sure that this all works properly. While this is motivated by what is arguably a new feature (a xenstore watch in domU to change the accelerator when the configuration changes) most of the changes are in fact bug fixes that this (and the rest of our 3.2.0-rc1 testing) turned up, and so I would recommend the patch be applied to 3.2.0 In detail, the changes are as follows: netback: - module_get/put not called consistently on one code path, now fixed - strcmp used where !strcmp was intended on disconnect - backend accelerator removal while active interfaces present now supported - code cleanup (better code reuse, more consistent function names, comments, etc) - bump protocol version to reflect these changes netfront: - add watch on accel configuration in xenstore, and respond to changes - correct handling of errors from accelerator probe (could in theory lead to BUG() before when using accelerators) - simplify plugin API (remove need for netfront_accelerator_ready(), and netfront_load_accelerator() now internal only) and less "accelerator" code in netfront.c - correct locking for removal of frontend accelerator (could lead to spin lock deadlock before when using accelerators) - bump protocol version to reflect these changes Signed-off-by: Kieran Mansley <kmansley@solarflare.com> Allow network accelerators to deal with hot-plug/unplug on physical devices Add watch for acceleration configuration in frontend diff -r fc406f9e9a0a drivers/xen/netback/accel.c --- a/drivers/xen/netback/accel.c Fri Dec 14 11:28:40 2007 +0000 +++ b/drivers/xen/netback/accel.c Mon Dec 17 15:44:18 2007 +0000 @@ -76,12 +76,27 @@ static int match_accelerator(struct xenb } } -/* - * Notify all suitable backends that a new accelerator is available - * and connected. This will also notify the accelerator plugin module + +static void do_probe(struct backend_info *be, + struct netback_accelerator *accelerator, + struct xenbus_device *xendev) +{ + be->accelerator = accelerator; + atomic_inc(&be->accelerator->use_count); + if (be->accelerator->hooks->probe(xendev) != 0) { + atomic_dec(&be->accelerator->use_count); + module_put(be->accelerator->hooks->owner); + be->accelerator = NULL; + } +} + + +/* + * Notify suitable backends that a new accelerator is available and + * connected. This will also notify the accelerator plugin module * that it is being used for a device through the probe hook. */ -static int netback_accelerator_tell_backend(struct device *dev, void *arg) +static int netback_accelerator_probe_backend(struct device *dev, void *arg) { struct netback_accelerator *accelerator = (struct netback_accelerator *)arg; @@ -90,14 +105,37 @@ static int netback_accelerator_tell_back if (!strcmp("vif", xendev->devicetype)) { struct backend_info *be = xendev->dev.driver_data; - if (match_accelerator(xendev, be, accelerator)) { - be->accelerator = accelerator; - atomic_inc(&be->accelerator->use_count); - be->accelerator->hooks->probe(xendev); - } - } - return 0; -} + if (match_accelerator(xendev, be, accelerator) && + try_module_get(accelerator->hooks->owner)) { + do_probe(be, accelerator, xendev); + } + } + return 0; +} + + +/* + * Notify suitable backends that an accelerator is unavailable. + */ +static int netback_accelerator_remove_backend(struct device *dev, void *arg) +{ + struct xenbus_device *xendev = to_xenbus_device(dev); + struct netback_accelerator *accelerator = + (struct netback_accelerator *)arg; + + if (!strcmp("vif", xendev->devicetype)) { + struct backend_info *be = xendev->dev.driver_data; + + if (be->accelerator == accelerator) { + be->accelerator->hooks->remove(xendev); + atomic_dec(&be->accelerator->use_count); + module_put(be->accelerator->hooks->owner); + be->accelerator = NULL; + } + } + return 0; +} + /* @@ -133,7 +171,6 @@ int netback_connect_accelerator(unsigned return -ENOMEM; } - new_accelerator->id = id; eth_name_len = strlen(eth_name)+1; @@ -152,44 +189,22 @@ int netback_connect_accelerator(unsigned mutex_lock(&accelerators_mutex); list_add(&new_accelerator->link, &accelerators_list); - mutex_unlock(&accelerators_mutex); /* tell existing backends about new plugin */ xenbus_for_each_backend(new_accelerator, - netback_accelerator_tell_backend); + netback_accelerator_probe_backend); + + mutex_unlock(&accelerators_mutex); return 0; } EXPORT_SYMBOL_GPL(netback_connect_accelerator); - - -/* - * Remove the link from backend state to a particular accelerator - */ -static int netback_accelerator_cleanup_backend(struct device *dev, void *arg) -{ - struct netback_accelerator *accelerator = - (struct netback_accelerator *)arg; - struct xenbus_device *xendev = to_xenbus_device(dev); - - if (!strcmp("vif", xendev->devicetype)) { - struct backend_info *be = xendev->dev.driver_data; - if (be->accelerator == accelerator) - be->accelerator = NULL; - } - return 0; -} /* * Disconnect an accelerator plugin module that has previously been * connected. - * - * This should only be allowed when there are no remaining users - - * i.e. it is not necessary to go through and clear all the hooks, as - * they should have already been removed. This is enforced by taking - * a module reference to the plugin while the interfaces are in use */ void netback_disconnect_accelerator(int id, const char *eth_name) { @@ -197,17 +212,14 @@ void netback_disconnect_accelerator(int mutex_lock(&accelerators_mutex); list_for_each_entry_safe(accelerator, next, &accelerators_list, link) { - if (strcmp(eth_name, accelerator->eth_name)) { + if (!strcmp(eth_name, accelerator->eth_name)) { + xenbus_for_each_backend + (accelerator, netback_accelerator_remove_backend); BUG_ON(atomic_read(&accelerator->use_count) != 0); - list_del(&accelerator->link); - mutex_unlock(&accelerators_mutex); - - xenbus_for_each_backend(accelerator, - netback_accelerator_cleanup_backend); - + list_del(&accelerator->link); kfree(accelerator->eth_name); kfree(accelerator); - return; + break; } } mutex_unlock(&accelerators_mutex); @@ -228,9 +240,7 @@ void netback_probe_accelerators(struct b list_for_each_entry(accelerator, &accelerators_list, link) { if (match_accelerator(dev, be, accelerator) && try_module_get(accelerator->hooks->owner)) { - be->accelerator = accelerator; - atomic_inc(&be->accelerator->use_count); - be->accelerator->hooks->probe(dev); + do_probe(be, accelerator, dev); break; } } @@ -241,13 +251,15 @@ void netback_remove_accelerators(struct void netback_remove_accelerators(struct backend_info *be, struct xenbus_device *dev) { + mutex_lock(&accelerators_mutex); /* Notify the accelerator (if any) of this device''s removal */ - if (be->accelerator) { + if (be->accelerator != NULL) { be->accelerator->hooks->remove(dev); atomic_dec(&be->accelerator->use_count); module_put(be->accelerator->hooks->owner); - } - be->accelerator = NULL; + be->accelerator = NULL; + } + mutex_unlock(&accelerators_mutex); } diff -r fc406f9e9a0a drivers/xen/netback/common.h --- a/drivers/xen/netback/common.h Fri Dec 14 11:28:40 2007 +0000 +++ b/drivers/xen/netback/common.h Mon Dec 17 15:44:18 2007 +0000 @@ -150,7 +150,7 @@ struct backend_info { struct netback_accelerator *accelerator; }; -#define NETBACK_ACCEL_VERSION 0x00010000 +#define NETBACK_ACCEL_VERSION 0x00010001 /* * Connect an accelerator plugin module to netback. Returns zero on diff -r fc406f9e9a0a drivers/xen/netfront/accel.c --- a/drivers/xen/netfront/accel.c Fri Dec 14 11:28:40 2007 +0000 +++ b/drivers/xen/netfront/accel.c Tue Dec 18 13:10:41 2007 +0000 @@ -45,6 +45,12 @@ #define WPRINTK(fmt, args...) \ printk(KERN_WARNING "netfront/accel: " fmt, ##args) +static int netfront_remove_accelerator(struct netfront_info *np, + struct xenbus_device *dev); +static int netfront_load_accelerator(struct netfront_info *np, + struct xenbus_device *dev, + const char *frontend); + /* * List of all netfront accelerator plugin modules available. Each * list entry is of type struct netfront_accelerator. @@ -83,6 +89,120 @@ void netif_exit_accel(void) /* + * Watch the configured accelerator and change plugin if it''s modified + */ +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20) +static void accel_watch_work(struct work_struct *context) +#else +static void accel_watch_work(void *context) +#endif +{ +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20) + struct netfront_accel_vif_state *vif_state = + container_of(context, struct netfront_accel_vif_state, + accel_work); +#else + struct netfront_accel_vif_state *vif_state = + (struct netfront_accel_vif_state *)context; +#endif + struct netfront_info *np = vif_state->np; + char *accel_frontend; + int accel_len, rc = -1; + + mutex_lock(&accelerator_mutex); + + accel_frontend = xenbus_read(XBT_NIL, np->xbdev->otherend, + "accel-frontend", &accel_len); + if (IS_ERR(accel_frontend)) { + accel_frontend = NULL; + netfront_remove_accelerator(np, np->xbdev); + } else { + /* If this is the first time, request the accelerator, + otherwise only request one if it has changed */ + if (vif_state->accel_frontend == NULL) { + rc = netfront_load_accelerator(np, np->xbdev, + accel_frontend); + } else { + if (strncmp(vif_state->accel_frontend, accel_frontend, + accel_len)) { + netfront_remove_accelerator(np, np->xbdev); + rc = netfront_load_accelerator(np, np->xbdev, + accel_frontend); + } + } + } + + /* Get rid of previous state and replace with the new name */ + if (vif_state->accel_frontend != NULL) + kfree(vif_state->accel_frontend); + vif_state->accel_frontend = accel_frontend; + + mutex_unlock(&accelerator_mutex); + + if (rc == 0) { + DPRINTK("requesting module %s\n", accel_frontend); + request_module("%s", accel_frontend); + /* + * Module should now call netfront_accelerator_loaded() once + * it''s up and running, and we can continue from there + */ + } +} + + +static void accel_watch_changed(struct xenbus_watch *watch, + const char **vec, unsigned int len) +{ + struct netfront_accel_vif_state *vif_state = + container_of(watch, struct netfront_accel_vif_state, + accel_watch); + schedule_work(&vif_state->accel_work); +} + + +void netfront_accelerator_add_watch(struct netfront_info *np) +{ + int err; + + /* Check we''re not trying to overwrite an existing watch */ + BUG_ON(np->accel_vif_state.accel_watch.node != NULL); + + /* Get a watch on the accelerator plugin */ + err = xenbus_watch_path2(np->xbdev, np->xbdev->otherend, + "accel-frontend", + &np->accel_vif_state.accel_watch, + accel_watch_changed); + if (err) { + DPRINTK("%s: Failed to register accel watch: %d\n", + __FUNCTION__, err); + np->accel_vif_state.accel_watch.node = NULL; + } +} + + +static +void netfront_accelerator_remove_watch(struct netfront_info *np) +{ + struct netfront_accel_vif_state *vif_state = &np->accel_vif_state; + + /* Get rid of watch on accelerator plugin */ + if (vif_state->accel_watch.node != NULL) { + unregister_xenbus_watch(&vif_state->accel_watch); + kfree(vif_state->accel_watch.node); + vif_state->accel_watch.node = NULL; + + flush_scheduled_work(); + + /* Clean up any state left from watch */ + if (vif_state->accel_frontend != NULL) { + kfree(vif_state->accel_frontend); + vif_state->accel_frontend = NULL; + } + } +} + + +/* * Initialise the accel_vif_state field in the netfront state */ void init_accelerator_vif(struct netfront_info *np, @@ -93,6 +213,13 @@ void init_accelerator_vif(struct netfron /* It''s assumed that these things don''t change */ np->accel_vif_state.np = np; np->accel_vif_state.dev = dev; + +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20) + INIT_WORK(&np->accel_vif_state.accel_work, accel_watch_work); +#else + INIT_WORK(&np->accel_vif_state.accel_work, accel_watch_work, + &np->accel_vif_state); +#endif } @@ -202,6 +329,7 @@ static void accelerator_probe_new_vif(st struct netfront_accelerator *accelerator) { struct netfront_accel_hooks *hooks; + unsigned long flags; DPRINTK("\n"); @@ -211,32 +339,35 @@ static void accelerator_probe_new_vif(st hooks = accelerator->hooks; if (hooks) { - hooks->new_device(np->netdev, dev); - /* - * Hooks will get linked into vif_state by a future - * call by the accelerator to netfront_accelerator_ready() - */ + 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); + } } return; } + /* * Request that a particular netfront accelerator plugin is loaded. * Usually called as a result of the vif configuration specifying - * which one to use. - */ -int netfront_load_accelerator(struct netfront_info *np, - struct xenbus_device *dev, - const char *frontend) + * which one to use. Must be called with accelerator_mutex held + */ +static int netfront_load_accelerator(struct netfront_info *np, + struct xenbus_device *dev, + const char *frontend) { struct netfront_accelerator *accelerator; int rc = 0; unsigned long flags; DPRINTK(" %s\n", frontend); - - mutex_lock(&accelerator_mutex); spin_lock_irqsave(&accelerators_lock, flags); @@ -249,8 +380,6 @@ int netfront_load_accelerator(struct net spin_unlock_irqrestore(&accelerators_lock, flags); accelerator_probe_new_vif(np, dev, accelerator); - - mutex_unlock(&accelerator_mutex); return 0; } } @@ -258,7 +387,6 @@ int netfront_load_accelerator(struct net /* Couldn''t find it, so create a new one and load the module */ if ((rc = init_accelerator(frontend, &accelerator, NULL)) < 0) { spin_unlock_irqrestore(&accelerators_lock, flags); - mutex_unlock(&accelerator_mutex); return rc; } @@ -266,18 +394,6 @@ int netfront_load_accelerator(struct net /* Include this frontend device on the accelerator''s list */ add_accelerator_vif(accelerator, np); - - mutex_unlock(&accelerator_mutex); - - DPRINTK("requesting module %s\n", frontend); - - /* load module */ - request_module("%s", frontend); - - /* - * Module should now call netfront_accelerator_loaded() once - * it''s up and running, and we can continue from there - */ return rc; } @@ -294,6 +410,7 @@ accelerator_probe_vifs(struct netfront_a struct netfront_accel_hooks *hooks) { struct netfront_accel_vif_state *vif_state, *tmp; + unsigned long flags; DPRINTK("%p\n", accelerator); @@ -313,13 +430,15 @@ accelerator_probe_vifs(struct netfront_a link) { struct netfront_info *np = vif_state->np; - hooks->new_device(np->netdev, vif_state->dev); - - /* - * Hooks will get linked into vif_state by a call to - * netfront_accelerator_ready() once accelerator - * plugin is ready for action - */ + if (hooks->new_device(np->netdev, vif_state->dev) == 0) { + spin_lock_irqsave + (&accelerator->vif_states_lock, flags); + + accelerator_set_vif_state_hooks(vif_state); + + spin_unlock_irqrestore + (&accelerator->vif_states_lock, flags); + } } } @@ -385,48 +504,6 @@ EXPORT_SYMBOL_GPL(netfront_accelerator_l /* - * Called by the accelerator module after it has been probed with a - * network device to say that it is ready to start accelerating - * traffic on that device - */ -void netfront_accelerator_ready(const char *frontend, - struct xenbus_device *dev) -{ - struct netfront_accelerator *accelerator; - struct netfront_accel_vif_state *accel_vif_state; - unsigned long flags, flags1; - - DPRINTK("%s %p\n", frontend, dev); - - spin_lock_irqsave(&accelerators_lock, flags); - - list_for_each_entry(accelerator, &accelerators_list, link) { - if (match_accelerator(frontend, accelerator)) { - /* - * Mutex not held so need vif_states_lock for - * list - */ - spin_lock_irqsave - (&accelerator->vif_states_lock, flags1); - - list_for_each_entry(accel_vif_state, - &accelerator->vif_states, link) { - if (accel_vif_state->dev == dev) - accelerator_set_vif_state_hooks - (accel_vif_state); - } - - spin_unlock_irqrestore - (&accelerator->vif_states_lock, flags1); - break; - } - } - spin_unlock_irqrestore(&accelerators_lock, flags); -} -EXPORT_SYMBOL_GPL(netfront_accelerator_ready); - - -/* * Remove the hooks from a single vif state. */ static void @@ -467,16 +544,21 @@ static void accelerator_remove_hooks(str if(vif_state->hooks) { hooks = vif_state->hooks; - accelerator_remove_single_hook(accelerator, vif_state); /* Last chance to get statistics from the accelerator */ hooks->get_stats(vif_state->np->netdev, &vif_state->np->stats); - } - - spin_unlock_irqrestore(&accelerator->vif_states_lock, flags); - - accelerator->hooks->remove(vif_state->dev); + + spin_unlock_irqrestore(&accelerator->vif_states_lock, + flags); + + accelerator_remove_single_hook(accelerator, vif_state); + + accelerator->hooks->remove(vif_state->dev); + } else { + spin_unlock_irqrestore(&accelerator->vif_states_lock, + flags); + } } accelerator->hooks = NULL; @@ -522,6 +604,12 @@ static int do_remove(struct netfront_inf if (np->accel_vif_state.hooks) { hooks = np->accel_vif_state.hooks; + + /* Last chance to get statistics from the accelerator */ + hooks->get_stats(np->netdev, &np->stats); + + spin_unlock_irqrestore(&accelerator->vif_states_lock, + *lock_flags); /* * Try and do the opposite of accelerator_probe_new_vif @@ -531,14 +619,6 @@ static int do_remove(struct netfront_inf accelerator_remove_single_hook(accelerator, &np->accel_vif_state); - /* Last chance to get statistics from the accelerator */ - hooks->get_stats(np->netdev, &np->stats); - } - - if (accelerator->hooks) { - spin_unlock_irqrestore(&accelerator->vif_states_lock, - *lock_flags); - rc = accelerator->hooks->remove(dev); spin_lock_irqsave(&accelerator->vif_states_lock, *lock_flags); @@ -546,21 +626,19 @@ static int do_remove(struct netfront_inf return rc; } - - -int netfront_accelerator_call_remove(struct netfront_info *np, - struct xenbus_device *dev) + + +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; - mutex_lock(&accelerator_mutex); - /* Check that we''ve got a device that was accelerated */ if (np->accelerator == NULL) - goto out; + return rc; accelerator = np->accelerator; @@ -579,17 +657,30 @@ int netfront_accelerator_call_remove(str np->accelerator = NULL; spin_unlock_irqrestore(&accelerator->vif_states_lock, flags); - out: + + return rc; +} + + +int netfront_accelerator_call_remove(struct netfront_info *np, + struct xenbus_device *dev) +{ + int rc; + netfront_accelerator_remove_watch(np); + mutex_lock(&accelerator_mutex); + rc = netfront_remove_accelerator(np, dev); mutex_unlock(&accelerator_mutex); return rc; } - + int netfront_accelerator_suspend(struct netfront_info *np, struct xenbus_device *dev) { unsigned long flags; int rc = 0; + + netfront_accelerator_remove_watch(np); mutex_lock(&accelerator_mutex); @@ -641,6 +732,7 @@ int netfront_accelerator_suspend_cancel( out: mutex_unlock(&accelerator_mutex); + netfront_accelerator_add_watch(np); return 0; } diff -r fc406f9e9a0a drivers/xen/netfront/netfront.c --- a/drivers/xen/netfront/netfront.c Fri Dec 14 11:28:40 2007 +0000 +++ b/drivers/xen/netfront/netfront.c Mon Dec 17 15:44:18 2007 +0000 @@ -378,6 +378,10 @@ static int talk_to_backend(struct xenbus if (err) goto out; + /* This will load an accelerator if one is configured when the + * watch fires */ + netfront_accelerator_add_watch(info); + again: err = xenbus_transaction_start(&xbt); if (err) { @@ -452,6 +456,7 @@ again: xenbus_transaction_end(xbt, 1); xenbus_dev_fatal(dev, err, "%s", message); destroy_ring: + netfront_accelerator_call_remove(info, dev); netif_disconnect_backend(info); out: return err; @@ -1746,9 +1751,7 @@ static int network_connect(struct net_de struct sk_buff *skb; grant_ref_t ref; netif_rx_request_t *req; - unsigned int feature_rx_copy, feature_rx_flip, feature_accel; - char *accel_frontend; - int accel_len; + unsigned int feature_rx_copy, feature_rx_flip; err = xenbus_scanf(XBT_NIL, np->xbdev->otherend, "feature-rx-copy", "%u", &feature_rx_copy); @@ -1759,12 +1762,6 @@ static int network_connect(struct net_de if (err != 1) feature_rx_flip = 1; - feature_accel = 1; - accel_frontend = xenbus_read(XBT_NIL, np->xbdev->otherend, - "accel-frontend", &accel_len); - if (IS_ERR(accel_frontend)) - feature_accel = 0; - /* * Copy packets on receive path if: * (a) This was requested by user, and the backend supports it; or @@ -1776,11 +1773,6 @@ static int network_connect(struct net_de err = talk_to_backend(np->xbdev, np); if (err) return err; - - if (feature_accel) { - netfront_load_accelerator(np, np->xbdev, accel_frontend); - kfree(accel_frontend); - } xennet_set_features(dev); diff -r fc406f9e9a0a drivers/xen/netfront/netfront.h --- a/drivers/xen/netfront/netfront.h Fri Dec 14 11:28:40 2007 +0000 +++ b/drivers/xen/netfront/netfront.h Mon Dec 17 15:44:18 2007 +0000 @@ -95,7 +95,7 @@ struct netfront_accel_hooks { /* Version of API/protocol for communication between netfront and acceleration plugin supported */ -#define NETFRONT_ACCEL_VERSION 0x00010002 +#define NETFRONT_ACCEL_VERSION 0x00010003 /* * Per-netfront device state for the accelerator. This is used to @@ -108,6 +108,13 @@ struct netfront_accel_vif_state { struct xenbus_device *dev; struct netfront_info *np; struct netfront_accel_hooks *hooks; + + /* Watch on the accelerator configuration value */ + struct xenbus_watch accel_watch; + /* Work item to process change in accelerator */ + struct work_struct accel_work; + /* The string from xenbus last time accel_watch fired */ + char *accel_frontend; }; /* @@ -210,18 +217,6 @@ extern int netfront_accelerator_loaded(i struct netfront_accel_hooks *hooks); /* - * Called when an accelerator plugin is ready to accelerate a device * - * that has been passed to it from netfront using the "new_device" - * hook. - * - * frontend: the string describing the accelerator. Must match the - * one passed to netfront_accelerator_loaded() - * dev: the xenbus device the plugin was asked to accelerate - */ -extern void netfront_accelerator_ready(const char *frontend, - struct xenbus_device *dev); - -/* * Called by an accelerator plugin module when it is about to unload. * * frontend: the string describing the accelerator. Must match the @@ -235,7 +230,6 @@ extern void netfront_accelerator_stop(co * wake, false if still busy */ extern int netfront_check_queue_ready(struct net_device *net_dev); - /* Internal-to-netfront Functions */ @@ -267,9 +261,7 @@ int netfront_accelerator_call_get_stats( int netfront_accelerator_call_get_stats(struct netfront_info *np, struct net_device *dev); extern -int netfront_load_accelerator(struct netfront_info *np, - struct xenbus_device *dev, - const char *frontend); +void netfront_accelerator_add_watch(struct netfront_info *np); extern void netif_init_accel(void); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel