Re: [Xen-devel] [PATCH] introduce and used relaxed cpumask operations

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 01/21/2015 02:35 PM, Jan Beulich wrote:
>>>> On 21.01.15 at 15:28, <george.dunlap@xxxxxxxxxxxxx> wrote:
>> On 01/19/2015 03:58 PM, Jan Beulich wrote:
>>> Using atomic (LOCKed on x86) bitops for certain of the operations on
>>> cpumask_t is overkill when the variables aren't concurrently accessible
>>> (e.g. local function variables, or due to explicit locking). Introduce
>>> alternatives using non-atomic bitops and use them where appropriate.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> I'm wondering if it might be sensible to have more informative names for
>> these, that would make it easier for coders / reviewers to see what
>> aspect makes the cpumask suitable for the relaxed access; for instance,
>> "local_cpumask_set_cpu()" for local variables, and
>> "locked_cpumask_set_cpu()" for  cpumasks which we know are locked.  (Or
>> perhaps cpumask_set_cpu_local and cpumask_set_cpu_locked.)
> 
> Makes a lot of sense, except that it means even more typing.

Well perhaps we could save some typing by renaming it "_cmsksc()"
instead? :-)

The most expensive and annoying part of development is finding bugs in
code; the second is understanding code that was written a long time ago.
 Investing a bit of extra typing being able to avoid these errors and
making things easier makes sense to me.

But, it's just a suggestion, and I'm not the one writing the patch, so I
wouldn't hold it up based on that.

> 
>>> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
>>>      }
>>>      else
>>>      {
>>> -        cpumask_t cur_cpu;
>>> -        cpumask_clear(&cur_cpu);
>>> -        cpumask_set_cpu(cpu, &cur_cpu);
>>> -        snext = __runq_pick(ops, &cur_cpu);
>>> +        snext = __runq_pick(ops, cpumask_of(cpu));
>>>          if ( snext == NULL )
>>>              snext = rt_vcpu(idle_vcpu[cpu]);
>>>  
>>
>> This bit really needs explicit mention in the changelog.
> 
> Already done in response to Andrew's similar request.

Ah, sorry -- I saw that but for some reason thought he was talking about
a different hunk.

As I said, I think having a more informative name would be better, but
I'll leave that up to you to decide.

With the changelog update, scheduler parts are:

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel