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@xxxxxxxxxxxx>
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@xxxxxxxxxxx>
Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
