[Intel-gfx] [PATCH 3/3] drm/i915: Group the irq breadcrumb variables into the same cacheline

[Chris Wilson]

On Wed, Jul 06, 2016 at 10:18:32AM +0100, Tvrtko Ursulin wrote:
> 
> On 06/07/16 08:45, Chris Wilson wrote:
> >As we inspect both the tasklet (to check for an active bottom-half) and
> >set the irq-posted flag at the same time (both in the interrupt handler
> >and then in the bottom-halt), group those two together into the same
> >cacheline. (Not having total control over placement of the struct means
> >we can't guarantee the cacheline boundary, we need to align the kmalloc
> >and then each struct, but the grouping should help.)
> 
> Any actual difference or just tidy?

Just motivated by tidying. I expect this to be in the noise (but since I
have the tools, I should check just in case).

> >@@ -167,16 +167,20 @@ struct intel_engine_cs {
> >  	 * the overhead of waking that client is much preferred.
> >  	 */
> >  	struct intel_breadcrumbs {
> >+		struct task_struct *irq_tasklet; /* bh for user interrupts */
> 
> Tasklet was kind of passable, irq_tasklet is imho worse. I think
> anyone who see that name would thing this handles interrupts of some
> sort. :)

My thinking was to give a similar name to the three variables used in
the irq handler (and bottom-half) and move them aside from the spinlock.
 
> How about first_wait_task ?
> 
> I know it may feel like pointless bike-shedding and maybe it is so I
> am leaving it with you.

Similarity argument still holds imo.

irq_seqno_bh ?

> >+		unsigned long irq_count;
> >+		bool irq_posted;
> >+
> >  		spinlock_t lock; /* protects the lists of requests */
> >  		struct rb_root waiters; /* sorted by retirement, priority */
> >  		struct rb_root signals; /* sorted by retirement */
> >  		struct intel_wait *first_wait; /* oldest waiter by retirement */
> >-		struct task_struct *tasklet; /* bh for user interrupts */
> >  		struct task_struct *signaler; /* used for fence signalling */
> >  		struct drm_i915_gem_request *first_signal;
> >  		struct timer_list fake_irq; /* used after a missed interrupt */
> >-		bool irq_enabled;
> >-		bool rpm_wakelock;
> >+
> >+		bool irq_enabled : 1;
> >+		bool rpm_wakelock : 1;
> 
> Is there much point in having bitfields? To me two plain bools would
> be just fine and smaller code.

In this case a fractionally smaller struct (-4 bytes)
The code size in this case is identical

   text	   data	    bss	    dec	    hex	
1067277	   4565	    416	1072258	 105c82	as bool
1067277	   4565	    416	1072258	 105c82	as bool : 1

since we only do very simple test and sets.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre