* [PATCH v2] mm/slub: Simplify __kmem_cache_alias() @ 2022-06-03 14:35 sxwjean 2022-06-04 9:42 ` Hyeonggon Yoo 0 siblings, 1 reply; 6+ messages in thread From: sxwjean @ 2022-06-03 14:35 UTC (permalink / raw) To: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo, songmuchun Cc: linux-mm, linux-kernel, Xiongwei Song From: Xiongwei Song <xiongwei.song@windriver.com> There is no need to do anything if sysfs_slab_alias() return nonzero value after getting a mergeable cache. Signed-off-by: Xiongwei Song <xiongwei.song@windriver.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> --- v2: Collect Reviewed-by tag from Muchun. --- mm/slub.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index d8d5abf49f5f..9444277d669a 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, s = find_mergeable(size, align, flags, name, ctor); if (s) { + if (sysfs_slab_alias(s, name)) + return NULL; + s->refcount++; /* @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, */ s->object_size = max(s->object_size, size); s->inuse = max(s->inuse, ALIGN(size, sizeof(void *))); - - if (sysfs_slab_alias(s, name)) { - s->refcount--; - s = NULL; - } } return s; -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mm/slub: Simplify __kmem_cache_alias() 2022-06-03 14:35 [PATCH v2] mm/slub: Simplify __kmem_cache_alias() sxwjean @ 2022-06-04 9:42 ` Hyeonggon Yoo 2022-06-05 7:04 ` Xiongwei Song 0 siblings, 1 reply; 6+ messages in thread From: Hyeonggon Yoo @ 2022-06-04 9:42 UTC (permalink / raw) To: sxwjean Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, songmuchun, linux-mm, linux-kernel, Xiongwei Song On Fri, Jun 03, 2022 at 10:35:55PM +0800, sxwjean@me.com wrote: > From: Xiongwei Song <xiongwei.song@windriver.com> > > There is no need to do anything if sysfs_slab_alias() return nonzero > value after getting a mergeable cache. > > Signed-off-by: Xiongwei Song <xiongwei.song@windriver.com> > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > --- > v2: Collect Reviewed-by tag from Muchun. > --- > mm/slub.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index d8d5abf49f5f..9444277d669a 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > > s = find_mergeable(size, align, flags, name, ctor); > if (s) { > + if (sysfs_slab_alias(s, name)) > + return NULL; > + > s->refcount++; > I think we should not expose sysfs attributes before initializing what can be read via sysfs attribute (object_size). > /* > @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > */ > s->object_size = max(s->object_size, size); this calculation should be done before sysfs_slab_alias(). Thanks, Hyeonggon > s->inuse = max(s->inuse, ALIGN(size, sizeof(void *))); > - > - if (sysfs_slab_alias(s, name)) { > - s->refcount--; > - s = NULL; > - } > } > > return s; > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mm/slub: Simplify __kmem_cache_alias() 2022-06-04 9:42 ` Hyeonggon Yoo @ 2022-06-05 7:04 ` Xiongwei Song 2022-06-14 8:51 ` Hyeonggon Yoo 2022-06-15 8:37 ` Vlastimil Babka 0 siblings, 2 replies; 6+ messages in thread From: Xiongwei Song @ 2022-06-05 7:04 UTC (permalink / raw) To: Hyeonggon Yoo Cc: Xiongwei Song, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, Muchun Song, linux-mm @ kvack . org, Linux Kernel Mailing List, Xiongwei Song On Sat, Jun 4, 2022 at 5:43 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > On Fri, Jun 03, 2022 at 10:35:55PM +0800, sxwjean@me.com wrote: > > From: Xiongwei Song <xiongwei.song@windriver.com> > > > > There is no need to do anything if sysfs_slab_alias() return nonzero > > value after getting a mergeable cache. > > > > Signed-off-by: Xiongwei Song <xiongwei.song@windriver.com> > > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > > --- > > v2: Collect Reviewed-by tag from Muchun. > > --- > > mm/slub.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index d8d5abf49f5f..9444277d669a 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > > > > s = find_mergeable(size, align, flags, name, ctor); > > if (s) { > > + if (sysfs_slab_alias(s, name)) > > + return NULL; > > + > > s->refcount++; > > > > I think we should not expose sysfs attributes before initializing > what can be read via sysfs attribute (object_size). > > > /* > > @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > > */ > > s->object_size = max(s->object_size, size); > > this calculation should be done before sysfs_slab_alias(). Yeah, understood. Should we restore s->object_size and s->inuse if sysfs_slab_alias() returns non zero value? Regards, Xiongwwei > > Thanks, > Hyeonggon > > > s->inuse = max(s->inuse, ALIGN(size, sizeof(void *))); > > - > > - if (sysfs_slab_alias(s, name)) { > > - s->refcount--; > > - s = NULL; > > - } > > } > > > > return s; > > -- > > 2.30.2 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mm/slub: Simplify __kmem_cache_alias() 2022-06-05 7:04 ` Xiongwei Song @ 2022-06-14 8:51 ` Hyeonggon Yoo 2022-06-15 8:37 ` Vlastimil Babka 1 sibling, 0 replies; 6+ messages in thread From: Hyeonggon Yoo @ 2022-06-14 8:51 UTC (permalink / raw) To: Xiongwei Song Cc: Xiongwei Song, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, Muchun Song, linux-mm @ kvack . org, Linux Kernel Mailing List, Xiongwei Song On Sun, Jun 05, 2022 at 03:04:44PM +0800, Xiongwei Song wrote: > On Sat, Jun 4, 2022 at 5:43 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > > > On Fri, Jun 03, 2022 at 10:35:55PM +0800, sxwjean@me.com wrote: > > > From: Xiongwei Song <xiongwei.song@windriver.com> > > > > > > There is no need to do anything if sysfs_slab_alias() return nonzero > > > value after getting a mergeable cache. > > > > > > Signed-off-by: Xiongwei Song <xiongwei.song@windriver.com> > > > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > v2: Collect Reviewed-by tag from Muchun. > > > --- > > > mm/slub.c | 8 +++----- > > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > > > diff --git a/mm/slub.c b/mm/slub.c > > > index d8d5abf49f5f..9444277d669a 100644 > > > --- a/mm/slub.c > > > +++ b/mm/slub.c > > > @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > > > > > > s = find_mergeable(size, align, flags, name, ctor); > > > if (s) { > > > + if (sysfs_slab_alias(s, name)) > > > + return NULL; > > > + > > > s->refcount++; > > > > > > > I think we should not expose sysfs attributes before initializing > > what can be read via sysfs attribute (object_size). > > > > > /* > > > @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > > > */ > > > s->object_size = max(s->object_size, size); > > > > this calculation should be done before sysfs_slab_alias(). > Sorry for the late reply. > Yeah, understood. Should we restore s->object_size and s->inuse if > sysfs_slab_alias() returns non zero value? In my opinion, yes. Thanks, Hyeonggon > Regards, > Xiongwwei > > > > > Thanks, > > Hyeonggon > > > > > s->inuse = max(s->inuse, ALIGN(size, sizeof(void *))); > > > - > > > - if (sysfs_slab_alias(s, name)) { > > > - s->refcount--; > > > - s = NULL; > > > - } > > > } > > > > > > return s; > > > -- > > > 2.30.2 > > > > > -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mm/slub: Simplify __kmem_cache_alias() 2022-06-05 7:04 ` Xiongwei Song 2022-06-14 8:51 ` Hyeonggon Yoo @ 2022-06-15 8:37 ` Vlastimil Babka 2022-06-15 11:09 ` Xiongwei Song 1 sibling, 1 reply; 6+ messages in thread From: Vlastimil Babka @ 2022-06-15 8:37 UTC (permalink / raw) To: Xiongwei Song, Hyeonggon Yoo Cc: Xiongwei Song, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Roman Gushchin, Muchun Song, linux-mm @ kvack . org, Linux Kernel Mailing List, Xiongwei Song On 6/5/22 09:04, Xiongwei Song wrote: > On Sat, Jun 4, 2022 at 5:43 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: >> >> On Fri, Jun 03, 2022 at 10:35:55PM +0800, sxwjean@me.com wrote: >> > From: Xiongwei Song <xiongwei.song@windriver.com> >> > >> > There is no need to do anything if sysfs_slab_alias() return nonzero >> > value after getting a mergeable cache. >> > >> > Signed-off-by: Xiongwei Song <xiongwei.song@windriver.com> >> > Reviewed-by: Muchun Song <songmuchun@bytedance.com> >> > --- >> > v2: Collect Reviewed-by tag from Muchun. Hmm I added v1 (with the Reviewed tag) before getting to the v2 thread. But I think it's fine, see below. >> > --- >> > mm/slub.c | 8 +++----- >> > 1 file changed, 3 insertions(+), 5 deletions(-) >> > >> > diff --git a/mm/slub.c b/mm/slub.c >> > index d8d5abf49f5f..9444277d669a 100644 >> > --- a/mm/slub.c >> > +++ b/mm/slub.c >> > @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, >> > >> > s = find_mergeable(size, align, flags, name, ctor); >> > if (s) { >> > + if (sysfs_slab_alias(s, name)) >> > + return NULL; >> > + >> > s->refcount++; >> > >> >> I think we should not expose sysfs attributes before initializing >> what can be read via sysfs attribute (object_size). Hmm I don't think they are unitialized. They have an old value from the cache we are merging with, which is updated if the new aliased cache has a larger one. So yeah we might briefly during creation expose an alias that will have an incorrect value, but I doubt anything will break. The values are not stable anyway as new aliases are added, as we are bumping them for the 'root' cache and all aliases that share it already. >> > /* >> > @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, >> > */ >> > s->object_size = max(s->object_size, size); >> >> this calculation should be done before sysfs_slab_alias(). > > Yeah, understood. Should we restore s->object_size and s->inuse if > sysfs_slab_alias() returns non zero value? And by bailing out early this patch effectively achieves that, so I'd say it's a better state than before the patch so I'll keep it unless proven otherwise. Thanks! > Regards, > Xiongwwei > >> >> Thanks, >> Hyeonggon >> >> > s->inuse = max(s->inuse, ALIGN(size, sizeof(void *))); >> > - >> > - if (sysfs_slab_alias(s, name)) { >> > - s->refcount--; >> > - s = NULL; >> > - } >> > } >> > >> > return s; >> > -- >> > 2.30.2 >> > >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mm/slub: Simplify __kmem_cache_alias() 2022-06-15 8:37 ` Vlastimil Babka @ 2022-06-15 11:09 ` Xiongwei Song 0 siblings, 0 replies; 6+ messages in thread From: Xiongwei Song @ 2022-06-15 11:09 UTC (permalink / raw) To: Vlastimil Babka Cc: Hyeonggon Yoo, Xiongwei Song, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Roman Gushchin, Muchun Song, linux-mm @ kvack . org, Linux Kernel Mailing List, Xiongwei Song On Wed, Jun 15, 2022 at 4:37 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 6/5/22 09:04, Xiongwei Song wrote: > > On Sat, Jun 4, 2022 at 5:43 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > >> > >> On Fri, Jun 03, 2022 at 10:35:55PM +0800, sxwjean@me.com wrote: > >> > From: Xiongwei Song <xiongwei.song@windriver.com> > >> > > >> > There is no need to do anything if sysfs_slab_alias() return nonzero > >> > value after getting a mergeable cache. > >> > > >> > Signed-off-by: Xiongwei Song <xiongwei.song@windriver.com> > >> > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > >> > --- > >> > v2: Collect Reviewed-by tag from Muchun. > > Hmm I added v1 (with the Reviewed tag) before getting to the v2 thread. But > I think it's fine, see below. > > >> > --- > >> > mm/slub.c | 8 +++----- > >> > 1 file changed, 3 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/mm/slub.c b/mm/slub.c > >> > index d8d5abf49f5f..9444277d669a 100644 > >> > --- a/mm/slub.c > >> > +++ b/mm/slub.c > >> > @@ -4861,6 +4861,9 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > >> > > >> > s = find_mergeable(size, align, flags, name, ctor); > >> > if (s) { > >> > + if (sysfs_slab_alias(s, name)) > >> > + return NULL; > >> > + > >> > s->refcount++; > >> > > >> > >> I think we should not expose sysfs attributes before initializing > >> what can be read via sysfs attribute (object_size). > > Hmm I don't think they are unitialized. They have an old value from the > cache we are merging with, which is updated if the new aliased cache has a > larger one. > So yeah we might briefly during creation expose an alias that will have an > incorrect value, but I doubt anything will break. The values are not stable > anyway as new aliases are added, as we are bumping them for the 'root' cache > and all aliases that share it already. > > >> > /* > >> > @@ -4869,11 +4872,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align, > >> > */ > >> > s->object_size = max(s->object_size, size); > >> > >> this calculation should be done before sysfs_slab_alias(). > > > > Yeah, understood. Should we restore s->object_size and s->inuse if > > sysfs_slab_alias() returns non zero value? > > And by bailing out early this patch effectively achieves that, so I'd say > it's a better state than before the patch so I'll keep it unless proven > otherwise. Thanks! Thank you for your comments Vlastimil and Hyeonggon. Regards, Xiongwei > > > Regards, > > Xiongwwei > > > >> > >> Thanks, > >> Hyeonggon > >> > >> > s->inuse = max(s->inuse, ALIGN(size, sizeof(void *))); > >> > - > >> > - if (sysfs_slab_alias(s, name)) { > >> > - s->refcount--; > >> > - s = NULL; > >> > - } > >> > } > >> > > >> > return s; > >> > -- > >> > 2.30.2 > >> > > >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-15 11:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-03 14:35 [PATCH v2] mm/slub: Simplify __kmem_cache_alias() sxwjean 2022-06-04 9:42 ` Hyeonggon Yoo 2022-06-05 7:04 ` Xiongwei Song 2022-06-14 8:51 ` Hyeonggon Yoo 2022-06-15 8:37 ` Vlastimil Babka 2022-06-15 11:09 ` Xiongwei Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.