linux-kernel - Re: [RFC PATCH v1 1/1] sched/fair: select idle cpu from idle cpumask in sched domain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Sep 2020 12:31:13 +0100
From:   Qais Yousef <qais.yousef@....com>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     "Li, Aubrey" <aubrey.li@...ux.intel.com>,
        Aubrey Li <aubrey.li@...el.com>, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        tim.c.chen@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 1/1] sched/fair: select idle cpu from idle cpumask
 in sched domain

On 09/14/20 12:26, Valentin Schneider wrote:
> 
> On 14/09/20 12:08, Qais Yousef wrote:
> > On 09/14/20 11:31, Valentin Schneider wrote:
> >>
> >> On 12/09/20 00:04, Li, Aubrey wrote:
> >> >>> +++ b/include/linux/sched/topology.h
> >> >>> @@ -65,8 +65,21 @@ struct sched_domain_shared {
> >> >>>     atomic_t	ref;
> >> >>>     atomic_t	nr_busy_cpus;
> >> >>>     int		has_idle_cores;
> >> >>> +	/*
> >> >>> +	 * Span of all idle CPUs in this domain.
> >> >>> +	 *
> >> >>> +	 * NOTE: this field is variable length. (Allocated dynamically
> >> >>> +	 * by attaching extra space to the end of the structure,
> >> >>> +	 * depending on how many CPUs the kernel has booted up with)
> >> >>> +	 */
> >> >>> +	unsigned long	idle_cpus_span[];
> >> >>
> >> >> Can't you use cpumask_var_t and zalloc_cpumask_var() instead?
> >> >
> >> > I can use the existing free code. Do we have a problem of this?
> >> >
> >>
> >> Nah, flexible array members are the preferred approach here; this also
> >
> > Is this your opinion or a rule written somewhere I missed?
> 
> I don't think there's a written rule, but AIUI it is preferred by at
> least Peter:
> 
> https://lore.kernel.org/linux-pm/20180612125930.GP12217@hirez.programming.kicks-ass.net/
> https://lore.kernel.org/lkml/20180619110734.GO2458@hirez.programming.kicks-ass.net/
> 
> And my opinion is that, if you can, having fewer separate allocation is better.

+1

> 
> >
> >> means we don't let CONFIG_CPUMASK_OFFSTACK dictate where this gets
> >> allocated.
> >>
> >> See struct numa_group, struct sched_group, struct sched_domain, struct
> >> em_perf_domain...
> >
> > struct root_domain, struct cpupri_vec, struct generic_pm_domain,
> > struct irq_common_data..
> >
> > Use cpumask_var_t.
> >
> > Both approach look correct to me, so no objection in principle. cpumask_var_t
> > looks neater IMO and will be necessary once more than one cpumask are required
> > in a struct.
> >
> 
> You're right in that cpumask_var_t becomes necessary when you need more
> than one mask. For those that use it despite requiring only one mask
> (cpupri stuff, struct nohz too), I'm not sure.

I don't have a strong opinoin. cpumask_var_t is more readable and maintainble
IMO. But it's not a big deal. Any form can be easily changed.

Thanks

--
Qais Yousef