cpuset: Add a missing unlock in cpuset_write_resmask()

PATCH In review Li Zefan 3 participants 4 messages Feb 24, 2011 – Mar 1, 2011 Memory management

lore

Share & tools

lore

Li Zefan

author

2011-02-24 06:57 UTC #0 lore raw

PATCH 1/1 cpuset: Add a missing unlock in cpuset_write_resmask()

Don't forget to release cgroup_mutex if alloc_trial_cpuset() fails.

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

---
 kernel/cpuset.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

kernel/cpuset.c +3 -1

index 1ca786a..6272503 100644
@@ -1561,8 +1561,10 @@ static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft,
 		return -ENODEV;
 
 	trialcs = alloc_trial_cpuset(cs);
-	if (!trialcs)
+	if (!trialcs) {
+		cgroup_unlock();
 		return -ENOMEM;
+	}
 
 	switch (cft->private) {
 	case FILE_CPULIST:

Signature · 1 line

1.7.3.1

David Rientjes

Acked-by on v1 1/1

2011-02-24 07:06 UTC #1 lore raw

Quoted 3 lines · Li Zefan

Don't forget to release cgroup_mutex if alloc_trial_cpuset() fails.
Signed-off-by Li Zefan <[email protected]>

Acked-by David Rientjes <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2011-02-28 23:56 UTC #2 lore raw

In reply to Li Zefan

Quoted 13 lines · On Thu, 24 Feb 2011 14:58:58 +0800 Li Zefan <[email protected]> wrote:

Don't forget to release cgroup_mutex if alloc_trial_cpuset() fails.
Signed-off-by Li Zefan <[email protected]>
---
 kernel/cpuset.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
kernel/cpuset.c +0 -0
index 1ca786a..6272503 100644
@@ -1561,8 +1561,10 @@ static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft,
 		return -ENODEV;

kernel/cpuset.c static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft, · line 1570

Quoted 10 lines

kernel/cpuset.c +3 -1

 
 	trialcs = alloc_trial_cpuset(cs);
-	if (!trialcs)
+	if (!trialcs) {
+		cgroup_unlock();
 		return -ENOMEM;
+	}
 
 	switch (cft->private) {
 	case FILE_CPULIST:

It would be better to avoid multiple returns - it leads to more maintainable code and often shorter code:

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset-add-a-missing-unlock-in-cpuset_write_resmask-fix +3 -2

@@ -1562,8 +1562,8 @@ static int cpuset_write_resmask(struct c
 
 	trialcs = alloc_trial_cpuset(cs);
 	if (!trialcs) {
-		cgroup_unlock();
-		return -ENOMEM;
+		retval = -ENOMEM;
+		goto out;
 	}
 
 	switch (cft->private) {

@@ -1579,6 +1579,7 @@ static int cpuset_write_resmask(struct c
 	}
 
 	free_trial_cpuset(trialcs);
+out:
 	cgroup_unlock();
 	return retval;
 }

also, alloc_trial_cpuset() is a fairly slow-looking function. cpuset_write_resmask() could run alloc_trial_cpuset() before running cgroup_lock_live_group(), thereby reducing lock hold times.

Li Zefan

author

2011-03-01 01:03 UTC #3 lore raw

Quoted 10 lines

(quoted diff) +3 -1

@@ -1561,8 +1561,10 @@ static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft,
 		return -ENODEV;
 
 	trialcs = alloc_trial_cpuset(cs);
-	if (!trialcs)
+	if (!trialcs) {
+		cgroup_unlock();
 		return -ENOMEM;
+	}

Quoted 6 lines

Review static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft, · line 1570
Quoted 2 lines
(quoted diff) +0 -0
 	switch (cft->private) {
 	case FILE_CPULIST:
It would be better to avoid multiple returns - it leads to more maintainable code and often shorter code:

I have no strong opinion on this.

Quoted 23 lines

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset-add-a-missing-unlock-in-cpuset_write_resmask-fix +3 -2

@@ -1562,8 +1562,8 @@ static int cpuset_write_resmask(struct c
 
 	trialcs = alloc_trial_cpuset(cs);
 	if (!trialcs) {
-		cgroup_unlock();
-		return -ENOMEM;
+		retval = -ENOMEM;
+		goto out;
 	}
 
 	switch (cft->private) {

@@ -1579,6 +1579,7 @@ static int cpuset_write_resmask(struct c
 	}
 
 	free_trial_cpuset(trialcs);
+out:
 	cgroup_unlock();
 	return retval;
 }

Quoted 4 lines

also, alloc_trial_cpuset() is a fairly slow-looking function. cpuset_write_resmask() could run alloc_trial_cpuset() before running cgroup_lock_live_group(), thereby reducing lock hold times.

Nope. alloc_trial_cpuset() will read 'cs', so it must be protected by the lock.