Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Dave Gordon <david.s.gordon@intel.com>]

On 02/12/15 13:46, Chris Wilson wrote:
> On Wed, Dec 02, 2015 at 01:29:21PM +0000, Dave Gordon wrote:
>> On 25/11/15 09:23, Daniel Vetter wrote:
>>> On Tue, Nov 24, 2015 at 11:47:26PM +0000, Chris Wilson wrote:
>>>> On Tue, Nov 24, 2015 at 10:26:01PM +0000, Chris Wilson wrote:
>>>>> On Tue, Nov 24, 2015 at 07:36:25PM +0200, Jani Nikula wrote:
>>>>>>   /* Iterate over initialised rings */
>>>>>>   #define for_each_ring(ring__, dev_priv__, i__) \
>>>>>>   	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
>>>>>> -		if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))
>>>>>> +		for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
>>>>>
>>>>> Idly wondering if we would be happy with
>>>>>
>>>>> for_each_ring(ring__, dev_priv__)
>>>>> 	for ((ring__) = &(dev_priv__)->ring[0];
>>>>> 	     (ring__) <= &(dev_priv__)->ring[I915_NUM_RINGS];
>>>>> 	     (ring__)++)
>>>>> 	     for_each_if(intel_ring_initialized(ring__))
>>>>>
>>>>> ?
>>>>>
>>>>> The downside is that we have used i__ in several places rather than
>>>>> ring->id.
>>>>
>>>> Fwiw, 13 files changed, 113 insertions(+), 140 deletions(-)
>>>>
>>>> Seems a reasonable shrinkage.
>>>
>>> Maybe for_each_engine even, and phase out for_each_ring completely?
>>> -Daniel
>>>
>>
>> Wouldn't it be nicer (and safer) not to build macros that fold the
>> loop structure into the macro (in contravention of kernel
>> programming guidelines).
>>
>> So how about NOT including the actual for() inside the macro, so
>> that instead of writing
>>
>> 	for_each_engine(engine, dev_priv)
>> 		do_stuff(engine)
>>
>> we would write it as
>>
>> 	for (EACH_ENGINE(engine, dev_priv))
>> 		initialise(engine)
>>
>> 	for (EACH_ACTIVE_ENGINE(engine, dev_priv)) {
>> 		service(engine)
>> 		restart(engine)
>> 	}
>>
>> etc. The for-loop is visible and the scope doesn't give you any surprises.
>>
>> [The EACH_ENGINE() macros expands to a semicolon-separated triplet
>> of expressions; still a violation of the "don't use macros to
>> redefine C syntax" guideline, but much less egregious than macros
>> that contain embedded 'for's and 'if's.
>
> for_each() is common practice in the kernel, so hiding the for() inside
> the macro isn't that egregious. The problem is defining EACH_ACTIVE_ENGINE()
> simply, preferrably without the use of another loop inside the for(;;).

Nothing wrong with another loop :) Although, to keep it tidy, it can be 
inside an inline helper function that skips over the unwanted items in 
the iteration. I think it's better to have the if-ready condition test 
inside the iterator (and therefore clearly bounded) than let it dangle 
at the end of the macro.

> One is to pack the i915->engines[] and have i915->num_engines and
> intel_lookup_engine (or an i915->engine_for_id[]) for the occasional
> case where we look up e.g. &i915->engines[BCS].
> -Chris

Or, put the active ones on a linked list, or keep a bitmask of which 
ones have been initialised inside the dev_priv structure, so you don't 
have to even dereference the engine[] array to work out whether a 
particular engine is initialised. Apropos which, wouldn't it be much 
more efficient to do that, because intel_ring_initialized() is quite 
heavyweight and the results surely don't change often, if at all, during 
normal operation. So we should only evaluate it when something has 
changed, and cache the bool result for use in all those for_each() loops!

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