[Open-FCoE] [RFC PATCH V3 11/13] fcoe: use RTNL mutex to serialize create/destroy and protect hostlist

Joe Eykholt jeykholt at cisco.com
Thu Jul 9 01:04:41 UTC 2009


Chris Leech wrote:
> Rather than rely on the hostlist_lock to be held while creating exchange
> managers, serialize fcoe instance creation and destruction with a mutex.
> This will allow the hostlist addition to be moved out of fcoe_if_create(),
> which will simplify NPIV support.
> 
> RTNL mutex was used because it needs to be held in parts of both the create
> and destroy path anyway, and because the netdevice notifier will be called
> with RTNL held.  If a new mutex was added and fine-grained use of RTNL was
> maintained, then destroying the interface when the netdevice was unregistered
> would deadlock on RTNL unless it was deferred using a workqueue or similar.
> 
> Signed-off-by: Chris Leech <christopher.leech at intel.com>
> ---
> 
>  drivers/scsi/fcoe/fcoe.c |   68 ++++++++++++++++++++++++++++------------------
>  1 files changed, 41 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 49a9d5b..a4b8991 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -49,9 +49,9 @@ MODULE_AUTHOR("Open-FCoE.org");
>  MODULE_DESCRIPTION("FCoE");
>  MODULE_LICENSE("GPL v2");
>  
> -/* fcoe host list */
> +/* fcoe host list
> + * must be accessed under rtnl_lock */
>  LIST_HEAD(fcoe_hostlist);
> -DEFINE_RWLOCK(fcoe_hostlist_lock);
>  DEFINE_PER_CPU(struct fcoe_percpu_s, fcoe_percpu);
>  
>  /* Function Prototypes */
> @@ -186,13 +186,11 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
>  	 * or enter promiscuous mode if not capable of listening
>  	 * for multiple unicast MACs.
>  	 */
> -	rtnl_lock();
>  	memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
>  	dev_unicast_add(netdev, flogi_maddr);
>  	if (fip->spma)
>  		dev_unicast_add(netdev, fip->ctl_src_addr);
>  	dev_mc_add(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> -	rtnl_unlock();
>  
>  	/*
>  	 * setup the receive function from ethernet driver
> @@ -259,7 +257,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
>  	dev_remove_pack(&fcoe->fip_packet_type);
>  
>  	/* Delete secondary MAC addresses */
> -	rtnl_lock();
>  	memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
>  	dev_unicast_delete(netdev, flogi_maddr);
>  	if (!is_zero_ether_addr(fip->data_src_addr))
> @@ -267,7 +264,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
>  	if (fip->spma)
>  		dev_unicast_delete(netdev, fip->ctl_src_addr);
>  	dev_mc_delete(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> -	rtnl_unlock();
>  }
>  
>  /**
> @@ -516,8 +512,6 @@ bool fcoe_oem_match(struct fc_frame *fp)
>   * fcoe_em_config() - allocates em for this lport
>   * @lp: the fcoe that em is to allocated for
>   *
> - * Called with write fcoe_hostlist_lock held.
> - *
>   * Returns : 0 on success
>   */
>  static inline int fcoe_em_config(struct fc_lport *lp)
> @@ -739,11 +733,14 @@ static struct fc_lport *fcoe_if_create(struct fcoe_interface *fcoe,
>  
>  	/*
>  	 * fcoe_em_alloc() and fcoe_hostlist_add() both
> -	 * need to be atomic under fcoe_hostlist_lock
> +	 * need to be atomic with respect to other changes to the hostlist
>  	 * since fcoe_em_alloc() looks for an existing EM
>  	 * instance on host list updated by fcoe_hostlist_add().
> +	 *
> +	 * This is OK because the entire create and destroy paths are called
> +	 * with the RTNL mutex held.
>  	 */
> -	write_lock(&fcoe_hostlist_lock);
> +
>  	/* lport exch manager allocation */
>  	rc = fcoe_em_config(lport);
>  	if (rc) {
> @@ -754,7 +751,6 @@ static struct fc_lport *fcoe_if_create(struct fcoe_interface *fcoe,
>  
>  	/* add to lports list */
>  	fcoe_hostlist_add(lport);
> -	write_unlock(&fcoe_hostlist_lock);
>  
>  	dev_hold(netdev);
>  	fcoe_interface_get(fcoe);
> @@ -795,6 +791,7 @@ static int __init fcoe_if_init(void)
>  int __exit fcoe_if_exit(void)
>  {
>  	fc_release_transport(scsi_transport_fcoe_sw);
> +	scsi_transport_fcoe_sw = NULL;
>  	return 0;
>  }
>  
> @@ -1524,14 +1521,14 @@ static int fcoe_device_notification(struct notifier_block *notifier,
>  	u32 mfs;
>  	int rc = NOTIFY_OK;
>  
> -	read_lock(&fcoe_hostlist_lock);
> +	ASSERT_RTNL();
> +
>  	list_for_each_entry(fcoe, &fcoe_hostlist, list) {
>  		if (fcoe->real_dev == real_dev) {
>  			lp = fcoe->ctlr.lp;
>  			break;
>  		}
>  	}
> -	read_unlock(&fcoe_hostlist_lock);
>  	if (lp == NULL) {
>  		rc = NOTIFY_DONE;
>  		goto out;
> @@ -1674,6 +1671,18 @@ static int fcoe_destroy(const char *buffer, struct kernel_param *kp)
>  	struct fc_lport *lport;
>  	int rc;
>  
> +	rtnl_lock();
> +
> +	/*
> +	 * Make sure the module has been initialized, and is not about to be
> +	 * removed.  Module paramter sysfs files are writable before the
> +	 * module_init function is called and after module_exit.
> +	 */
> +	if (!scsi_transport_fcoe_sw) {
> +		rc = -ENODEV;
> +		goto out_nodev;
> +	}
> +
>  	netdev = fcoe_if_to_netdev(buffer);
>  	if (!netdev) {
>  		rc = -ENODEV;
> @@ -1693,6 +1702,7 @@ static int fcoe_destroy(const char *buffer, struct kernel_param *kp)
>  out_putdev:
>  	dev_put(netdev);
>  out_nodev:
> +	rtnl_unlock();
>  	return rc;
>  }
>  
> @@ -1710,6 +1720,18 @@ static int fcoe_create(const char *buffer, struct kernel_param *kp)
>  	struct fc_lport *lport;
>  	struct net_device *netdev;
>  
> +	rtnl_lock();
> +
> +	/*
> +	 * Make sure the module has been initialized, and is not about to be
> +	 * removed.  Module paramter sysfs files are writable before the
> +	 * module_init function is called and after module_exit.
> +	 */
> +	if (!scsi_transport_fcoe_sw) {
> +		rc = -ENODEV;
> +		goto out_nodev;
> +	}
> +
>  	netdev = fcoe_if_to_netdev(buffer);
>  	if (!netdev) {
>  		rc = -ENODEV;
> @@ -1746,14 +1768,15 @@ static int fcoe_create(const char *buffer, struct kernel_param *kp)
>  	if (!fcoe_link_ok(lport))
>  		fcoe_ctlr_link_up(&fcoe->ctlr);
>  
> -	dev_put(netdev);
> -	return 0;
> +	rc = 0;
> +	goto out_putdev;
>  
>  out_free:
>  	fcoe_interface_put(fcoe);
>  out_putdev:
>  	dev_put(netdev);
>  out_nodev:
> +	rtnl_unlock();
>  	return rc;
>  }
>  
> @@ -1872,8 +1895,6 @@ int fcoe_reset(struct Scsi_Host *shost)
>   * fcoe_hostlist_lookup_port() - find the corresponding lport by a given device
>   * @dev: this is currently ptr to net_device
>   *
> - * Called with fcoe_hostlist_lock held.
> - *
>   * Returns: NULL or the located fcoe_port
>   */
>  static struct fcoe_interface *
> @@ -1898,10 +1919,7 @@ struct fc_lport *fcoe_hostlist_lookup(const struct net_device *netdev)
>  {
>  	struct fcoe_interface *fcoe;
>  
> -	read_lock(&fcoe_hostlist_lock);
>  	fcoe = fcoe_hostlist_lookup_port(netdev);
> -	read_unlock(&fcoe_hostlist_lock);
> -
>  	return (fcoe) ? fcoe->ctlr.lp : NULL;
>  }
>  
> @@ -1909,8 +1927,6 @@ struct fc_lport *fcoe_hostlist_lookup(const struct net_device *netdev)
>   * fcoe_hostlist_add() - Add a lport to lports list
>   * @lp: ptr to the fc_lport to be added
>   *
> - * Called with write fcoe_hostlist_lock held.
> - *
>   * Returns: 0 for success
>   */
>  int fcoe_hostlist_add(const struct fc_lport *lport)
> @@ -1937,11 +1953,9 @@ int fcoe_hostlist_remove(const struct fc_lport *lport)
>  {
>  	struct fcoe_interface *fcoe;
>  
> -	write_lock_bh(&fcoe_hostlist_lock);
>  	fcoe = fcoe_hostlist_lookup_port(fcoe_netdev(lport));
>  	BUG_ON(!fcoe);
>  	list_del(&fcoe->list);
> -	write_unlock_bh(&fcoe_hostlist_lock);
>  
>  	return 0;
>  }
> @@ -1957,9 +1971,6 @@ static int __init fcoe_init(void)
>  	int rc = 0;
>  	struct fcoe_percpu_s *p;
>  
> -	INIT_LIST_HEAD(&fcoe_hostlist);
> -	rwlock_init(&fcoe_hostlist_lock);
> -
>  	for_each_possible_cpu(cpu) {
>  		p = &per_cpu(fcoe_percpu, cpu);
>  		skb_queue_head_init(&p->fcoe_rx_list);
> @@ -2000,6 +2011,7 @@ static void __exit fcoe_exit(void)
>  	struct fcoe_interface *fcoe, *tmp;
>  
>  	fcoe_dev_cleanup();
> +	rtnl_lock();
>  
>  	/* releases the associated fcoe hosts */
>  	list_for_each_entry_safe(fcoe, tmp, &fcoe_hostlist, list)
> @@ -2012,5 +2024,7 @@ static void __exit fcoe_exit(void)
>  
>  	/* detach from scsi transport */
>  	fcoe_if_exit();
> +
> +	rtnl_unlock();
>  }
>  module_exit(fcoe_exit);
> 
> _______________________________________________
> devel mailing list
> devel at open-fcoe.org
> http://www.open-fcoe.org/mailman/listinfo/devel

This looks good to me.  Thanks for doing it.

	Joe




More information about the devel mailing list