[openib-general] [PATCH] (repost) no qp lock on poll, separate sq/rq locks

[Michael S. Tsirkin]

Quoting r. Roland Dreier <roland at topspin.com>:
> Subject: Re: [openib-general] [PATCH] (repost) no qp lock on poll, separate sq/rq locks
> 
> Ugh, I think I missed something when I thought about this the first
> time around.  It seems the test for WQ overflow assumes that all WQs
> have a power-of-2 size, which we currently don't enforce for Tavor
> mode.

Actually the old code had this

wq->last_comp = wq->max - 1;

which led me to assumed its a power of two.

> It seems there are two possible solutions:
> 
>   Round up WQ sizes for Tavor as well.  I don't like this because it
>   could potentially use a lot of extra memory.
> 
>   Or, add one more counter back into the WQ struct so we can keep
>   track of both the next index to use as well as the total number of
>   WQEs posted in Tavor mode (we still only need one counter in
>   mem-free mode).
> 
> I implemented the second option.  Does this patch look reasonable?
> 
>  - R.
> 
> Index: hw/mthca/mthca_provider.h
> ===================================================================
> --- hw/mthca/mthca_provider.h	(revision 1912)
> +++ hw/mthca/mthca_provider.h	(working copy)
> @@ -167,15 +167,16 @@ struct mthca_cq {
>  
>  struct mthca_wq {
>  	spinlock_t lock;
> -	int   max;
> -	int   next;
> -	int   last_comp;
> -	void *last;
> -	int   max_gs;
> -	int   wqe_shift;
> +	int        max;
> +	unsigned   next_ind;
> +	unsigned   head;
> +	unsigned   tail;
> +	void      *last;
> +	int        max_gs;
> +	int        wqe_shift;
>  
> -	int   db_index;		/* Arbel only */
> -	u32  *db;
> +	int        db_index;	/* Arbel only */
> +	u32       *db;
>  };
>  
>  struct mthca_qp {
> Index: hw/mthca/mthca_cq.c
> ===================================================================
> --- hw/mthca/mthca_cq.c	(revision 1912)
> +++ hw/mthca/mthca_cq.c	(working copy)
> @@ -453,7 +453,7 @@ static inline int mthca_poll_one(struct 
>  		entry->wr_id = (*cur_qp)->wrid[wqe_index];
>  	}
>  
> -	wq->last_comp = wqe_index;
> +	++wq->tail;
>  
>  	if (0)
>  		mthca_dbg(dev, "%s completion for QP %06x, index %d (nr %d)\n",

You seem to assume that each completion is for exactly one wqe.
How does this work for selective signaling?


-- 
MST - Michael S. Tsirkin