Re: [PATCH for-next 05/20] RDMA/hns: Add command queue support for hip08 RoCE driver

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

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

On Mon, Sep 25, 2017 at 01:06:53PM -0400, Doug Ledford wrote:
> On Wed, 2017-08-30 at 17:23 +0800, Wei Hu (Xavier) wrote:
>
> So, we set the send timeout limit here:
>
> > +	/* Setup Tx write back timeout */
> > +	priv->cmq.tx_timeout = HNS_ROCE_CMQ_TX_TIMEOUT;
> > +
>
> [ snip ]
>
> > +int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
> > +		      struct hns_roce_cmq_desc *desc, int num)
> > +{
> > +	struct hns_roce_v2_priv *priv = (struct hns_roce_v2_priv
> > *)hr_dev->priv;
> > +	struct hns_roce_v2_cmq_ring *csq = &priv->cmq.csq;
> > +	struct hns_roce_cmq_desc *desc_to_use;
> > +	bool complete = false;
> > +	u32 timeout = 0;
> > +	int handle = 0;
> > +	u16 desc_ret;
> > +	int ret = 0;
> > +	int ntc;
> > +
> > +	spin_lock_bh(&csq->lock);
>
> and we take a bh lock here...
>
> > +	if (num > hns_roce_cmq_space(csq)) {
> > +		spin_unlock_bh(&csq->lock);
> > +		return -EBUSY;
> > +	}
> > +
> > +	/*
> > +	 * Record the location of desc in the cmq for this time
> > +	 * which will be use for hardware to write back
> > +	 */
> > +	ntc = csq->next_to_use;
> > +
> > +	while (handle < num) {
> > +		desc_to_use = &csq->desc[csq->next_to_use];
> > +		*desc_to_use = desc[handle];
> > +		dev_dbg(hr_dev->dev, "set cmq desc:\n");
> > +		csq->next_to_use++;
> > +		if (csq->next_to_use == csq->desc_num)
> > +			csq->next_to_use = 0;
> > +		handle++;
> > +	}
> > +
> > +	/* Write to hardware */
> > +	roce_write(hr_dev, ROCEE_TX_CMQ_TAIL_REG, csq->next_to_use);
> > +
> > +	/*
> > +	 * If the command is sync, wait for the firmware to write
> > back,
> > +	 * if multi descriptors to be sent, use the first one to
> > check
> > +	 */
> > +	if ((desc->flag) & HNS_ROCE_CMD_FLAG_NO_INTR) {
> > +		do {
> > +			if (hns_roce_cmq_csq_done(hr_dev))
> > +				break;
> > +			usleep_range(1000, 2000);
> > +			timeout++;
> > +		} while (timeout < priv->cmq.tx_timeout);
> > +	}
>
> then we spin here for a maximum amount of time between 200 and 400ms,
> so 1/4 to 1/2 a second.  All the time we are holding the bh lock on
> this CPU.  That seems excessive to me.  If we are going to spin that
> long, can you find a way to allocate/reserve your resources, send the
> command, then drop the bh lock while you spin, and retake it before you
> complete once the spinning is done?

They don't allocate anything in this loop, but checking the pointers are
the same, see hns_roce_cmq_csq_done.

>
> > +#define HNS_ROCE_CMQ_TX_TIMEOUT			200
>
> or you could reduce the size of this define...
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>
> --
> 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: 833 bytes --]