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: <20200423104827.GD3737@quack2.suse.cz>
Date:   Thu, 23 Apr 2020 12:48:27 +0200
From:   Jan Kara <jack@...e.cz>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Joel Fernandes <joel@...lfernandes.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFC] fs: Use slab constructor to initialize conn objects in
 fsnotify

On Thu 23-04-20 08:24:23, Amir Goldstein wrote:
> On Thu, Apr 23, 2020 at 7:45 AM Joel Fernandes <joel@...lfernandes.org> 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 <jack@...e.com>
SUSE Labs, CR