Re: [PATCH rdma-rc 4/9] IB/mlx4: Don't return errors from poll_cq

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4215 bytes --]

On Mon, Aug 29, 2016 at 12:41:19PM +0300, Leon Romanovsky wrote:
> On Sun, Aug 28, 2016 at 07:05:51PM +0300, Sagi Grimberg wrote:
> >
> > > 		/* SRQ is also in the radix tree */
> > > 		msrq = mlx4_srq_lookup(to_mdev(cq->ibcq.device)->dev,
> > > 				       srq_num);
> > >-		if (unlikely(!msrq)) {
> > >-			pr_warn("CQ %06x with entry for unknown SRQN %06x\n",
> > >-				cq->mcq.cqn, srq_num);
> > >-			return -EINVAL;
> > >-		}
> > > 	}
> >
> > BTW, this is completely unrelated to this patch, but the current
> > implementation of shared receive queues in Mellanox drivers is
> > *very* inefficient. Each completion that relates to a srq the
> > mlx4/mlx5 drivers perform a device-wide locked srq lookup which
> > is pretty bad... (it kills consumers that want to use more
> > then one SRQ)
> >
> > Other drivers that support kernel SRQs that I've looked at are ocrdma and
> > i40iw and they don't seem to have this lock everything approach.
> >
> > At the very-least we should try to make it a rcu + percpu_ref
> > instead of a killer device-wide lock. It'd be even better if
> > we use refcounting in the IB core and have the drivers not worry
> > about the kernel consumers destroying SRQs while processing IO
> > (i.e. when all the related QPs and CQs are destroyed).
>
> This can be as a beginning.
> diff --git a/drivers/net/ethernet/mellanox/mlx4/srq.c b/drivers/net/ethernet/mellanox/mlx4/srq.c
> index 6714662..e53d366 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/srq.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/srq.c
> @@ -303,10 +303,10 @@ struct mlx4_srq *mlx4_srq_lookup(struct mlx4_dev *dev, u32 srqn)
>  	struct mlx4_srq *srq;
>  	unsigned long flags;
>
> -	spin_lock_irqsave(&srq_table->lock, flags);
> +	rcu_read_lock();
>  	srq = radix_tree_lookup(&srq_table->tree,
>  				srqn & (dev->caps.num_srqs - 1));
> -	spin_unlock_irqrestore(&srq_table->lock, flags);
> +	rcu_read_unlock();
>
>  	return srq;
>  }

Or more complete patch (untested),

From 8695e5b807610e08de055b04ddd16caa6a5b61f2 Mon Sep 17 00:00:00 2001
From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date: Mon, 29 Aug 2016 12:42:24 +0300
Subject: [PATCH] IB/mlx4: Use RCU to perform radix tree lookup for SRQ

Radix tree lookup can be performed without locking.

Suggested-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/net/ethernet/mellanox/mlx4/srq.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/srq.c b/drivers/net/ethernet/mellanox/mlx4/srq.c
index 6714662..f44d089 100644
--- a/drivers/net/ethernet/mellanox/mlx4/srq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/srq.c
@@ -45,15 +45,12 @@ void mlx4_srq_event(struct mlx4_dev *dev, u32 srqn, int event_type)
 	struct mlx4_srq_table *srq_table = &mlx4_priv(dev)->srq_table;
 	struct mlx4_srq *srq;

-	spin_lock(&srq_table->lock);
-
+	rcu_read_lock();
 	srq = radix_tree_lookup(&srq_table->tree, srqn & (dev->caps.num_srqs - 1));
+	rcu_read_unlock();
 	if (srq)
 		atomic_inc(&srq->refcount);
-
-	spin_unlock(&srq_table->lock);
-
-	if (!srq) {
+	else {
 		mlx4_warn(dev, "Async event for bogus SRQ %08x\n", srqn);
 		return;
 	}
@@ -301,12 +298,11 @@ struct mlx4_srq *mlx4_srq_lookup(struct mlx4_dev *dev, u32 srqn)
 {
 	struct mlx4_srq_table *srq_table = &mlx4_priv(dev)->srq_table;
 	struct mlx4_srq *srq;
-	unsigned long flags;

-	spin_lock_irqsave(&srq_table->lock, flags);
+	rcu_read_lock();
 	srq = radix_tree_lookup(&srq_table->tree,
 				srqn & (dev->caps.num_srqs - 1));
-	spin_unlock_irqrestore(&srq_table->lock, flags);
+	rcu_read_unlock();

 	return srq;
 }
--
2.7.4


>
> >
> > I have some code in the works on this but it's not high on my todo
> > list at the moment. Mellanox folks, any thoughts on this?
> > --
> > 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



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]