[openib-general] [PATCH] (fixed) cqe lookup speedup

[Michael S. Tsirkin]

Quoting r. Grant Grundler <iod00d at hp.com>:
> Subject: Re: [openib-general] [PATCH] (fixed) cqe lookup speedup
> 
> Michael,
> just is questions about the code...maybe roland can answer.

All this is style, and most refers to code I didnt change.
This is for Roland to decide.

> On Wed, Jan 26, 2005 at 04:52:18PM +0200, Michael S. Tsirkin wrote:
> > Index: hw/mthca/mthca_cq.c
> > ===================================================================
> > --- hw/mthca/mthca_cq.c	(revision 1653)
> > +++ hw/mthca/mthca_cq.c	(working copy)
> > @@ -147,20 +147,21 @@ static inline struct mthca_cqe *get_cqe(
> >  			+ (entry * MTHCA_CQ_ENTRY_SIZE) % PAGE_SIZE;
> >  }
> >  
> > -static inline int cqe_sw(struct mthca_cq *cq, int i)
> > +static inline struct mthca_cqe *cqe_sw(struct mthca_cq *cq, int i)
> >  {
> > -	return !(MTHCA_CQ_ENTRY_OWNER_HW &
> > -		 get_cqe(cq, i)->owner);
> > +	struct mthca_cqe *cqe;
> > +	cqe = get_cqe(cq, i);
> 
> this could be one line:
> 	struct mthca_cqe *cqe = get_cqe(cq, i);
> 
> > @@ -775,7 +784,7 @@ void mthca_free_cq(struct mthca_dev *dev
> >  		int j;
> >  
> >  		printk(KERN_ERR "context for CQN %x (cons index %x, next sw %d)\n",
> > -		       cq->cqn, cq->cons_index, next_cqe_sw(cq));
> > +		       cq->cqn, cq->cons_index, next_cqe_sw(cq)?1:0);
> >  		for (j = 0; j < 16; ++j)
> >  			printk(KERN_ERR "[%2x] %08x\n", j * 4, be32_to_cpu(ctx[j]));
> >  	}
> 
> This code chunk is "dead" code. It starts with "if (0)".
> Can it just be deleted?
> 
> 
> In mthca_poll_one():
> 	        if ((cqe->opcode & MTHCA_ERROR_CQE_OPCODE_MASK) ==
> 		    MTHCA_ERROR_CQE_OPCODE_MASK) {
> 			is_error = 1;
> 			is_send = cqe->opcode & 1;
> 		} else
> 			is_send = cqe->is_send & 0x80;
> 
> Could be better written as:
> 	is_error = (cqe->opcode & MTHCA_ERROR_CQE_OPCODE_MASK) ==							 MTHCA_ERROR_CQE_OPCODE_MASK);
> 
> 	is_send = is_error ? (cqe->opcode & 1) : (cqe->is_send & 0x80);
> 
> BTW, I dislike use of "is_send" to branch the code path several times
> later instead of just having two different code paths that call the same
> in-line functions. Interested in a patch to change this?
> 
> thanks,
> grant

-- 
I dont speak for Mellanox