[PATCH] mm/shmem: set default tmpfs size according to memcg limit

[PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
without "-o size=XXX".
When we mount tmpfs in a container(i.e. docker), it is also
totalram_pages / 2 regardless of the memory limit on this container.
That may easily cause OOM if tmpfs occupied too much memory when swap is
off.
So when we mount tmpfs in a memcg, the default size should be limited by
the memcg memory.limit.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/memcontrol.h |  1 +
 mm/memcontrol.c            |  2 +-
 mm/shmem.c                 | 20 +++++++++++++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 69966c4..79c6709 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -265,6 +265,7 @@ struct mem_cgroup {
 	/* WARNING: nodeinfo must be the last member here */
 };
 
+extern struct mutex memcg_limit_mutex;
 extern struct mem_cgroup *root_mem_cgroup;
 
 static inline bool mem_cgroup_disabled(void)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 661f046..ad32f3c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2464,7 +2464,7 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 }
 #endif
 
-static DEFINE_MUTEX(memcg_limit_mutex);
+DEFINE_MUTEX(memcg_limit_mutex);
 
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 				   unsigned long limit)
diff --git a/mm/shmem.c b/mm/shmem.c
index 07a1d22..1c320dd 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -35,6 +35,7 @@
 #include <linux/uio.h>
 #include <linux/khugepaged.h>
 #include <linux/hugetlb.h>
+#include <linux/memcontrol.h>
 
 #include <asm/tlbflush.h> /* for arch/microblaze update_mmu_cache() */
 
@@ -108,7 +109,24 @@ struct shmem_falloc {
 #ifdef CONFIG_TMPFS
 static unsigned long shmem_default_max_blocks(void)
 {
-	return totalram_pages / 2;
+	unsigned long size;
+
+#ifdef CONFIG_MEMCG
+	struct mem_cgroup *memcg = mem_cgroup_from_task(current);
+
+	if (memcg == NULL || memcg == root_mem_cgroup)
+		size = totalram_pages / 2;
+	else {
+		mutex_lock(&memcg_limit_mutex);
+		size = memcg->memory.limit > totalram_pages ?
+				 totalram_pages / 2 : memcg->memory.limit / 2;
+		mutex_unlock(&memcg_limit_mutex);
+	}
+#else
+	size = totalram_pages / 2;
+#endif
+
+	return size;
 }
 
 static unsigned long shmem_default_max_inodes(void)
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Shakeel Butt]

On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
> Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
> without "-o size=XXX".
> When we mount tmpfs in a container(i.e. docker), it is also
> totalram_pages / 2 regardless of the memory limit on this container.
> That may easily cause OOM if tmpfs occupied too much memory when swap is
> off.
> So when we mount tmpfs in a memcg, the default size should be limited by
> the memcg memory.limit.
>

The pages of the tmpfs files are charged to the memcg of allocators
which can be in memcg different from the memcg in which the mount
operation happened. So, tying the size of a tmpfs mount where it was
mounted does not make much sense.

Also mount operation which requires CAP_SYS_ADMIN, is usually
performed by node controller (or job loader) which don't necessarily
run in the memcg of the actual job.

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/memcontrol.h |  1 +
>  mm/memcontrol.c            |  2 +-
>  mm/shmem.c                 | 20 +++++++++++++++++++-
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 69966c4..79c6709 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -265,6 +265,7 @@ struct mem_cgroup {
>         /* WARNING: nodeinfo must be the last member here */
>  };
>
> +extern struct mutex memcg_limit_mutex;
>  extern struct mem_cgroup *root_mem_cgroup;
>
>  static inline bool mem_cgroup_disabled(void)
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 661f046..ad32f3c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2464,7 +2464,7 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
>  }
>  #endif
>
> -static DEFINE_MUTEX(memcg_limit_mutex);
> +DEFINE_MUTEX(memcg_limit_mutex);

This mutex is only needed for updating the limit.

>
>  static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
>                                    unsigned long limit)
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 07a1d22..1c320dd 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -35,6 +35,7 @@
>  #include <linux/uio.h>
>  #include <linux/khugepaged.h>
>  #include <linux/hugetlb.h>
> +#include <linux/memcontrol.h>
>
>  #include <asm/tlbflush.h> /* for arch/microblaze update_mmu_cache() */
>
> @@ -108,7 +109,24 @@ struct shmem_falloc {
>  #ifdef CONFIG_TMPFS
>  static unsigned long shmem_default_max_blocks(void)
>  {
> -       return totalram_pages / 2;
> +       unsigned long size;
> +
> +#ifdef CONFIG_MEMCG
> +       struct mem_cgroup *memcg = mem_cgroup_from_task(current);
> +
> +       if (memcg == NULL || memcg == root_mem_cgroup)
> +               size = totalram_pages / 2;
> +       else {
> +               mutex_lock(&memcg_limit_mutex);
> +               size = memcg->memory.limit > totalram_pages ?
> +                                totalram_pages / 2 : memcg->memory.limit / 2;
> +               mutex_unlock(&memcg_limit_mutex);
> +       }
> +#else
> +       size = totalram_pages / 2;
> +#endif
> +
> +       return size;
>  }
>
>  static unsigned long shmem_default_max_inodes(void)
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-17 12:43 GMT+08:00 Shakeel Butt <shakeelb@google.com>:
> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>> Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>> without "-o size=XXX".
>> When we mount tmpfs in a container(i.e. docker), it is also
>> totalram_pages / 2 regardless of the memory limit on this container.
>> That may easily cause OOM if tmpfs occupied too much memory when swap is
>> off.
>> So when we mount tmpfs in a memcg, the default size should be limited by
>> the memcg memory.limit.
>>
>
> The pages of the tmpfs files are charged to the memcg of allocators
> which can be in memcg different from the memcg in which the mount
> operation happened.

Yes.
But the issue is tmpfs files contributed to memory.usage_in_bytes
should be limited.
Let me take an example.
The physical memory size is 1G, and we create a memory cgroup then set the
memory.limit_in_bytes of it to 256M.
Then in this memory cgroup we do bellow test:
     1. mount -t tmpfs tmpfs /mount
         the size of which will be 1G / 2 by default.
      2. write files into this tmpfs
         as the limit of this memory cgroup is 256M while the size of
tmpfs is 512M,
         these files will occupy the while memory in this cgroup and
finally out of memory.


> So, tying the size of a tmpfs mount where it was
> mounted does not make much sense.
>
> Also mount operation which requires CAP_SYS_ADMIN, is usually
> performed by node controller (or job loader) which don't necessarily
> run in the memcg of the actual job.
>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> ---
>>  include/linux/memcontrol.h |  1 +
>>  mm/memcontrol.c            |  2 +-
>>  mm/shmem.c                 | 20 +++++++++++++++++++-
>>  3 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 69966c4..79c6709 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -265,6 +265,7 @@ struct mem_cgroup {
>>         /* WARNING: nodeinfo must be the last member here */
>>  };
>>
>> +extern struct mutex memcg_limit_mutex;
>>  extern struct mem_cgroup *root_mem_cgroup;
>>
>>  static inline bool mem_cgroup_disabled(void)
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 661f046..ad32f3c 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2464,7 +2464,7 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
>>  }
>>  #endif
>>
>> -static DEFINE_MUTEX(memcg_limit_mutex);
>> +DEFINE_MUTEX(memcg_limit_mutex);
>
> This mutex is only needed for updating the limit.
>

Thanks for explaination :)

>>
>>  static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
>>                                    unsigned long limit)
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 07a1d22..1c320dd 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/uio.h>
>>  #include <linux/khugepaged.h>
>>  #include <linux/hugetlb.h>
>> +#include <linux/memcontrol.h>
>>
>>  #include <asm/tlbflush.h> /* for arch/microblaze update_mmu_cache() */
>>
>> @@ -108,7 +109,24 @@ struct shmem_falloc {
>>  #ifdef CONFIG_TMPFS
>>  static unsigned long shmem_default_max_blocks(void)
>>  {
>> -       return totalram_pages / 2;
>> +       unsigned long size;
>> +
>> +#ifdef CONFIG_MEMCG
>> +       struct mem_cgroup *memcg = mem_cgroup_from_task(current);
>> +
>> +       if (memcg == NULL || memcg == root_mem_cgroup)
>> +               size = totalram_pages / 2;
>> +       else {
>> +               mutex_lock(&memcg_limit_mutex);
>> +               size = memcg->memory.limit > totalram_pages ?
>> +                                totalram_pages / 2 : memcg->memory.limit / 2;
>> +               mutex_unlock(&memcg_limit_mutex);
>> +       }
>> +#else
>> +       size = totalram_pages / 2;
>> +#endif
>> +
>> +       return size;
>>  }
>>
>>  static unsigned long shmem_default_max_inodes(void)
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe cgroups" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Roman Gushchin]

On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
> > without "-o size=XXX".
> > When we mount tmpfs in a container(i.e. docker), it is also
> > totalram_pages / 2 regardless of the memory limit on this container.
> > That may easily cause OOM if tmpfs occupied too much memory when swap is
> > off.
> > So when we mount tmpfs in a memcg, the default size should be limited by
> > the memcg memory.limit.
> >
> 
> The pages of the tmpfs files are charged to the memcg of allocators
> which can be in memcg different from the memcg in which the mount
> operation happened. So, tying the size of a tmpfs mount where it was
> mounted does not make much sense.

Also, memory limit is adjustable, and using a particular limit value
at a moment of tmpfs mounting doesn't provide any warranties further.

Is there a reason why the userspace app which is mounting tmpfs can't
set the size based on memory.limit?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
> On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
>> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>> > without "-o size=XXX".
>> > When we mount tmpfs in a container(i.e. docker), it is also
>> > totalram_pages / 2 regardless of the memory limit on this container.
>> > That may easily cause OOM if tmpfs occupied too much memory when swap is
>> > off.
>> > So when we mount tmpfs in a memcg, the default size should be limited by
>> > the memcg memory.limit.
>> >
>>
>> The pages of the tmpfs files are charged to the memcg of allocators
>> which can be in memcg different from the memcg in which the mount
>> operation happened. So, tying the size of a tmpfs mount where it was
>> mounted does not make much sense.
>
> Also, memory limit is adjustable,

Yes. But that's irrelevant.

> and using a particular limit value
> at a moment of tmpfs mounting doesn't provide any warranties further.
>

I can not agree.
The default size of tmpfs is totalram / 2, the reason we do this is to
provide any warranties further IMHO.

> Is there a reason why the userspace app which is mounting tmpfs can't
> set the size based on memory.limit?

That's because of misuse.
The application should set size with "-o size=" when mount tmpfs, but
not all applications do this.
As we can't guarantee that all applications will do this, we should
give them a proper default value.

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Roman Gushchin]

On Sat, Nov 18, 2017 at 12:20:40AM +0800, Yafang Shao wrote:
> 2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
> > On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
> >> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
> >> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
> >> > without "-o size=XXX".
> >> > When we mount tmpfs in a container(i.e. docker), it is also
> >> > totalram_pages / 2 regardless of the memory limit on this container.
> >> > That may easily cause OOM if tmpfs occupied too much memory when swap is
> >> > off.
> >> > So when we mount tmpfs in a memcg, the default size should be limited by
> >> > the memcg memory.limit.
> >> >
> >>
> >> The pages of the tmpfs files are charged to the memcg of allocators
> >> which can be in memcg different from the memcg in which the mount
> >> operation happened. So, tying the size of a tmpfs mount where it was
> >> mounted does not make much sense.
> >
> > Also, memory limit is adjustable,
> 
> Yes. But that's irrelevant.
> 
> > and using a particular limit value
> > at a moment of tmpfs mounting doesn't provide any warranties further.
> >
> 
> I can not agree.
> The default size of tmpfs is totalram / 2, the reason we do this is to
> provide any warranties further IMHO.
> 
> > Is there a reason why the userspace app which is mounting tmpfs can't
> > set the size based on memory.limit?
> 
> That's because of misuse.
> The application should set size with "-o size=" when mount tmpfs, but
> not all applications do this.
> As we can't guarantee that all applications will do this, we should
> give them a proper default value.

The value you're suggesting is proper only if an app which is mounting
tmpfs resides in the same memcg and the memory limit will not be adjusted
significantly later. Otherwise you can end up with a default value, which
is worse than totalram/2, for instance, if tmpfs is mounted by some helper,
which is located in a separate and very limited memcg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-18 0:45 GMT+08:00 Roman Gushchin <guro@fb.com>:
> On Sat, Nov 18, 2017 at 12:20:40AM +0800, Yafang Shao wrote:
>> 2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
>> > On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
>> >> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>> >> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>> >> > without "-o size=XXX".
>> >> > When we mount tmpfs in a container(i.e. docker), it is also
>> >> > totalram_pages / 2 regardless of the memory limit on this container.
>> >> > That may easily cause OOM if tmpfs occupied too much memory when swap is
>> >> > off.
>> >> > So when we mount tmpfs in a memcg, the default size should be limited by
>> >> > the memcg memory.limit.
>> >> >
>> >>
>> >> The pages of the tmpfs files are charged to the memcg of allocators
>> >> which can be in memcg different from the memcg in which the mount
>> >> operation happened. So, tying the size of a tmpfs mount where it was
>> >> mounted does not make much sense.
>> >
>> > Also, memory limit is adjustable,
>>
>> Yes. But that's irrelevant.
>>
>> > and using a particular limit value
>> > at a moment of tmpfs mounting doesn't provide any warranties further.
>> >
>>
>> I can not agree.
>> The default size of tmpfs is totalram / 2, the reason we do this is to
>> provide any warranties further IMHO.
>>
>> > Is there a reason why the userspace app which is mounting tmpfs can't
>> > set the size based on memory.limit?
>>
>> That's because of misuse.
>> The application should set size with "-o size=" when mount tmpfs, but
>> not all applications do this.
>> As we can't guarantee that all applications will do this, we should
>> give them a proper default value.
>
> The value you're suggesting is proper only if an app which is mounting
> tmpfs resides in the same memcg

Yes.
But maybe that's mostly used today?

> and the memory limit will not be adjusted
> significantly later.

There's a similar issue for physical memory adjusted by memory hotplug.
So what will happen if the physical memory adjusted significantly later ?

> Otherwise you can end up with a default value, which
> is worse than totalram/2, for instance, if tmpfs is mounted by some helper,
> which is located in a separate and very limited memcg.

That may happen.
Maybe we could improve the solution to handle this issue ?


Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Shakeel Butt]

On Fri, Nov 17, 2017 at 9:09 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> 2017-11-18 0:45 GMT+08:00 Roman Gushchin <guro@fb.com>:
>> On Sat, Nov 18, 2017 at 12:20:40AM +0800, Yafang Shao wrote:
>>> 2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>> > On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
>>> >> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>> >> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>>> >> > without "-o size=XXX".
>>> >> > When we mount tmpfs in a container(i.e. docker), it is also
>>> >> > totalram_pages / 2 regardless of the memory limit on this container.
>>> >> > That may easily cause OOM if tmpfs occupied too much memory when swap is
>>> >> > off.
>>> >> > So when we mount tmpfs in a memcg, the default size should be limited by
>>> >> > the memcg memory.limit.
>>> >> >
>>> >>
>>> >> The pages of the tmpfs files are charged to the memcg of allocators
>>> >> which can be in memcg different from the memcg in which the mount
>>> >> operation happened. So, tying the size of a tmpfs mount where it was
>>> >> mounted does not make much sense.
>>> >
>>> > Also, memory limit is adjustable,
>>>
>>> Yes. But that's irrelevant.
>>>
>>> > and using a particular limit value
>>> > at a moment of tmpfs mounting doesn't provide any warranties further.
>>> >
>>>
>>> I can not agree.
>>> The default size of tmpfs is totalram / 2, the reason we do this is to
>>> provide any warranties further IMHO.
>>>
>>> > Is there a reason why the userspace app which is mounting tmpfs can't
>>> > set the size based on memory.limit?
>>>
>>> That's because of misuse.
>>> The application should set size with "-o size=" when mount tmpfs, but
>>> not all applications do this.
>>> As we can't guarantee that all applications will do this, we should
>>> give them a proper default value.
>>
>> The value you're suggesting is proper only if an app which is mounting
>> tmpfs resides in the same memcg
>
> Yes.
> But maybe that's mostly used today?
>
>> and the memory limit will not be adjusted
>> significantly later.
>
> There's a similar issue for physical memory adjusted by memory hotplug.
> So what will happen if the physical memory adjusted significantly later ?
>
>> Otherwise you can end up with a default value, which
>> is worse than totalram/2, for instance, if tmpfs is mounted by some helper,
>> which is located in a separate and very limited memcg.
>
> That may happen.
> Maybe we could improve the solution to handle this issue ?
>
>

Let's backtrack, what is the actual concern? If a user/process inside
a memcg is allocating pages for a file on a tmpfs mounted without size
parameter, you want the OS to return ENOSPC (if allocation is done by
write syscall) earlier to not cause the user/process's memcg to OOM.
Is that right?

First, there is no guarantee to not cause OOM by restricting tmpfs to
half the size of memcg limit due to the presence of other memory
charged to that memcg. The memcg can OOM even before the tmpfs hits
its size.

Second, the users who really care to avoid such scenario should just
set the size parameter of tmpfs.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-18 1:35 GMT+08:00 Shakeel Butt <shakeelb@google.com>:
> On Fri, Nov 17, 2017 at 9:09 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
>> 2017-11-18 0:45 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>> On Sat, Nov 18, 2017 at 12:20:40AM +0800, Yafang Shao wrote:
>>>> 2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>>> > On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
>>>> >> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>>> >> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>>>> >> > without "-o size=XXX".
>>>> >> > When we mount tmpfs in a container(i.e. docker), it is also
>>>> >> > totalram_pages / 2 regardless of the memory limit on this container.
>>>> >> > That may easily cause OOM if tmpfs occupied too much memory when swap is
>>>> >> > off.
>>>> >> > So when we mount tmpfs in a memcg, the default size should be limited by
>>>> >> > the memcg memory.limit.
>>>> >> >
>>>> >>
>>>> >> The pages of the tmpfs files are charged to the memcg of allocators
>>>> >> which can be in memcg different from the memcg in which the mount
>>>> >> operation happened. So, tying the size of a tmpfs mount where it was
>>>> >> mounted does not make much sense.
>>>> >
>>>> > Also, memory limit is adjustable,
>>>>
>>>> Yes. But that's irrelevant.
>>>>
>>>> > and using a particular limit value
>>>> > at a moment of tmpfs mounting doesn't provide any warranties further.
>>>> >
>>>>
>>>> I can not agree.
>>>> The default size of tmpfs is totalram / 2, the reason we do this is to
>>>> provide any warranties further IMHO.
>>>>
>>>> > Is there a reason why the userspace app which is mounting tmpfs can't
>>>> > set the size based on memory.limit?
>>>>
>>>> That's because of misuse.
>>>> The application should set size with "-o size=" when mount tmpfs, but
>>>> not all applications do this.
>>>> As we can't guarantee that all applications will do this, we should
>>>> give them a proper default value.
>>>
>>> The value you're suggesting is proper only if an app which is mounting
>>> tmpfs resides in the same memcg
>>
>> Yes.
>> But maybe that's mostly used today?
>>
>>> and the memory limit will not be adjusted
>>> significantly later.
>>
>> There's a similar issue for physical memory adjusted by memory hotplug.
>> So what will happen if the physical memory adjusted significantly later ?
>>
>>> Otherwise you can end up with a default value, which
>>> is worse than totalram/2, for instance, if tmpfs is mounted by some helper,
>>> which is located in a separate and very limited memcg.
>>
>> That may happen.
>> Maybe we could improve the solution to handle this issue ?
>>
>>
>
> Let's backtrack, what is the actual concern? If a user/process inside
> a memcg is allocating pages for a file on a tmpfs mounted without size
> parameter, you want the OS to return ENOSPC (if allocation is done by
> write syscall) earlier to not cause the user/process's memcg to OOM.
> Is that right?
>

Right.

> First, there is no guarantee to not cause OOM by restricting tmpfs to
> half the size of memcg limit due to the presence of other memory
> charged to that memcg. The memcg can OOM even before the tmpfs hits
> its size.
>

Just guarantee that the OOM not caused by misuse of tmpfs.

> Second, the users who really care to avoid such scenario should just
> set the size parameter of tmpfs.

Of couse that is the best way.
But we can not ensue all applications will do it.
That's why I introduce a proper defalut value for them.


Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Shakeel Butt]

On Fri, Nov 17, 2017 at 9:41 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> 2017-11-18 1:35 GMT+08:00 Shakeel Butt <shakeelb@google.com>:
>> On Fri, Nov 17, 2017 at 9:09 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>> 2017-11-18 0:45 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>>> On Sat, Nov 18, 2017 at 12:20:40AM +0800, Yafang Shao wrote:
>>>>> 2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>>>> > On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
>>>>> >> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>>>> >> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>>>>> >> > without "-o size=XXX".
>>>>> >> > When we mount tmpfs in a container(i.e. docker), it is also
>>>>> >> > totalram_pages / 2 regardless of the memory limit on this container.
>>>>> >> > That may easily cause OOM if tmpfs occupied too much memory when swap is
>>>>> >> > off.
>>>>> >> > So when we mount tmpfs in a memcg, the default size should be limited by
>>>>> >> > the memcg memory.limit.
>>>>> >> >
>>>>> >>
>>>>> >> The pages of the tmpfs files are charged to the memcg of allocators
>>>>> >> which can be in memcg different from the memcg in which the mount
>>>>> >> operation happened. So, tying the size of a tmpfs mount where it was
>>>>> >> mounted does not make much sense.
>>>>> >
>>>>> > Also, memory limit is adjustable,
>>>>>
>>>>> Yes. But that's irrelevant.
>>>>>
>>>>> > and using a particular limit value
>>>>> > at a moment of tmpfs mounting doesn't provide any warranties further.
>>>>> >
>>>>>
>>>>> I can not agree.
>>>>> The default size of tmpfs is totalram / 2, the reason we do this is to
>>>>> provide any warranties further IMHO.
>>>>>
>>>>> > Is there a reason why the userspace app which is mounting tmpfs can't
>>>>> > set the size based on memory.limit?
>>>>>
>>>>> That's because of misuse.
>>>>> The application should set size with "-o size=" when mount tmpfs, but
>>>>> not all applications do this.
>>>>> As we can't guarantee that all applications will do this, we should
>>>>> give them a proper default value.
>>>>
>>>> The value you're suggesting is proper only if an app which is mounting
>>>> tmpfs resides in the same memcg
>>>
>>> Yes.
>>> But maybe that's mostly used today?
>>>
>>>> and the memory limit will not be adjusted
>>>> significantly later.
>>>
>>> There's a similar issue for physical memory adjusted by memory hotplug.
>>> So what will happen if the physical memory adjusted significantly later ?
>>>
>>>> Otherwise you can end up with a default value, which
>>>> is worse than totalram/2, for instance, if tmpfs is mounted by some helper,
>>>> which is located in a separate and very limited memcg.
>>>
>>> That may happen.
>>> Maybe we could improve the solution to handle this issue ?
>>>
>>>
>>
>> Let's backtrack, what is the actual concern? If a user/process inside
>> a memcg is allocating pages for a file on a tmpfs mounted without size
>> parameter, you want the OS to return ENOSPC (if allocation is done by
>> write syscall) earlier to not cause the user/process's memcg to OOM.
>> Is that right?
>>
>
> Right.
>
>> First, there is no guarantee to not cause OOM by restricting tmpfs to
>> half the size of memcg limit due to the presence of other memory
>> charged to that memcg. The memcg can OOM even before the tmpfs hits
>> its size.
>>
>
> Just guarantee that the OOM not caused by misuse of tmpfs.
>
>> Second, the users who really care to avoid such scenario should just
>> set the size parameter of tmpfs.
>
> Of couse that is the best way.
> But we can not ensue all applications will do it.
> That's why I introduce a proper defalut value for them.
>

I think we disagree on the how to get proper default value. Unless you
can restrict that all the memory allocated for a tmpfs mount will be
charged to a specific memcg, you should not just pick limit of the
memcg of the process mounting the tmpfs to set the default of tmpfs
mount. If you can restrict tmpfs charging to a specific memcg then the
limit of that memcg should be used to set the default of the tmpfs
mount. However this feature is not present in the upstream kernel at
the moment (We have this feature in our local kernel and I am planning
to upstream that).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-18 1:49 GMT+08:00 Shakeel Butt <shakeelb@google.com>:
> On Fri, Nov 17, 2017 at 9:41 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
>> 2017-11-18 1:35 GMT+08:00 Shakeel Butt <shakeelb@google.com>:
>>> On Fri, Nov 17, 2017 at 9:09 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>>> 2017-11-18 0:45 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>>>> On Sat, Nov 18, 2017 at 12:20:40AM +0800, Yafang Shao wrote:
>>>>>> 2017-11-17 23:55 GMT+08:00 Roman Gushchin <guro@fb.com>:
>>>>>> > On Thu, Nov 16, 2017 at 08:43:17PM -0800, Shakeel Butt wrote:
>>>>>> >> On Thu, Nov 16, 2017 at 7:09 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>>>>> >> > Currently the default tmpfs size is totalram_pages / 2 if mount tmpfs
>>>>>> >> > without "-o size=XXX".
>>>>>> >> > When we mount tmpfs in a container(i.e. docker), it is also
>>>>>> >> > totalram_pages / 2 regardless of the memory limit on this container.
>>>>>> >> > That may easily cause OOM if tmpfs occupied too much memory when swap is
>>>>>> >> > off.
>>>>>> >> > So when we mount tmpfs in a memcg, the default size should be limited by
>>>>>> >> > the memcg memory.limit.
>>>>>> >> >
>>>>>> >>
>>>>>> >> The pages of the tmpfs files are charged to the memcg of allocators
>>>>>> >> which can be in memcg different from the memcg in which the mount
>>>>>> >> operation happened. So, tying the size of a tmpfs mount where it was
>>>>>> >> mounted does not make much sense.
>>>>>> >
>>>>>> > Also, memory limit is adjustable,
>>>>>>
>>>>>> Yes. But that's irrelevant.
>>>>>>
>>>>>> > and using a particular limit value
>>>>>> > at a moment of tmpfs mounting doesn't provide any warranties further.
>>>>>> >
>>>>>>
>>>>>> I can not agree.
>>>>>> The default size of tmpfs is totalram / 2, the reason we do this is to
>>>>>> provide any warranties further IMHO.
>>>>>>
>>>>>> > Is there a reason why the userspace app which is mounting tmpfs can't
>>>>>> > set the size based on memory.limit?
>>>>>>
>>>>>> That's because of misuse.
>>>>>> The application should set size with "-o size=" when mount tmpfs, but
>>>>>> not all applications do this.
>>>>>> As we can't guarantee that all applications will do this, we should
>>>>>> give them a proper default value.
>>>>>
>>>>> The value you're suggesting is proper only if an app which is mounting
>>>>> tmpfs resides in the same memcg
>>>>
>>>> Yes.
>>>> But maybe that's mostly used today?
>>>>
>>>>> and the memory limit will not be adjusted
>>>>> significantly later.
>>>>
>>>> There's a similar issue for physical memory adjusted by memory hotplug.
>>>> So what will happen if the physical memory adjusted significantly later ?
>>>>
>>>>> Otherwise you can end up with a default value, which
>>>>> is worse than totalram/2, for instance, if tmpfs is mounted by some helper,
>>>>> which is located in a separate and very limited memcg.
>>>>
>>>> That may happen.
>>>> Maybe we could improve the solution to handle this issue ?
>>>>
>>>>
>>>
>>> Let's backtrack, what is the actual concern? If a user/process inside
>>> a memcg is allocating pages for a file on a tmpfs mounted without size
>>> parameter, you want the OS to return ENOSPC (if allocation is done by
>>> write syscall) earlier to not cause the user/process's memcg to OOM.
>>> Is that right?
>>>
>>
>> Right.
>>
>>> First, there is no guarantee to not cause OOM by restricting tmpfs to
>>> half the size of memcg limit due to the presence of other memory
>>> charged to that memcg. The memcg can OOM even before the tmpfs hits
>>> its size.
>>>
>>
>> Just guarantee that the OOM not caused by misuse of tmpfs.
>>
>>> Second, the users who really care to avoid such scenario should just
>>> set the size parameter of tmpfs.
>>
>> Of couse that is the best way.
>> But we can not ensue all applications will do it.
>> That's why I introduce a proper defalut value for them.
>>
>
> I think we disagree on the how to get proper default value. Unless you
> can restrict that all the memory allocated for a tmpfs mount will be
> charged to a specific memcg, you should not just pick limit of the
> memcg of the process mounting the tmpfs to set the default of tmpfs
> mount. If you can restrict tmpfs charging to a specific memcg then the
> limit of that memcg should be used to set the default of the tmpfs
> mount. However this feature is not present in the upstream kernel at
> the moment (We have this feature in our local kernel and I am planning
> to upstream that).

That will be fine if you could upstream this feature ASAP :)


Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Michal Hocko]

On Fri 17-11-17 09:49:54, Shakeel Butt wrote:
> On Fri, Nov 17, 2017 at 9:41 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
[...]
> > Of couse that is the best way.
> > But we can not ensue all applications will do it.
> > That's why I introduce a proper defalut value for them.
> >
> 
> I think we disagree on the how to get proper default value. Unless you
> can restrict that all the memory allocated for a tmpfs mount will be
> charged to a specific memcg, you should not just pick limit of the
> memcg of the process mounting the tmpfs to set the default of tmpfs
> mount. If you can restrict tmpfs charging to a specific memcg then the
> limit of that memcg should be used to set the default of the tmpfs
> mount. However this feature is not present in the upstream kernel at
> the moment (We have this feature in our local kernel and I am planning
> to upstream that).

I think the whole problem is that containers pretend to be independent
while they share a non-reclaimable resource. Fix this and you will not
have a problem. I am afraid that the only real fix is to make tmpfs
private per container instance and that is something you can easily
achieve in the userspace.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-20 20:04 GMT+08:00 Michal Hocko <mhocko@kernel.org>:
> On Fri 17-11-17 09:49:54, Shakeel Butt wrote:
>> On Fri, Nov 17, 2017 at 9:41 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> [...]
>> > Of couse that is the best way.
>> > But we can not ensue all applications will do it.
>> > That's why I introduce a proper defalut value for them.
>> >
>>
>> I think we disagree on the how to get proper default value. Unless you
>> can restrict that all the memory allocated for a tmpfs mount will be
>> charged to a specific memcg, you should not just pick limit of the
>> memcg of the process mounting the tmpfs to set the default of tmpfs
>> mount. If you can restrict tmpfs charging to a specific memcg then the
>> limit of that memcg should be used to set the default of the tmpfs
>> mount. However this feature is not present in the upstream kernel at
>> the moment (We have this feature in our local kernel and I am planning
>> to upstream that).
>
> I think the whole problem is that containers pretend to be independent
> while they share a non-reclaimable resource. Fix this and you will not
> have a problem. I am afraid that the only real fix is to make tmpfs
> private per container instance and that is something you can easily
> achieve in the userspace.
>

Agree with you.

Introduce tmpfs stat in memory cgroup, something like
memory.tmpfs.limit
memory.tmpfs.usage

IMHO this is the best solution.

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Michal Hocko]

On Mon 20-11-17 20:16:15, Yafang Shao wrote:
> 2017-11-20 20:04 GMT+08:00 Michal Hocko <mhocko@kernel.org>:
> > On Fri 17-11-17 09:49:54, Shakeel Butt wrote:
> >> On Fri, Nov 17, 2017 at 9:41 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
> > [...]
> >> > Of couse that is the best way.
> >> > But we can not ensue all applications will do it.
> >> > That's why I introduce a proper defalut value for them.
> >> >
> >>
> >> I think we disagree on the how to get proper default value. Unless you
> >> can restrict that all the memory allocated for a tmpfs mount will be
> >> charged to a specific memcg, you should not just pick limit of the
> >> memcg of the process mounting the tmpfs to set the default of tmpfs
> >> mount. If you can restrict tmpfs charging to a specific memcg then the
> >> limit of that memcg should be used to set the default of the tmpfs
> >> mount. However this feature is not present in the upstream kernel at
> >> the moment (We have this feature in our local kernel and I am planning
> >> to upstream that).
> >
> > I think the whole problem is that containers pretend to be independent
> > while they share a non-reclaimable resource. Fix this and you will not
> > have a problem. I am afraid that the only real fix is to make tmpfs
> > private per container instance and that is something you can easily
> > achieve in the userspace.
> >
> 
> Agree with you.

I suspect you misunderstood...

> Introduce tmpfs stat in memory cgroup, something like
> memory.tmpfs.limit
> memory.tmpfs.usage
> 
> IMHO this is the best solution.

No, you misunderstood. I do not think that we want to split tmpfs out of
the regular limit. We used to have something like that for user vs.
kernel memory accounting  in v1 and that turned to be not working well.

What you really want to do is to make a private mount per container to
ensure that the resource is really _yours_
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/shmem: set default tmpfs size according to memcg limit

[Yafang Shao]

2017-11-20 20:39 GMT+08:00 Michal Hocko <mhocko@kernel.org>:
> On Mon 20-11-17 20:16:15, Yafang Shao wrote:
>> 2017-11-20 20:04 GMT+08:00 Michal Hocko <mhocko@kernel.org>:
>> > On Fri 17-11-17 09:49:54, Shakeel Butt wrote:
>> >> On Fri, Nov 17, 2017 at 9:41 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
>> > [...]
>> >> > Of couse that is the best way.
>> >> > But we can not ensue all applications will do it.
>> >> > That's why I introduce a proper defalut value for them.
>> >> >
>> >>
>> >> I think we disagree on the how to get proper default value. Unless you
>> >> can restrict that all the memory allocated for a tmpfs mount will be
>> >> charged to a specific memcg, you should not just pick limit of the
>> >> memcg of the process mounting the tmpfs to set the default of tmpfs
>> >> mount. If you can restrict tmpfs charging to a specific memcg then the
>> >> limit of that memcg should be used to set the default of the tmpfs
>> >> mount. However this feature is not present in the upstream kernel at
>> >> the moment (We have this feature in our local kernel and I am planning
>> >> to upstream that).
>> >
>> > I think the whole problem is that containers pretend to be independent
>> > while they share a non-reclaimable resource. Fix this and you will not
>> > have a problem. I am afraid that the only real fix is to make tmpfs
>> > private per container instance and that is something you can easily
>> > achieve in the userspace.
>> >
>>
>> Agree with you.
>
> I suspect you misunderstood...
>
>> Introduce tmpfs stat in memory cgroup, something like
>> memory.tmpfs.limit
>> memory.tmpfs.usage
>>
>> IMHO this is the best solution.
>
> No, you misunderstood. I do not think that we want to split tmpfs out of
> the regular limit. We used to have something like that for user vs.
> kernel memory accounting  in v1 and that turned to be not working well.
>

understood.
That really doesn't work well.

> What you really want to do is to make a private mount per container to
> ensure that the resource is really _yours_
> --

That is what I'm doing it currently.
Then setting the default size depends on the container memory limit works well.

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>