[openib-general] Re: [PATCH] Move SDP to dynamic device enumeration

Libor Michalek libor at topspin.com
Fri Sep 24 08:45:40 PDT 2004


Roland Dreier wrote:
>
> This moves the SDP in my tree to using the struct ib_client method for
> device enumeration.  There are still problems with adding and removing
> devices because the ip2pr module is still using static methods, but I
> think this fixes up SDP.
> 
> Libor, seem OK to commit?

Roland,

  Sorry that I missed this earlier. This looks good. Although, the change
in FMR initialization, once implemented, will result in more virtual 
address space utilization on systems with multiple HCAs...

-Libor


> Index: infiniband/ulp/sdp/sdp_conn.c
> ===================================================================
> --- infiniband/ulp/sdp/sdp_conn.c	(revision 836)
> +++ infiniband/ulp/sdp/sdp_conn.c	(working copy)
> @@ -28,6 +28,16 @@
>  static char _recv_pool_name[] = TS_SDP_SOCK_RECV_DATA_NAME;
>  
>  static struct sdev_root _dev_root_s;
> +
> +static void sdp_device_init_one(struct ib_device *device);
> +static void sdp_device_remove_one(struct ib_device *device);
> +
> +static struct ib_client sdp_client = {
> +	.name   = "sdp",
> +	.add    = sdp_device_init_one,
> +	.remove = sdp_device_remove_one
> +};
> +
>  /* --------------------------------------------------------------------- */
>  /*                                                                       */
>  /* module specific functions                                             */
> @@ -1016,27 +1026,17 @@
>  	/*
>  	 * look up correct HCA and port
>  	 */
> -	for (hca = _dev_root_s.hca_list; NULL != hca; hca = hca->next) {
> +	hca = ib_get_client_data(device, &sdp_client);
> +	if (!hca)
> +		return -ERANGE;
>  
> -		if (device == hca->ca) {
> -
> -			for (port = hca->port_list; NULL != port;
> -			     port = port->next) {
> -
> -				if (hw_port == port->index) {
> -
> -					break;
> -				}
> -			}
> -
> +	for (port = hca->port_list; NULL != port; port = port->next)
> +		if (hw_port == port->index)
>  			break;
> -		}
> -	}
>  
> -	if (NULL == hca || NULL == port) {
> -
> +	if (!port)
>  		return -ERANGE;
> -	}
> +
>  	/*
>  	 * allocate creation parameters
>  	 */
> @@ -1815,12 +1815,6 @@
>  			 off_t start_index,
>  			 long *end_index)
>  {
> -	struct sdev_hca_port *port;
> -	struct sdev_hca *hca;
> -	u64 subnet_prefix;
> -	u64 guid;
> -	int hca_count;
> -	int port_count;
>  	int offset = 0;
>  
>  	TS_CHECK_NULL(buffer, -EINVAL);
> @@ -1857,40 +1851,8 @@
>  		offset += sprintf((buffer + offset),
>  				  "max receive buffered:     <%d>\n",
>  				  _dev_root_s.recv_buff_max);
> -
> -		offset += sprintf((buffer + offset), "HCAs:\n");
>  	}
> -	/*
> -	 * HCA loop
> -	 */
> -	for (hca = _dev_root_s.hca_list, hca_count = 0;
> -	     NULL != hca; hca = hca->next, hca_count++) {
>  
> -		offset += sprintf((buffer + offset),
> -				  "  hca %02x: ca <%p> pd <%p> mem <%p> "
> -				  "l_key <%08x>\n",
> -				  hca_count, hca->ca, hca->pd, 
> -				  hca->mem_h, hca->l_key);
> -
> -		for (port = hca->port_list, port_count = 0;
> -		     NULL != port; port = port->next, port_count++) {
> -
> -			subnet_prefix = cpu_to_be64(*(u64 *) (port->gid));
> -			guid = cpu_to_be64(*(u64 *)(port->gid + sizeof(u64)));
> -
> -			offset += sprintf((buffer + offset),
> -					  "    port %02x: index <%d> gid "
> -					  "<%08x%08x:%08x%08x>\n",
> -					  port_count,
> -					  port->index,
> -					  (u32)((subnet_prefix >> 32) &
> -						0xffffffff),
> -					  (u32)(subnet_prefix & 0xffffffff),
> -					  (u32)((guid >> 32) & 0xffffffff),
> -					  (u32)(guid & 0xffffffff));
> -		}
> -	}
> -
>  	return offset;
>  } /* sdp_proc_dump_device */
>  
> @@ -1899,232 +1861,200 @@
>  /* initialization/cleanup functions                                      */
>  /*                                                                       */
>  /* --------------------------------------------------------------------- */
> +
>  /* ========================================================================= */
>  /*.._sdp_device_table_init -- create hca list */
> -static s32 _sdp_device_table_init(struct sdev_root *dev_root)
> +static void sdp_device_init_one(struct ib_device *device)
>  {
>  #ifdef _TS_SDP_AIO_SUPPORT
>  	struct ib_fmr_pool_param fmr_param_s;
>  #endif
>  	struct ib_phys_buf buffer_list;
>  	struct ib_device_attr node_info;
> -	struct ib_device *hca_handle;
>  	struct sdev_hca_port *port;
>  	struct sdev_hca *hca;
> -	s32 result;
> -	s32 hca_count;
> -	s32 port_count;
> -	s32 fmr_size;
> +	int result;
> +	int port_count;
>  
> -	TS_CHECK_NULL(dev_root, -EINVAL);
> +	result = ib_query_device(device, &node_info);
> +	if (0 != result) {
>  
> -	TS_TRACE(MOD_LNX_SDP, T_VERY_VERBOSE, TRACE_FLOW_INOUT,
> -		 "INIT: Probing HCA/Port list.");
> +		TS_TRACE(MOD_LNX_SDP, T_VERBOSE, TRACE_FLOW_FATAL,
> +			 "INIT: Error <%d> fetching HCA <%s> type.",
> +			 result, device->name);
> +		return;
> +	}
>  	/*
> -	 * first count number of HCA's
> +	 * allocate per-HCA structure
>  	 */
> -	hca_count = 0;
> +	hca = kmalloc(sizeof(struct sdev_hca), GFP_KERNEL);
> +	if (NULL == hca) {
>  
> -	while (ib_device_get_by_index(hca_count)) {
> -
> -		hca_count++;
> +		TS_TRACE(MOD_LNX_SDP, T_VERBOSE, TRACE_FLOW_FATAL,
> +			 "INIT: Error allocating HCA <%s> memory.",
> +			 device->name);
> +		return;
>  	}
> +	/*
> +	 * init and insert into list.
> +	 */
> +	memset(hca, 0, sizeof(struct sdev_hca));
>  
> -	fmr_size = TS_SDP_FMR_POOL_SIZE / hca_count;
> +	hca->fmr_pool = NULL;
> +	hca->mem_h    = NULL;
> +	hca->pd       = NULL;
> +	hca->ca       = device;
> +	/*
> +	 * protection domain
> +	 */
> +	hca->pd = ib_alloc_pd(hca->ca);
> +	if (IS_ERR(hca->pd)) {
>  
> -	for (hca_count = 0;
> -	     (hca_handle = ib_device_get_by_index(hca_count)) != NULL;
> -	     hca_count++) {
> -		if (!hca_handle || !try_module_get(hca_handle->owner))
> -			continue;
> +		TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> +			 "INIT: Error <%d> creating HCA <%s> protection domain.",
> +			 PTR_ERR(hca->pd), device->name);
> +		goto error;
> +	}
> +	/*
> +	 * memory registration
> +	 */
> +	buffer_list.addr = 0;
> +	buffer_list.size = (unsigned long)high_memory - PAGE_OFFSET;
>  
> -		result = ib_query_device(hca_handle, &node_info);
> -		if (0 != result) {
> +	hca->iova = 0;
>  
> -			TS_TRACE(MOD_LNX_SDP, T_VERBOSE, TRACE_FLOW_FATAL,
> -				 "INIT: Error <%d> fetching HCA <%x:%d> type.",
> -				 result, hca_handle, hca_count);
> -			goto error;
> -		}
> -		/*
> -		 * allocate per-HCA structure
> -		 */
> -		hca = kmalloc(sizeof(struct sdev_hca), GFP_KERNEL);
> -		if (NULL == hca) {
> +	hca->mem_h = ib_reg_phys_mr(hca->pd, 
> +				    &buffer_list, 
> +				    1,	/* list_len */
> +				    IB_ACCESS_LOCAL_WRITE,
> +				    &hca->iova);
> +	if (IS_ERR(hca->mem_h)) {
> +		result = PTR_ERR(hca->mem_h);
> +		TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> +			 "INIT: Error <%d> registering HCA <%s> memory.",
> +			 result, device->name);
> +		goto error;
> +	}
>  
> -			TS_TRACE(MOD_LNX_SDP, T_VERBOSE, TRACE_FLOW_FATAL,
> -				 "INIT: Error allocating HCA <%x:%d> memory.",
> -				 hca_handle, hca_count);
> -			result = -ENOMEM;
> -			goto error;
> -		}
> -		/*
> -		 * init and insert into list.
> -		 */
> -		memset(hca, 0, sizeof(struct sdev_hca));
> +	hca->l_key = hca->mem_h->lkey;
> +	hca->r_key = hca->mem_h->rkey;
>  
> -		hca->next = dev_root->hca_list;
> -		dev_root->hca_list = hca;
> +#ifdef _TS_SDP_AIO_SUPPORT
> +	/*
> +	 * FMR allocation
> +	 */
> +	fmr_param_s.pool_size = TS_SDP_FMR_POOL_SIZE;
> +	fmr_param_s.dirty_watermark = TS_SDP_FMR_DIRTY_SIZE;
> +	fmr_param_s.cache = 1;
> +	fmr_param_s.max_pages_per_fmr = TS_SDP_IOCB_PAGES_MAX;
> +	fmr_param_s.access = (IB_ACCESS_LOCAL_WRITE  |
> +			      IB_ACCESS_REMOTE_WRITE |
> +			      IB_ACCESS_REMOTE_READ);
>  
> -		hca->fmr_pool = NULL;
> -		hca->mem_h    = NULL;
> -		hca->pd       = NULL;
> -		hca->ca = hca_handle;
> -		/*
> -		 * protection domain
> -		 */
> -		hca->pd = ib_alloc_pd(hca->ca);
> -		if (IS_ERR(hca->pd)) {
> +	fmr_param_s.flush_function = NULL;
> +	/*
> +	 * create SDP memory pool
> +	 */
> +	result = ib_create_fmr_pool(hca->pd,
> +				    &fmr_param_s, 
> +				    &hca->fmr_pool);
> +	if (0 > result) {
>  
> -			TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> -				 "INIT: Error <%d> creating HCA <%x:%d> protection domain.",
> -				 PTR_ERR(hca->pd), hca_handle, hca_count);
> -			goto error;
> -		}
> -		/*
> -		 * memory registration
> -		 */
> -		buffer_list.addr = 0;
> -		buffer_list.size = (unsigned long)high_memory - PAGE_OFFSET;
> +		TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> +			 "INIT: Error <%d> creating HCA <%s> fast memory pool.",
> +			 result, hca->ca);
> +		goto error;
> +	}
> +#endif /* _TS_SDP_AIO_SUPPORT */
> +	/*
> +	 * port allocation
> +	 */
> +	for (port_count = 0; port_count < node_info.phys_port_cnt; port_count++) {
>  
> -		hca->iova = 0;
> +		port = kmalloc(sizeof(struct sdev_hca_port), 
> +			       GFP_KERNEL);
> +		if (NULL == port) {
>  
> -		hca->mem_h = ib_reg_phys_mr(hca->pd, 
> -					    &buffer_list, 
> -					    1,	/* list_len */
> -					    IB_ACCESS_LOCAL_WRITE,
> -					    &hca->iova);
> -		if (IS_ERR(hca->mem_h)) {
> -			result = PTR_ERR(hca->mem_h);
> -			TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> -				 "INIT: Error <%d> registering HCA <%x:%d> memory.",
> -				 result, hca_handle, hca_count);
> +			TS_TRACE(MOD_LNX_SDP, T_VERBOSE,
> +				 TRACE_FLOW_FATAL,
> +				 "INIT: Error allocating HCA <%s> port <%d:%d> memory.",
> +				 device->name, port_count,
> +				 node_info.phys_port_cnt);
> +
>  			goto error;
>  		}
>  
> -		hca->l_key = hca->mem_h->lkey;
> -		hca->r_key = hca->mem_h->rkey;
> +		memset(port, 0, sizeof(struct sdev_hca_port));
>  
> -#ifdef _TS_SDP_AIO_SUPPORT
> -		/*
> -		 * FMR allocation
> -		 */
> -		fmr_param_s.pool_size = fmr_size;
> -		fmr_param_s.dirty_watermark = TS_SDP_FMR_DIRTY_SIZE;
> -		fmr_param_s.cache = 1;
> -		fmr_param_s.max_pages_per_fmr = TS_SDP_IOCB_PAGES_MAX;
> -		fmr_param_s.access = (IB_ACCESS_LOCAL_WRITE  |
> -				      IB_ACCESS_REMOTE_WRITE |
> -				      IB_ACCESS_REMOTE_READ);
> +		port->index    = port_count + 1;
> +		port->next     = hca->port_list;
> +		hca->port_list = port;
>  
> -		fmr_param_s.flush_function = NULL;
> -		/*
> -		 * create SDP memory pool
> -		 */
> -		result = ib_create_fmr_pool(hca->pd,
> -					    &fmr_param_s, 
> -					    &hca->fmr_pool);
> -		if (0 > result) {
> +		result = ib_query_gid(hca->ca, 
> +				      port->index, 
> +				      0,	/* index */
> +				      (union ib_gid *) port->gid);
> +		if (0 != result) {
>  
> -			TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> -				 "INIT: Error <%d> creating HCA <%d:%d> fast memory pool.",
> -				 result, hca->ca, hca_count);
> +			TS_TRACE(MOD_LNX_SDP, T_VERBOSE,
> +				 TRACE_FLOW_FATAL,
> +				 "INIT: Error <%d> getting GID for HCA <%s> port <%d:%d>",
> +				 result, hca->ca,
> +				 port->index, node_info.phys_port_cnt);
>  			goto error;
>  		}
> -#endif /* _TS_SDP_AIO_SUPPORT */
> -		/*
> -		 * port allocation
> -		 */
> -		for (port_count = 0; port_count < node_info.phys_port_cnt;
> -		     port_count++) {
> +	}
>  
> -			port = kmalloc(sizeof(struct sdev_hca_port), 
> -				       GFP_KERNEL);
> -			if (NULL == port) {
> +	ib_set_client_data(device, &sdp_client, hca);
>  
> -				TS_TRACE(MOD_LNX_SDP, T_VERBOSE,
> -					 TRACE_FLOW_FATAL,
> -					 "INIT: Error allocating HCA <%d:%d> port <%x:%d> memory.",
> -					 hca_handle, hca_count, port_count,
> -					 node_info.phys_port_cnt);
> +	return;
>  
> -				result = -ENOMEM;
> -				goto error;
> -			}
> +error:
> +	for (port = hca->port_list; NULL != port; port = hca->port_list) {
> +		hca->port_list = port->next;
> +		port->next = NULL;
>  
> -			memset(port, 0, sizeof(struct sdev_hca_port));
> +		kfree(port);
> +	}
>  
> -			port->index    = port_count + 1;
> -			port->next     = hca->port_list;
> -			hca->port_list = port;
> +	if (hca->fmr_pool)
> +		(void)ib_destroy_fmr_pool(hca->fmr_pool);
>  
> -			result = ib_query_gid(hca->ca, 
> -					      port->index, 
> -					      0,	/* index */
> -					      (union ib_gid *) port->gid);
> -			if (0 != result) {
> +	if (hca->mem_h)
> +		(void)ib_dereg_mr(hca->mem_h);
>  
> -				TS_TRACE(MOD_LNX_SDP, T_VERBOSE,
> -					 TRACE_FLOW_FATAL,
> -					 "INIT: Error <%d> getting GID for HCA <%d:%d> port <%d:%d>",
> -					 result, hca->ca, hca_count,
> -					 port->index, node_info.phys_port_cnt);
> -				goto error;
> -			}
> -		}
> -	}
> +	if (hca->pd)
> +		(void)ib_dealloc_pd(hca->pd);
>  
> -	return 0;
> -error:
> -	return result;
> +	kfree(hca);
>  } /* _sdp_device_table_init */
>  
>  /* ========================================================================= */
>  /*.._sdp_device_table_cleanup -- delete hca list */
> -static s32 _sdp_device_table_cleanup(struct sdev_root *dev_root)
> +static void sdp_device_remove_one(struct ib_device *device)
>  {
>  	struct sdev_hca_port *port;
>  	struct sdev_hca *hca;
>  
> -	TS_CHECK_NULL(dev_root, -EINVAL);
> -	/*
> -	 * free all hca/ports
> -	 */
> -	for (hca = dev_root->hca_list; NULL != hca; hca = dev_root->hca_list) {
> +	hca = ib_get_client_data(device, &sdp_client);
>  
> -		dev_root->hca_list = hca->next;
> -		hca->next = NULL;
> +	for (port = hca->port_list; NULL != port; port = hca->port_list) {
> +		hca->port_list = port->next;
> +		port->next = NULL;
>  
> -		for (port = hca->port_list; NULL != port; port = hca->port_list) {
> +		kfree(port);
> +	}
>  
> -			hca->port_list = port->next;
> -			port->next = NULL;
> +	if (hca->fmr_pool)
> +		(void)ib_destroy_fmr_pool(hca->fmr_pool);
>  
> -			kfree(port);
> -		}
> +	if (hca->mem_h)
> +		(void)ib_dereg_mr(hca->mem_h);
>  
> -		if (NULL != hca->fmr_pool) {
> +	if (hca->pd)
> +		(void)ib_dealloc_pd(hca->pd);
>  
> -			(void)ib_destroy_fmr_pool(hca->fmr_pool);
> -		}
> -
> -		if (hca->mem_h) {
> -
> -			(void)ib_dereg_mr(hca->mem_h);
> -		}
> -
> -		if (hca->pd) {
> -
> -			(void)ib_dealloc_pd(hca->pd);
> -		}
> -
> -		if (hca->ca)
> -			module_put(hca->ca->owner);
> -
> -		kfree(hca);
> -	}
> -
> -	return 0;
> +	kfree(hca);
>  } /* _sdp_device_table_cleanup */
>  
>  /* ========================================================================= */
> @@ -2170,11 +2100,11 @@
>  	/*
>  	 * Get HCA/port list
>  	 */
> -	result = _sdp_device_table_init(&_dev_root_s);
> +	result = ib_register_client(&sdp_client);
>  	if (0 > result) {
>  
>  		TS_TRACE(MOD_LNX_SDP, T_TERSE, TRACE_FLOW_FATAL,
> -			 "INIT: Error <%d> building HCA/port list.", result);
> +			 "INIT: Error <%d> registering SDP client.", result);
>  		goto error_hca;
>  	}
>  	/*
> @@ -2281,8 +2211,8 @@
>  	free_pages((unsigned long)_dev_root_s.sk_array, _dev_root_s.sk_ordr);
>  error_array:
>  error_size:
> +	ib_unregister_client(&sdp_client);
>  error_hca:
> -	(void)_sdp_device_table_cleanup(&_dev_root_s);
>  	return result;
>  } /* sdp_conn_table_init */
>  
> @@ -2302,7 +2232,7 @@
>  	/*
>  	 * delete list of HCAs/PORTs
>  	 */
> -	(void)_sdp_device_table_cleanup(&_dev_root_s);
> +	ib_unregister_client(&sdp_client);
>  	/*
>  	 * drop socket table
>  	 */
> Index: infiniband/ulp/sdp/sdp_dev.h
> ===================================================================
> --- infiniband/ulp/sdp/sdp_dev.h	(revision 803)
> +++ infiniband/ulp/sdp/sdp_dev.h	(working copy)
> @@ -167,12 +167,8 @@
>  	int send_buff_max;
>  	int send_usig_max;
>  	/*
> -	 * devices. list of installed HCA's and some associated parameters
> -	 */
> -	struct sdev_hca *hca_list;
> -	/*
>  	 * connections. The table is a simple linked list, since it does not
> -	 * need to require fast lookup capabilities.
> +	 * need fast lookup capabilities.
>  	 */
>  	u32 sk_size;  /* socket array size */
>  	u32 sk_ordr;  /* order size of region. */



More information about the general mailing list