On Mon, 26 Nov 2012 20:09:00 +0200 Ville Syrj?l? <ville.syrjala at linux.intel.com> wrote: > On Mon, Nov 26, 2012 at 05:46:48PM +0000, Chris Wilson wrote: > > On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrj?l? <ville.syrjala at linux.intel.com> wrote: > > > On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote: > > > > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala at linux.intel.com wrote: > > > > > From: Ville Syrj?l? <ville.syrjala at linux.intel.com> > > > > > > > > > > Ringbuffer tail pointer must be qword aligned. Warn if someone > > > > > makes a mistake and forgets to pad the ring when the commands > > > > > inserted into the ring don't align to qword naturally. > > > > > > > > The assertion should be that we wrote precisely the number of dwords we > > > > declared in intel_ring_begin(). Which is one of the important factors to > > > > check whenever reviewing such code. This assertion (which should be a > > > > BUG_ON) is no substitute for such review. > > > > > > Yeah. I was considering adding some reserved_space field to the ring, > > > populate it in ring_begin(), and and make sure it was correctly > > > consumed at ring_advance(). If you think that sounds good, I can cook > > > up a patch for it. > > > > To be honest, I was thinking of a firing squad for the author and > > reviewers of any such patch that gets intel_ring_begin()..end() wrong. > > I was mainly thinking it might helpful during development, to catch > obvious bugs. A compile time check would be even better though. > It is nice for development (the version that Chris was hinting at), if the perf cost is provably low, or you can wrap it neatly in some #if blocks, I'd be in favor. Since you're fairly new to this world though, I'll mention that my opinion is almost always the minority, and the kiss of death in some cases... sorry. -- Ben Widawsky, Intel Open Source Technology Center
