linux-kernel - Re: [RFC] fs: Use slab constructor to initialize conn objects in fsnotify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200423044518.GA162422@google.com>
Date:   Thu, 23 Apr 2020 00:45:18 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     linux-kernel@...r.kernel.org
Cc:     Amir Goldstein <amir73il@...il.com>, Jan Kara <jack@...e.cz>,
        linux-fsdevel@...r.kernel.org
Subject: Re: [RFC] fs: Use slab constructor to initialize conn objects in
 fsnotify

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) <joel@...lfernandes.org>
> ---
> 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