Re: [RFC][PATCH 3/3] enic: add h/w interfaces

[Roland Dreier <rdreier@cisco.com>]

 > > spinning for 100 usecs is pretty nasty... can this be changed to
 > > usleep()?
 > 
 > No because we can't sleep in some of the calling contexts.

I don't think it's a merge-blocker but it is definitely worth thinking
about how to avoid udelay(100) -- a 100+ microsecond latency is pretty
nasty for lots of cases.

 > > not sure why you're making this volatile here... I suspect it doesn't do
 > > what you really want on architectures with a weak memory ordering model,
 > > so it would be better to make things explicit with a memory barrier plus
 > > a comment explaining what you're doing.
 > 
 > We're using volatile here to make the color bit in the descriptor is
 > read first before the any of the other desc fields.  The hardware
 > guarantees the color bit is the last byte (bit) written on the
 > descriptor.  I'll put in a comment explaining what we're doing.

So then volatile doesn't actually do what you want.  You need rmb() to
make sure CPUs with weak ordering models don't reorder things.  Volatile
makes sure the compiler doesn't reorder things but out-of-order CPUs can
easily execute the reads in a different order than that.

 > > This looks way too big to inline.
 > 
 > It's called in the performance path in several places.

My guess is that the benefit of having only one copy in I$ outweighs the
function call overhead.  I guess you could see if you could measure a
difference either way.  And if things are equal then the smaller code is
preferable because it reduces the cache pressure on other code that
might be running.

 - R.