On Wed, Apr 13, 2022 at 2:03 PM Bob Pearson <rpearsonhpe@xxxxxxxxx> wrote:
>
> On 4/13/22 00:58, Zhu Yanjun wrote:
> > On Wed, Apr 13, 2022 at 1:31 PM Bob Pearson <rpearsonhpe@xxxxxxxxx> wrote:
> >>
> >> rxe_mcast.c currently uses _irqsave spinlocks for rxe->mcg_lock
> >> while rxe_recv.c uses _bh spinlocks for the same lock. Adding
> >> tracing shows that the code in rxe_mcast.c is all executed in_task()
> >> context while the code in rxe_recv.c that refers to the lock
> >> executes in softirq context. This causes a lockdep warning in code
> >> that executes multicast I/O including blktests check srp.
> >>
> >> Change the locking of rxe->mcg_lock in rxe_mcast.c to use
> >> spin_(un)lock_bh().
> >
> > Now RXE is not stable. We should focus on the problem of RXE.
> >
> > Zhu Yanjun
>
> This a real bug and triggers lockdep warnings when I run blktests srp.
> The blktests test suite apparently uses multicast. It is obviously wrong
> you can't use both _bh and _irqsave locks and pass lockdep checking.
>
> What tests are not running correctly for you?
The crash related with mr is resolved?
Zhu Yanjun
>
> Bob
> >
> >>
> >> With this change the following locks in rdma_rxe which are all _bh
> >> show no lockdep warnings.
> >>
> >> atomic_ops_lock
> >> mw->lock
> >> port->port_lock
> >> qp->state_lock
> >> rxe->mcg_lock
> >> rxe->mmap_offset_lock
> >> rxe->pending_lock
> >> task->state_lock
> >>
> >> The only other remaining lock is pool->xa.xa_lock which
> >> must either be some combination of _bh and _irq types depending
> >> on the object type (== ah or not) or plain spin_lock if
> >> the read side operations are converted to use rcu_read_lock().
> >>
> >> Fixes: 6090a0c4c7c6 ("RDMA/rxe: Cleanup rxe_mcast.c")
> >> Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
> >> ---
> >> drivers/infiniband/sw/rxe/rxe_mcast.c | 36 +++++++++++----------------
> >> 1 file changed, 15 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c
> >> index ae8f11cb704a..7f50566b8d89 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_mcast.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c
> >> @@ -143,11 +143,10 @@ static struct rxe_mcg *__rxe_lookup_mcg(struct rxe_dev *rxe,
> >> struct rxe_mcg *rxe_lookup_mcg(struct rxe_dev *rxe, union ib_gid *mgid)
> >> {
> >> struct rxe_mcg *mcg;
> >> - unsigned long flags;
> >>
> >> - spin_lock_irqsave(&rxe->mcg_lock, flags);
> >> + spin_lock_bh(&rxe->mcg_lock);
> >> mcg = __rxe_lookup_mcg(rxe, mgid);
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >>
> >> return mcg;
> >> }
> >> @@ -198,7 +197,6 @@ static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid,
> >> static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid)
> >> {
> >> struct rxe_mcg *mcg, *tmp;
> >> - unsigned long flags;
> >> int err;
> >>
> >> if (rxe->attr.max_mcast_grp == 0)
> >> @@ -214,7 +212,7 @@ static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid)
> >> if (!mcg)
> >> return ERR_PTR(-ENOMEM);
> >>
> >> - spin_lock_irqsave(&rxe->mcg_lock, flags);
> >> + spin_lock_bh(&rxe->mcg_lock);
> >> /* re-check to see if someone else just added it */
> >> tmp = __rxe_lookup_mcg(rxe, mgid);
> >> if (tmp) {
> >> @@ -232,12 +230,12 @@ static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid)
> >> if (err)
> >> goto err_dec;
> >> out:
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >> return mcg;
> >>
> >> err_dec:
> >> atomic_dec(&rxe->mcg_num);
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >> kfree(mcg);
> >> return ERR_PTR(err);
> >> }
> >> @@ -280,11 +278,9 @@ static void __rxe_destroy_mcg(struct rxe_mcg *mcg)
> >> */
> >> static void rxe_destroy_mcg(struct rxe_mcg *mcg)
> >> {
> >> - unsigned long flags;
> >> -
> >> - spin_lock_irqsave(&mcg->rxe->mcg_lock, flags);
> >> + spin_lock_bh(&mcg->rxe->mcg_lock);
> >> __rxe_destroy_mcg(mcg);
> >> - spin_unlock_irqrestore(&mcg->rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&mcg->rxe->mcg_lock);
> >> }
> >>
> >> /**
> >> @@ -339,25 +335,24 @@ static int rxe_attach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp)
> >> {
> >> struct rxe_dev *rxe = mcg->rxe;
> >> struct rxe_mca *mca, *tmp;
> >> - unsigned long flags;
> >> int err;
> >>
> >> /* check to see if the qp is already a member of the group */
> >> - spin_lock_irqsave(&rxe->mcg_lock, flags);
> >> + spin_lock_bh(&rxe->mcg_lock);
> >> list_for_each_entry(mca, &mcg->qp_list, qp_list) {
> >> if (mca->qp == qp) {
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >> return 0;
> >> }
> >> }
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >>
> >> /* speculative alloc new mca without using GFP_ATOMIC */
> >> mca = kzalloc(sizeof(*mca), GFP_KERNEL);
> >> if (!mca)
> >> return -ENOMEM;
> >>
> >> - spin_lock_irqsave(&rxe->mcg_lock, flags);
> >> + spin_lock_bh(&rxe->mcg_lock);
> >> /* re-check to see if someone else just attached qp */
> >> list_for_each_entry(tmp, &mcg->qp_list, qp_list) {
> >> if (tmp->qp == qp) {
> >> @@ -371,7 +366,7 @@ static int rxe_attach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp)
> >> if (err)
> >> kfree(mca);
> >> out:
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >> return err;
> >> }
> >>
> >> @@ -405,9 +400,8 @@ static int rxe_detach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp)
> >> {
> >> struct rxe_dev *rxe = mcg->rxe;
> >> struct rxe_mca *mca, *tmp;
> >> - unsigned long flags;
> >>
> >> - spin_lock_irqsave(&rxe->mcg_lock, flags);
> >> + spin_lock_bh(&rxe->mcg_lock);
> >> list_for_each_entry_safe(mca, tmp, &mcg->qp_list, qp_list) {
> >> if (mca->qp == qp) {
> >> __rxe_cleanup_mca(mca, mcg);
> >> @@ -421,13 +415,13 @@ static int rxe_detach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp)
> >> if (atomic_read(&mcg->qp_num) <= 0)
> >> __rxe_destroy_mcg(mcg);
> >>
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >> return 0;
> >> }
> >> }
> >>
> >> /* we didn't find the qp on the list */
> >> - spin_unlock_irqrestore(&rxe->mcg_lock, flags);
> >> + spin_unlock_bh(&rxe->mcg_lock);
> >> return -EINVAL;
> >> }
> >>
> >> --
> >> 2.32.0
> >>
>
