Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On 7/23/2015 10:08 PM, Jason Gunthorpe wrote: On Thu, Jul 23, 2015 at 01:07:56PM +0300, Sagi Grimberg wrote: On 7/22/2015 10:05 PM, Jason Gunthorpe wrote: The reason I named max_entries is because might might not be pages but real SG elements. It stands for maximum registration entries. Do you have a better name? I wouldn't try and be both.. Use 'max_num_sg' and document that no aggregate scatterlist with length larger than 'max_num_sg*PAGE_SIZE' or with more entries than max_num_sg can be submitted? Maybe document with ARB_SG that it is not length limited? OK, I can do that. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On Thu, Jul 23, 2015 at 01:07:56PM +0300, Sagi Grimberg wrote: On 7/22/2015 10:05 PM, Jason Gunthorpe wrote: The reason I named max_entries is because might might not be pages but real SG elements. It stands for maximum registration entries. Do you have a better name? I wouldn't try and be both.. Use 'max_num_sg' and document that no aggregate scatterlist with length larger than 'max_num_sg*PAGE_SIZE' or with more entries than max_num_sg can be submitted? Maybe document with ARB_SG that it is not length limited? Jason -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On 7/22/2015 10:05 PM, Jason Gunthorpe wrote:
On Wed, Jul 22, 2015 at 07:58:23PM +0300, Sagi Grimberg wrote:
On 7/22/2015 7:44 PM, Christoph Hellwig wrote:
On Wed, Jul 22, 2015 at 10:34:05AM -0600, Jason Gunthorpe wrote:
+/**
+ * ib_alloc_mr() - Allocates a memory region
+ * @pd:protection domain associated with the region
+ * @mr_type: memory region type
+ * @max_entries: maximum registration entries available
+ * @flags: create flags
+ */
Can you update this comment to elaborate some more on what the
parameters are? 'max_entries' is the number of s/g elements or
something?
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
};
Sure would be nice to have some documentation for what these things
do..
Agreed on both counts. Otherwise this looks pretty good to me.
I can add some more documentation here...
So, I was wrong, 'max_entries' is the number of page entires, not
really the s/g element limit?
The max_entries stands for the maximum number of sg entries. Other than
that, the SG list must meet the requirements documented in ib_map_mr_sg.
The reason I named max_entries is because might might not be pages but
real SG elements. It stands for maximum registration entries.
Do you have a better name?
In other words the ULP can submit at most max_entires*PAGE_SIZE bytes
for the non ARB_SG case
For the ARB_SG case.. It is some other more difficult computation?
Not really. The ULP needs to submit sg_nents max_entries. The SG
list needs to meed the alignment requirements.
For ARB_SG, the condition is the same, but the SG is free from the
alignment constraints.
It is somewhat ugly to ask for this upfront as a hard limit..
Is there any reason we can't use a hint_prealloc_pages as the argument
here, and then realloc in the map routine if the hint turns out to be
too small for a particular s/g list?
The reason is that it is not possible. The memory key allocation
reserves resources in the device translation tables. realloc means
reallocating the memory key. In any event, this is not possible in
the IO path.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On 7/23/2015 12:30 PM, Christoph Hellwig wrote:
On Thu, Jul 23, 2015 at 12:57:34AM +, Hefty, Sean wrote:
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
If we're going to go through the trouble of changing everything, I vote
for dropping the word 'fast'. It's a marketing term. It's goofy. And
the IB spec is goofy for using it.
So IB_MR_TYPE_MEM_REG?
Yes. Especially as the infrastructure will be usable to support FMR
on legacy adapters as well except that instead of the ib_post_send it'll
need a call to the FMR code at the very end.
While we're at it wonder if we should consolidate the type and the
flags field as well, as the split between the two is a little confusing.
I can do that.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On Thu, Jul 23, 2015 at 12:57:34AM +, Hefty, Sean wrote:
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
If we're going to go through the trouble of changing everything, I vote
for dropping the word 'fast'. It's a marketing term. It's goofy. And
the IB spec is goofy for using it.
Yes. Especially as the infrastructure will be usable to support FMR
on legacy adapters as well except that instead of the ib_post_send it'll
need a call to the FMR code at the very end.
While we're at it wonder if we should consolidate the type and the
flags field as well, as the split between the two is a little confusing.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
+/**
+ * ib_alloc_mr() - Allocates a memory region
+ * @pd:protection domain associated with the region
+ * @mr_type: memory region type
+ * @max_entries: maximum registration entries available
+ * @flags: create flags
+ */
Can you update this comment to elaborate some more on what the
parameters are? 'max_entries' is the number of s/g elements or
something?
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
};
Sure would be nice to have some documentation for what these things
do..
Jason
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On Wed, Jul 22, 2015 at 10:34:05AM -0600, Jason Gunthorpe wrote:
+/**
+ * ib_alloc_mr() - Allocates a memory region
+ * @pd:protection domain associated with the region
+ * @mr_type: memory region type
+ * @max_entries: maximum registration entries available
+ * @flags: create flags
+ */
Can you update this comment to elaborate some more on what the
parameters are? 'max_entries' is the number of s/g elements or
something?
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
};
Sure would be nice to have some documentation for what these things
do..
Agreed on both counts. Otherwise this looks pretty good to me.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On Wed, Jul 22, 2015 at 07:59:16PM +0300, Sagi Grimberg wrote: Do we want to pull ib_get_dma_mr() here with type IB_MR_TYPE_DMA? I want to get rid of ib_get_dma_mr... My plan was to get rid of it as my last series shows for all lkey usages and then rename it to: ib_get_insecure_all_physical_rkey For the remaining usages, and a future kernel version will taint the kernel if anyone calls it. Jason -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On 7/22/2015 8:01 PM, Jason Gunthorpe wrote: On Wed, Jul 22, 2015 at 07:59:16PM +0300, Sagi Grimberg wrote: Do we want to pull ib_get_dma_mr() here with type IB_MR_TYPE_DMA? I want to get rid of ib_get_dma_mr... That's why I asked :) So I'll take it as a no... -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On 7/22/2015 7:44 PM, Christoph Hellwig wrote:
On Wed, Jul 22, 2015 at 10:34:05AM -0600, Jason Gunthorpe wrote:
+/**
+ * ib_alloc_mr() - Allocates a memory region
+ * @pd:protection domain associated with the region
+ * @mr_type: memory region type
+ * @max_entries: maximum registration entries available
+ * @flags: create flags
+ */
Can you update this comment to elaborate some more on what the
parameters are? 'max_entries' is the number of s/g elements or
something?
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
};
Sure would be nice to have some documentation for what these things
do..
Agreed on both counts. Otherwise this looks pretty good to me.
I can add some more documentation here...
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH WIP 01/43] IB: Modify ib_create_mr API
On Wed, Jul 22, 2015 at 07:58:23PM +0300, Sagi Grimberg wrote:
On 7/22/2015 7:44 PM, Christoph Hellwig wrote:
On Wed, Jul 22, 2015 at 10:34:05AM -0600, Jason Gunthorpe wrote:
+/**
+ * ib_alloc_mr() - Allocates a memory region
+ * @pd:protection domain associated with the region
+ * @mr_type: memory region type
+ * @max_entries: maximum registration entries available
+ * @flags: create flags
+ */
Can you update this comment to elaborate some more on what the
parameters are? 'max_entries' is the number of s/g elements or
something?
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
};
Sure would be nice to have some documentation for what these things
do..
Agreed on both counts. Otherwise this looks pretty good to me.
I can add some more documentation here...
So, I was wrong, 'max_entries' is the number of page entires, not
really the s/g element limit?
In other words the ULP can submit at most max_entires*PAGE_SIZE bytes
for the non ARB_SG case
For the ARB_SG case.. It is some other more difficult computation?
It is somewhat ugly to ask for this upfront as a hard limit..
Is there any reason we can't use a hint_prealloc_pages as the argument
here, and then realloc in the map routine if the hint turns out to be
too small for a particular s/g list?
It looks like all drivers can support this.
That would make it much easier to use correctly, and free ULPs from
dealing with any impedance mismatch with core kernel code that assumes
a sg list length limit, or overall side limit, not some oddball
computation based on pages...
Jason
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH WIP 01/43] IB: Modify ib_create_mr API
+enum ib_mr_type {
+ IB_MR_TYPE_FAST_REG,
+ IB_MR_TYPE_SIGNATURE,
If we're going to go through the trouble of changing everything, I vote for
dropping the word 'fast'. It's a marketing term. It's goofy. And the IB spec
is goofy for using it.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
