Re: [Xen-devel] [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.18 at 17:35, <george.dunlap@xxxxxxxxxx> wrote:
> On 09/07/2018 04:17 PM, Jan Beulich wrote:
>>>>> On 07.09.18 at 15:56, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 07/09/18 09:03, Jan Beulich wrote:
>>>>>>> On 06.09.18 at 14:08, <andrew.cooper3@xxxxxxxxxx> 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.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel