[openib-general] Re: [PATCH] mthca: fix posting long work request lists

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rolandd at cisco.com>:
> Subject: Re: [PATCH] mthca: fix posting long work request lists
> 
> Does this look OK?

Nope. comments below.

> I added SRQ support and also moved the doorbell[2]
> declaration to the top of the functions (since I'm not convinved all
> versions of gcc are smart enough to see that two copies of doorbell[]
> can share stack slots).

True. AFAIK no existing gcc version is smart enough for this.

>  - R.
> 
> Index: infiniband/hw/mthca/mthca_srq.c
> ===================================================================
> --- infiniband/hw/mthca/mthca_srq.c	(revision 3989)
> +++ infiniband/hw/mthca/mthca_srq.c	(working copy)
> @@ -414,6 +414,7 @@ int mthca_tavor_post_srq_recv(struct ib_
>  {
>  	struct mthca_dev *dev = to_mdev(ibsrq->device);
>  	struct mthca_srq *srq = to_msrq(ibsrq);
> +	__be32 doorbell[2];
>  	unsigned long flags;
>  	int err = 0;
>  	int first_ind;
> @@ -429,6 +430,23 @@ int mthca_tavor_post_srq_recv(struct ib_
>  	first_ind = srq->first_free;
>  
>  	for (nreq = 0; wr; ++nreq, wr = wr->next) {
> +		if (unlikely(nreq == MTHCA_TAVOR_MAX_WQES_PER_RECV_DB)) {
> +			doorbell[0] = cpu_to_be32(first_ind << srq->wqe_shift);
> +			doorbell[1] = cpu_to_be32((srq->srqn << 8) | nreq);

this one's wrong. nreq will overflow into bit 8.
You want to put 0 there since we know nreq is 256:
			doorbell[1] = cpu_to_be32(qp->qpn << 8);

Thats what my patch does for RQ, check it out.

You also need nreq = 0 here, I think (better here where we've just read it
than after the barrier).

> +
> +			/*
> +			 * Make sure that descriptors are written
> +			 * before doorbell is rung.
> +			 */
> +			wmb();
> +
> +			mthca_write64(doorbell,
> +				      dev->kar + MTHCA_RECEIVE_DOORBELL,
> +				      MTHCA_GET_DOORBELL_LOCK(&dev->doorbell_lock));
> +
> +			first_ind = srq->first_free;
> +		}
> +
>  		ind = srq->first_free;
>  
>  		if (ind < 0) {
> @@ -491,8 +509,6 @@ int mthca_tavor_post_srq_recv(struct ib_
>  	}
>  
>  	if (likely(nreq)) {
> -		__be32 doorbell[2];
> -
>  		doorbell[0] = cpu_to_be32(first_ind << srq->wqe_shift);
>  		doorbell[1] = cpu_to_be32((srq->srqn << 8) | nreq);
>  
> Index: infiniband/hw/mthca/mthca_wqe.h
> ===================================================================
> --- infiniband/hw/mthca/mthca_wqe.h	(revision 3989)
> +++ infiniband/hw/mthca/mthca_wqe.h	(working copy)
> @@ -49,7 +49,8 @@ enum {
>  };
>  
>  enum {
> -	MTHCA_INVAL_LKEY = 0x100
> +	MTHCA_INVAL_LKEY			= 0x100,
> +	MTHCA_TAVOR_MAX_WQES_PER_RECV_DB	= 256
>  };
>  
>  struct mthca_next_seg {
> Index: infiniband/hw/mthca/mthca_qp.c
> ===================================================================
> --- infiniband/hw/mthca/mthca_qp.c	(revision 4003)
> +++ infiniband/hw/mthca/mthca_qp.c	(working copy)
> @@ -1705,6 +1705,7 @@ int mthca_tavor_post_receive(struct ib_q
>  {
>  	struct mthca_dev *dev = to_mdev(ibqp->device);
>  	struct mthca_qp *qp = to_mqp(ibqp);
> +	__be32 doorbell[2];
>  	unsigned long flags;
>  	int err = 0;
>  	int nreq;
> @@ -1722,6 +1723,21 @@ int mthca_tavor_post_receive(struct ib_q
>  	ind = qp->rq.next_ind;
>  
>  	for (nreq = 0; wr; ++nreq, wr = wr->next) {
> +		if (unlikely(nreq == MTHCA_TAVOR_MAX_WQES_PER_RECV_DB)) {
> +			doorbell[0] = cpu_to_be32((qp->rq.next_ind << qp->rq.wqe_shift) | size0);
> +			doorbell[1] = cpu_to_be32((qp->qpn << 8) | nreq);

This one's wrong too. nreq will overflow into bit 8.
You want to put 0 there:
			doorbell[1] = cpu_to_be32(qp->qpn << 8);

> +
> +			wmb();
> +
> +			mthca_write64(doorbell,
> +				      dev->kar + MTHCA_RECEIVE_DOORBELL,
> +				      MTHCA_GET_DOORBELL_LOCK(&dev->doorbell_lock));
> +
> +			qp->rq.head += nreq;

I know I did it differently, but I now think its better to replace
nreq with MTHCA_TAVOR_MAX_WQES_PER_RECV_DB in the line above since
wmb should prevent gcc from knowing the value of nreq here or storing
it in a register.

> +			nreq  = 0;
> +			size0 = 0;
> +		}
> +
>  		if (mthca_wq_overflow(&qp->rq, nreq, qp->ibqp.recv_cq)) {
>  			mthca_err(dev, "RQ %06x full (%u head, %u tail,"
>  					" %d max, %d nreq)\n", qp->qpn,
> @@ -1779,8 +1795,6 @@ int mthca_tavor_post_receive(struct ib_q
>  
>  out:
>  	if (likely(nreq)) {
> -		__be32 doorbell[2];
> -
>  		doorbell[0] = cpu_to_be32((qp->rq.next_ind << qp->rq.wqe_shift) | size0);
>  		doorbell[1] = cpu_to_be32((qp->qpn << 8) | nreq);
>  
> 

-- 
MST