From: Amir Goldstein <amir73il@gmail.com>
To: Mel Gorman <mgorman@techsingularity.net>
Cc: Jan Kara <jack@suse.cz>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] fsnotify: Rearrange fast path to minimise overhead when there is no watcher
Date: Mon, 8 Jun 2020 21:12:44 +0300 [thread overview]
Message-ID: <CAOQ4uxgcUHuqiXFPO5mX=rvDwP-DOoTZrXvpVNphwEMFYHtyCw@mail.gmail.com> (raw)
In-Reply-To: <20200608180130.GJ3127@techsingularity.net>
> > > didn't look too closely at your series as I'm not familiar with fsnotify
> > > in general. However, at a glance it looks like fsnotify_parent() executes
> > > a substantial amount of code even if there are no watchers but I could
> > > be wrong.
> > >
> >
> > I don't about substantial, I would say it is on par with the amount of
> > code that you tries to optimize out of fsnotify().
> >
> > Before bailing out with DCACHE_FSNOTIFY_PARENT_WATCHED
> > test, it also references d_inode->i_sb, real_mount(path->mnt)
> > and fetches all their ->x_fsnotify_mask fields.
> >
> > I changed the call pattern from open/modify/... hooks from:
> > fsnotify_parent(...);
> > fsnotify(...);
> >
> > to:
> > fsnotify_parent(...); /* which calls fsnotify() */
> >
> > So the NULL marks optimization could be done in beginning of
> > fsnotify_parent() and it will be just as effective as it is in fsnotify().
> >
>
> Something like that may be required because
>
> 5.7.0 5.7.0 5.7.0 5.7.0
> vanilla fastfsnotify-v1r1 fastfsnotify-v2r1 amir-20200608
> Amean 1 0.4837 ( 0.00%) 0.4630 * 4.27%* 0.4597 * 4.96%* 0.4967 * -2.69%*
> Amean 3 1.5447 ( 0.00%) 1.4557 ( 5.76%) 1.5310 ( 0.88%) 1.6587 * -7.38%*
> Amean 5 2.6037 ( 0.00%) 2.4363 ( 6.43%) 2.4237 ( 6.91%) 2.6400 ( -1.40%)
> Amean 7 3.5987 ( 0.00%) 3.4757 ( 3.42%) 3.6543 ( -1.55%) 3.9040 * -8.48%*
> Amean 12 5.8267 ( 0.00%) 5.6983 ( 2.20%) 5.5903 ( 4.06%) 6.2593 ( -7.43%)
> Amean 18 8.4400 ( 0.00%) 8.1327 ( 3.64%) 7.7150 * 8.59%* 8.9940 ( -6.56%)
> Amean 24 11.0187 ( 0.00%) 10.0290 * 8.98%* 9.8977 * 10.17%* 11.7247 * -6.41%*
> Amean 30 13.1013 ( 0.00%) 12.8510 ( 1.91%) 12.2087 * 6.81%* 14.0290 * -7.08%*
> Amean 32 13.9190 ( 0.00%) 13.2410 ( 4.87%) 13.2900 ( 4.52%) 14.7140 * -5.71%*
>
> vanilla and fastnotify-v1r1 are the same. fastfsnotify-v2r1 is just the
> fsnotify_parent() change which is mostly worse and may indicate that the
> first patch was reasonable. amir-20200608 is your branch as of today and
> it appears to introduce a substantial regression albeit in an extreme case
> where fsnotify overhead is visible. The regressions are mostly larger
> than noise with the caveat it may be machine specific given that the
> machine is overloaded. I accept that adding extra functional to fsnotify
> may be desirable but ideally it would not hurt the case where there are
> no watchers at all.
>
Of course.
And thanks for catching this regression even before I posted the patches :-)
> So what's the right way forward? The patch as-is even though the fsnotify()
> change itself may be marginal, a patch that just inlines the fast path
> of fsnotify_parent or wait for the additional functionality and try and
> address the overhead on top?
>
>
Let me add your optimizations on top of my branch with the needed
adaptations and send you a branch for testing.
Thanks,
Amir.
next prev parent reply other threads:[~2020-06-08 18:12 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-08 14:05 [PATCH] fsnotify: Rearrange fast path to minimise overhead when there is no watcher Mel Gorman
2020-06-08 15:03 ` Amir Goldstein
2020-06-08 16:06 ` Mel Gorman
2020-06-08 16:26 ` Amir Goldstein
2020-06-08 18:01 ` Mel Gorman
2020-06-08 18:12 ` Amir Goldstein [this message]
2020-06-08 20:39 ` Amir Goldstein
2020-06-10 12:59 ` Mel Gorman
2020-06-10 13:24 ` Amir Goldstein
2020-06-12 9:38 ` Amir Goldstein
2020-06-12 12:42 ` Mel Gorman
2020-06-08 15:19 ` Jan Kara
2020-06-08 16:50 ` Mel Gorman
2020-06-08 17:45 ` Amir Goldstein
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOQ4uxgcUHuqiXFPO5mX=rvDwP-DOoTZrXvpVNphwEMFYHtyCw@mail.gmail.com' \
--to=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@techsingularity.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).