Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200423044050.162093-1-joel@joelfernandes.org>
 <20200423044518.GA162422@google.com> <CAOQ4uxgifK_XTkJO69-hQvR4xQGPgHNGKJPv6-MNgHcQat5UBQ@mail.gmail.com>
 <20200423104827.GD3737@quack2.suse.cz>
In-Reply-To: <20200423104827.GD3737@quack2.suse.cz>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Thu, 23 Apr 2020 09:24:02 -0400
Message-ID: <CAEXW_YT4behgV05BqU1PG1o0-FUNnRowdntem5n7bvraGVXz3Q@mail.gmail.com>
Subject: Re: [RFC] fs: Use slab constructor to initialize conn objects in fsnotify
To:     Jan Kara <jack@suse.cz>
Cc:     Amir Goldstein <amir73il@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Apr 23, 2020 at 6:48 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 23-04-20 08:24:23, Amir Goldstein wrote:
> > On Thu, Apr 23, 2020 at 7:45 AM Joel Fernandes <joel@joelfernandes.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.

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