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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx