[openib-general] Re: [PATCH] [RFC] use device max_map_per_fmr in fmr pool

[Michael S. Tsirkin]

Quoting r. Or Gerlitz <ogerlitz at voltaire.com>:
> Subject: [PATCH] [RFC] use device max_map_per_fmr in fmr pool
> 
> Roland,
> 
> Remapping an FMR the maximum number of times the device allows should
> reduce the amortized cost of using fmrs to the minimum possible.
> 
> 
> I was thinking on a patch of this spirit (which is not complete i know 
> since down the code there's a usage of IB_FMR_MAX_REMAPS) but i figured
> out that mthca does not implement filling device_attr.max_map_per_fmr.
> >From what i understand, struct mthca_limits should be enhanced to support
> it, something which i failed to do... i think its related to 
> 2^ log2(some function of the MPT size) but i am not sure.
> 
> I understand that this relates also to the thread which does an 
> optimizations for sinai... anyway it would be good if for 2.6.17 
> the max remaps attribute would be supported so we can consider enhance 
> the fmr pool to use it.
> 
> Or.

Does this actually help performance?
How about increasing IB_FMR_MAX_REMAPS from 32 to say 128 and checking?

> Index: fmr_pool.c
> ===================================================================
> --- fmr_pool.c	(revision 5524)
> +++ fmr_pool.c	(working copy)
> @@ -214,6 +214,7 @@ struct ib_fmr_pool *ib_create_fmr_pool(s
>  {
>  	struct ib_device   *device;
>  	struct ib_fmr_pool *pool;
> +	struct ib_device_attr device_attr;
>  	int i;
>  	int ret;
>  
> @@ -228,6 +229,12 @@ struct ib_fmr_pool *ib_create_fmr_pool(s
>  		return ERR_PTR(-ENOSYS);
>  	}
>  
> +	ret = ib_query_device(device, &device_attr);
> +	if (ret) {
> +		printk(KERN_WARNING "couldn't query device");
> +		return ERR_PTR(ret);
> +	}
> +
>  	pool = kmalloc(sizeof *pool, GFP_KERNEL);
>  	if (!pool) {
>  		printk(KERN_WARNING "couldn't allocate pool struct");
> @@ -279,7 +286,7 @@ struct ib_fmr_pool *ib_create_fmr_pool(s
>  		struct ib_pool_fmr *fmr;
>  		struct ib_fmr_attr attr = {
>  			.max_pages  = params->max_pages_per_fmr,
> -			.max_maps   = IB_FMR_MAX_REMAPS,
> +			.max_maps   = device_attr.max_map_per_fmr,
>  			.page_shift = params->page_shift
>  		};

Allocating resources up to a maximum supported by a device is rarely optimal.
Its easy to imagine a device where FMR number of maps is a shared resource,
so allocating one FMR with max_map_per_fmr will not let you create any more
of these.

Something like
	.max_maps = min(IB_FMR_MAX_REMAPS, device_attr.max_map_per_fmr)
Would make more sense in my opinion.

We could also change ib_alloc_fmr implementation in mthca to report the actual
max_maps supported by this fmr (e.g. device could round this up to the power of
2 or something).

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies