[PATCH] IPC: account for kmem usage on mqueue and msg

[PATCH] IPC: account for kmem usage on mqueue and msg

[Aristeu Rozanski]

When kmem accounting switched from account by default to only account if
flagged by __GFP_ACCOUNT, IPC mqueue and messages was left out.

Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Alexey Dobriyan <adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Vladimir Davydov <vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Stefan Schimanski <sttts-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reported-by: Stefan Schimanski <sttts-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 ipc/msgutil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index a521999..bf74eaa 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -53,7 +53,7 @@ static struct msg_msg *alloc_msg(size_t len)
 	size_t alen;
 
 	alen = min(len, DATALEN_MSG);
-	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL);
+	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL_ACCOUNT);
 	if (msg == NULL)
 		return NULL;
 
@@ -65,7 +65,7 @@ static struct msg_msg *alloc_msg(size_t len)
 	while (len > 0) {
 		struct msg_msgseg *seg;
 		alen = min(len, DATALEN_SEG);
-		seg = kmalloc(sizeof(*seg) + alen, GFP_KERNEL);
+		seg = kmalloc(sizeof(*seg) + alen, GFP_KERNEL_ACCOUNT);
 		if (seg == NULL)
 			goto out_err;
 		*pseg = seg;
-- 
1.8.3.1

Re: [PATCH] IPC: account for kmem usage on mqueue and msg

[Michal Hocko]

On Tue 18-10-16 11:54:35, Aristeu Rozanski wrote:
> When kmem accounting switched from account by default to only account if
> flagged by __GFP_ACCOUNT, IPC mqueue and messages was left out.

I guess this was because ipv msgqueue shouldn't allow for memory
consumption runaways. The number of message queues and messages pending
in them is bounded and not a large amount of memory AFAIR. Anyway I do
not see anything unreasonable about accounting the memory to the caller.

Have you noticed this by a code inspection or you have a real world
usecase where the missing accounting caused some problems? This would be
a useful information for the changelog.

> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Alexey Dobriyan <adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org>
> Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Cc: Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Vladimir Davydov <vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Stefan Schimanski <sttts-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reported-by: Stefan Schimanski <sttts-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I am not familiar with this code enough to give my ack but I do not see
anything obviously wrong either.

> ---
>  ipc/msgutil.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ipc/msgutil.c b/ipc/msgutil.c
> index a521999..bf74eaa 100644
> --- a/ipc/msgutil.c
> +++ b/ipc/msgutil.c
> @@ -53,7 +53,7 @@ static struct msg_msg *alloc_msg(size_t len)
>  	size_t alen;
>  
>  	alen = min(len, DATALEN_MSG);
> -	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL);
> +	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL_ACCOUNT);
>  	if (msg == NULL)
>  		return NULL;
>  
> @@ -65,7 +65,7 @@ static struct msg_msg *alloc_msg(size_t len)
>  	while (len > 0) {
>  		struct msg_msgseg *seg;
>  		alen = min(len, DATALEN_SEG);
> -		seg = kmalloc(sizeof(*seg) + alen, GFP_KERNEL);
> +		seg = kmalloc(sizeof(*seg) + alen, GFP_KERNEL_ACCOUNT);
>  		if (seg == NULL)
>  			goto out_err;
>  		*pseg = seg;
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] IPC: account for kmem usage on mqueue and msg

[Aristeu Rozanski]

Hi Michal,
On Tue, Oct 18, 2016 at 08:26:19PM +0200, Michal Hocko wrote:
> I guess this was because ipv msgqueue shouldn't allow for memory
> consumption runaways. The number of message queues and messages pending
> in them is bounded and not a large amount of memory AFAIR. Anyway I do
> not see anything unreasonable about accounting the memory to the caller.
> 
> Have you noticed this by a code inspection or you have a real world
> usecase where the missing accounting caused some problems? This would be
> a useful information for the changelog.

Stefan noticed this and filled an internal BZ and I saw it with a
test program while comparing with our 3.10 kernel. We haven't hit this
issue in production (AFAIK) but it's a concern while using containers.

Each message is up to 16MB and you can't get too many messages queued.
While it's something that won't allow a container to eat a large portion
of memory, it's significant enough to be accounted for.

-- 
Aristeu

Re: [PATCH] IPC: account for kmem usage on mqueue and msg

[Alexey Dobriyan]

On Tue, Oct 18, 2016 at 08:26:19PM +0200, Michal Hocko wrote:
> On Tue 18-10-16 11:54:35, Aristeu Rozanski wrote:
> > When kmem accounting switched from account by default to only account if
> > flagged by __GFP_ACCOUNT, IPC mqueue and messages was left out.
> 
> I guess this was because ipv msgqueue shouldn't allow for memory
> consumption runaways. The number of message queues and messages pending
> in them is bounded and not a large amount of memory AFAIR. Anyway I do
> not see anything unreasonable about accounting the memory to the caller.

It should be accounted because receiver may not intentionally or unintentionally
consume messages.

Re: [PATCH] IPC: account for kmem usage on mqueue and msg

[Michal Hocko]

On Wed 19-10-16 09:46:58, Stefan Schimanski wrote:
> The production use case at hand is that mqueues should be customizable via
> sysctls in Docker containers in a Kubernetes cluster. This can only be
> safely allowed to the users of the cluster (without the risk that they can
> cause resource shortage on a node, influencing other users' containers) if
> all resources they control are bounded, i.e. accounted for.

THis is a valuable information and should be a part of the changelog
IMHO. 

> On Tue, Oct 18, 2016 at 9:30 PM, Alexey Dobriyan <adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> wrote:
> 
> > On Tue, Oct 18, 2016 at 08:26:19PM +0200, Michal Hocko wrote:
> > > On Tue 18-10-16 11:54:35, Aristeu Rozanski wrote:
> > > > When kmem accounting switched from account by default to only account
> > if
> > > > flagged by __GFP_ACCOUNT, IPC mqueue and messages was left out.
> > >
> > > I guess this was because ipv msgqueue shouldn't allow for memory
> > > consumption runaways. The number of message queues and messages pending
> > > in them is bounded and not a large amount of memory AFAIR. Anyway I do
> > > not see anything unreasonable about accounting the memory to the caller.
> >
> > It should be accounted because receiver may not intentionally or
> > unintentionally
> > consume messages.
> >

-- 
Michal Hocko
SUSE Labs