[PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Matt Helsley]

cgroup subsystems may find the O_APPEND and O_NONBLOCK flags useful
in determining the way they handle writes. For now only pass these
flags through the write_string op.

A subsequent patch will make use of the O_NONBLOCK flag in the
cgroup freezer write_string op.

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
Cc: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

(Cc'ing mem controller maintainers in case they find this useful..)
Cc: Balbir Singh <balbir-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h   |    3 ++-
 kernel/cgroup.c          |    5 +++--
 kernel/cgroup_freezer.c  |    5 ++---
 kernel/cpuset.c          |    2 +-
 mm/memcontrol.c          |    2 +-
 security/device_cgroup.c |    2 +-
 6 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 90bba9e..627da35 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -315,8 +315,9 @@ struct cftype {
 	 * buffer of maximum length determined by max_write_len.
 	 * Returns 0 or -ve error code.
 	 */
+#define CFTYPE_F_FLAGS (O_APPEND|O_NONBLOCK)
 	int (*write_string)(struct cgroup *cgrp, struct cftype *cft,
-			    const char *buffer);
+			    unsigned int f_flags, const char *buffer);
 	/*
 	 * trigger() callback can be used to get some kick from the
 	 * userspace, when the actual string written is not important
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b6eadfe..af07d05 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1449,7 +1449,7 @@ bool cgroup_lock_live_group(struct cgroup *cgrp)
 }
 
 static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
-				      const char *buffer)
+				      unsigned int f_flags, const char *buffer)
 {
 	BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
 	if (!cgroup_lock_live_group(cgrp))
@@ -1534,7 +1534,8 @@ static ssize_t cgroup_write_string(struct cgroup *cgrp, struct cftype *cft,
 
 	buffer[nbytes] = 0;     /* nul-terminate */
 	strstrip(buffer);
-	retval = cft->write_string(cgrp, cft, buffer);
+	retval = cft->write_string(cgrp, cft, file->f_flags & CFTYPE_F_FLAGS,
+				   buffer);
 	if (!retval)
 		retval = nbytes;
 out:
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 7925850..c97680f 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -343,9 +343,8 @@ out:
 	return retval;
 }
 
-static int freezer_write(struct cgroup *cgroup,
-			 struct cftype *cft,
-			 const char *buffer)
+static int freezer_write(struct cgroup *cgroup, struct cftype *cft,
+			 unsigned int f_flags, const char *buffer)
 {
 	int retval;
 	enum freezer_state goal_state;
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 7e75a41..de7f397 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1470,7 +1470,7 @@ static int cpuset_write_s64(struct cgroup *cgrp, struct cftype *cft, s64 val)
  * Common handling for a write to a "cpus" or "mems" file.
  */
 static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft,
-				const char *buf)
+				unsigned int f_flags, const char *buf)
 {
 	int retval = 0;
 	struct cpuset *cs = cgroup_cs(cgrp);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index fd4529d..fd9c737 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2072,7 +2072,7 @@ static u64 mem_cgroup_read(struct cgroup *cont, struct cftype *cft)
  * RES_LIMIT.
  */
 static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
-			    const char *buffer)
+			    unsigned int f_flags, const char *buffer)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
 	int type, name;
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index b8186ba..65d32ce 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -428,7 +428,7 @@ handle:
 }
 
 static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
-				  const char *buffer)
+				  unsigned int f_flags, const char *buffer)
 {
 	int retval;
 
-- 
1.5.6.3

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[KAMEZAWA Hiroyuki]

On Mon, 26 Oct 2009 08:15:29 -0700
Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:

> cgroup subsystems may find the O_APPEND and O_NONBLOCK flags useful
> in determining the way they handle writes. For now only pass these
> flags through the write_string op.
> 
> A subsequent patch will make use of the O_NONBLOCK flag in the
> cgroup freezer write_string op.
> 
> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> Cc: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> 
> (Cc'ing mem controller maintainers in case they find this useful..)
> Cc: Balbir Singh <balbir-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

Hmm, maybe useful if we decided to add memory.drop_memory or some...
But now, we have no plan to add backup thread(for asynchronous job).
or some trylock ops.
Anyway, no objection from me if you want this.

-Kame




> ---
>  include/linux/cgroup.h   |    3 ++-
>  kernel/cgroup.c          |    5 +++--
>  kernel/cgroup_freezer.c  |    5 ++---
>  kernel/cpuset.c          |    2 +-
>  mm/memcontrol.c          |    2 +-
>  security/device_cgroup.c |    2 +-
>  6 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 90bba9e..627da35 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -315,8 +315,9 @@ struct cftype {
>  	 * buffer of maximum length determined by max_write_len.
>  	 * Returns 0 or -ve error code.
>  	 */
> +#define CFTYPE_F_FLAGS (O_APPEND|O_NONBLOCK)
>  	int (*write_string)(struct cgroup *cgrp, struct cftype *cft,
> -			    const char *buffer);
> +			    unsigned int f_flags, const char *buffer);
>  	/*
>  	 * trigger() callback can be used to get some kick from the
>  	 * userspace, when the actual string written is not important
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index b6eadfe..af07d05 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1449,7 +1449,7 @@ bool cgroup_lock_live_group(struct cgroup *cgrp)
>  }
>  
>  static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
> -				      const char *buffer)
> +				      unsigned int f_flags, const char *buffer)
>  {
>  	BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
>  	if (!cgroup_lock_live_group(cgrp))
> @@ -1534,7 +1534,8 @@ static ssize_t cgroup_write_string(struct cgroup *cgrp, struct cftype *cft,
>  
>  	buffer[nbytes] = 0;     /* nul-terminate */
>  	strstrip(buffer);
> -	retval = cft->write_string(cgrp, cft, buffer);
> +	retval = cft->write_string(cgrp, cft, file->f_flags & CFTYPE_F_FLAGS,
> +				   buffer);
>  	if (!retval)
>  		retval = nbytes;
>  out:
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 7925850..c97680f 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -343,9 +343,8 @@ out:
>  	return retval;
>  }
>  
> -static int freezer_write(struct cgroup *cgroup,
> -			 struct cftype *cft,
> -			 const char *buffer)
> +static int freezer_write(struct cgroup *cgroup, struct cftype *cft,
> +			 unsigned int f_flags, const char *buffer)
>  {
>  	int retval;
>  	enum freezer_state goal_state;
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 7e75a41..de7f397 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1470,7 +1470,7 @@ static int cpuset_write_s64(struct cgroup *cgrp, struct cftype *cft, s64 val)
>   * Common handling for a write to a "cpus" or "mems" file.
>   */
>  static int cpuset_write_resmask(struct cgroup *cgrp, struct cftype *cft,
> -				const char *buf)
> +				unsigned int f_flags, const char *buf)
>  {
>  	int retval = 0;
>  	struct cpuset *cs = cgroup_cs(cgrp);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index fd4529d..fd9c737 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2072,7 +2072,7 @@ static u64 mem_cgroup_read(struct cgroup *cont, struct cftype *cft)
>   * RES_LIMIT.
>   */
>  static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
> -			    const char *buffer)
> +			    unsigned int f_flags, const char *buffer)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
>  	int type, name;
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index b8186ba..65d32ce 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -428,7 +428,7 @@ handle:
>  }
>  
>  static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
> -				  const char *buffer)
> +				  unsigned int f_flags, const char *buffer)
>  {
>  	int retval;
>  
> -- 
> 1.5.6.3
> 
> 
> 

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> [2009-10-27 09:03:11]:

> On Mon, 26 Oct 2009 08:15:29 -0700
> Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > cgroup subsystems may find the O_APPEND and O_NONBLOCK flags useful
> > in determining the way they handle writes. For now only pass these
> > flags through the write_string op.
> > 
> > A subsequent patch will make use of the O_NONBLOCK flag in the
> > cgroup freezer write_string op.
> > 
> > Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > Cc: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> > Cc: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > 
> > (Cc'ing mem controller maintainers in case they find this useful..)
> > Cc: Balbir Singh <balbir-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> Hmm, maybe useful if we decided to add memory.drop_memory or some...
> But now, we have no plan to add backup thread(for asynchronous job).
> or some trylock ops.
> Anyway, no objection from me if you want this.
> 

I see no issues as well, but no use at the moment right away.


-- 
	Balbir

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Li Zefan]

Matt Helsley wrote:
> cgroup subsystems may find the O_APPEND and O_NONBLOCK flags useful
> in determining the way they handle writes. For now only pass these
> flags through the write_string op.
> 
> A subsequent patch will make use of the O_NONBLOCK flag in the
> cgroup freezer write_string op.
> 
> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> Cc: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> 
> (Cc'ing mem controller maintainers in case they find this useful..)
> Cc: Balbir Singh <balbir-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> ---
>  include/linux/cgroup.h   |    3 ++-
>  kernel/cgroup.c          |    5 +++--
>  kernel/cgroup_freezer.c  |    5 ++---
>  kernel/cpuset.c          |    2 +-
>  mm/memcontrol.c          |    2 +-
>  security/device_cgroup.c |    2 +-
>  6 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 90bba9e..627da35 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -315,8 +315,9 @@ struct cftype {
>  	 * buffer of maximum length determined by max_write_len.
>  	 * Returns 0 or -ve error code.
>  	 */
> +#define CFTYPE_F_FLAGS (O_APPEND|O_NONBLOCK)
>  	int (*write_string)(struct cgroup *cgrp, struct cftype *cft,
> -			    const char *buffer);
> +			    unsigned int f_flags, const char *buffer);

I think maybe it's better to store struct file *file to struct cftype,
so we don't need to change write_string(), write(), write_u64()
and write_s64().

>  	/*
>  	 * trigger() callback can be used to get some kick from the
>  	 * userspace, when the actual string written is not important

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Paul Menage]

On Tue, Oct 27, 2009 at 6:04 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>
> I think maybe it's better to store struct file *file to struct cftype,
> so we don't need to change write_string(), write(), write_u64()
> and write_s64().

We can't do that - multiple open files could be using the same cftype
at the same time. I'd be inclined if necessary to just pass the struct
file* in, rather than risk needing to change it to pass more
parameters later.

Paul

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Paul Menage]

On Tue, Oct 27, 2009 at 11:04 PM, Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, Oct 27, 2009 at 6:04 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>>
>> I think maybe it's better to store struct file *file to struct cftype,
>> so we don't need to change write_string(), write(), write_u64()
>> and write_s64().
>
> We can't do that - multiple open files could be using the same cftype
> at the same time. I'd be inclined if necessary to just pass the struct
> file* in, rather than risk needing to change it to pass more
> parameters later.

And I imagine that the number of handlers that actually make use of
the cftype* is rather small. If we pass the file* to the handler
instead of passing the cftype*, and provide an inline function to get
from the file* to the cftype*, then we can avoid adding an extra
parameter to the handlers.

Paul

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Balbir Singh]

* menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> [2009-10-27 23:06:19]:

> On Tue, Oct 27, 2009 at 11:04 PM, Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, Oct 27, 2009 at 6:04 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> >>
> >> I think maybe it's better to store struct file *file to struct cftype,
> >> so we don't need to change write_string(), write(), write_u64()
> >> and write_s64().
> >
> > We can't do that - multiple open files could be using the same cftype
> > at the same time. I'd be inclined if necessary to just pass the struct
> > file* in, rather than risk needing to change it to pass more
> > parameters later.
> 
> And I imagine that the number of handlers that actually make use of
> the cftype* is rather small. If we pass the file* to the handler
> instead of passing the cftype*, and provide an inline function to get
> from the file* to the cftype*, then we can avoid adding an extra
> parameter to the handlers.
>

This sounds like a reasonable approach except for the cost of
indirection (which I assume is acceptable). 

-- 
	Balbir

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Matt Helsley]

On Tue, Oct 27, 2009 at 11:06:19PM -0700, Paul Menage wrote:
> On Tue, Oct 27, 2009 at 11:04 PM, Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, Oct 27, 2009 at 6:04 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> >>
> >> I think maybe it's better to store struct file *file to struct cftype,
> >> so we don't need to change write_string(), write(), write_u64()
> >> and write_s64().
> >
> > We can't do that - multiple open files could be using the same cftype
> > at the same time. I'd be inclined if necessary to just pass the struct
> > file* in, rather than risk needing to change it to pass more
> > parameters later.
> 
> And I imagine that the number of handlers that actually make use of
> the cftype* is rather small. If we pass the file* to the handler
> instead of passing the cftype*, and provide an inline function to get
> from the file* to the cftype*, then we can avoid adding an extra
> parameter to the handlers.

OK, I've done this for write_string(). I named the inline function
cgroup_file_cftype(). It seems that, if we do this for one, then 
we should do it for all of the cgroup file io ops. Agreed?

Cheers,
	-Matt Helsley

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Li Zefan]

>>>> I think maybe it's better to store struct file *file to struct cftype,
>>>> so we don't need to change write_string(), write(), write_u64()
>>>> and write_s64().
>>> We can't do that - multiple open files could be using the same cftype
>>> at the same time. I'd be inclined if necessary to just pass the struct
>>> file* in, rather than risk needing to change it to pass more
>>> parameters later.
>> And I imagine that the number of handlers that actually make use of
>> the cftype* is rather small. If we pass the file* to the handler
>> instead of passing the cftype*, and provide an inline function to get
>> from the file* to the cftype*, then we can avoid adding an extra
>> parameter to the handlers.
> 
> OK, I've done this for write_string(). I named the inline function
> cgroup_file_cftype(). It seems that, if we do this for one, then 
> we should do it for all of the cgroup file io ops. Agreed?
> 

I think so, to make interfaces consistent.

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Paul Menage]

On Wed, Oct 28, 2009 at 12:24 AM, Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> OK, I've done this for write_string(). I named the inline function
> cgroup_file_cftype(). It seems that, if we do this for one, then
> we should do it for all of the cgroup file io ops. Agreed?

Yes.

Thanks,

Paul

Re: [PATCH 1/2] [RFC] Add file f_flags to cgroup write_string ops

[Matt Helsley]

On Wed, Oct 28, 2009 at 12:50:14PM +0530, Balbir Singh wrote:
> * menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> [2009-10-27 23:06:19]:
> 
> > On Tue, Oct 27, 2009 at 11:04 PM, Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > > On Tue, Oct 27, 2009 at 6:04 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> > >>
> > >> I think maybe it's better to store struct file *file to struct cftype,
> > >> so we don't need to change write_string(), write(), write_u64()
> > >> and write_s64().
> > >
> > > We can't do that - multiple open files could be using the same cftype
> > > at the same time. I'd be inclined if necessary to just pass the struct
> > > file* in, rather than risk needing to change it to pass more
> > > parameters later.
> > 
> > And I imagine that the number of handlers that actually make use of
> > the cftype* is rather small. If we pass the file* to the handler
> > instead of passing the cftype*, and provide an inline function to get
> > from the file* to the cftype*, then we can avoid adding an extra
> > parameter to the handlers.
> >
> 
> This sounds like a reasonable approach except for the cost of
> indirection (which I assume is acceptable). 

Suprisingly few places require the struct cftype *. So they don't pay
any additional cost. Note that the code in kernel/cgroup.c always
fetches the op pointer by fetching and dereferencing the struct cftype *.
So fetching it with an inline shouldn't add much to the data cache
footprint.

The minisclue savings come in two places where struct file * was already
passed.

Cheers,
	-Matt Helsley