Message ID | 20201202121434.75099-1-songmuchun@bytedance.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mm/memcontrol: make the slab calculation consistent | expand |
On Wed, Dec 02, 2020 at 08:14:34PM +0800, Muchun Song wrote: > Although the ratio of the slab is one, we also should read the ratio > from the related memory_stats instead of hard-coding. And the local > variable of size is already the value of slab_unreclaimable. So we > do not need to read again. Simplify the code here. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > mm/memcontrol.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) Hi Muchun! > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 9922f1510956..03a9c64560f6 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1545,12 +1545,22 @@ static int __init memory_stats_init(void) > int i; > > for (i = 0; i < ARRAY_SIZE(memory_stats); i++) { > + switch (memory_stats[i].idx) { > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > - if (memory_stats[i].idx == NR_ANON_THPS || > - memory_stats[i].idx == NR_FILE_THPS || > - memory_stats[i].idx == NR_SHMEM_THPS) > + case NR_ANON_THPS: > + case NR_FILE_THPS: > + case NR_SHMEM_THPS: > memory_stats[i].ratio = HPAGE_PMD_SIZE; > + break; > #endif > + case NR_SLAB_UNRECLAIMABLE_B: > + VM_BUG_ON(i < 1); > + VM_BUG_ON(memory_stats[i - 1].idx != NR_SLAB_RECLAIMABLE_B); Please, convert these to BUILD_BUG_ON(), they don't have to be runtime checks. > + break; > + default: > + break; > + } > + > VM_BUG_ON(!memory_stats[i].ratio); > VM_BUG_ON(memory_stats[i].idx >= MEMCG_NR_STAT); > } > @@ -1587,8 +1597,10 @@ static char *memory_stat_format(struct mem_cgroup *memcg) > seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); > Can you, please, add a small comment here stating that we're printing unreclaimable, reclaimable and the sum of both? It will simplify the reading of the code. > if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) { > - size = memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) + > - memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B); > + int idx = i - 1; > + > + size += memcg_page_state(memcg, memory_stats[idx].idx) * > + memory_stats[idx].ratio; > seq_buf_printf(&s, "slab %llu\n", size); > } > } Otherwise the patch looks good to me! Please, feel free to add Acked-by: Roman Gushchin <guro@fb.com> after addressing my comments. Thanks! > -- > 2.11.0 >
On Thu, Dec 3, 2020 at 5:16 AM Roman Gushchin <guro@fb.com> wrote: > > On Wed, Dec 02, 2020 at 08:14:34PM +0800, Muchun Song wrote: > > Although the ratio of the slab is one, we also should read the ratio > > from the related memory_stats instead of hard-coding. And the local > > variable of size is already the value of slab_unreclaimable. So we > > do not need to read again. Simplify the code here. > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > mm/memcontrol.c | 22 +++++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > Hi Muchun! > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 9922f1510956..03a9c64560f6 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -1545,12 +1545,22 @@ static int __init memory_stats_init(void) > > int i; > > > > for (i = 0; i < ARRAY_SIZE(memory_stats); i++) { > > + switch (memory_stats[i].idx) { > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > - if (memory_stats[i].idx == NR_ANON_THPS || > > - memory_stats[i].idx == NR_FILE_THPS || > > - memory_stats[i].idx == NR_SHMEM_THPS) > > + case NR_ANON_THPS: > > + case NR_FILE_THPS: > > + case NR_SHMEM_THPS: > > memory_stats[i].ratio = HPAGE_PMD_SIZE; > > + break; > > #endif > > + case NR_SLAB_UNRECLAIMABLE_B: > > + VM_BUG_ON(i < 1); > > + VM_BUG_ON(memory_stats[i - 1].idx != NR_SLAB_RECLAIMABLE_B); > > Please, convert these to BUILD_BUG_ON(), they don't have to be runtime checks. Agree. But here we cannot use BUILD_BUG_ON(). The compiler will complain about it. > > > > + break; > > + default: > > + break; > > + } > > + > > VM_BUG_ON(!memory_stats[i].ratio); > > VM_BUG_ON(memory_stats[i].idx >= MEMCG_NR_STAT); > > } > > @@ -1587,8 +1597,10 @@ static char *memory_stat_format(struct mem_cgroup *memcg) > > seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); > > > > Can you, please, add a small comment here stating that we're printing > unreclaimable, reclaimable and the sum of both? It will simplify the reading of the code. Will do. > > > if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) { > > - size = memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) + > > - memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B); > > + int idx = i - 1; > > + > > + size += memcg_page_state(memcg, memory_stats[idx].idx) * > > + memory_stats[idx].ratio; > > seq_buf_printf(&s, "slab %llu\n", size); > > } > > } > > Otherwise the patch looks good to me! Please, feel free to add > Acked-by: Roman Gushchin <guro@fb.com> > after addressing my comments. > > Thanks! > > -- > > 2.11.0 > >
On Thu, Dec 03, 2020 at 10:53:33AM +0800, Muchun Song wrote: > On Thu, Dec 3, 2020 at 5:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > On Wed, Dec 02, 2020 at 08:14:34PM +0800, Muchun Song wrote: > > > Although the ratio of the slab is one, we also should read the ratio > > > from the related memory_stats instead of hard-coding. And the local > > > variable of size is already the value of slab_unreclaimable. So we > > > do not need to read again. Simplify the code here. > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > mm/memcontrol.c | 22 +++++++++++++++++----- > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > Hi Muchun! > > > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index 9922f1510956..03a9c64560f6 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -1545,12 +1545,22 @@ static int __init memory_stats_init(void) > > > int i; > > > > > > for (i = 0; i < ARRAY_SIZE(memory_stats); i++) { > > > + switch (memory_stats[i].idx) { > > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > > - if (memory_stats[i].idx == NR_ANON_THPS || > > > - memory_stats[i].idx == NR_FILE_THPS || > > > - memory_stats[i].idx == NR_SHMEM_THPS) > > > + case NR_ANON_THPS: > > > + case NR_FILE_THPS: > > > + case NR_SHMEM_THPS: > > > memory_stats[i].ratio = HPAGE_PMD_SIZE; > > > + break; > > > #endif > > > + case NR_SLAB_UNRECLAIMABLE_B: > > > + VM_BUG_ON(i < 1); > > > + VM_BUG_ON(memory_stats[i - 1].idx != NR_SLAB_RECLAIMABLE_B); > > > > Please, convert these to BUILD_BUG_ON(), they don't have to be runtime checks. > > Agree. But here we cannot use BUILD_BUG_ON(). The compiler will > complain about it. We can! We just need to change the condition. All we really need to check is that NR_SLAB_UNRECLAIMABLE_B immediately following NR_SLAB_RECLAIMABLE_B. Something like BUILD_BUG_ON(NR_SLAB_UNRECLAIMABLE_B != NR_SLAB_RECLAIMABLE_B + 1) should work (completely untested). > > > > > > > > + break; > > > + default: > > > + break; > > > + } > > > + > > > VM_BUG_ON(!memory_stats[i].ratio); > > > VM_BUG_ON(memory_stats[i].idx >= MEMCG_NR_STAT); > > > } > > > @@ -1587,8 +1597,10 @@ static char *memory_stat_format(struct mem_cgroup *memcg) > > > seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); > > > > > > > Can you, please, add a small comment here stating that we're printing > > unreclaimable, reclaimable and the sum of both? It will simplify the reading of the code. > > Will do. Thank you!
On Thu, Dec 3, 2020 at 11:21 AM Roman Gushchin <guro@fb.com> wrote: > > On Thu, Dec 03, 2020 at 10:53:33AM +0800, Muchun Song wrote: > > On Thu, Dec 3, 2020 at 5:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > On Wed, Dec 02, 2020 at 08:14:34PM +0800, Muchun Song wrote: > > > > Although the ratio of the slab is one, we also should read the ratio > > > > from the related memory_stats instead of hard-coding. And the local > > > > variable of size is already the value of slab_unreclaimable. So we > > > > do not need to read again. Simplify the code here. > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > --- > > > > mm/memcontrol.c | 22 +++++++++++++++++----- > > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > Hi Muchun! > > > > > > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > index 9922f1510956..03a9c64560f6 100644 > > > > --- a/mm/memcontrol.c > > > > +++ b/mm/memcontrol.c > > > > @@ -1545,12 +1545,22 @@ static int __init memory_stats_init(void) > > > > int i; > > > > > > > > for (i = 0; i < ARRAY_SIZE(memory_stats); i++) { > > > > + switch (memory_stats[i].idx) { > > > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > > > - if (memory_stats[i].idx == NR_ANON_THPS || > > > > - memory_stats[i].idx == NR_FILE_THPS || > > > > - memory_stats[i].idx == NR_SHMEM_THPS) > > > > + case NR_ANON_THPS: > > > > + case NR_FILE_THPS: > > > > + case NR_SHMEM_THPS: > > > > memory_stats[i].ratio = HPAGE_PMD_SIZE; > > > > + break; > > > > #endif > > > > + case NR_SLAB_UNRECLAIMABLE_B: > > > > + VM_BUG_ON(i < 1); > > > > + VM_BUG_ON(memory_stats[i - 1].idx != NR_SLAB_RECLAIMABLE_B); > > > > > > Please, convert these to BUILD_BUG_ON(), they don't have to be runtime checks. > > > > Agree. But here we cannot use BUILD_BUG_ON(). The compiler will > > complain about it. > > We can! > > We just need to change the condition. All we really need to check is that > NR_SLAB_UNRECLAIMABLE_B immediately following NR_SLAB_RECLAIMABLE_B. But I think that we need to check that memory_stats[i] immediately following memory_stats[j] where i is the index of NR_SLAB_UNRECLAIMABLE_B and j is the index of NR_SLAB_RECLAIMABLE_B. > > Something like BUILD_BUG_ON(NR_SLAB_UNRECLAIMABLE_B != NR_SLAB_RECLAIMABLE_B + 1) So this cannot work. Thanks. > should work (completely untested). > > > > > > > > > > > > > + break; > > > > + default: > > > > + break; > > > > + } > > > > + > > > > VM_BUG_ON(!memory_stats[i].ratio); > > > > VM_BUG_ON(memory_stats[i].idx >= MEMCG_NR_STAT); > > > > } > > > > @@ -1587,8 +1597,10 @@ static char *memory_stat_format(struct mem_cgroup *memcg) > > > > seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); > > > > > > > > > > Can you, please, add a small comment here stating that we're printing > > > unreclaimable, reclaimable and the sum of both? It will simplify the reading of the code. > > > > Will do. > > Thank you!
On Thu, Dec 03, 2020 at 11:36:54AM +0800, Muchun Song wrote: > On Thu, Dec 3, 2020 at 11:21 AM Roman Gushchin <guro@fb.com> wrote: > > > > On Thu, Dec 03, 2020 at 10:53:33AM +0800, Muchun Song wrote: > > > On Thu, Dec 3, 2020 at 5:16 AM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > On Wed, Dec 02, 2020 at 08:14:34PM +0800, Muchun Song wrote: > > > > > Although the ratio of the slab is one, we also should read the ratio > > > > > from the related memory_stats instead of hard-coding. And the local > > > > > variable of size is already the value of slab_unreclaimable. So we > > > > > do not need to read again. Simplify the code here. > > > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > > --- > > > > > mm/memcontrol.c | 22 +++++++++++++++++----- > > > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > > > Hi Muchun! > > > > > > > > > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > > index 9922f1510956..03a9c64560f6 100644 > > > > > --- a/mm/memcontrol.c > > > > > +++ b/mm/memcontrol.c > > > > > @@ -1545,12 +1545,22 @@ static int __init memory_stats_init(void) > > > > > int i; > > > > > > > > > > for (i = 0; i < ARRAY_SIZE(memory_stats); i++) { > > > > > + switch (memory_stats[i].idx) { > > > > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > > > > - if (memory_stats[i].idx == NR_ANON_THPS || > > > > > - memory_stats[i].idx == NR_FILE_THPS || > > > > > - memory_stats[i].idx == NR_SHMEM_THPS) > > > > > + case NR_ANON_THPS: > > > > > + case NR_FILE_THPS: > > > > > + case NR_SHMEM_THPS: > > > > > memory_stats[i].ratio = HPAGE_PMD_SIZE; > > > > > + break; > > > > > #endif > > > > > + case NR_SLAB_UNRECLAIMABLE_B: > > > > > + VM_BUG_ON(i < 1); > > > > > + VM_BUG_ON(memory_stats[i - 1].idx != NR_SLAB_RECLAIMABLE_B); > > > > > > > > Please, convert these to BUILD_BUG_ON(), they don't have to be runtime checks. > > > > > > Agree. But here we cannot use BUILD_BUG_ON(). The compiler will > > > complain about it. > > > > We can! > > > > We just need to change the condition. All we really need to check is that > > NR_SLAB_UNRECLAIMABLE_B immediately following NR_SLAB_RECLAIMABLE_B. > > But I think that we need to check that memory_stats[i] immediately following > memory_stats[j] where i is the index of NR_SLAB_UNRECLAIMABLE_B and > j is the index of NR_SLAB_RECLAIMABLE_B. Ok, I see. Thanks!
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 9922f1510956..03a9c64560f6 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1545,12 +1545,22 @@ static int __init memory_stats_init(void) int i; for (i = 0; i < ARRAY_SIZE(memory_stats); i++) { + switch (memory_stats[i].idx) { #ifdef CONFIG_TRANSPARENT_HUGEPAGE - if (memory_stats[i].idx == NR_ANON_THPS || - memory_stats[i].idx == NR_FILE_THPS || - memory_stats[i].idx == NR_SHMEM_THPS) + case NR_ANON_THPS: + case NR_FILE_THPS: + case NR_SHMEM_THPS: memory_stats[i].ratio = HPAGE_PMD_SIZE; + break; #endif + case NR_SLAB_UNRECLAIMABLE_B: + VM_BUG_ON(i < 1); + VM_BUG_ON(memory_stats[i - 1].idx != NR_SLAB_RECLAIMABLE_B); + break; + default: + break; + } + VM_BUG_ON(!memory_stats[i].ratio); VM_BUG_ON(memory_stats[i].idx >= MEMCG_NR_STAT); } @@ -1587,8 +1597,10 @@ static char *memory_stat_format(struct mem_cgroup *memcg) seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) { - size = memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) + - memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B); + int idx = i - 1; + + size += memcg_page_state(memcg, memory_stats[idx].idx) * + memory_stats[idx].ratio; seq_buf_printf(&s, "slab %llu\n", size); } }
Although the ratio of the slab is one, we also should read the ratio from the related memory_stats instead of hard-coding. And the local variable of size is already the value of slab_unreclaimable. So we do not need to read again. Simplify the code here. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/memcontrol.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)