Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 5 Oct 2022 16:38:41 -0700
From:   Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ricardo Neri <ricardo.neri@intel.com>,
        "Ravi V. Shankar" <ravi.v.shankar@intel.com>,
        Ben Segall <bsegall@google.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Len Brown <len.brown@intel.com>, Mel Gorman <mgorman@suse.de>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Valentin Schneider <vschneid@redhat.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org, "Tim C . Chen" <tim.c.chen@intel.com>
Subject: Re: [RFC PATCH 09/23] sched/fair: Use task-class performance score
 to pick the busiest group
Message-ID: <20221005233841.GA29251@ranerica-svr.sc.intel.com>
References: <20220909231205.14009-1-ricardo.neri-calderon@linux.intel.com>
 <20220909231205.14009-10-ricardo.neri-calderon@linux.intel.com>
 <YzLYDPU+upHeUG65@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YzLYDPU+upHeUG65@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Sep 27, 2022 at 01:01:32PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 09, 2022 at 04:11:51PM -0700, Ricardo Neri wrote:
> > update_sd_pick_busiest() keeps on selecting as the busiest group scheduling
> > groups of identical priority. Since both groups have the same priority,
> > either group is a good choice. The classes of tasks in the scheduling
> > groups can break this tie.
> > 
> > Pick as busiest the scheduling group that yields a higher task-class
> > performance score after load balancing.
> 
> > +/**
> > + * sched_asym_class_pick - Select a sched group based on classes of tasks
> > + * @a:		A scheduling group
> > + * @b:		A second scheduling group
> > + * @a_stats:	Load balancing statistics of @a
> > + * @b_stats:	Load balancing statistics of @b
> > + *
> > + * Returns: true if @a has the same priority and @a has classes of tasks that
> > + * yield higher overall throughput after load balance. Returns false otherwise.
> > + */
> > +static bool sched_asym_class_pick(struct sched_group *a,
> > +				  struct sched_group *b,
> > +				  struct sg_lb_stats *a_stats,
> > +				  struct sg_lb_stats *b_stats)
> > +{
> > +	/*
> > +	 * Only use the class-specific preference selection if both sched
> > +	 * groups have the same priority.
> > +	 */
> > +	if (arch_asym_cpu_priority(a->asym_prefer_cpu) !=
> > +	    arch_asym_cpu_priority(b->asym_prefer_cpu))
> > +		return false;
> > +
> > +	return sched_asym_class_prefer(a_stats, b_stats);
> > +}
> > +
> >  #else /* CONFIG_SCHED_TASK_CLASSES */
> >  static void update_rq_task_classes_stats(struct sg_lb_task_class_stats *class_sgs,
> >  					 struct rq *rq)
> 
> > @@ -9049,6 +9111,12 @@ static bool update_sd_pick_busiest(struct lb_env *env,
> >  		/* Prefer to move from lowest priority CPU's work */
> >  		if (sched_asym_prefer(sg->asym_prefer_cpu, sds->busiest->asym_prefer_cpu))
> >  			return false;
> > +
> > +		/* @sg and @sds::busiest have the same priority. */
> > +		if (sched_asym_class_pick(sds->busiest, sg, &sds->busiest_stat, sgs))
> > +			return false;
> > +
> > +		/* @sg has lower priority than @sds::busiest. */
> >  		break;
> >  
> >  	case group_misfit_task:
> 
> So why does only this one instance of asym_prefer() require tie
> breaking?

This is the only place in which two sched groups with running tasks and of
equal priority are compared.

In all other places sched_asym_prefer() is used to compare the destination
CPU with others. Since asym_packing is done only when the destination CPU is
idle, there is no need to break this tie.

> 
> I must also re-iterate how much I hate having two different means of
> dealing with big-little topologies.

Yes, that is true. We discussed the challenges of using EAS on x86 during
this year's Linux Plumbers Conference. For the discussion at hand, I think
that the most relevant part is the difficulty to assess CPU capacity
on Intel processors given the presence of SMT, a large range of turbo
frequencies and hardware-controlled frequency scaling.

We are looking into these challenges.

This patchset focuses mainly on the infrastructure to support IPC classes of
tasks in the scheduler. We use it on this tie break when using asym_packing,
but it could be used in capacity computations.

> 
> And while looking through this, I must ask about the comment that goes
> with sched_set_itmt_core_prio() vs the sg->asym_prefer_cpu assignment in
> init_sched_groups_capacity(), what-up ?!

Are you referring to this comment?

	"No need to rebuild sched domain after updating
	 the CPU priorities. The sched domains have no
	 dependency on CPU priorities"

If yes, then it looks wrong to me. Sched domains are rebuilt after updating
priorities.

Thanks and BR,
Ricardo