From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2 2/2] sched_credit2.c: runqueue_per_core code Date: Mon, 16 Mar 2015 12:51:45 +0000 Message-ID: <5506D1E1.8050404@eu.citrix.com> References: <20150313181109.GA3179@gmail.com> <55032C85.6090805@citrix.com> <550336EE.60909@eu.citrix.com> <5506DF40020000780006A407@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5506DF40020000780006A407@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: Andrew Cooper , dario.faggioli@citrix.com, xen-devel@lists.xen.org, George.Dunlap@citrix.com, Uma Sharma List-Id: xen-devel@lists.xenproject.org On 03/16/2015 12:48 PM, Jan Beulich wrote: >>>> On 13.03.15 at 20:13, wrote: >> On 03/13/2015 06:29 PM, Andrew Cooper wrote: >>>> @@ -1940,10 +1946,10 @@ static void init_pcpu(const struct scheduler *ops, int cpu) >>>> >>>> /* Figure out which runqueue to put it in */ >>>> /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */ >>>> - if ( cpu == 0 ) >>>> - rqi = 0; >>>> + if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET ) >>>> + rqi = cpu ? cpu_to_socket(cpu) : boot_cpu_to_socket(); >>> >>> This conditional is bogus. If cpu0 is offlined and re-onlined, it must >>> use cpu_to_core() >>> >>> This entire hunk should probably be >>> >>> rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ? >>> cpu_to_socket(cpu) : cpu_to_core(cpu); >>> >>> (with suitable alignment) >> >> You're ignoring the fact that she's following suit from existing code; >> and that that code is there for a reason: When this is first called for >> cpu 0, cpu_to_socket() (and cpu_to_core()) return garbage since they >> haven't been initialized yet. >> >> That is something that needs to be fixed, but it's not Uma's job to fix it. > > Them returning garbage isn't what needs fixing. Instead the code > here should use a different condition to check whether this is the > boot CPU (e.g. looking at system_state). And that can very well be > done directly in this patch. What do you suggest, then? -George