[PATCH] Allow cpusets to be configured/built on non-SMP systems

[PATCH] Allow cpusets to be configured/built on non-SMP systems

[Paul Menage]

Allow cpusets to be configured/built on non-SMP systems

Currently it's impossible to build cpusets under UML on x86-64, since
cpusets depends on SMP and x86-64 UML doesn't support SMP.

There's code in cpusets that doesn't depend on SMP.  This patch
surrounds the minimum amount of cpusets code with #ifdef CONFIG_SMP in
order to allow cpusets to build/run on UP systems (for testing
purposes under UML).

Signed-off-by: Paul Menage <menage@google.com>

---

 init/Kconfig    |    2 +-
 kernel/cpuset.c |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 92d2c64..3f20aa2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -477,7 +477,7 @@ config CGROUP_DEVICE
 
 config CPUSETS
 	bool "Cpuset support"
-	depends on SMP && CGROUPS
+	depends on CGROUPS
 	help
 	  This option will let you create and manage CPUSETs which
 	  allow dynamically partitioning a system into sets of CPUs and
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index a46d693..b4d12d8 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -517,6 +517,8 @@ static int validate_change(const struct cpuset *cur, const struct cpuset *trial)
 	return 0;
 }
 
+#ifdef CONFIG_SMP
+
 /*
  * Helper routine for generate_sched_domains().
  * Do cpusets a, b have overlapping cpus_allowed masks?
@@ -811,6 +813,16 @@ static void do_rebuild_sched_domains(struct work_struct *unused)
 
 	put_online_cpus();
 }
+#else
+static void do_rebuild_sched_domains(struct work_struct *unused)
+{
+}
+
+static int generate_sched_domains(struct cpumask **domains,
+			struct sched_domain_attr **attributes)
+{
+}
+#endif // CONFIG_SMP
 
 static DECLARE_WORK(rebuild_sched_domains_work, do_rebuild_sched_domains);
 
@@ -1164,8 +1176,10 @@ int current_cpuset_is_being_rebound(void)
 
 static int update_relax_domain_level(struct cpuset *cs, s64 val)
 {
+#ifdef CONFIG_SMP
 	if (val < -1 || val >= SD_LV_MAX)
 		return -EINVAL;
+#endif
 
 	if (val != cs->relax_domain_level) {
 		cs->relax_domain_level = val;

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Li Zefan]

> +static int generate_sched_domains(struct cpumask **domains,
> +			struct sched_domain_attr **attributes)
> +{

return 0;

> +}

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Li Zefan]

Paul Menage wrote:
> Allow cpusets to be configured/built on non-SMP systems
> 
> Currently it's impossible to build cpusets under UML on x86-64, since
> cpusets depends on SMP and x86-64 UML doesn't support SMP.
> 
> There's code in cpusets that doesn't depend on SMP.  This patch
> surrounds the minimum amount of cpusets code with #ifdef CONFIG_SMP in
> order to allow cpusets to build/run on UP systems (for testing
> purposes under UML).
> 

The patch is ok for this purpose.

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

> Signed-off-by: Paul Menage <menage@google.com>
> 
> ---
> 
>  init/Kconfig    |    2 +-
>  kernel/cpuset.c |   14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletions(-)
...
> +static int generate_sched_domains(struct cpumask **domains,
> +			struct sched_domain_attr **attributes)
> +{

Except here should "return 0;", otherwise emit a compile warining.

> +}

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Paul Menage]

On Mon, Mar 2, 2009 at 7:17 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> +static int generate_sched_domains(struct cpumask **domains,
>> +                     struct sched_domain_attr **attributes)
>> +{
>
> Except here should "return 0;", otherwise emit a compile warining.
>

Good catch - the weird thing is that (in my UML build) it doesn't
actually generate that warning. Mysterious.

I'll resend with the extra return.

Paul

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Paul Menage]

On Mon, Mar 2, 2009 at 10:01 PM, Paul Menage <menage@google.com> wrote:
> On Mon, Mar 2, 2009 at 7:17 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>> +static int generate_sched_domains(struct cpumask **domains,
>>> +                     struct sched_domain_attr **attributes)
>>> +{
>>
>> Except here should "return 0;", otherwise emit a compile warining.
>>
>
> Good catch - the weird thing is that (in my UML build) it doesn't
> actually generate that warning. Mysterious.

Hmm, my guess is that since UML doesn't have hotplug either, the only
reference to generate_sched_domains doesn't actually invoke it, so gcc
doesn't even bother compiling it.

Paul

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Paul Menage]

On Mon, Mar 2, 2009 at 10:01 PM, Paul Menage <menage@google.com> wrote:
> On Mon, Mar 2, 2009 at 7:17 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>> +static int generate_sched_domains(struct cpumask **domains,
>>> +                     struct sched_domain_attr **attributes)
>>> +{
>>
>> Except here should "return 0;", otherwise emit a compile warining.
>>
>
> Good catch - the weird thing is that (in my UML build) it doesn't
> actually generate that warning. Mysterious.
>
> I'll resend with the extra return.

After looking at the sched domains code it's not clear to me that
returning 0 is necessarily the right thing to do -
partition_sched_domains() says that 0 is a special case used for
destroying existing domains? Would returning 1 and setting up a single
dummy domain be better?

Given that this return code only matters when CONFIG_HOTPLUG_CPU &&
!CONFIG_SMP it's unlikely to ever be used, but I guess it's better to
get it right.

Paul

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Li Zefan]

Paul Menage wrote:
> On Mon, Mar 2, 2009 at 10:01 PM, Paul Menage <menage@google.com> wrote:
>> On Mon, Mar 2, 2009 at 7:17 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>>> +static int generate_sched_domains(struct cpumask **domains,
>>>> +                     struct sched_domain_attr **attributes)
>>>> +{
>>> Except here should "return 0;", otherwise emit a compile warining.
>>>
>> Good catch - the weird thing is that (in my UML build) it doesn't
>> actually generate that warning. Mysterious.
>>
>> I'll resend with the extra return.
> 
> After looking at the sched domains code it's not clear to me that
> returning 0 is necessarily the right thing to do -
> partition_sched_domains() says that 0 is a special case used for
> destroying existing domains? Would returning 1 and setting up a single
> dummy domain be better?
> 

Yes, return 1 seems more reasonable. And if we do this, should we also set
*domains to NULL like this?

static int generate_sched_domains(struct cpumask **domains,
                     struct sched_domain_attr **attributes)
{
	*domains = NULL;
	return 1;
}

because otherwise partition_sched_domains() will access invalid memory:

void partition_sched_domains(int ndoms_new, struct cpumask *doms_new,
			     struct sched_domain_attr *dattr_new)
{
	...
	n = doms_new ? ndoms_new : 0;

	for (i = 0; i < ndoms_cur; i++) {
		for (j = 0; j < n && !new_topology; j++) {
			// *****here*****/
			if (cpumask_equal(&doms_cur[i], &doms_new[j])
			    && dattrs_equal(dattr_cur, i, dattr_new, j))
				goto match1;
		}
	...
	}

> Given that this return code only matters when CONFIG_HOTPLUG_CPU &&
> !CONFIG_SMP it's unlikely to ever be used

That's why I didn't comment on this.

> but I guess it's better to get it right.
> 

But I agree with you. :)

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Li Zefan]

Li Zefan wrote:
> Paul Menage wrote:
>> On Mon, Mar 2, 2009 at 10:01 PM, Paul Menage <menage@google.com> wrote:
>>> On Mon, Mar 2, 2009 at 7:17 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>>>> +static int generate_sched_domains(struct cpumask **domains,
>>>>> +                     struct sched_domain_attr **attributes)
>>>>> +{
>>>> Except here should "return 0;", otherwise emit a compile warining.
>>>>
>>> Good catch - the weird thing is that (in my UML build) it doesn't
>>> actually generate that warning. Mysterious.
>>>
>>> I'll resend with the extra return.
>> After looking at the sched domains code it's not clear to me that
>> returning 0 is necessarily the right thing to do -
>> partition_sched_domains() says that 0 is a special case used for
>> destroying existing domains? Would returning 1 and setting up a single
>> dummy domain be better?
>>
> 

partition_sched_domains() says (0, NULL, ...) is used for destroying existing
domains, (1, NULL, ...) will fallback to the single default domain.

But partition_sched_domains() is a stub if !CONFIG_SMP

> Yes, return 1 seems more reasonable. And if we do this, should we also set
> *domains to NULL like this?
> 
> static int generate_sched_domains(struct cpumask **domains,
>                      struct sched_domain_attr **attributes)
> {
> 	*domains = NULL;
> 	return 1;
> }
> 

Re: [PATCH] Allow cpusets to be configured/built on non-SMP systems

[Ingo Molnar]

* Paul Menage <menage@google.com> wrote:

> Allow cpusets to be configured/built on non-SMP systems
> 
> Currently it's impossible to build cpusets under UML on x86-64, since
> cpusets depends on SMP and x86-64 UML doesn't support SMP.
> 
> There's code in cpusets that doesn't depend on SMP.  This patch
> surrounds the minimum amount of cpusets code with #ifdef CONFIG_SMP in
> order to allow cpusets to build/run on UP systems (for testing
> purposes under UML).
> 
> Signed-off-by: Paul Menage <menage@google.com>
> 
> ---
> 
>  init/Kconfig    |    2 +-
>  kernel/cpuset.c |   14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 92d2c64..3f20aa2 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -477,7 +477,7 @@ config CGROUP_DEVICE
>  
>  config CPUSETS
>  	bool "Cpuset support"
> -	depends on SMP && CGROUPS
> +	depends on CGROUPS
>  	help
>  	  This option will let you create and manage CPUSETs which
>  	  allow dynamically partitioning a system into sets of CPUs and
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index a46d693..b4d12d8 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
>
> @@ -517,6 +517,8 @@ static int validate_change(const struct cpuset *cur, const struct cpuset *trial)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SMP
> +
>  /*
>   * Helper routine for generate_sched_domains().
>   * Do cpusets a, b have overlapping cpus_allowed masks?
> @@ -811,6 +813,16 @@ static void do_rebuild_sched_domains(struct work_struct *unused)
>  
>  	put_online_cpus();
>  }
> +#else

Please use:

> +#else /* !CONFIG_SMP: */

to show that it's the UP branch - even if the #ifdef itself has 
scrolled off from screen already.

> +static void do_rebuild_sched_domains(struct work_struct *unused)
> +{
> +}
> +
> +static int generate_sched_domains(struct cpumask **domains,
> +			struct sched_domain_attr **attributes)
> +{
> +}
> +#endif // CONFIG_SMP

Please dont use C++ comments.

Looks good otherwise. (Btw., please try to Cc: PeterZ and me to 
cpusets patches in the future, especially when it affects the 
scheduler.)

	Ingo

[PATCH] Allow cpusets to be configured/built on non-SMP systems

[Paul Menage]

Allow cpusets to be configured/built on non-SMP systems

Currently it's impossible to build cpusets under UML on x86-64, since
cpusets depends on SMP and x86-64 UML doesn't support SMP.

There's code in cpusets that doesn't depend on SMP.  This patch
surrounds the minimum amount of cpusets code with #ifdef CONFIG_SMP in
order to allow cpusets to build/run on UP systems (for testing
purposes under UML).

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>
Signed-off-by: Paul Menage <menage@google.com>

---

Updated to include an (almost-certainly unused) result from
generate_sched_domains().

 init/Kconfig    |    2 +-
 kernel/cpuset.c |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 92d2c64..3f20aa2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -477,7 +477,7 @@ config CGROUP_DEVICE
 
 config CPUSETS
 	bool "Cpuset support"
-	depends on SMP && CGROUPS
+	depends on CGROUPS
 	help
 	  This option will let you create and manage CPUSETs which
 	  allow dynamically partitioning a system into sets of CPUs and
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index a46d693..321fbf1 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -517,6 +517,7 @@ static int validate_change(const struct cpuset *cur, const struct cpuset *trial)
 	return 0;
 }
 
+#ifdef CONFIG_SMP
 /*
  * Helper routine for generate_sched_domains().
  * Do cpusets a, b have overlapping cpus_allowed masks?
@@ -811,6 +812,18 @@ static void do_rebuild_sched_domains(struct work_struct *unused)
 
 	put_online_cpus();
 }
+#else /* !CONFIG_SMP */
+static void do_rebuild_sched_domains(struct work_struct *unused)
+{
+}
+
+static int generate_sched_domains(struct cpumask **domains,
+			struct sched_domain_attr **attributes)
+{
+	*domains = NULL;
+	return 1;
+}
+#endif /* CONFIG_SMP */
 
 static DECLARE_WORK(rebuild_sched_domains_work, do_rebuild_sched_domains);
 
@@ -1164,8 +1177,10 @@ int current_cpuset_is_being_rebound(void)
 
 static int update_relax_domain_level(struct cpuset *cs, s64 val)
 {
+#ifdef CONFIG_SMP
 	if (val < -1 || val >= SD_LV_MAX)
 		return -EINVAL;
+#endif
 
 	if (val != cs->relax_domain_level) {
 		cs->relax_domain_level = val;