sched: rewrite SCHED_CPUMASK_ALLOC

PATCH Applied Li Zefan 2 participants 6 messages Nov 5, 2008 – Nov 7, 2008

lore

Share & tools

lore

Li Zefan

author

2008-11-05 05:53 UTC #0 lore raw

PATCH 1/1 sched: rewrite SCHED_CPUMASK_ALLOC

The #if/#endif is ugly. Let SCHED_CPUMASK_ALLOC do kmalloc().

Also remove unnecessary pointer casting.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/sched.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

kernel/sched.c +10 ~~-11~~

index e8819bc..dee79ad 100644
@@ -7318,13 +7318,13 @@ struct allmasks {
 };
 
 #if	NR_CPUS > 128
-#define	SCHED_CPUMASK_ALLOC		1
-#define	SCHED_CPUMASK_FREE(v)		kfree(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_ALLOC(v)		v = kmalloc(sizeof(*v), GFP_KERNEL)
+#define SCHED_CPUMASK_FREE(v)		kfree(v)
 #else
-#define	SCHED_CPUMASK_ALLOC		0
-#define	SCHED_CPUMASK_FREE(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_ALLOC(v)
+#define SCHED_CPUMASK_FREE(v)
 #endif
 
 #define	SCHED_CPUMASK_VAR(v, a) 	cpumask_t *v = (cpumask_t *) \

@@ -7400,9 +7400,8 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
 		return -ENOMEM;
 	}
 
-#if SCHED_CPUMASK_ALLOC
 	/* get space for all scratch cpumask variables */
-	allmasks = kmalloc(sizeof(*allmasks), GFP_KERNEL);
+	SCHED_CPUMASK_ALLOC(allmasks);
 	if (!allmasks) {
 		printk(KERN_WARNING "Cannot alloc cpumask array\n");
 		kfree(rd);

@@ -7411,7 +7410,7 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
 #endif
 		return -ENOMEM;
 	}
-#endif
+
 	tmpmask = (cpumask_t *)allmasks;

@@ -7665,13 +7664,13 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
 		cpu_attach_domain(sd, rd, i);
 	}
 
-	SCHED_CPUMASK_FREE((void *)allmasks);
+	SCHED_CPUMASK_FREE(allmasks);
 	return 0;
 
 #ifdef CONFIG_NUMA
 error:
 	free_sched_groups(cpu_map, tmpmask);
-	SCHED_CPUMASK_FREE((void *)allmasks);
+	SCHED_CPUMASK_FREE(allmasks);
 	return -ENOMEM;
 #endif
 }

Signature · 1 line

1.5.4.rc3

Ingo Molnar

2008-11-06 08:01 UTC #1 lore raw

Quoted 5 lines · Li Zefan

#if NR_CPUS > 128

kernel/sched.c +1 -3

-#define	SCHED_CPUMASK_ALLOC		1
-#define	SCHED_CPUMASK_FREE(v)		kfree(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks *v

kernel/sched.c struct allmasks { · line 7328

Quoted 10 lines

kernel/sched.c +5 -3

+#define SCHED_CPUMASK_ALLOC(v)		v = kmalloc(sizeof(*v), GFP_KERNEL)
+#define SCHED_CPUMASK_FREE(v)		kfree(v)
 #else
-#define	SCHED_CPUMASK_ALLOC		0
-#define	SCHED_CPUMASK_FREE(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_ALLOC(v)
+#define SCHED_CPUMASK_FREE(v)
 #endif

ok, the #ifdef removal is nice, but we can make it even cleaner:
please just change it to a sched_cpumask_alloc() inline. That way SCHED_CPUMASK_DECLARE() can go away as well: just declare the variable, it will be unused in the <128 CPUs case.

Ingo

Li Zefan

author

2008-11-07 07:20 UTC #2 lore raw

Quoted 12 lines

#if NR_CPUS > 128

kernel/sched.c +4 -6

-#define	SCHED_CPUMASK_ALLOC		1
-#define	SCHED_CPUMASK_FREE(v)		kfree(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_ALLOC(v)		v = kmalloc(sizeof(*v), GFP_KERNEL)
+#define SCHED_CPUMASK_FREE(v)		kfree(v)
 #else
-#define	SCHED_CPUMASK_ALLOC		0
-#define	SCHED_CPUMASK_FREE(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v

Quoted 9 lines

kernel/sched.c struct allmasks { · line 7328
Quoted 3 lines
kernel/sched.c +2 -0
+#define SCHED_CPUMASK_ALLOC(v)
+#define SCHED_CPUMASK_FREE(v)
 #endif
ok, the #ifdef removal is nice, but we can make it even cleaner:
please just change it to a sched_cpumask_alloc() inline. That way SCHED_CPUMASK_DECLARE() can go away as well: just declare the variable, it will be unused in the <128 CPUs case.

I don't catch you. :(

In the <128 CPUs case, SCHED_CPUMASK_DECLARE is needed to declare a local variable(struct allmasks) and a pointer pointing to it. So I don't know what you mean by 'unused' and how to remove the DECLARE macro..

Ingo Molnar

2008-11-07 08:16 UTC #3 lore raw

Quoted 22 lines · Li Zefan

Quoted 12 lines
Quoted 12 lines
#if NR_CPUS > 128
kernel/sched.c +4 -6
-#define	SCHED_CPUMASK_ALLOC		1
-#define	SCHED_CPUMASK_FREE(v)		kfree(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_ALLOC(v)		v = kmalloc(sizeof(*v), GFP_KERNEL)
+#define SCHED_CPUMASK_FREE(v)		kfree(v)
 #else
-#define	SCHED_CPUMASK_ALLOC		0
-#define	SCHED_CPUMASK_FREE(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
Quoted 9 lines
kernel/sched.c struct allmasks { · line 7328
Quoted 3 lines
kernel/sched.c +2 -0
+#define SCHED_CPUMASK_ALLOC(v)
+#define SCHED_CPUMASK_FREE(v)
 #endif
ok, the #ifdef removal is nice, but we can make it even cleaner:
please just change it to a sched_cpumask_alloc() inline. That way SCHED_CPUMASK_DECLARE() can go away as well: just declare the variable, it will be unused in the <128 CPUs case.

Quoted 6 lines

I don't catch you. :(
In the <128 CPUs case, SCHED_CPUMASK_DECLARE is needed to declare a local variable(struct allmasks) and a pointer pointing to it. So I don't know what you mean by 'unused' and how to remove the DECLARE macro..

Sorry, you are right - keep the SCHED_CPUMASK_DECLARE() macro please.

But the other macros can become an inline function just fine, correct?

Ingo

Li Zefan

author

2008-11-07 09:06 UTC #4 lore raw

Sorry, you are right - keep the SCHED_CPUMASK_DECLARE() macro please.

Quoted 3 lines

But the other macros can become an inline function just fine, correct?

Yes, but sched_cpumask_alloc() should be declared this way:
void sched_cpumask_alloc(struct **allmasks)
but not:
struct *allmasks sched_cpumask_alloc(void)

Because the latter is not workable for <128 CPUs case.

(patch is based on [PATCH] sched: fix memory leak in a failing path)

==============

From: Li Zefan <[email protected]>
Date: Fri, 7 Nov 2008 16:49:47 +0800
Subject: [PATCH] sched: rewrite SCHED_CPUMASK_ALLOC

The #if/#endif is ugly. Change SCHED_CPUMASK_ALLOC and SCHED_CPUMASK_FREE to static inline functions.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/sched.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

kernel/sched.c +18 ~~-11~~

index 24a70d5..fe4f15e 100644
@@ -7318,13 +7318,21 @@ struct allmasks {
 };
 
 #if	NR_CPUS > 128
-#define	SCHED_CPUMASK_ALLOC		1
-#define	SCHED_CPUMASK_FREE(v)		kfree(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks *v
+static inline void sched_cpumask_alloc(struct allmasks **masks)
+{
+	*masks = kmalloc(sizeof(**masks), GFP_KERNEL);
+}
+static inline void sched_cpumask_free(struct allmasks *masks)
+{
+	kfree(masks);
+}
 #else
-#define	SCHED_CPUMASK_ALLOC		0
-#define	SCHED_CPUMASK_FREE(v)
-#define	SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+#define SCHED_CPUMASK_DECLARE(v)	struct allmasks _v, *v = &_v
+static inline void sched_cpumask_alloc(struct allmasks **masks)
+{ }
+static inline void sched_cpumask_free(struct allmasks *masks)
+{ }
 #endif
 
 #define	SCHED_CPUMASK_VAR(v, a) 	cpumask_t *v = (cpumask_t *) \

@@ -7400,9 +7408,8 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
 		return -ENOMEM;
 	}
 
-#if SCHED_CPUMASK_ALLOC
 	/* get space for all scratch cpumask variables */
-	allmasks = kmalloc(sizeof(*allmasks), GFP_KERNEL);
+	sched_cpumask_alloc(&allmasks);
 	if (!allmasks) {
 		printk(KERN_WARNING "Cannot alloc cpumask array\n");
 		kfree(rd);

@@ -7411,7 +7418,7 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
 #endif
 		return -ENOMEM;
 	}
-#endif
+
 	tmpmask = (cpumask_t *)allmasks;

@@ -7665,13 +7672,13 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
 		cpu_attach_domain(sd, rd, i);
 	}
 
-	SCHED_CPUMASK_FREE((void *)allmasks);
+	sched_cpumask_free(allmasks);
 	return 0;
 
 #ifdef CONFIG_NUMA
 error:
 	free_sched_groups(cpu_map, tmpmask);
-	SCHED_CPUMASK_FREE((void *)allmasks);
+	sched_cpumask_free(allmasks);
 	kfree(rd);
 	return -ENOMEM;
 #endif

Signature · 1 line

1.5.4.rc3

Ingo Molnar

Applied

2008-11-07 09:31 UTC #5 lore raw

Quoted 5 lines · Li Zefan

Sorry, you are right - keep the SCHED_CPUMASK_DECLARE() macro please.
Quoted 3 lines
But the other macros can become an inline function just fine, correct?

Quoted 8 lines

Yes, but sched_cpumask_alloc() should be declared this way:
void sched_cpumask_alloc(struct **allmasks)
but not:
struct *allmasks sched_cpumask_alloc(void)
Because the latter is not workable for <128 CPUs case.
(patch is based on [PATCH] sched: fix memory leak in a failing path)

applied to tip/sched/core, thanks!

Ingo