CGroups: misc cleanups to write_string patchset
This patch contains cleanups suggested by reviewers for the recent
write_string() patchset:
- pair cgroup_lock_live_group() with cgroup_unlock() in cgroup.c for
clarity, rather than directly unlocking cgroup_mutex.
- make the return type of cgroup_lock_live_group() a bool
- use a #define'd constant for the local buffer size in read/write functions
Signed-off-by: Paul Menage <[email protected]>
---
include/linux/cgroup.h | 4 ++--
kernel/cgroup.c | 21 ++++++++++++---------
2 files changed, 14 insertions(+), 11 deletions(-)
Index: cws2-2.6.26-rc5-mm3/include/linux/cgroup.h
===================================================================
--- cws2-2.6.26-rc5-mm3.orig/include/linux/cgroup.h
+++ cws2-2.6.26-rc5-mm3/include/linux/cgroup.h
@@ -21,11 +21,13 @@
struct cgroupfs_root;
struct cgroup_subsys;
struct inode;
+struct cgroup;
extern int cgroup_init_early(void);
extern int cgroup_init(void);
extern void cgroup_init_smp(void);
extern void cgroup_lock(void);
+extern bool cgroup_lock_live_group(struct cgroup *cgrp);
extern void cgroup_unlock(void);
extern void cgroup_fork(struct task_struct *p);
extern void cgroup_fork_callbacks(struct task_struct *p);
@@ -295,8 +297,6 @@ int cgroup_add_files(struct cgroup *cgrp
int cgroup_is_removed(const struct cgroup *cgrp);
-int cgroup_lock_live_group(struct cgroup *cgrp);
-
int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
int cgroup_task_count(const struct cgroup *cgrp);
Index: cws2-2.6.26-rc5-mm3/kernel/cgroup.c
===================================================================
--- cws2-2.6.26-rc5-mm3.orig/kernel/cgroup.c
+++ cws2-2.6.26-rc5-mm3/kernel/cgroup.c
@@ -1331,10 +1331,10 @@ enum cgroup_filetype {
* cgroup_lock_live_group - take cgroup_mutex and check that cgrp is alive.
* @cgrp: the cgroup to be checked for liveness
*
- * Returns true (with lock held) on success, or false (with no lock
- * held) on failure.
+ * On success, returns true; the lock should be later released with
+ * cgroup_unlock(). On failure returns false with no lock held.
*/
-int cgroup_lock_live_group(struct cgroup *cgrp)
+bool cgroup_lock_live_group(struct cgroup *cgrp)
{
mutex_lock(&cgroup_mutex);
if (cgroup_is_removed(cgrp)) {
@@ -1351,7 +1351,7 @@ static int cgroup_release_agent_write(st
if (!cgroup_lock_live_group(cgrp))
return -ENODEV;
strcpy(cgrp->root->release_agent_path, buffer);
- mutex_unlock(&cgroup_mutex);
+ cgroup_unlock();
return 0;
}
@@ -1362,16 +1362,19 @@ static int cgroup_release_agent_show(str
return -ENODEV;
seq_puts(seq, cgrp->root->release_agent_path);
seq_putc(seq, '\n');
- mutex_unlock(&cgroup_mutex);
+ cgroup_unlock();
return 0;
}
+/* A buffer size big enough for numbers or short strings */
+#define CGROUP_LOCAL_BUFFER_SIZE 64
+
static ssize_t cgroup_write_X64(struct cgroup *cgrp, struct cftype *cft,
struct file *file,
const char __user *userbuf,
size_t nbytes, loff_t *unused_ppos)
{
- char buffer[64];
+ char buffer[CGROUP_LOCAL_BUFFER_SIZE];
int retval = 0;
char *end;
@@ -1405,7 +1408,7 @@ static ssize_t cgroup_write_string(struc
const char __user *userbuf,
size_t nbytes, loff_t *unused_ppos)
{
- char local_buffer[64];
+ char local_buffer[CGROUP_LOCAL_BUFFER_SIZE];
int retval = 0;
size_t max_bytes = cft->max_write_len;
char *buffer = local_buffer;
@@ -1459,7 +1462,7 @@ static ssize_t cgroup_read_u64(struct cg
char __user *buf, size_t nbytes,
loff_t *ppos)
{
- char tmp[64];
+ char tmp[CGROUP_LOCAL_BUFFER_SIZE];
u64 val = cft->read_u64(cgrp, cft);
int len = sprintf(tmp, "%llu\n", (unsigned long long) val);
@@ -1471,7 +1474,7 @@ static ssize_t cgroup_read_s64(struct cg
char __user *buf, size_t nbytes,
loff_t *ppos)
{
- char tmp[64];
+ char tmp[CGROUP_LOCAL_BUFFER_SIZE];
s64 val = cft->read_s64(cgrp, cft);
int len = sprintf(tmp, "%lld\n", (long long) val);
Quoting Paul Menage ([email protected]):
> CGroups: misc cleanups to write_string patchset
>
> This patch contains cleanups suggested by reviewers for the recent
> write_string() patchset:
>
> - pair cgroup_lock_live_group() with cgroup_unlock() in cgroup.c for
> clarity, rather than directly unlocking cgroup_mutex.
>
> - make the return type of cgroup_lock_live_group() a bool
>
> - use a #define'd constant for the local buffer size in read/write functions
>
> Signed-off-by: Paul Menage <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
The use of cgroup_unlock() really is much more readable.
thanks,
-serge
>
> ---
> include/linux/cgroup.h | 4 ++--
> kernel/cgroup.c | 21 ++++++++++++---------
> 2 files changed, 14 insertions(+), 11 deletions(-)
>
> Index: cws2-2.6.26-rc5-mm3/include/linux/cgroup.h
> ===================================================================
> --- cws2-2.6.26-rc5-mm3.orig/include/linux/cgroup.h
> +++ cws2-2.6.26-rc5-mm3/include/linux/cgroup.h
> @@ -21,11 +21,13 @@
> struct cgroupfs_root;
> struct cgroup_subsys;
> struct inode;
> +struct cgroup;
>
> extern int cgroup_init_early(void);
> extern int cgroup_init(void);
> extern void cgroup_init_smp(void);
> extern void cgroup_lock(void);
> +extern bool cgroup_lock_live_group(struct cgroup *cgrp);
> extern void cgroup_unlock(void);
> extern void cgroup_fork(struct task_struct *p);
> extern void cgroup_fork_callbacks(struct task_struct *p);
> @@ -295,8 +297,6 @@ int cgroup_add_files(struct cgroup *cgrp
>
> int cgroup_is_removed(const struct cgroup *cgrp);
>
> -int cgroup_lock_live_group(struct cgroup *cgrp);
> -
> int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
>
> int cgroup_task_count(const struct cgroup *cgrp);
> Index: cws2-2.6.26-rc5-mm3/kernel/cgroup.c
> ===================================================================
> --- cws2-2.6.26-rc5-mm3.orig/kernel/cgroup.c
> +++ cws2-2.6.26-rc5-mm3/kernel/cgroup.c
> @@ -1331,10 +1331,10 @@ enum cgroup_filetype {
> * cgroup_lock_live_group - take cgroup_mutex and check that cgrp is alive.
> * @cgrp: the cgroup to be checked for liveness
> *
> - * Returns true (with lock held) on success, or false (with no lock
> - * held) on failure.
> + * On success, returns true; the lock should be later released with
> + * cgroup_unlock(). On failure returns false with no lock held.
> */
> -int cgroup_lock_live_group(struct cgroup *cgrp)
> +bool cgroup_lock_live_group(struct cgroup *cgrp)
> {
> mutex_lock(&cgroup_mutex);
> if (cgroup_is_removed(cgrp)) {
> @@ -1351,7 +1351,7 @@ static int cgroup_release_agent_write(st
> if (!cgroup_lock_live_group(cgrp))
> return -ENODEV;
> strcpy(cgrp->root->release_agent_path, buffer);
> - mutex_unlock(&cgroup_mutex);
> + cgroup_unlock();
> return 0;
> }
>
> @@ -1362,16 +1362,19 @@ static int cgroup_release_agent_show(str
> return -ENODEV;
> seq_puts(seq, cgrp->root->release_agent_path);
> seq_putc(seq, '\n');
> - mutex_unlock(&cgroup_mutex);
> + cgroup_unlock();
> return 0;
> }
>
> +/* A buffer size big enough for numbers or short strings */
> +#define CGROUP_LOCAL_BUFFER_SIZE 64 +
> static ssize_t cgroup_write_X64(struct cgroup *cgrp, struct cftype *cft,
> struct file *file,
> const char __user *userbuf,
> size_t nbytes, loff_t *unused_ppos)
> {
> - char buffer[64];
> + char buffer[CGROUP_LOCAL_BUFFER_SIZE];
> int retval = 0;
> char *end;
>
> @@ -1405,7 +1408,7 @@ static ssize_t cgroup_write_string(struc
> const char __user *userbuf,
> size_t nbytes, loff_t *unused_ppos)
> {
> - char local_buffer[64];
> + char local_buffer[CGROUP_LOCAL_BUFFER_SIZE];
> int retval = 0;
> size_t max_bytes = cft->max_write_len;
> char *buffer = local_buffer;
> @@ -1459,7 +1462,7 @@ static ssize_t cgroup_read_u64(struct cg
> char __user *buf, size_t nbytes,
> loff_t *ppos)
> {
> - char tmp[64];
> + char tmp[CGROUP_LOCAL_BUFFER_SIZE];
> u64 val = cft->read_u64(cgrp, cft);
> int len = sprintf(tmp, "%llu\n", (unsigned long long) val);
>
> @@ -1471,7 +1474,7 @@ static ssize_t cgroup_read_s64(struct cg
> char __user *buf, size_t nbytes,
> loff_t *ppos)
> {
> - char tmp[64];
> + char tmp[CGROUP_LOCAL_BUFFER_SIZE];
> s64 val = cft->read_s64(cgrp, cft);
> int len = sprintf(tmp, "%lld\n", (long long) val);