| Message ID | 20200614123923.99189-4-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/slub: Fix slabs_node return value | expand |
2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성: > > The function of __kmem_cache_shutdown() is that release all resources > used by the slab cache, while currently it stop release resources when > the preceding node is not empty. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > mm/slub.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index b73505df3de2..4e477ef0f2b9 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s) > */ > int __kmem_cache_shutdown(struct kmem_cache *s) > { > + int ret = 0; > int node; > struct kmem_cache_node *n; > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s) > /* Attempt to free all objects */ > for_each_kmem_cache_node(s, node, n) { > free_partial(s, n); > - if (node_nr_slabs(n)) > - return 1; > + if (!ret && node_nr_slabs(n)) > + ret = 1; > } I don't think that this is an improvement. If the shutdown condition isn't met, we don't need to process further. Just 'return 1' looks okay to me. And, with this change, sysfs_slab_remove() is called even if the shutdown is failed. It's better not to have side effects when failing. Thanks.
On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@gmail.com> wrote: > > 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성: > > > > The function of __kmem_cache_shutdown() is that release all resources > > used by the slab cache, while currently it stop release resources when > > the preceding node is not empty. > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > mm/slub.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index b73505df3de2..4e477ef0f2b9 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s) > > */ > > int __kmem_cache_shutdown(struct kmem_cache *s) > > { > > + int ret = 0; > > int node; > > struct kmem_cache_node *n; > > > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s) > > /* Attempt to free all objects */ > > for_each_kmem_cache_node(s, node, n) { > > free_partial(s, n); > > - if (node_nr_slabs(n)) > > - return 1; > > + if (!ret && node_nr_slabs(n)) > > + ret = 1; > > } > > I don't think that this is an improvement. > > If the shutdown condition isn't met, we don't need to process further. > Just 'return 1' looks okay to me. > > And, with this change, sysfs_slab_remove() is called even if the > shutdown is failed. > It's better not to have side effects when failing. If someone calls __kmem_cache_shutdown, he may want to release resources used by the slab cache as much as possible. If we continue, we may release more pages. From this point, is it an improvement?
2020년 6월 15일 (월) 오후 3:41, Muchun Song <songmuchun@bytedance.com>님이 작성: > > On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@gmail.com> wrote: > > > > 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성: > > > > > > The function of __kmem_cache_shutdown() is that release all resources > > > used by the slab cache, while currently it stop release resources when > > > the preceding node is not empty. > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > mm/slub.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/mm/slub.c b/mm/slub.c > > > index b73505df3de2..4e477ef0f2b9 100644 > > > --- a/mm/slub.c > > > +++ b/mm/slub.c > > > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s) > > > */ > > > int __kmem_cache_shutdown(struct kmem_cache *s) > > > { > > > + int ret = 0; > > > int node; > > > struct kmem_cache_node *n; > > > > > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s) > > > /* Attempt to free all objects */ > > > for_each_kmem_cache_node(s, node, n) { > > > free_partial(s, n); > > > - if (node_nr_slabs(n)) > > > - return 1; > > > + if (!ret && node_nr_slabs(n)) > > > + ret = 1; > > > } > > > > I don't think that this is an improvement. > > > > If the shutdown condition isn't met, we don't need to process further. > > Just 'return 1' looks okay to me. > > > > And, with this change, sysfs_slab_remove() is called even if the > > shutdown is failed. > > It's better not to have side effects when failing. > > If someone calls __kmem_cache_shutdown, he may want to release > resources used by the slab cache as much as possible. If we continue, > we may release more pages. From this point, is it an improvement? My opinion is not strong but I still think that it's not useful enough to complicate the code. If shutdown is failed, it implies there are some bugs and someone should fix it. Releasing more resources would mitigate the resource problem but doesn't change the situation that someone should fix it soon. Anyway, I don't object more if you don't agree with my opinion. In that case, please fix not to call sysfs_slab_remove() when ret is 1. Thanks.
On Mon, Jun 15, 2020 at 3:25 PM Joonsoo Kim <js1304@gmail.com> wrote: > > 2020년 6월 15일 (월) 오후 3:41, Muchun Song <songmuchun@bytedance.com>님이 작성: > > > > On Mon, Jun 15, 2020 at 2:23 PM Joonsoo Kim <js1304@gmail.com> wrote: > > > > > > 2020년 6월 14일 (일) 오후 9:39, Muchun Song <songmuchun@bytedance.com>님이 작성: > > > > > > > > The function of __kmem_cache_shutdown() is that release all resources > > > > used by the slab cache, while currently it stop release resources when > > > > the preceding node is not empty. > > > > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > --- > > > > mm/slub.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/mm/slub.c b/mm/slub.c > > > > index b73505df3de2..4e477ef0f2b9 100644 > > > > --- a/mm/slub.c > > > > +++ b/mm/slub.c > > > > @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s) > > > > */ > > > > int __kmem_cache_shutdown(struct kmem_cache *s) > > > > { > > > > + int ret = 0; > > > > int node; > > > > struct kmem_cache_node *n; > > > > > > > > @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s) > > > > /* Attempt to free all objects */ > > > > for_each_kmem_cache_node(s, node, n) { > > > > free_partial(s, n); > > > > - if (node_nr_slabs(n)) > > > > - return 1; > > > > + if (!ret && node_nr_slabs(n)) > > > > + ret = 1; > > > > } > > > > > > I don't think that this is an improvement. > > > > > > If the shutdown condition isn't met, we don't need to process further. > > > Just 'return 1' looks okay to me. > > > > > > And, with this change, sysfs_slab_remove() is called even if the > > > shutdown is failed. > > > It's better not to have side effects when failing. > > > > If someone calls __kmem_cache_shutdown, he may want to release > > resources used by the slab cache as much as possible. If we continue, > > we may release more pages. From this point, is it an improvement? > > My opinion is not strong but I still think that it's not useful enough > to complicate > the code. > > If shutdown is failed, it implies there are some bugs and someone should fix it. Yeah, I agree with you. > Releasing more resources would mitigate the resource problem but doesn't > change the situation that someone should fix it soon. > > Anyway, I don't object more if you don't agree with my opinion. In that case, > please fix not to call sysfs_slab_remove() when ret is 1. > Yeah, we should call sysfs_slab_remove only when ret is zero. Thanks very much.
diff --git a/mm/slub.c b/mm/slub.c index b73505df3de2..4e477ef0f2b9 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3839,6 +3839,7 @@ bool __kmem_cache_empty(struct kmem_cache *s) */ int __kmem_cache_shutdown(struct kmem_cache *s) { + int ret = 0; int node; struct kmem_cache_node *n; @@ -3846,11 +3847,11 @@ int __kmem_cache_shutdown(struct kmem_cache *s) /* Attempt to free all objects */ for_each_kmem_cache_node(s, node, n) { free_partial(s, n); - if (node_nr_slabs(n)) - return 1; + if (!ret && node_nr_slabs(n)) + ret = 1; } sysfs_slab_remove(s); - return 0; + return ret; } /********************************************************************
The function of __kmem_cache_shutdown() is that release all resources used by the slab cache, while currently it stop release resources when the preceding node is not empty. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/slub.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)