[openib-general] Re: [PATCH] mthca: fix registration for giant MRs

[Michael S. Tsirkin]

Quoting r. Roland Dreier <roland at topspin.com>:
> Subject: [PATCH] mthca: fix registration for giant MRs
> 
> Here's a patch that allows mthca to break up registration of giant
> userspace MRs into multiple firmware commands.  The net effect of this
> is that there should no longer be any limit (beyond physical memory)
> on the size of MRs that userspace can register.
> 
> Arlin -- could you give this a shot and see if it works for you?
> 
> MST -- can you eyeball this patch and see if it looks OK?
> 
> If no one sees problems with this, I'll commit it soon.
> 
>  - R.

Looks good. A couple of comments:

------

1)

--- mthca/mthca_mr.c	(revision 2468)
+++ mthca/mthca_mr.c	(working copy)
+struct mthca_mtt {
+	int order;
+	u32 first_seg;
+	int fmr;
+};
+

And later in mthca_alloc_mtt:

+	mtt->first_seg = __mthca_alloc_mtt(dev, mtt->order,
+					   fmr ? dev->mr_table.fmr_mtt_buddy :
+					   &dev->mr_table.mtt_buddy);

I'd like to suggest we keep passing struct mthca_buddy *to mthca_alloc_mtt,
instead of passing around and keeping in memory the binary fmr flag, since
all that this flag does is select the right allocator, and callers of
mthca_alloc_mtt/mthca_free_mtt already know which it is.

	mthca_alloc_mtt(dev, order, dev->mr_tabel.fmr_mtt_buddy)
is in my opinion clearer than
	mthca_alloc_mtt(dev, order, 1)

When I implemented fmr support I took pains to ensure on memfree at least fmr
and mr mtts are exactly the same, and I have plans to make it so on all 64 bit
OSes, and also to use that to speed up allocations, so if possible lets not
break that with the fmr flag.

------

2)

+	for (i = MTHCA_MTT_SEG_SIZE / 8; i < size; i <<= 1)
+		++mtt->order;

At some point it'd be nice to support zero-sized regions, but meanwhile,
isnt this better written as

mtt->order = fls(max(size, MTHCA_MTT_SEG_SIZE / 8) - 1);

------

3)

>  static int mthca_dereg_mr(struct ib_mr *mr)
> --- mthca/mthca_provider.h	(revision 2470)
> +++ mthca/mthca_provider.h	(working copy)
> @@ -62,18 +62,18 @@ struct mthca_ucontext {
>  	struct mthca_user_db_table *db_tab;
>  };
>  
> +struct mthca_mtt;
> +
>  struct mthca_mr {
> -	struct ib_mr ibmr;
> -	int order;
> -	u32 first_seg;
> +	struct ib_mr      ibmr;
> +	struct mthca_mtt *mtt;
>  };


Why dont we keep mthca_mtt by instance in struct mthca_mr, like this:
  struct mthca_mr {
 	struct ib_mr      ibmr;
 	struct mthca_mtt mtt;
  };

Saves keeping around an extra small chunk of memory that we
need to kfree/kmalloc/check.
mthca_alloc_mtt could get struct mthca_mtt* and use that.

-- 
MST