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: Fri, 13 Mar 2015 19:13:50 +0000 Message-ID: <550336EE.60909@eu.citrix.com> References: <20150313181109.GA3179@gmail.com> <55032C85.6090805@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55032C85.6090805@citrix.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: Andrew Cooper , Uma Sharma , xen-devel@lists.xen.org Cc: dario.faggioli@citrix.com, George.Dunlap@citrix.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On 03/13/2015 06:29 PM, Andrew Cooper wrote: >> +#define CREDIT2_OPT_RUNQUEUE_CORE 1 >> +#define CREDIT2_OPT_RUNQUEUE_SOCKET 2 > > You can drop _OPT out of the name. It serves only to make the constant > longer. I suggested _OPT so that nobody would be confused into thinking they were used in the core mechanism. > Also, this should probably be an enum as the exact numbers themselves > are not interesting. I also suggested #defines, since there are only 2, and the rest of the code doesn't really use enums. > >> >> int opt_migrate_resist=500; >> integer_param("sched_credit2_migrate_resist", opt_migrate_resist); >> +char __initdata opt_credit2_runqueue_string[10] = "core"; >> +string_param("credit2_runqueue", opt_credit2_runqueue_string); >> +int opt_credit2_runqueue = CREDIT2_OPT_RUNQUEUE_CORE; >> >> /* >> * Useful macros >> @@ -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. -George