[openib-general] qp lock in mthca_poll_cq

[Michael S. Tsirkin]

Hello, Roland!
Thanks, the comment in mthca_provider.h clarified things to me.
A couple of small things I'd like to remark on:

Quoting r. Roland Dreier (roland at topspin.com) "Re: [openib-general] qp lock in mthca_poll_cq":
> There are two separate uses of the QP during CQ poll, which is why
> refcount is an atomic_t and we also take the spinlock.
> 
> First, the refcount is used to make sure that destroy_qp does not
> get rid of the QP struct while the QP is being accessed to handle
> poll_cq.

Wouldnt locking the cq while QP is being destroyed also work?
And maybe the eq which gets the async events.

> It is an atomic_t because it may be accessed without the QP
> lock being held (eg when an async event is received for the QP).

I see. Maybe this code:

  spin_lock(&dev->qp_table.lock);
  qp = mthca_array_get(&dev->qp_table.qp, qpn & (dev->limits.num_qps - 1));
  if (qp)
    atomic_inc(&qp->refcount);
  spin_unlock(&dev->qp_table.lock);

and this

  if (atomic_dec_and_test(&qp->refcount))
    wake_up(&qp->wait);

Shall be factored in inline functions?
There appear to be several users of such constructs
in mthca_cq and mthca_qp.

Something like mthca_qp_get / mthca_qp_put.

>
> Second, the QP's lock is taken during CQE processing because other
> non-atomic parts of the QP struct such as the number of WQEs
> outstanding _are_ modified and need to be protected against concurrent
> access from the send/receive post routine.
> 
> It might be possible to avoid taking the QP lock in poll_cq by making
> the current WQE count an atomic_t, but I'm not sure if that's a win
> because it means that send/receive posting would have to use atomic
> accesses as well (and I don't think you can make posting WQEs lock-free).

It seems so, since one has to serialize the access to the queue itself.

> Some of my reasoning is in the comment near the bottom of
> mthca_provider.h too.

It says there:
 * We have one global lock that protects dev->cq/qp_table.  Each
 * struct mthca_cq/qp also has its own lock.  No locks should be
 * nested inside each other.

However mthca_poll_one takes qp->lock while cq->lock is being held
in  mthca_poll_cq.
Isn't that nesting?


>  - Roland

MST