Re: [PATCH v1 01/24] IB/core: Introduce new fast registration API
On 9/28/2015 11:57 PM, Bart Van Assche wrote:
On 09/24/2015 12:37 AM, Sagi Grimberg wrote:
On 9/23/2015 12:21 AM, Bart Van Assche wrote:
On 09/17/2015 02:42 AM, Sagi Grimberg wrote:
+} else if (last_page_off + dma_len < mr->page_size) {
+/* chunk this fragment with the last */
+last_end_dma_addr += dma_len;
+last_page_off += dma_len;
+mr->length += dma_len;
+continue;
Shouldn't this code update last_page_addr ?
Actually I think it doesn't since it is only relevant for the else
statement where we are passing the page_size boundary.
Hello Sagi,
Suppose that the following sg-list is passed to this function as {
offset, length } pairs and that this list has not been coalesced by the
DMA mapping code: [ { 0, page_size / 4 }, { page_size / 4, page_size / 4
}, { 2 * page_size / 4, page_size / 2 } ]. I think the algorithm in
patch 01/24 will map the above sample sg-list onto two pages. Shouldn't
that sg-list be mapped onto one page instead ?
It seems to... In order to get that correct we'd need to change the
condition to (last_page_off + dma_len <= mr->page_size)
I'll change for v2.
Thanks.
Sagi.
--
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 v1 01/24] IB/core: Introduce new fast registration API
On 9/29/2015 9:47 AM, Sagi Grimberg wrote: Shouldn't higher layers take care of this? Trying to implement the same coalescing algorithm at various layers isn't very optimal, although we need to decide and document which one is responsible. The block layer can take care of it, but I'm not sure about NFS/RDS at the moment (IIRC Steve specifically asked if this API would take care of chunking contig sg elements) so I'd rather keep it in until we are absolutely sure we don't need it. I can add a documentation statement for it. Actually its documented: * Constraints: * - The first sg element is allowed to have an offset. * - Each sg element must be aligned to page_size (or physically * contiguous to the previous element). In case an sg element has a * non contiguous offset, the mapping prefix will not include it. * - The last sg element is allowed to have length less than page_size. -- 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 v1 01/24] IB/core: Introduce new fast registration API
Shouldn't higher layers take care of this? Trying to implement the same coalescing algorithm at various layers isn't very optimal, although we need to decide and document which one is responsible. The block layer can take care of it, but I'm not sure about NFS/RDS at the moment (IIRC Steve specifically asked if this API would take care of chunking contig sg elements) so I'd rather keep it in until we are absolutely sure we don't need it. I can add a documentation statement for 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
Re: [PATCH v1 01/24] IB/core: Introduce new fast registration API
On Mon, Sep 28, 2015 at 01:57:52PM -0700, Bart Van Assche wrote:
> >Actually I think it doesn't since it is only relevant for the else
> >statement where we are passing the page_size boundary.
>
> Hello Sagi,
>
> Suppose that the following sg-list is passed to this function as { offset,
> length } pairs and that this list has not been coalesced by the DMA mapping
> code: [ { 0, page_size / 4 }, { page_size / 4, page_size / 4 }, { 2 *
> page_size / 4, page_size / 2 } ]. I think the algorithm in patch 01/24 will
> map the above sample sg-list onto two pages. Shouldn't that sg-list be
> mapped onto one page instead ?
Shouldn't higher layers take care of this? Trying to implement the same
coalescing algorithm at various layers isn't very optimal, although we
need to decide and document which one is responsible.
--
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 v1 01/24] IB/core: Introduce new fast registration API
On 09/24/2015 12:37 AM, Sagi Grimberg wrote:
On 9/23/2015 12:21 AM, Bart Van Assche wrote:
On 09/17/2015 02:42 AM, Sagi Grimberg wrote:
+} else if (last_page_off + dma_len < mr->page_size) {
+/* chunk this fragment with the last */
+last_end_dma_addr += dma_len;
+last_page_off += dma_len;
+mr->length += dma_len;
+continue;
Shouldn't this code update last_page_addr ?
Actually I think it doesn't since it is only relevant for the else
statement where we are passing the page_size boundary.
Hello Sagi,
Suppose that the following sg-list is passed to this function as {
offset, length } pairs and that this list has not been coalesced by the
DMA mapping code: [ { 0, page_size / 4 }, { page_size / 4, page_size / 4
}, { 2 * page_size / 4, page_size / 2 } ]. I think the algorithm in
patch 01/24 will map the above sample sg-list onto two pages. Shouldn't
that sg-list be mapped onto one page instead ?
Thanks,
Bart.
--
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 v1 01/24] IB/core: Introduce new fast registration API
On 9/23/2015 12:21 AM, Bart Van Assche wrote:
On 09/17/2015 02:42 AM, Sagi Grimberg wrote:
The new fast registration verg ib_map_mr_sg receives a scatterlist
^ verb ?
Will fix. Thanks.
+/**
+ * ib_map_mr_sg() - Map a memory region with the largest prefix of
+ * a dma mapped SG list
This description could be made more clear. How about the following:
Register the largest possible prefix of a DMA-mapped SG-list
+} else if (last_page_off + dma_len < mr->page_size) {
+/* chunk this fragment with the last */
+last_end_dma_addr += dma_len;
+last_page_off += dma_len;
+mr->length += dma_len;
+continue;
Shouldn't this code update last_page_addr ?
Actually I think it doesn't since it is only relevant for the else
statement where we are passing the page_size boundary.
--
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 v1 01/24] IB/core: Introduce new fast registration API
On 09/17/2015 02:42 AM, Sagi Grimberg wrote:
The new fast registration verg ib_map_mr_sg receives a scatterlist
^ verb ?
+/**
+ * ib_map_mr_sg() - Map a memory region with the largest prefix of
+ * a dma mapped SG list
This description could be made more clear. How about the following:
Register the largest possible prefix of a DMA-mapped SG-list
+ } else if (last_page_off + dma_len < mr->page_size) {
+ /* chunk this fragment with the last */
+ last_end_dma_addr += dma_len;
+ last_page_off += dma_len;
+ mr->length += dma_len;
+ continue;
Shouldn't this code update last_page_addr ?
--
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
