While reading the famous slab paper [1], I noticed that the conn->lock
spinlock and conn->list hlist in fsnotify code is being initialized
during every object allocation. This seems a good fit for the
constructor within the slab to take advantage of the slab design. Move
the initializtion to that.
spin_lock_init(&conn->lock);
INIT_HLIST_HEAD(&conn->list);
[1] https://pdfs.semanticscholar.org/1acc/3a14da69dd240f2fbc11d00e09610263bdbd.pdf
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
I've only build-tested. I am not very familiar with this code, so I
kindly request the maintainers/reviewers to take a look and see if it
makes sense. From what I see, the ->list is always empty during
allocation. The spinlock allocation also seems unnecesary.
fs/notify/fsnotify.c | 15 +++++++++++++--
fs/notify/mark.c | 2 --
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 46f2255800091..55ed450da3295 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -385,6 +385,14 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
}
EXPORT_SYMBOL_GPL(fsnotify);
+static void fsnotify_ctor(void *addr)
+{
+ struct fsnotify_mark_connector *conn = addr;
+
+ spin_lock_init(&conn->lock);
+ INIT_HLIST_HEAD(&conn->list);
+}
+
static __init int fsnotify_init(void)
{
int ret;
@@ -395,8 +403,11 @@ static __init int fsnotify_init(void)
if (ret)
panic("initializing fsnotify_mark_srcu");
- fsnotify_mark_connector_cachep = KMEM_CACHE(fsnotify_mark_connector,
- SLAB_PANIC);
+ fsnotify_mark_connector_cachep =
+ kmem_cache_create("fsnotify_mark_connector",
+ sizeof(struct fsnotify_mark_connector),
+ __alignof__(struct fsnotify_mark_connector),
+ SLAB_PANIC, fsnotify_ctor);
return 0;
}
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 1d96216dffd19..d388e7f45e2ea 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -479,8 +479,6 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
if (!conn)
return -ENOMEM;
- spin_lock_init(&conn->lock);
- INIT_HLIST_HEAD(&conn->list);
conn->type = type;
conn->obj = connp;
/* Cache fsid of filesystem containing the object */
--
2.26.1.301.g55bc3eb7cb9-goog
On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> While reading the famous slab paper [1], I noticed that the conn->lock
> spinlock and conn->list hlist in fsnotify code is being initialized
> during every object allocation. This seems a good fit for the
> constructor within the slab to take advantage of the slab design. Move
> the initializtion to that.
>
> spin_lock_init(&conn->lock);
> INIT_HLIST_HEAD(&conn->list);
>
> [1] https://pdfs.semanticscholar.org/1acc/3a14da69dd240f2fbc11d00e09610263bdbd.pdf
>
The commit message could be better. Just to clarify, doing it this way is
more efficient because the object will only have its spinlock init and hlist
init happen during object construction, not object allocation.
Construction happens when slab is populated, destruction happens when slab
returns memory to Linux kernel buddy allocator. Fast path is allocation.
thanks,
- Joel
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
> I've only build-tested. I am not very familiar with this code, so I
> kindly request the maintainers/reviewers to take a look and see if it
> makes sense. From what I see, the ->list is always empty during
> allocation. The spinlock allocation also seems unnecesary.
>
>
> fs/notify/fsnotify.c | 15 +++++++++++++--
> fs/notify/mark.c | 2 --
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 46f2255800091..55ed450da3295 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -385,6 +385,14 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
> }
> EXPORT_SYMBOL_GPL(fsnotify);
>
> +static void fsnotify_ctor(void *addr)
> +{
> + struct fsnotify_mark_connector *conn = addr;
> +
> + spin_lock_init(&conn->lock);
> + INIT_HLIST_HEAD(&conn->list);
> +}
> +
> static __init int fsnotify_init(void)
> {
> int ret;
> @@ -395,8 +403,11 @@ static __init int fsnotify_init(void)
> if (ret)
> panic("initializing fsnotify_mark_srcu");
>
> - fsnotify_mark_connector_cachep = KMEM_CACHE(fsnotify_mark_connector,
> - SLAB_PANIC);
> + fsnotify_mark_connector_cachep =
> + kmem_cache_create("fsnotify_mark_connector",
> + sizeof(struct fsnotify_mark_connector),
> + __alignof__(struct fsnotify_mark_connector),
> + SLAB_PANIC, fsnotify_ctor);
>
> return 0;
> }
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 1d96216dffd19..d388e7f45e2ea 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -479,8 +479,6 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
> conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
> if (!conn)
> return -ENOMEM;
> - spin_lock_init(&conn->lock);
> - INIT_HLIST_HEAD(&conn->list);
> conn->type = type;
> conn->obj = connp;
> /* Cache fsid of filesystem containing the object */
> --
> 2.26.1.301.g55bc3eb7cb9-goog
On Thu, Apr 23, 2020 at 7:45 AM Joel Fernandes <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> > While reading the famous slab paper [1], I noticed that the conn->lock
> > spinlock and conn->list hlist in fsnotify code is being initialized
> > during every object allocation. This seems a good fit for the
> > constructor within the slab to take advantage of the slab design. Move
> > the initializtion to that.
> >
> > spin_lock_init(&conn->lock);
> > INIT_HLIST_HEAD(&conn->list);
> >
> > [1] https://pdfs.semanticscholar.org/1acc/3a14da69dd240f2fbc11d00e09610263bdbd.pdf
> >
>
> The commit message could be better. Just to clarify, doing it this way is
> more efficient because the object will only have its spinlock init and hlist
> init happen during object construction, not object allocation.
>
This change may be correct, but completely unjustified IMO.
conn objects are very rarely allocated, from user syscall path only.
I see no reason to micro optimize this.
Perhaps there is another justification to do this, but not efficiency.
Thanks,
Amir.
On Thu 23-04-20 08:24:23, Amir Goldstein wrote:
> On Thu, Apr 23, 2020 at 7:45 AM Joel Fernandes <[email protected]> wrote:
> >
> > On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> > > While reading the famous slab paper [1], I noticed that the conn->lock
> > > spinlock and conn->list hlist in fsnotify code is being initialized
> > > during every object allocation. This seems a good fit for the
> > > constructor within the slab to take advantage of the slab design. Move
> > > the initializtion to that.
> > >
> > > spin_lock_init(&conn->lock);
> > > INIT_HLIST_HEAD(&conn->list);
> > >
> > > [1] https://pdfs.semanticscholar.org/1acc/3a14da69dd240f2fbc11d00e09610263bdbd.pdf
> > >
> >
> > The commit message could be better. Just to clarify, doing it this way is
> > more efficient because the object will only have its spinlock init and hlist
> > init happen during object construction, not object allocation.
> >
>
> This change may be correct, but completely unjustified IMO.
> conn objects are very rarely allocated, from user syscall path only.
> I see no reason to micro optimize this.
>
> Perhaps there is another justification to do this, but not efficiency.
Thanks for the suggestion Joel but I agree with Amir here. In principle
using constructor is correct however it puts initialization of object in
two places which makes the code harder to follow and the allocation of
connector does not happen frequently enough for optimizing out these two
stores to matter in any tangible way.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Apr 23, 2020 at 7:40 AM Matthew Wilcox <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> > While reading the famous slab paper [1], I noticed that the conn->lock
> > spinlock and conn->list hlist in fsnotify code is being initialized
> > during every object allocation. This seems a good fit for the
> > constructor within the slab to take advantage of the slab design. Move
> > the initializtion to that.
>
> The slab paper was written a number of years ago when CPU caches were
> not as they are today. With this patch, every time you allocate a
> new page, we dirty the entire page, and then the dirty cachelines will
> gradually fall out of cache as the other objects on the page are not used
> immediately. Then, when we actually use one of the objects on the page,
> we bring those cachelines back in and dirty them again by initialising
> 'type' and 'obj'. The two stores to initialise lock and list are almost
> free when done in fsnotify_attach_connector_to_object(), but are costly
> when done in a slab constructor.
Thanks a lot for this reasoning. Basically, you're saying when a slab
allocates a page, it would construct all objects which end up dirtying
the entire page before the object is even allocated. That makes sense.
There's one improvement (although probably verys small) that the paper mentions:
Also according to the paper you referenced, the instruction cache is
what would also benefit. Those spinlock and hlist initialization
instructions wouldn't cost L1 I-cache footprint for every allocation.
> There are very few places where a slab constructor is justified with a
> modern CPU. We've considered removing the functionality before.
I see, thanks again for the insights.
- Joel
>
> > @@ -479,8 +479,6 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
> > conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
> > if (!conn)
> > return -ENOMEM;
> > - spin_lock_init(&conn->lock);
> > - INIT_HLIST_HEAD(&conn->list);
> > conn->type = type;
> > conn->obj = connp;
> > /* Cache fsid of filesystem containing the object */
> > --
> > 2.26.1.301.g55bc3eb7cb9-goog
On Thu, Apr 23, 2020 at 6:48 AM Jan Kara <[email protected]> wrote:
>
> On Thu 23-04-20 08:24:23, Amir Goldstein wrote:
> > On Thu, Apr 23, 2020 at 7:45 AM Joel Fernandes <[email protected]> wrote:
> > >
> > > On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> > > > While reading the famous slab paper [1], I noticed that the conn->lock
> > > > spinlock and conn->list hlist in fsnotify code is being initialized
> > > > during every object allocation. This seems a good fit for the
> > > > constructor within the slab to take advantage of the slab design. Move
> > > > the initializtion to that.
> > > >
> > > > spin_lock_init(&conn->lock);
> > > > INIT_HLIST_HEAD(&conn->list);
> > > >
> > > > [1] https://pdfs.semanticscholar.org/1acc/3a14da69dd240f2fbc11d00e09610263bdbd.pdf
> > > >
> > >
> > > The commit message could be better. Just to clarify, doing it this way is
> > > more efficient because the object will only have its spinlock init and hlist
> > > init happen during object construction, not object allocation.
> > >
> >
> > This change may be correct, but completely unjustified IMO.
> > conn objects are very rarely allocated, from user syscall path only.
> > I see no reason to micro optimize this.
> >
> > Perhaps there is another justification to do this, but not efficiency.
>
> Thanks for the suggestion Joel but I agree with Amir here. In principle
> using constructor is correct however it puts initialization of object in
> two places which makes the code harder to follow and the allocation of
> connector does not happen frequently enough for optimizing out these two
> stores to matter in any tangible way.
Thanks a lot Jan and Amir for your comments on the RFC patch. I am
glad I got learn about this concept and appreciate the discussion very
much.
I agree with your analysis about the lack of constructor benefit with
infrequent allocations, the other ones being: splitting object
initialization into 2 code paths and also dirtying the entire page and
the L1 cache that Matthew mentioned.
- Joel
On Thu, Apr 23, 2020 at 9:20 AM Joel Fernandes <[email protected]> wrote:
>
> There's one improvement (although probably verys small) that the paper mentions:
> Also according to the paper you referenced, the instruction cache is
> what would also benefit. Those spinlock and hlist initialization
> instructions wouldn't cost L1 I-cache footprint for every allocation.
Just to add to my statement: Obviously, this benefit gives diminishing
returns in this code path considering that the allocations here are
infrequent. But I just mentioned it here for completeness sake.
Thanks,
- Joel
On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> While reading the famous slab paper [1], I noticed that the conn->lock
> spinlock and conn->list hlist in fsnotify code is being initialized
> during every object allocation. This seems a good fit for the
> constructor within the slab to take advantage of the slab design. Move
> the initializtion to that.
The slab paper was written a number of years ago when CPU caches were
not as they are today. With this patch, every time you allocate a
new page, we dirty the entire page, and then the dirty cachelines will
gradually fall out of cache as the other objects on the page are not used
immediately. Then, when we actually use one of the objects on the page,
we bring those cachelines back in and dirty them again by initialising
'type' and 'obj'. The two stores to initialise lock and list are almost
free when done in fsnotify_attach_connector_to_object(), but are costly
when done in a slab constructor.
There are very few places where a slab constructor is justified with a
modern CPU. We've considered removing the functionality before.
> @@ -479,8 +479,6 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
> conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
> if (!conn)
> return -ENOMEM;
> - spin_lock_init(&conn->lock);
> - INIT_HLIST_HEAD(&conn->list);
> conn->type = type;
> conn->obj = connp;
> /* Cache fsid of filesystem containing the object */
> --
> 2.26.1.301.g55bc3eb7cb9-goog
On Thu, Apr 23, 2020 at 9:20 AM Joel Fernandes <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 7:40 AM Matthew Wilcox <[email protected]> wrote:
> >
> > On Thu, Apr 23, 2020 at 12:40:50AM -0400, Joel Fernandes (Google) wrote:
> > > While reading the famous slab paper [1], I noticed that the conn->lock
> > > spinlock and conn->list hlist in fsnotify code is being initialized
> > > during every object allocation. This seems a good fit for the
> > > constructor within the slab to take advantage of the slab design. Move
> > > the initializtion to that.
> >
> > The slab paper was written a number of years ago when CPU caches were
> > not as they are today. With this patch, every time you allocate a
> > new page, we dirty the entire page, and then the dirty cachelines will
> > gradually fall out of cache as the other objects on the page are not used
> > immediately. Then, when we actually use one of the objects on the page,
> > we bring those cachelines back in and dirty them again by initialising
> > 'type' and 'obj'. The two stores to initialise lock and list are almost
> > free when done in fsnotify_attach_connector_to_object(), but are costly
> > when done in a slab constructor.
>
> Thanks a lot for this reasoning. Basically, you're saying when a slab
> allocates a page, it would construct all objects which end up dirtying
> the entire page before the object is even allocated. That makes sense.
>
> There's one improvement (although probably verys small) that the paper mentions:
> Also according to the paper you referenced, the instruction cache is
Correcting myself, the paper wasn't referenced by you but by a
colleague :) Apologies for mistyping :)
Thanks,
- Joel
> what would also benefit. Those spinlock and hlist initialization
> instructions wouldn't cost L1 I-cache footprint for every allocation.
>
> > There are very few places where a slab constructor is justified with a
> > modern CPU. We've considered removing the functionality before.
>
> I see, thanks again for the insights.
>
> - Joel
>
> >
> > > @@ -479,8 +479,6 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
> > > conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
> > > if (!conn)
> > > return -ENOMEM;
> > > - spin_lock_init(&conn->lock);
> > > - INIT_HLIST_HEAD(&conn->list);
> > > conn->type = type;
> > > conn->obj = connp;
> > > /* Cache fsid of filesystem containing the object */
> > > --
> > > 2.26.1.301.g55bc3eb7cb9-goog