Re: [Xen-devel] [PATCH 1/2] IRQ: allocate CPU masks dynamically

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.11.11 at 17:02, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/11/11 15:51, Jan Beulich wrote:
>>>>> On 03.11.11 at 16:43, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 03/11/11 15:17, Jan Beulich wrote:
>>>>>>> On 03.11.11 at 15:49, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> On 03/11/11 14:26, Jan Beulich wrote:
>>>>>> IRQ: allocate CPU masks dynamically
>>>>>>
>>>>>> This includes delaying the initialization of dynamically created IRQs
>>>>>> until their actual first use and some further elimination of uses of
>>>>>> struct irq_cfg.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>
>>>>> One query which may or may not affect the patch.  Would we get better
>>>>> caching characteristics if all cpumasks were allocated in consecutive
>>>>> memory, rather than having 3 individual allocs in arch_init_one_irq_desc ?
>>>> That was what the first version of the patch did, rejected by Keir
>>>> (and not liked too much by me either).
>>>>
>>>> Jan
>>> My understanding of the objection was hiding the variables themselves as
>>> an array in the code.
>>>
>>> An alternative approach such as alloc'ing 3*sizeof(cpu mask) (cache
>>> aligned) and assigning the relevant pointers to the current
>>> cpumask_var_t's would be a suitable approach which causes the cpumasks
>>> to be in contiguous memory, but not changing how they are referenced in
>>> the code.
>> That would mean just open-coding what the former patch did by
>> abstraction. In my opinion that is even worse - either we want a
>> generally usable mechanism to do this, or we don't do it at all.
>>
>> Jan
> 
> That is an interesting point, but I dont think it is worse.
> 
> There are very few cases where we want to be doing this, so open coding
> is not too bad.  With a comment explaining why, I would have thought it
> would be fine.

This isn't how I think maintainable code should be written - whenever
we'd adjust the CPU mask allocation mechanism in some way, we'd
have to touch that code separately. Depending on the nature of the
change, it may even get overlooked (i.e. when no build failure results).

Just consider how ugly it would be if there were no
{,z}alloc_cpumask_var() wrappers (which already did change from their
original version in that we're now able to allocate just the portion that
we know will actually get used).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel