[PATCH 1/2] cpuset: remove an unncessary forward declaration

[PATCH 1/2] cpuset: remove an unncessary forward declaration

[Li Zefan]

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cpuset.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index bf69717..9d8d637 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -68,9 +68,6 @@
  */
 int number_of_cpusets __read_mostly;
 
-/* Forward declare cgroup structures */
-struct cgroup_subsys cpuset_subsys;
-
 /* See "Frequency meter" comments, below. */
 
 struct fmeter {
-- 
1.8.0.2

[PATCH 2/2] cpuset: remove redundant checks in file write functions

[Li Zefan]

Now cgroup core gets a reference to the css when a cgroup file is
opened(), and the reference is dropped at file release. so it's
guaranteed the cpuset is online during the write function.

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cpuset.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 9d8d637..8c4b3c4 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1589,8 +1589,6 @@ static int cpuset_write_u64(struct cgroup_subsys_state *css, struct cftype *cft,
 	int retval = -ENODEV;
 
 	mutex_lock(&cpuset_mutex);
-	if (!is_cpuset_online(cs))
-		goto out_unlock;
 
 	switch (type) {
 	case FILE_CPU_EXCLUSIVE:
@@ -1624,7 +1622,7 @@ static int cpuset_write_u64(struct cgroup_subsys_state *css, struct cftype *cft,
 		retval = -EINVAL;
 		break;
 	}
-out_unlock:
+
 	mutex_unlock(&cpuset_mutex);
 	return retval;
 }
@@ -1637,8 +1635,6 @@ static int cpuset_write_s64(struct cgroup_subsys_state *css, struct cftype *cft,
 	int retval = -ENODEV;
 
 	mutex_lock(&cpuset_mutex);
-	if (!is_cpuset_online(cs))
-		goto out_unlock;
 
 	switch (type) {
 	case FILE_SCHED_RELAX_DOMAIN_LEVEL:
@@ -1648,7 +1644,7 @@ static int cpuset_write_s64(struct cgroup_subsys_state *css, struct cftype *cft,
 		retval = -EINVAL;
 		break;
 	}
-out_unlock:
+
 	mutex_unlock(&cpuset_mutex);
 	return retval;
 }
@@ -1677,8 +1673,6 @@ static int cpuset_write_resmask(struct cgroup_subsys_state *css,
 	flush_work(&cpuset_hotplug_work);
 
 	mutex_lock(&cpuset_mutex);
-	if (!is_cpuset_online(cs))
-		goto out_unlock;
 
 	trialcs = alloc_trial_cpuset(cs);
 	if (!trialcs) {
-- 
1.8.0.2

Re: [PATCH 2/2] cpuset: remove redundant checks in file write functions

[Li Zefan]

On 2013/8/13 9:17, Li Zefan wrote:
> Now cgroup core gets a reference to the css when a cgroup file is
> opened(), and the reference is dropped at file release. so it's
> guaranteed the cpuset is online during the write function.
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>
> ---
>  kernel/cpuset.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 

Please don't apply this patch. I'll send a new one when the bug
fix I just sent is accepted.

Re: [PATCH 1/2] cpuset: remove an unncessary forward declaration

[Tejun Heo]

On Tue, Aug 13, 2013 at 09:17:33AM +0800, Li Zefan wrote:
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>

Applied to cgroup/for-3.12.

Thanks.

-- 
tejun

Re: [PATCH 2/2] cpuset: remove redundant checks in file write functions

[Tejun Heo]

On Tue, Aug 13, 2013 at 09:17:53AM +0800, Li Zefan wrote:
> Now cgroup core gets a reference to the css when a cgroup file is
> opened(), and the reference is dropped at file release. so it's
> guaranteed the cpuset is online during the write function.

Hmmm... it doesn't really guarantee that as css's can be offlined with
residual css refcnts, os the css may well be offlined by the time it
reaches the rw functions.  What's guaranteed is that their refcnts
wouldn't be zero.  Eventually we need to implement proper sever
semantics (probably by replacing the custom fs implementation with
sysfs) but right now controllers still need to deal with offline
css's.

Thanks.

-- 
tejun

Re: [PATCH 2/2] cpuset: remove redundant checks in file write functions

[Li Zefan]

On 2013/8/13 23:05, Tejun Heo wrote:
> On Tue, Aug 13, 2013 at 09:17:53AM +0800, Li Zefan wrote:
>> Now cgroup core gets a reference to the css when a cgroup file is
>> opened(), and the reference is dropped at file release. so it's
>> guaranteed the cpuset is online during the write function.
> 
> Hmmm... it doesn't really guarantee that as css's can be offlined with
> residual css refcnts, os the css may well be offlined by the time it
> reaches the rw functions.  What's guaranteed is that their refcnts
> wouldn't be zero.

Oh, right.

But most controllers don't check this in those read/write functions.
It shoudn't do any harm not checking online/offline status.

> Eventually we need to implement proper sever
> semantics (probably by replacing the custom fs implementation with
> sysfs) but right now controllers still need to deal with offline
> css's.
> 
> Thanks.
> 

Re: [PATCH 2/2] cpuset: remove redundant checks in file write functions

[Tejun Heo]

Hello, Li.

On Wed, Aug 14, 2013 at 10:01:32AM +0800, Li Zefan wrote:
> But most controllers don't check this in those read/write functions.
> It shoudn't do any harm not checking online/offline status.

It depends on the specific controller.  For controllers which make
clear distinction between online and offline states for, say,
hierarchical config propagation, it could lead to buggy behavior if
the function body afterwards assume that the current node is online.
I don't know whether this is the case for the functions converted here
but even if so the patch needs to update the description.

Thanks.

-- 
tejun

Re: [PATCH 2/2] cpuset: remove redundant checks in file write functions

[Li Zefan]

On 2013/8/14 21:30, Tejun Heo wrote:
> Hello, Li.
> 
> On Wed, Aug 14, 2013 at 10:01:32AM +0800, Li Zefan wrote:
>> But most controllers don't check this in those read/write functions.
>> It shoudn't do any harm not checking online/offline status.
> 
> It depends on the specific controller.  For controllers which make
> clear distinction between online and offline states for, say,
> hierarchical config propagation, it could lead to buggy behavior if
> the function body afterwards assume that the current node is online.

In those cases, there must be some comments, otherwise it would be
confusing if and when one needs to check the online/offline states.

> I don't know whether this is the case for the functions converted here
> but even if so the patch needs to update the description.
> 

I'll check for sure.