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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/07/16 10:36, Chris Wilson wrote:
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.

Never mind then, leave it all as it was.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux