From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] introduce and used relaxed cpumask operations Date: Wed, 21 Jan 2015 14:42:50 +0000 Message-ID: <54BFBAEA.50002@eu.citrix.com> References: <54BD379C0200007800056965@mail.emea.novell.com> <54BFB775.7030702@eu.citrix.com> <54BFC7290200007800057BAE@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDwYb-0002Y7-Df for xen-devel@lists.xenproject.org; Wed, 21 Jan 2015 14:46:57 +0000 In-Reply-To: <54BFC7290200007800057BAE@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: KeirFraser , Andrew Cooper , Tim Deegan , Ian Campbell , xen-devel , Ian Jackson List-Id: xen-devel@lists.xenproject.org On 01/21/2015 02:35 PM, Jan Beulich wrote: >>>> On 21.01.15 at 15:28, 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 >> >> 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