On 07/09/18 16:53, Jan Beulich wrote:
>>>> On 07.09.18 at 17:35, <george.dun...@citrix.com> wrote:
>> On 09/07/2018 04:17 PM, Jan Beulich wrote:
>>>>>> On 07.09.18 at 15:56, <andrew.coop...@citrix.com> wrote:
>>>> On 07/09/18 09:03, Jan Beulich wrote:
>>>>>>>> On 06.09.18 at 14:08, <andrew.coop...@citrix.com> wrote:
>>>>>> @@ -2059,11 +2058,10 @@ csched_dump_pcpu(const struct scheduler *ops,
>>>>>> int
>> cpu)
>>>>>> spc = CSCHED_PCPU(cpu);
>>>>>> runq = &spc->runq;
>>>>>>
>>>>>> - cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_sibling_mask,
>> cpu));
>>>>>> - printk("CPU[%02d] nr_run=%d, sort=%d, sibling=%s, ",
>>>>>> - cpu, spc->nr_runnable, spc->runq_sort_last, cpustr);
>>>>>> - cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask,
>>>>>> cpu));
>>>>>> - printk("core=%s\n", cpustr);
>>>>>> + printk("CPU[%02d] nr_run=%d, sort=%d, sibling=%*pb, core=%*pb\n",
>>>>>> + cpu, spc->nr_runnable, spc->runq_sort_last,
>>>>>> + nr_cpu_ids, per_cpu(cpu_sibling_mask, cpu),
>>>>>> + nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
>>>>> Strictly speaking here and elsewhere you should wrap the CPU mask
>>>>> accesses in cpumask_bits().
>>>> Why? Its barely used, and is another example of a helper which only adds
>>>> to code volume.
>>> If anyone added (e.g. for debugging) a leading field to struct cpumask,
>>> your code would break, while all code anywhere else would still be fine.
>> Do all other current users use cpumask_bits() for dereferencing?
>>
>> I took Andy's "Its[sic] barely used" comment to mean there were lots of
>> other places which also just passed a cpumask_t pointer directly into
>> something expecting a bitmap. If all other use cases either use
>> cpumask_bits() or ->bits, then we should do the same here. If there are
>> lots of places where we assume (void *)mask == (void *)mask->bits, then
>> we should probably document that the structure should match that (and
>> maybe add a BUILD_BUG_ON() if we can manage it).
> I'm unaware of places which don't go through ->bits.
All the printing, seeing as I didn't hit a single cpumask_bits() in this
series.
The cpumask infrastructure itself uses ->bits, which is less verbose
than the helper.
I don't think we need to go as far as having a BUILD_BUG_ON(), because I
don't expect that the layout of a cpumask would change, even for
debugging, but I also don't see the point in keeping cpumask_bits() when
almost nothing uses it.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
