[PATCH 0/2] ODP performance improvements

[PATCH 0/2] ODP performance improvements

[Guy Shapiro]

This patchset contains small changes that improve the performance of memory
map updates.

The patches reduce the execution time of ib_umem_odp_unmap_dma_pages when
invalidating 4MB of memory from 46us to 13us. This amounts to a reduction of
about 30% in the total execution time of the invalidation flow. As this flow is
called from the mmu_notifier hooks, this optimization can have a system-wide
performance impact.

Guy Shapiro (2):
  IB/core: dma map/unmap locking optimizations
  IB/core: dma unmap optimizations

 drivers/infiniband/core/umem_odp.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] IB/core: dma map/unmap locking optimizations

[Guy Shapiro]

Currently, while mapping or unmapping pages for ODP, the umem mutex is locked
and unlocked once for each page. Such lock/unlock operation take few tens to
hundreds of nsecs. This makes a significant impact when mapping or unmapping few
MBs of memory.

To avoid this, the mutex should be locked only once per operation, and not per
page.

Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Acked-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/umem_odp.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
index 8b8cc6f..aba4739 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -446,7 +446,6 @@ static int ib_umem_odp_map_dma_single_page(
 	int remove_existing_mapping = 0;
 	int ret = 0;
 
-	mutex_lock(&umem->odp_data->umem_mutex);
 	/*
 	 * Note: we avoid writing if seq is different from the initial seq, to
 	 * handle case of a racing notifier. This check also allows us to bail
@@ -479,8 +478,6 @@ static int ib_umem_odp_map_dma_single_page(
 	}
 
 out:
-	mutex_unlock(&umem->odp_data->umem_mutex);
-
 	/* On Demand Paging - avoid pinning the page */
 	if (umem->context->invalidate_range || !stored_page)
 		put_page(page);
@@ -586,6 +583,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
 
 		bcnt -= min_t(size_t, npages << PAGE_SHIFT, bcnt);
 		user_virt += npages << PAGE_SHIFT;
+		mutex_lock(&umem->odp_data->umem_mutex);
 		for (j = 0; j < npages; ++j) {
 			ret = ib_umem_odp_map_dma_single_page(
 				umem, k, base_virt_addr, local_page_list[j],
@@ -594,6 +592,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
 				break;
 			k++;
 		}
+		mutex_unlock(&umem->odp_data->umem_mutex);
 
 		if (ret < 0) {
 			/* Release left over pages when handling errors. */
@@ -633,9 +632,9 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 	 * faults from completion. We might be racing with other
 	 * invalidations, so we must make sure we free each page only
 	 * once. */
+	mutex_lock(&umem->odp_data->umem_mutex);
 	for (addr = virt; addr < bound; addr += (u64)umem->page_size) {
 		idx = (addr - ib_umem_start(umem)) / PAGE_SIZE;
-		mutex_lock(&umem->odp_data->umem_mutex);
 		if (umem->odp_data->page_list[idx]) {
 			struct page *page = umem->odp_data->page_list[idx];
 			struct page *head_page = compound_head(page);
@@ -663,7 +662,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 			umem->odp_data->page_list[idx] = NULL;
 			umem->odp_data->dma_list[idx] = 0;
 		}
-		mutex_unlock(&umem->odp_data->umem_mutex);
 	}
+	mutex_unlock(&umem->odp_data->umem_mutex);
 }
 EXPORT_SYMBOL(ib_umem_odp_unmap_dma_pages);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] IB/core: dma unmap optimizations

[Guy Shapiro]

While unmapping an ODP writable page, the dirty bit of the page is set. In
order to do so, the head of the compound page is found.
Currently, the compound head is found even on non-writable pages, where it is
never used, leading to unnecessary cpu barrier that impacts performance.

This patch moves the search for the compound head to be done only when needed.

Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Acked-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/umem_odp.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
index aba4739..40becdb 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -637,7 +637,6 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 		idx = (addr - ib_umem_start(umem)) / PAGE_SIZE;
 		if (umem->odp_data->page_list[idx]) {
 			struct page *page = umem->odp_data->page_list[idx];
-			struct page *head_page = compound_head(page);
 			dma_addr_t dma = umem->odp_data->dma_list[idx];
 			dma_addr_t dma_addr = dma & ODP_DMA_ADDR_MASK;
 
@@ -645,7 +644,8 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 
 			ib_dma_unmap_page(dev, dma_addr, PAGE_SIZE,
 					  DMA_BIDIRECTIONAL);
-			if (dma & ODP_WRITE_ALLOWED_BIT)
+			if (dma & ODP_WRITE_ALLOWED_BIT) {
+				struct page *head_page = compound_head(page);
 				/*
 				 * set_page_dirty prefers being called with
 				 * the page lock. However, MMU notifiers are
@@ -656,6 +656,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 				 * be removed.
 				 */
 				set_page_dirty(head_page);
+			}
 			/* on demand pinning support */
 			if (!umem->context->invalidate_range)
 				put_page(page);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/core: dma unmap optimizations

[Sagi Grimberg]

On 4/15/2015 6:17 PM, Guy Shapiro wrote:
> While unmapping an ODP writable page, the dirty bit of the page is set. In
> order to do so, the head of the compound page is found.
> Currently, the compound head is found even on non-writable pages, where it is
> never used, leading to unnecessary cpu barrier that impacts performance.
>
> This patch moves the search for the compound head to be done only when needed.
>
> Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Acked-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>   drivers/infiniband/core/umem_odp.c |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index aba4739..40becdb 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -637,7 +637,6 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
>   		idx = (addr - ib_umem_start(umem)) / PAGE_SIZE;
>   		if (umem->odp_data->page_list[idx]) {
>   			struct page *page = umem->odp_data->page_list[idx];
> -			struct page *head_page = compound_head(page);
>   			dma_addr_t dma = umem->odp_data->dma_list[idx];
>   			dma_addr_t dma_addr = dma & ODP_DMA_ADDR_MASK;
>
> @@ -645,7 +644,8 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
>
>   			ib_dma_unmap_page(dev, dma_addr, PAGE_SIZE,
>   					  DMA_BIDIRECTIONAL);
> -			if (dma & ODP_WRITE_ALLOWED_BIT)
> +			if (dma & ODP_WRITE_ALLOWED_BIT) {
> +				struct page *head_page = compound_head(page);
>   				/*
>   				 * set_page_dirty prefers being called with
>   				 * the page lock. However, MMU notifiers are
> @@ -656,6 +656,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
>   				 * be removed.
>   				 */
>   				set_page_dirty(head_page);

Just a nit:

The code is as readable with:
	set_page_dirty(compound_head(page));

and you relax the stack a little bit...

Otherwise:
Looks like a nice optimization.

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/core: dma map/unmap locking optimizations

[Sagi Grimberg]

On 4/15/2015 6:17 PM, Guy Shapiro wrote:
> Currently, while mapping or unmapping pages for ODP, the umem mutex is locked
> and unlocked once for each page. Such lock/unlock operation take few tens to
> hundreds of nsecs. This makes a significant impact when mapping or unmapping few
> MBs of memory.
>
> To avoid this, the mutex should be locked only once per operation, and not per
> page.
>
> Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Acked-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>   drivers/infiniband/core/umem_odp.c |    9 ++++-----
>   1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index 8b8cc6f..aba4739 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -446,7 +446,6 @@ static int ib_umem_odp_map_dma_single_page(
>   	int remove_existing_mapping = 0;
>   	int ret = 0;
>
> -	mutex_lock(&umem->odp_data->umem_mutex);
>   	/*
>   	 * Note: we avoid writing if seq is different from the initial seq, to
>   	 * handle case of a racing notifier. This check also allows us to bail
> @@ -479,8 +478,6 @@ static int ib_umem_odp_map_dma_single_page(
>   	}
>
>   out:
> -	mutex_unlock(&umem->odp_data->umem_mutex);
> -
>   	/* On Demand Paging - avoid pinning the page */
>   	if (umem->context->invalidate_range || !stored_page)
>   		put_page(page);
> @@ -586,6 +583,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
>
>   		bcnt -= min_t(size_t, npages << PAGE_SHIFT, bcnt);
>   		user_virt += npages << PAGE_SHIFT;
> +		mutex_lock(&umem->odp_data->umem_mutex);
>   		for (j = 0; j < npages; ++j) {
>   			ret = ib_umem_odp_map_dma_single_page(
>   				umem, k, base_virt_addr, local_page_list[j],
> @@ -594,6 +592,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
>   				break;
>   			k++;
>   		}
> +		mutex_unlock(&umem->odp_data->umem_mutex);
>
>   		if (ret < 0) {
>   			/* Release left over pages when handling errors. */
> @@ -633,9 +632,9 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
>   	 * faults from completion. We might be racing with other
>   	 * invalidations, so we must make sure we free each page only
>   	 * once. */
> +	mutex_lock(&umem->odp_data->umem_mutex);
>   	for (addr = virt; addr < bound; addr += (u64)umem->page_size) {
>   		idx = (addr - ib_umem_start(umem)) / PAGE_SIZE;
> -		mutex_lock(&umem->odp_data->umem_mutex);
>   		if (umem->odp_data->page_list[idx]) {
>   			struct page *page = umem->odp_data->page_list[idx];
>   			struct page *head_page = compound_head(page);
> @@ -663,7 +662,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
>   			umem->odp_data->page_list[idx] = NULL;
>   			umem->odp_data->dma_list[idx] = 0;
>   		}
> -		mutex_unlock(&umem->odp_data->umem_mutex);
>   	}
> +	mutex_unlock(&umem->odp_data->umem_mutex);
>   }
>   EXPORT_SYMBOL(ib_umem_odp_unmap_dma_pages);
>

Looks good.

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html