Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1264923 > unrolled thread

[PATCH 0/5] memcg/kmem: switch to white list policy

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] memcg/kmem: switch to white list policy Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-07 21:10 +0100
    [PATCH 4/5] vmalloc: allow to account vmalloc to memcg Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-07 21:10 +0100
    [PATCH 3/5] memcg: only account kmem allocations marked as __GFP_ACCOUNT Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-07 21:10 +0100
    [PATCH 1/5] Revert "kernfs: do not account ino_ida allocations to memcg" Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-07 21:10 +0100
    Re: [PATCH 0/5] memcg/kmem: switch to white list policy Michal Hocko <mhocko@kernel.org> - 2015-11-09 15:10 +0100
      Re: [PATCH 0/5] memcg/kmem: switch to white list policy Johannes Weiner <hannes@cmpxchg.org> - 2015-11-09 17:50 +0100
      Re: [PATCH 0/5] memcg/kmem: switch to white list policy Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-09 19:30 +0100
        Re: [PATCH 0/5] memcg/kmem: switch to white list policy Tejun Heo <tj@kernel.org> - 2015-11-09 20:00 +0100
          Re: [PATCH 0/5] memcg/kmem: switch to white list policy Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-09 20:30 +0100
            Re: [PATCH 0/5] memcg/kmem: switch to white list policy Tejun Heo <tj@kernel.org> - 2015-11-09 20:40 +0100
              Re: [PATCH 0/5] memcg/kmem: switch to white list policy Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-09 21:20 +0100
                Re: [PATCH 0/5] memcg/kmem: switch to white list policy Tejun Heo <tj@kernel.org> - 2015-11-09 21:40 +0100
                  Re: [PATCH 0/5] memcg/kmem: switch to white list policy Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-10 09:00 +0100
        Re: [PATCH 0/5] memcg/kmem: switch to white list policy Michal Hocko <mhocko@kernel.org> - 2015-11-11 09:20 +0100
    Re: [PATCH 5/5] Account certain kmem allocations to memcg Michal Hocko <mhocko@kernel.org> - 2015-11-09 15:50 +0100
      Re: [PATCH 5/5] Account certain kmem allocations to memcg Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-11-10 09:10 +0100
        Re: [PATCH 5/5] Account certain kmem allocations to memcg Michal Hocko <mhocko@kernel.org> - 2015-11-10 14:30 +0100

#1264923 — [PATCH 0/5] memcg/kmem: switch to white list policy

Hi,

Currently, all kmem allocations (namely every kmem_cache_alloc, kmalloc,
alloc_kmem_pages call) are accounted to memory cgroup automatically.
Callers have to explicitly opt out if they don't want/need accounting
for some reason. Such a design decision leads to several problems:

 - kmalloc users are highly sensitive to failures, many of them
   implicitly rely on the fact that kmalloc never fails, while memcg
   makes failures quite plausible.

 - A lot of objects are shared among different containers by design.
   Accounting such objects to one of containers is just unfair.
   Moreover, it might lead to pinning a dead memcg along with its kmem
   caches, which aren't tiny, which might result in noticeable increase
   in memory consumption for no apparent reason in the long run.

 - There are tons of short-lived objects. Accounting them to memcg will
   only result in slight noise and won't change the overall picture, but
   we still have to pay accounting overhead.

For more info, see

 - https://lkml.org/lkml/2015/11/5/365
 - https://lkml.org/lkml/2015/11/6/122

Therefore this patch switches to the white list policy. Now kmalloc
users have to explicitly opt in by passing __GFP_ACCOUNT flag.

Currently, the list of accounted objects is quite limited and only
includes those allocations that (1) are known to be easily triggered
from userspace and (2) can fail gracefully (for the full list see patch
no. 5) and it still misses many object types. However, accounting only
those objects should be a satisfactory approximation of the behavior we
used to have for most sane workloads.

Thanks,

Vladimir Davydov (5):
  Revert "kernfs: do not account ino_ida allocations to memcg"
  Revert "gfp: add __GFP_NOACCOUNT"
  memcg: only account kmem allocations marked as __GFP_ACCOUNT
  vmalloc: allow to account vmalloc to memcg
  Account certain kmem allocations to memcg

 arch/powerpc/platforms/cell/spufs/inode.c     |  2 +-
 drivers/staging/lustre/lustre/llite/super25.c |  3 ++-
 fs/9p/vfs_inode.c                             |  2 +-
 fs/adfs/super.c                               |  2 +-
 fs/affs/super.c                               |  2 +-
 fs/afs/super.c                                |  2 +-
 fs/befs/linuxvfs.c                            |  2 +-
 fs/bfs/inode.c                                |  2 +-
 fs/block_dev.c                                |  3 ++-
 fs/btrfs/inode.c                              |  2 +-
 fs/ceph/inode.c                               |  2 +-
 fs/cifs/cifsfs.c                              |  2 +-
 fs/coda/inode.c                               |  2 +-
 fs/dcache.c                                   |  5 +++--
 fs/ecryptfs/super.c                           |  3 ++-
 fs/efs/super.c                                |  2 +-
 fs/exec.c                                     |  5 +++--
 fs/exofs/super.c                              |  2 +-
 fs/ext2/super.c                               |  2 +-
 fs/ext4/super.c                               |  2 +-
 fs/f2fs/super.c                               |  2 +-
 fs/fat/inode.c                                |  2 +-
 fs/file.c                                     |  9 +++++----
 fs/fs_struct.c                                |  2 +-
 fs/fuse/inode.c                               |  4 ++--
 fs/gfs2/super.c                               |  2 +-
 fs/hfs/super.c                                |  2 +-
 fs/hfsplus/super.c                            |  2 +-
 fs/hostfs/hostfs_kern.c                       |  2 +-
 fs/hpfs/super.c                               |  2 +-
 fs/hugetlbfs/inode.c                          |  2 +-
 fs/inode.c                                    |  2 +-
 fs/isofs/inode.c                              |  2 +-
 fs/jffs2/super.c                              |  2 +-
 fs/jfs/super.c                                |  3 ++-
 fs/kernfs/dir.c                               |  9 +--------
 fs/logfs/inode.c                              |  2 +-
 fs/minix/inode.c                              |  2 +-
 fs/ncpfs/inode.c                              |  3 ++-
 fs/nfs/inode.c                                |  2 +-
 fs/nilfs2/super.c                             |  2 +-
 fs/ntfs/inode.c                               |  2 +-
 fs/ocfs2/dlmfs/dlmfs.c                        |  2 +-
 fs/ocfs2/super.c                              |  2 +-
 fs/openpromfs/inode.c                         |  2 +-
 fs/proc/inode.c                               |  3 ++-
 fs/qnx4/inode.c                               |  2 +-
 fs/qnx6/inode.c                               |  2 +-
 fs/reiserfs/super.c                           |  2 +-
 fs/romfs/super.c                              |  2 +-
 fs/squashfs/super.c                           |  2 +-
 fs/sysv/inode.c                               |  2 +-
 fs/ubifs/super.c                              |  2 +-
 fs/udf/super.c                                |  2 +-
 fs/ufs/super.c                                |  2 +-
 fs/xfs/kmem.h                                 |  7 ++++++-
 fs/xfs/xfs_icache.c                           |  2 +-
 include/linux/gfp.h                           |  6 ++++--
 include/linux/memcontrol.h                    |  2 +-
 include/linux/thread_info.h                   |  5 +++--
 ipc/mqueue.c                                  |  2 +-
 kernel/cred.c                                 |  4 ++--
 kernel/delayacct.c                            |  2 +-
 kernel/fork.c                                 | 11 ++++++-----
 kernel/pid.c                                  |  2 +-
 mm/kmemleak.c                                 |  3 +--
 mm/mmap.c                                     | 10 +++++-----
 mm/nommu.c                                    |  8 ++++----
 mm/page_alloc.c                               |  3 ++-
 mm/rmap.c                                     |  4 ++--
 mm/shmem.c                                    |  2 +-
 mm/vmalloc.c                                  |  6 +++---
 net/socket.c                                  |  4 ++--
 net/sunrpc/rpc_pipe.c                         |  2 +-
 74 files changed, 117 insertions(+), 106 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1264924 — [PATCH 4/5] vmalloc: allow to account vmalloc to memcg

This patch makes vmalloc family functions allocate vmalloc area pages
with alloc_kmem_pages so that if __GFP_ACCOUNT is set they will be
accounted to memcg. This is needed, at least, to account alloc_fdmem
allocations.

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 mm/vmalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9db9ef5e8481..259cfb32b7cf 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1476,7 +1476,7 @@ static void __vunmap(const void *addr, int deallocate_pages)
 			struct page *page = area->pages[i];
 
 			BUG_ON(!page);
-			__free_page(page);
+			__free_kmem_pages(page, 0);
 		}
 
 		if (area->flags & VM_VPAGES)
@@ -1607,9 +1607,9 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		struct page *page;
 
 		if (node == NUMA_NO_NODE)
-			page = alloc_page(alloc_mask);
+			page = alloc_kmem_pages(alloc_mask, order);
 		else
-			page = alloc_pages_node(node, alloc_mask, order);
+			page = alloc_kmem_pages_node(node, alloc_mask, order);
 
 		if (unlikely(!page)) {
 			/* Successfully allocated i pages, free them in __vunmap() */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264926 — [PATCH 3/5] memcg: only account kmem allocations marked as __GFP_ACCOUNT

Black-list kmem accounting policy (aka __GFP_NOACCOUNT) turned out to be
fragile and difficult to maintain, because there seem to be many more
allocations that should not be accounted than those that should be.
Besides, false accounting an allocation might result in much worse
consequences than not accounting at all, namely increased memory
consumption due to pinned dead kmem caches.

So this patch switches kmem accounting to the white-policy: now only
those kmem allocations that are marked as __GFP_ACCOUNT are accounted to
memcg. Currently, no kmem allocations are marked like this. The
following patches will mark several kmem allocations that are known to
be easily triggered from userspace and therefore should be accounted to
memcg.

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 include/linux/gfp.h        | 4 ++++
 include/linux/memcontrol.h | 2 ++
 mm/page_alloc.c            | 3 ++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 2b917ce34efc..61305a492356 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -30,6 +30,7 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL		0x20000u
 #define ___GFP_THISNODE		0x40000u
 #define ___GFP_RECLAIMABLE	0x80000u
+#define ___GFP_ACCOUNT		0x100000u
 #define ___GFP_NOTRACK		0x200000u
 #define ___GFP_NO_KSWAPD	0x400000u
 #define ___GFP_OTHER_NODE	0x800000u
@@ -90,6 +91,8 @@ struct vm_area_struct;
 #define __GFP_HARDWALL   ((__force gfp_t)___GFP_HARDWALL) /* Enforce hardwall cpuset memory allocs */
 #define __GFP_THISNODE	((__force gfp_t)___GFP_THISNODE)/* No fallback, no policies */
 #define __GFP_RECLAIMABLE ((__force gfp_t)___GFP_RECLAIMABLE) /* Page is reclaimable */
+#define __GFP_ACCOUNT	((__force gfp_t)___GFP_ACCOUNT)	/* Account to memcg (only relevant
+							 * to kmem allocations) */
 #define __GFP_NOTRACK	((__force gfp_t)___GFP_NOTRACK)  /* Don't track with kmemcheck */
 
 #define __GFP_NO_KSWAPD	((__force gfp_t)___GFP_NO_KSWAPD)
@@ -112,6 +115,7 @@ struct vm_area_struct;
 #define GFP_NOIO	(__GFP_WAIT)
 #define GFP_NOFS	(__GFP_WAIT | __GFP_IO)
 #define GFP_KERNEL	(__GFP_WAIT | __GFP_IO | __GFP_FS)
+#define GFP_KERNEL_ACCOUNT	(GFP_KERNEL | __GFP_ACCOUNT)
 #define GFP_TEMPORARY	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
 			 __GFP_RECLAIMABLE)
 #define GFP_USER	(__GFP_WAIT | __GFP_IO | __GFP_FS | __GFP_HARDWALL)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 2103f36b3bd3..c9d9a8e7b45f 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -773,6 +773,8 @@ static inline bool __memcg_kmem_bypass(gfp_t gfp)
 {
 	if (!memcg_kmem_enabled())
 		return true;
+	if (!(gfp & __GFP_ACCOUNT))
+		return true;
 	if (in_interrupt() || (!current->mm) || (current->flags & PF_KTHREAD))
 		return true;
 	return false;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 446bb36ee59d..8e22f5b27de0 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3420,7 +3420,8 @@ EXPORT_SYMBOL(__free_page_frag);
 
 /*
  * alloc_kmem_pages charges newly allocated pages to the kmem resource counter
- * of the current memory cgroup.
+ * of the current memory cgroup if __GFP_ACCOUNT is set, other than that it is
+ * equivalent to alloc_pages.
  *
  * It should be used when the caller would like to use kmalloc, but since the
  * allocation is large, it has to fall back to the page allocator.
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264927 — [PATCH 1/5] Revert "kernfs: do not account ino_ida allocations to memcg"

This reverts commit 499611ed451508a42d1d7d1faff10177827755d5.

Black-list kmem accounting policy (aka __GFP_NOACCOUNT) turned out to be
fragile and difficult to maintain, because there seem to be many more
allocations that should not be accounted than those that should be.
Besides, false accounting an allocation might result in much worse
consequences than not accounting at all, namely increased memory
consumption due to pinned dead kmem caches.

So it was decided to switch to the white-list policy. This patch reverts
bits introducing the black-list policy. The white-list policy will be
introduced later in the series.

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 fs/kernfs/dir.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 91e004518237..0239a0a76ed5 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -541,14 +541,7 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
 	if (!kn)
 		goto err_out1;
 
-	/*
-	 * If the ino of the sysfs entry created for a kmem cache gets
-	 * allocated from an ida layer, which is accounted to the memcg that
-	 * owns the cache, the memcg will get pinned forever. So do not account
-	 * ino ida allocations.
-	 */
-	ret = ida_simple_get(&root->ino_ida, 1, 0,
-			     GFP_KERNEL | __GFP_NOACCOUNT);
+	ret = ida_simple_get(&root->ino_ida, 1, 0, GFP_KERNEL);
 	if (ret < 0)
 		goto err_out2;
 	kn->ino = ret;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265703

On Sat 07-11-15 23:07:04, Vladimir Davydov wrote:
> Hi,
> 
> Currently, all kmem allocations (namely every kmem_cache_alloc, kmalloc,
> alloc_kmem_pages call) are accounted to memory cgroup automatically.
> Callers have to explicitly opt out if they don't want/need accounting
> for some reason. Such a design decision leads to several problems:
> 
>  - kmalloc users are highly sensitive to failures, many of them
>    implicitly rely on the fact that kmalloc never fails, while memcg
>    makes failures quite plausible.
> 
>  - A lot of objects are shared among different containers by design.
>    Accounting such objects to one of containers is just unfair.
>    Moreover, it might lead to pinning a dead memcg along with its kmem
>    caches, which aren't tiny, which might result in noticeable increase
>    in memory consumption for no apparent reason in the long run.
> 
>  - There are tons of short-lived objects. Accounting them to memcg will
>    only result in slight noise and won't change the overall picture, but
>    we still have to pay accounting overhead.

Yes, I think we should have gone that path since the very beginning.
Glauber even started with opt-in IIRC (caches were supposed to register
to be accounted). I do not remember what's led to the opt-out switch -
but I guess it has something to do with the user API how to select which
caches to track and also the original version from Google by Suleiman
Souhlal did the opt-out from the very beginning. Also kmem extension was
assumed to be used for "special" workloads.

> For more info, see
> 
>  - https://lkml.org/lkml/2015/11/5/365
>  - https://lkml.org/lkml/2015/11/6/122

Using lkml.org links tend to be quite painful because they quite often
do not work. http://lkml.kernel.org/r/$msg_id tends to work much better
IMO

http://lkml.kernel.org/r/20151105144002.GB15111%40dhcp22.suse.cz
http://lkml.kernel.org/r/20151106090555.GK29259@esperanza

> Therefore this patch switches to the white list policy. Now kmalloc
> users have to explicitly opt in by passing __GFP_ACCOUNT flag.
> 
> Currently, the list of accounted objects is quite limited and only
> includes those allocations that (1) are known to be easily triggered
> from userspace and (2) can fail gracefully (for the full list see patch
> no. 5) and it still misses many object types. However, accounting only
> those objects should be a satisfactory approximation of the behavior we
> used to have for most sane workloads.

I am _all_ for this semantic I am just not sure what to do with the
legacy kmem controller. Can we change its semantic? If we cannot do that
we would have to distinguish legacy and unified hierarchies during
runtime and add the flag automagically for the first one (that would
however require to keep __GFP_NOACCOUNT as well) which is all as clear
as mud. But maybe the workloads which are using kmem legacy API can cope
with that.

Anyway if we go this way then I think the kmem accounting would be safe
to be enabled by default with the cgroup2.

> Thanks,
> 
> Vladimir Davydov (5):
>   Revert "kernfs: do not account ino_ida allocations to memcg"
>   Revert "gfp: add __GFP_NOACCOUNT"

The patch ordering would break the bisectability. I would simply squash
both places into the patch which replaces the flag.

>   memcg: only account kmem allocations marked as __GFP_ACCOUNT
>   vmalloc: allow to account vmalloc to memcg
>   Account certain kmem allocations to memcg

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265845

On Mon, Nov 09, 2015 at 03:08:32PM +0100, Michal Hocko wrote:
> I am _all_ for this semantic I am just not sure what to do with the
> legacy kmem controller. Can we change its semantic? If we cannot do that
> we would have to distinguish legacy and unified hierarchies during
> runtime and add the flag automagically for the first one (that would
> however require to keep __GFP_NOACCOUNT as well) which is all as clear
> as mud. But maybe the workloads which are using kmem legacy API can cope
> with that.

I think we can make that change for the existing kmem accounting too,
simply because the whitelist should be covering all memory consumers
that actually matter for isolation in practice. Yes, there is a risk
for accidents, but we are not actually intending to change semantics.

> Anyway if we go this way then I think the kmem accounting would be safe
> to be enabled by default with the cgroup2.

Cool, I'm happy we're on the same page about this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265934

On Mon, Nov 09, 2015 at 03:08:32PM +0100, Michal Hocko wrote:
...
> > Therefore this patch switches to the white list policy. Now kmalloc
> > users have to explicitly opt in by passing __GFP_ACCOUNT flag.
> > 
> > Currently, the list of accounted objects is quite limited and only
> > includes those allocations that (1) are known to be easily triggered
> > from userspace and (2) can fail gracefully (for the full list see patch
> > no. 5) and it still misses many object types. However, accounting only
> > those objects should be a satisfactory approximation of the behavior we
> > used to have for most sane workloads.
> 
> I am _all_ for this semantic I am just not sure what to do with the
> legacy kmem controller. Can we change its semantic? If we cannot do that

I think we can. If somebody reports a "bug" caused by this change, i.e.
basically notices that something that used to be accounted is not any
longer, it will be trivial to fix by adding __GFP_ACCOUNT where
appropriate. If it is not, e.g. if accounting of objects of a particular
type leads to intense false-sharing, we would end up disabling
accounting for it anyway.

> we would have to distinguish legacy and unified hierarchies during
> runtime and add the flag automagically for the first one (that would
> however require to keep __GFP_NOACCOUNT as well) which is all as clear
> as mud. But maybe the workloads which are using kmem legacy API can cope
> with that.
> 
> Anyway if we go this way then I think the kmem accounting would be safe
> to be enabled by default with the cgroup2.
> 
> > Thanks,
> > 
> > Vladimir Davydov (5):
> >   Revert "kernfs: do not account ino_ida allocations to memcg"
> >   Revert "gfp: add __GFP_NOACCOUNT"
> 
> The patch ordering would break the bisectability. I would simply squash

How's that? AFAICS the kernel should compile after any first N=1..5
patches of the series applied.

> both places into the patch which replaces the flag.
> 

IMO it is more readable the way it is, but I don't insist.

Thanks,
Vladimir

> >   memcg: only account kmem allocations marked as __GFP_ACCOUNT
> >   vmalloc: allow to account vmalloc to memcg
> >   Account certain kmem allocations to memcg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265950

Hello, Vladmir.

On Mon, Nov 09, 2015 at 09:28:40PM +0300, Vladimir Davydov wrote:
> > I am _all_ for this semantic I am just not sure what to do with the
> > legacy kmem controller. Can we change its semantic? If we cannot do that
> 
> I think we can. If somebody reports a "bug" caused by this change, i.e.
> basically notices that something that used to be accounted is not any
> longer, it will be trivial to fix by adding __GFP_ACCOUNT where
> appropriate. If it is not, e.g. if accounting of objects of a particular
> type leads to intense false-sharing, we would end up disabling
> accounting for it anyway.

I agree too, if anything is meaningfully broken by the flip, it just
indicates that the whitelist needs to be expanded; however, I wonder
whether this would be done better at slab level rather than per
allocation site.

A class of objects which can consume noticeable amount of memory which
can be attributed to userland is likely to be on its own slab already
or separating it out to its own slab is likely to be a good idea.
Marking those slabs as kmemcg accounted seems better suited to the
semantics - it's always about classes of objects - and less
error-prone than marking individual allocation sites.

This also reduces the number of slabs to worry about and more
importantly makes it clear which slabs need to be replicated for
kmemcg accounting from the beginning and the slab part of
implementation can be far simpler / more static.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265957

On Mon, Nov 09, 2015 at 01:54:01PM -0500, Tejun Heo wrote:
> On Mon, Nov 09, 2015 at 09:28:40PM +0300, Vladimir Davydov wrote:
> > > I am _all_ for this semantic I am just not sure what to do with the
> > > legacy kmem controller. Can we change its semantic? If we cannot do that
> > 
> > I think we can. If somebody reports a "bug" caused by this change, i.e.
> > basically notices that something that used to be accounted is not any
> > longer, it will be trivial to fix by adding __GFP_ACCOUNT where
> > appropriate. If it is not, e.g. if accounting of objects of a particular
> > type leads to intense false-sharing, we would end up disabling
> > accounting for it anyway.
> 
> I agree too, if anything is meaningfully broken by the flip, it just
> indicates that the whitelist needs to be expanded; however, I wonder
> whether this would be done better at slab level rather than per
> allocation site.

I'd like to, but this is not as simple as it seems at first glance. The
problem is that slab caches of the same size are actively merged with
each other. If we just added SLAB_ACCOUNT flag, which would be passed to
kmem_cache_create to enable accounting, we'd divide all caches into two
groups that couldn't be merged with each other even if kmem accounting
was not used at all. This would be a show stopper.

Of course, we could rework slab merging so that kmem_cache_create
returned a new dummy cache even if it was actually merged. Such a cache
would point to the real cache, which would be used for allocations. This
wouldn't limit slab merging, but this would add one more dereference to
alloc path, which is even worse.

That's why I decided to go with marking individual allocations.

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265963

Hello, Vladmir.

On Mon, Nov 09, 2015 at 10:27:47PM +0300, Vladimir Davydov wrote:
> Of course, we could rework slab merging so that kmem_cache_create
> returned a new dummy cache even if it was actually merged. Such a cache
> would point to the real cache, which would be used for allocations. This
> wouldn't limit slab merging, but this would add one more dereference to
> alloc path, which is even worse.

Hmmm, this could be me not really understanding but why can't we let
all slabs to be merged regardless of SLAB_ACCOUNT flag for root memcg
and point to per-memcg slabs (may be merged among them but most likely
won't matter) for !root.  We're indirecting once anyway, no?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265988

On Mon, Nov 09, 2015 at 02:32:53PM -0500, Tejun Heo wrote:
> On Mon, Nov 09, 2015 at 10:27:47PM +0300, Vladimir Davydov wrote:
> > Of course, we could rework slab merging so that kmem_cache_create
> > returned a new dummy cache even if it was actually merged. Such a cache
> > would point to the real cache, which would be used for allocations. This
> > wouldn't limit slab merging, but this would add one more dereference to
> > alloc path, which is even worse.
> 
> Hmmm, this could be me not really understanding but why can't we let
> all slabs to be merged regardless of SLAB_ACCOUNT flag for root memcg
> and point to per-memcg slabs (may be merged among them but most likely

Because we won't be able to distinguish kmem_cache_alloc calls that
should be accounted from those that shouldn't. The problem is if two
caches

	A = kmem_cache_create(...)

and

	B = kmem_cache_create(...)

happen to be merged, A and B will point to the same kmem_cache struct.
As a result, there is no way to distinguish

	kmem_cache_alloc(A)

which we want to account from

	kmem_cache_alloc(B)

which we don't.

> won't matter) for !root.  We're indirecting once anyway, no?

If kmem accounting is not used, we aren't indirecting. That's why I
don't think we can use dummy kmem_cache struct for merged caches, where
we could store __GFP_ACCOUNT flag.

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265991

Hello, Vladimir.

On Mon, Nov 09, 2015 at 11:12:18PM +0300, Vladimir Davydov wrote:
> Because we won't be able to distinguish kmem_cache_alloc calls that
> should be accounted from those that shouldn't. The problem is if two
> caches
> 
> 	A = kmem_cache_create(...)
> 
> and
> 
> 	B = kmem_cache_create(...)
> 
> happen to be merged, A and B will point to the same kmem_cache struct.
> As a result, there is no way to distinguish
> 
> 	kmem_cache_alloc(A)
> 
> which we want to account from
> 
> 	kmem_cache_alloc(B)
> 
> which we don't.

Hmm.... can't we simply merge among !SLAB_ACCOUNT and SLAB_ACCOUNT
kmem_caches within themselves?  I don't think we'd be losing anything
by restricting merge at that level.  For anything to be tagged
SLAB_ACCOUNT, it has to have a potential to grow enormous after all.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266302

On Mon, Nov 09, 2015 at 03:30:53PM -0500, Tejun Heo wrote:
...
> Hmm.... can't we simply merge among !SLAB_ACCOUNT and SLAB_ACCOUNT
> kmem_caches within themselves?  I don't think we'd be losing anything
> by restricting merge at that level.  For anything to be tagged
> SLAB_ACCOUNT, it has to have a potential to grow enormous after all.

OK, I'll prepare v2 which will introduce SLAB_ACCOUNT and add it to
SLAB_MERGE_SAME. Let's see what slab maintainers think of it.

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267033

On Mon 09-11-15 21:28:40, Vladimir Davydov wrote:
> On Mon, Nov 09, 2015 at 03:08:32PM +0100, Michal Hocko wrote:
[...]
> > > Vladimir Davydov (5):
> > >   Revert "kernfs: do not account ino_ida allocations to memcg"
> > >   Revert "gfp: add __GFP_NOACCOUNT"
> > 
> > The patch ordering would break the bisectability. I would simply squash
> 
> How's that? AFAICS the kernel should compile after any first N=1..5
> patches of the series applied.

Sorry, forgot to comment on this. I didn't mean it would break
compilation. It would just reintroduce the bug fixed by "kernfs: do not
account ino_ida allocations to memcg". My understanding is that the bug
is quite unlikely and it will results in a pinned memcg which is much
less serious than a crash or other misbehavior.

I will leave whether this is serious enough to you but as the revert is
basically dropping the flag which can be trivially done in the patch
which renames it and changes its semantic I do not think splitting has
any large advantage.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1265737 — Re: [PATCH 5/5] Account certain kmem allocations to memcg

On Sat 07-11-15 23:07:09, Vladimir Davydov wrote:
> This patch marks those kmem allocations that are known to be easily
> triggered from userspace as __GFP_ACCOUNT, which makes them accounted to
> memcg. For the list, see below:
> 
>  - threadinfo
>  - task_struct
>  - task_delay_info
>  - pid
>  - cred
>  - mm_struct
>  - vm_area_struct and vm_region (nommu)
>  - anon_vma and anon_vma_chain
>  - signal_struct
>  - sighand_struct
>  - fs_struct
>  - files_struct
>  - fdtable and fdtable->full_fds_bits
>  - dentry and external_name
>  - inode for all filesystems. This is the most tedious part, because
>    most filesystems overwrite the alloc_inode method. Looks like using
>    __GFP_ACCOUNT in alloc_inode is going to become a new rule, like
>    passing SLAB_RECLAIM_ACCOUNT on inode cache creation.

I am wondering whether using a helper function to allocate an inode
cache would help in that regards. It would limit __GFP_ACCOUNT
penetration into fs code.

pipe buffers are trivial to abuse (e.g. via fd passing) so we want to
cap those as well. The following should do the trick AFAICS.
---
diff --git a/fs/pipe.c b/fs/pipe.c
index 8865f7963700..c4b7e8c08362 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -590,7 +590,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
 
 	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL);
 	if (pipe) {
-		pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * PIPE_DEF_BUFFERS, GFP_KERNEL);
+		pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * PIPE_DEF_BUFFERS, GFP_KERNEL | __GFP_ACCOUNT);
 		if (pipe->bufs) {
 			init_waitqueue_head(&pipe->wait);
 			pipe->r_counter = pipe->w_counter = 1;
@@ -971,7 +971,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long nr_pages)
 	if (nr_pages < pipe->nrbufs)
 		return -EBUSY;
 
-	bufs = kcalloc(nr_pages, sizeof(*bufs), GFP_KERNEL | __GFP_NOWARN);
+	bufs = kcalloc(nr_pages, sizeof(*bufs), GFP_KERNEL | __GFP_NOWARN | __GFP_ACCOUNT);
 	if (unlikely(!bufs))
 		return -ENOMEM;
 
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266306 — Re: [PATCH 5/5] Account certain kmem allocations to memcg

On Mon, Nov 09, 2015 at 03:39:55PM +0100, Michal Hocko wrote:
> On Sat 07-11-15 23:07:09, Vladimir Davydov wrote:
> > This patch marks those kmem allocations that are known to be easily
> > triggered from userspace as __GFP_ACCOUNT, which makes them accounted to
> > memcg. For the list, see below:
> > 
> >  - threadinfo
> >  - task_struct
> >  - task_delay_info
> >  - pid
> >  - cred
> >  - mm_struct
> >  - vm_area_struct and vm_region (nommu)
> >  - anon_vma and anon_vma_chain
> >  - signal_struct
> >  - sighand_struct
> >  - fs_struct
> >  - files_struct
> >  - fdtable and fdtable->full_fds_bits
> >  - dentry and external_name
> >  - inode for all filesystems. This is the most tedious part, because
> >    most filesystems overwrite the alloc_inode method. Looks like using
> >    __GFP_ACCOUNT in alloc_inode is going to become a new rule, like
> >    passing SLAB_RECLAIM_ACCOUNT on inode cache creation.
> 
> I am wondering whether using a helper function to allocate an inode
> cache would help in that regards. It would limit __GFP_ACCOUNT
> penetration into fs code.

I'm afraid that wouldn't free fs code from the need to use
__GFP_ACCOUNT, because there are other things that we might want to
account AFAICS, e.g. ext4_crypt_info_cachep or ext4_es_cachep.

> 
> pipe buffers are trivial to abuse (e.g. via fd passing) so we want to

You might also mention allocations caused by select/poll, page tables,
radix_tree_node, etc. They all might be abused, but the primary purpose
of this patch set is not catching abusers, but providing reasonable
level of isolation for most normal workloads. Let's add everything above
that in separate patches.

> cap those as well. The following should do the trick AFAICS.

Actually, no - you only account pipe metadata while anon pipe buffer
pages, which usually constitute most of memory consumed by a pipe, still
go unaccounted. I'm planning to make pipe accountable later.

> ---
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 8865f7963700..c4b7e8c08362 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -590,7 +590,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
>  
>  	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL);
>  	if (pipe) {
> -		pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * PIPE_DEF_BUFFERS, GFP_KERNEL);
> +		pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * PIPE_DEF_BUFFERS, GFP_KERNEL | __GFP_ACCOUNT);

GFP_KERNEL | __GFP_ACCOUNT are used really often, that's why I
introduced GFP_KERNEL_ACCOUNT.

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266488 — Re: [PATCH 5/5] Account certain kmem allocations to memcg

On Tue 10-11-15 11:07:09, Vladimir Davydov wrote:
> On Mon, Nov 09, 2015 at 03:39:55PM +0100, Michal Hocko wrote:
[...]
> > pipe buffers are trivial to abuse (e.g. via fd passing) so we want to
> 
> You might also mention allocations caused by select/poll, page tables,
> radix_tree_node, etc. They all might be abused, but the primary purpose
> of this patch set is not catching abusers, but providing reasonable
> level of isolation for most normal workloads. Let's add everything above
> that in separate patches.

Sure I do not have any objections against step by step approach.
 
> > cap those as well. The following should do the trick AFAICS.
> 
> Actually, no - you only account pipe metadata while anon pipe buffer
> pages, which usually constitute most of memory consumed by a pipe, still
> go unaccounted. I'm planning to make pipe accountable later.

You are right! I have missed pipe_write allocates the real page.

> > ---
> > diff --git a/fs/pipe.c b/fs/pipe.c
> > index 8865f7963700..c4b7e8c08362 100644
> > --- a/fs/pipe.c
> > +++ b/fs/pipe.c
> > @@ -590,7 +590,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
> >  
> >  	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL);
> >  	if (pipe) {
> > -		pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * PIPE_DEF_BUFFERS, GFP_KERNEL);
> > +		pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * PIPE_DEF_BUFFERS, GFP_KERNEL | __GFP_ACCOUNT);
> 
> GFP_KERNEL | __GFP_ACCOUNT are used really often, that's why I
> introduced GFP_KERNEL_ACCOUNT.

Sure that is better.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web