Date:   Fri, 10 Feb 2023 14:55:57 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Tim Chen <tim.c.chen@linux.intel.com>
CC:     "Chen, Tim C" <tim.c.chen@intel.com>,
        Ricardo Neri <ricardo.neri-calderon@linux.intel.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        "Neri, Ricardo" <ricardo.neri@intel.com>,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>,
        Ben Segall <bsegall@google.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        "Brown, Len" <len.brown@intel.com>, Mel Gorman <mgorman@suse.de>,
        "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Valentin Schneider <vschneid@redhat.com>,
        Ionela Voinescu <ionela.voinescu@arm.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 06/10] sched/fair: Use the prefer_sibling flag of the
 current sched domain
Message-ID: <Y+XqfXVHyqrv1/Ae@chenyu5-mobl1>
References: <20230207045838.11243-1-ricardo.neri-calderon@linux.intel.com>
 <20230207045838.11243-7-ricardo.neri-calderon@linux.intel.com>
 <Y+TyUZg86LYhtWeJ@chenyu5-mobl1>
 <DM6PR11MB4107F95164772B40B92F66A6DCD99@DM6PR11MB4107.namprd11.prod.outlook.com>
 <fb63369b2fc686730adefb6800eae4877e62e3b6.camel@linux.intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <fb63369b2fc686730adefb6800eae4877e62e3b6.camel@linux.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?3L5tdx467ADPTfw0zotRMCfN3lmw5lrDgn/XflgvhT5vIxYbPpioHi+n7X?=
 =?iso-8859-1?Q?qFAVICzsmie3504GLYjPkOCbWQFWn7/YbdBLfHdVr5Iphsar/CtTZZtT26?=
 =?iso-8859-1?Q?SdnIM0YmhQtleHNSM7ueYN+g3WqyQ+TfTRFpc3tmZYha2Kr06sk2/cyjRG?=
 =?iso-8859-1?Q?1eD3igBCARlFDpcUmG2ffyzd1/h5u5IfcrlIn/1qvejKtvwMrnx71w2yYx?=
 =?iso-8859-1?Q?hiU7sX1jXbSPHjvgSVEimbRva3grmnccF6EfZa51cpvS9szv1x8GlRRt6A?=
 =?iso-8859-1?Q?XHULWDJWFrBlMTmNf1Rv049B1yjMgN+M8LP7d5KYvKzoVdroFFbkuS3cB8?=
 =?iso-8859-1?Q?W1rFivr9Q8fArz1p6yw4NJpzHdiMOelxU3En1gBuHW+O7vT3WZPd7QwQt4?=
 =?iso-8859-1?Q?KTAmYjqOLh/CqcVQKNKdNYrvpQIupjT7T+3GM/87OT0oKowX32XTfgT3Nq?=
 =?iso-8859-1?Q?xpI54OeUJs9cWDAZKBlEb0EHjqc0lAsj5+UWAjsZ9kQjvR84KNVjqKsvar?=
 =?iso-8859-1?Q?HnoybV8kPJHX8MaKtHKYKBep5Sby5YZt31UNx7W9REI03eARS7RKBfsIrv?=
 =?iso-8859-1?Q?PBf2/FXgcYFqsnjA0AAGLPK3Hzbod/SyhqKr1AGh7y0utHkYLMO9HBPSHc?=
 =?iso-8859-1?Q?9GHC8WvLXdQIS8t4yCeCc3Yil3g2F+8OL2PBUtjnT3u1YpSAL7tsIop7gD?=
 =?iso-8859-1?Q?VAoloDx+YjqocQt3hWGjwRB8GXtqlD682crviJ8Kj4OCZLg8x2JYyxd1Kj?=
 =?iso-8859-1?Q?rmOC9uj1MHF7QW7F2otkvBekdMDa+xr6D8TdbE3pycp77TUBC9HlVOHwRI?=
 =?iso-8859-1?Q?Lbv1et1/z/eUY4d6a8pMUF+4JKMv/Om5V74m4r2PhHUQdCTi8VlCbvh8em?=
 =?iso-8859-1?Q?7h2qkD8lWTMjuYFcJmvwXgdfzB4S3Y74xDgu/gjAk6uBcVZSHZ9HjHGmsd?=
 =?iso-8859-1?Q?6hfkrRyTWABKEeBQYaITGcALbg4FCNEZTdhKZy2vrxMtuV1f0FqdMt/0jH?=
 =?iso-8859-1?Q?txZO7YrwKehfFPIXfvaqM3EN99GQKzKgMKtmlGMBXOObSFjd3+XNOERd/D?=
 =?iso-8859-1?Q?fCEmUipu1cyE7F6phRfYH9iXIumaS/w7aMiCiWKJ5zv2/gpc4keDbB9SUZ?=
 =?iso-8859-1?Q?Hz7xz6g+umLXONHsGoHkKwXnRnxHth8XBW9kBDazPNZKiejOEhaVuFWhST?=
 =?iso-8859-1?Q?Jj58IHSgxNjWs4XAvPUiug1Iv/GJGEvMLL53qem4jpoV4hbfiI5NsOGZQL?=
 =?iso-8859-1?Q?9MTgxTGrfGFo0nIsPh2Vi3kCQ/OgaMT4MZhX8VA0UMwm1584B/8CYQ3uB1?=
 =?iso-8859-1?Q?VB5JKa34Zf4Y6CkwDpTZ3bN7TO5dB80jsowJnwnzHG66lq9nTecVMs6B2r?=
 =?iso-8859-1?Q?1az61ninG2ZBkWnhEmC6ohfN1HU2Z/mFUbH0+1F6TBomm+R6lth1xf2UHw?=
 =?iso-8859-1?Q?gzQajkWNV6UTo1c0qQMLHyP6thTx6YwOfNX8NaZjS/q3YALXEazQnqanAL?=
 =?iso-8859-1?Q?+ONBKfU+cqHeP/rb/ZidF9JFnwoUqNBT2G4a3cQwtUT8wxY3NZrRWrUfRm?=
 =?iso-8859-1?Q?3vnMb/4HWHo9s7nhWZQUMRN78XR3+2gOYcq/mFFeJB7Y53uQ+ythc+x/ok?=
 =?iso-8859-1?Q?UHDGaxl8h9XBnT15UdB9OhIUY/dXf86nj6?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6f80d89c-4b5a-4036-568c-08db0b33e865
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Feb 2023 06:56:17.5124
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: odIdToYfBeILZlVd95IKZAvnfGIsrMEWG+G+8wAsYdjksCia6ZXAGV/X5Bj7GqyeCCSVXeKfkQGJ4n7PeANXAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7688
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2023-02-09 at 15:05:03 -0800, Tim Chen wrote:
> On Thu, 2023-02-09 at 20:00 +0000, Chen, Tim C wrote:
> > > > ?static inline void update_sd_lb_stats(struct lb_env *env, struct
> > > > sd_lb_stats *sds)? {
> > > > -???????struct sched_domain *child = env->sd->child;
> > > > ????????struct sched_group *sg = env->sd->groups;
> > > > ????????struct sg_lb_stats *local = &sds->local_stat;
> > > > ????????struct sg_lb_stats tmp_sgs;
> > > > @@ -10045,9 +10044,11 @@ static inline void
> > > > update_sd_lb_stats(struct
> > > lb_env *env, struct sd_lb_stats *sd
> > > > ????????????????sg = sg->next;
> > > > ????????} while (sg != env->sd->groups);
> > > > 
> > > > -???????/* Tag domain that child domain prefers tasks go to
> > > > siblings first */
> > > > -???????sds->prefer_sibling = child && child->flags &
> > > > SD_PREFER_SIBLING;
> > > > -
> > > > +???????/*
> > > > +??????? * Tag domain that @env::sd prefers to spread excess
> > > > tasks among
> > > > +??????? * sibling sched groups.
> > > > +??????? */
> > > > +???????sds->prefer_sibling = env->sd->flags & SD_PREFER_SIBLING;
> > > > 
> > > This does help fix the issue that non-SMT core fails to pull task
> > > from busy SMT-
> > > cores.
> > > And it also semantically changes the definination of prefer
> > > sibling. Do we also
> > > need to change this:
> > > ?????? if ((sd->flags & SD_ASYM_CPUCAPACITY) && sd->child)
> > > ?????????????? sd->child->flags &= ~SD_PREFER_SIBLING; might be:
> > > ?????? if ((sd->flags & SD_ASYM_CPUCAPACITY))
> > > ?????????????? sd->flags &= ~SD_PREFER_SIBLING;
> > > 
> > 
> > Yu,
> > 
> > I think you are talking about the code in sd_init() 
> > where SD_PREFER_SIBLING is first set
> > to "ON" and updated depending on SD_ASYM_CPUCAPACITY.? The intention
> > of the code
> > is if there are cpus in the scheduler domain that have differing cpu
> > capacities,
> > we do not want to do spreading among the child groups in the sched
> > domain.
> > So the flag is turned off in the child group level and not the parent
> > level. But with your above
> > change, the parent's flag is turned off, leaving the child level flag
> > on. 
> > This moves the level where spreading happens (SD_PREFER_SIBLING on) 
> > up one level which is undesired (see table below).
> >
Yes, it moves the flag 1 level up. And if I understand correctly, with Ricardo's patch
applied, we have changed the original meaning of SD_PREFER_SIBLING:
Original: Tasks in this sched domain want to be migrated to another sched domain.
After init change: Tasks in the sched group under this sched domain want to
                   be migrated to a sibling group.
> > 
> Sorry got a bad mail client messing up the table format.  Updated below
> 
> 			SD_ASYM_CPUCAPACITY	SD_PREFER_SIBLING after init             
> 						original code 	proposed
> SD Level		 	 
> root			ON			ON		OFF      (note: SD_PREFER_SIBLING unused at this level)
SD_PREFER_SIBLING is hornored in root level after the init proposal.
> first level             ON			OFF		OFF
Before the init proposed, tasks in first level sd do not want
to be spreaded to a sibling sd. After the init proposeal, tasks
in all sched groups under root sd, do not want to be spreaded
to a sibling sched group(AKA first level sd)

thanks,
Chenyu
> second level		OFF			OFF		ON
> third level             OFF			ON		ON
> 
> Tim