All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@redhat.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: kernel test robot <xiaolong.ye@intel.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	bfields@fieldses.org, linux-fsdevel@vger.kernel.org,
	linux-nfs@vger.kernel.org, lkp@01.org,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [lkp-robot] [fs/locks]  9d21d181d0: will-it-scale.per_process_ops -14.1% regression
Date: Thu, 01 Jun 2017 08:59:21 -0400	[thread overview]
Message-ID: <1496321961.2845.6.camel@redhat.com> (raw)
In-Reply-To: <8F2C3CFF-5C2D-41B0-A895-B1F074DA7943@redhat.com>

On Thu, 2017-06-01 at 07:49 -0400, Benjamin Coddington wrote:
> On 1 Jun 2017, at 7:41, Jeff Layton wrote:
> 
> > On Thu, 2017-06-01 at 10:05 +0800, kernel test robot wrote:
> > > Greeting,
> > > 
> > > FYI, we noticed a -14.1% regression of will-it-scale.per_process_ops 
> > > due to commit:
> > > 
> > > 
> > > commit: 9d21d181d06acab9a8e80eac2ec4eed77b656793 ("fs/locks: Set 
> > > fl_nspid at file_lock allocation")
> > > url: 
> > > https://github.com/0day-ci/linux/commits/Benjamin-Coddington/fs-locks-Alloc-file_lock-where-practical/20170527-050700
> > > 
> > > 
> > 
> > Ouch, that's a rather nasty performance hit. In hindsight, maybe we
> > shouldn't move those off the stack after all? Heck, if it's that
> > significant, maybe we should move the F_SETLK callers to allocate 
> > these
> > on the stack as well?
> 
> We can do that.  But, I think this is picking up the 
> locks_mandatory_area()
> allocation which is now removed.  The attached config has
> CONFIG_MANDATORY_FILE_LOCKING=y,  so there's allocation on every 
> read/write.
> 

I'm not so sure. That would only be the case if the thing were marked
for manadatory locking (a really rare thing).

The test is really simple and I don't think any read/write activity is
involved:

    https://github.com/antonblanchard/will-it-scale/blob/master/tests/lock1.c

...and the 0 day bisected it down to this patch, IIUC:

    https://github.com/0day-ci/linux/commit/9d21d181d06acab9a8e80eac2ec4eed77b656793

It seems likely that it's the extra get_pid/put_pid in the allocation
and free codepath. I expected those to be pretty cheap, but maybe
they're not?
-- 
Jeff Layton <jlayton@redhat.com>

WARNING: multiple messages have this Message-ID (diff)
From: Jeff Layton <jlayton@redhat.com>
To: lkp@lists.01.org
Subject: Re: [lkp-robot] [fs/locks] 9d21d181d0: will-it-scale.per_process_ops -14.1% regression
Date: Thu, 01 Jun 2017 08:59:21 -0400	[thread overview]
Message-ID: <1496321961.2845.6.camel@redhat.com> (raw)
In-Reply-To: <8F2C3CFF-5C2D-41B0-A895-B1F074DA7943@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On Thu, 2017-06-01 at 07:49 -0400, Benjamin Coddington wrote:
> On 1 Jun 2017, at 7:41, Jeff Layton wrote:
> 
> > On Thu, 2017-06-01 at 10:05 +0800, kernel test robot wrote:
> > > Greeting,
> > > 
> > > FYI, we noticed a -14.1% regression of will-it-scale.per_process_ops 
> > > due to commit:
> > > 
> > > 
> > > commit: 9d21d181d06acab9a8e80eac2ec4eed77b656793 ("fs/locks: Set 
> > > fl_nspid at file_lock allocation")
> > > url: 
> > > https://github.com/0day-ci/linux/commits/Benjamin-Coddington/fs-locks-Alloc-file_lock-where-practical/20170527-050700
> > > 
> > > 
> > 
> > Ouch, that's a rather nasty performance hit. In hindsight, maybe we
> > shouldn't move those off the stack after all? Heck, if it's that
> > significant, maybe we should move the F_SETLK callers to allocate 
> > these
> > on the stack as well?
> 
> We can do that.  But, I think this is picking up the 
> locks_mandatory_area()
> allocation which is now removed.  The attached config has
> CONFIG_MANDATORY_FILE_LOCKING=y,  so there's allocation on every 
> read/write.
> 

I'm not so sure. That would only be the case if the thing were marked
for manadatory locking (a really rare thing).

The test is really simple and I don't think any read/write activity is
involved:

    https://github.com/antonblanchard/will-it-scale/blob/master/tests/lock1.c

...and the 0 day bisected it down to this patch, IIUC:

    https://github.com/0day-ci/linux/commit/9d21d181d06acab9a8e80eac2ec4eed77b656793

It seems likely that it's the extra get_pid/put_pid in the allocation
and free codepath. I expected those to be pretty cheap, but maybe
they're not?
-- 
Jeff Layton <jlayton@redhat.com>

  reply	other threads:[~2017-06-01 12:59 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-26 20:14 [PATCH 0/3] Fixups for l_pid Benjamin Coddington
2017-05-26 20:14 ` [PATCH 1/3] fs/locks: Alloc file_lock where practical Benjamin Coddington
2017-05-27  9:56   ` Jeff Layton
2017-05-28  6:35   ` Christoph Hellwig
2017-05-26 20:14 ` [PATCH 2/3] fs/locks: Set fl_nspid at file_lock allocation Benjamin Coddington
2017-05-27 10:00   ` Jeff Layton
2017-06-01  2:05   ` [lkp-robot] [fs/locks] 9d21d181d0: will-it-scale.per_process_ops -14.1% regression kernel test robot
2017-06-01 11:41     ` Jeff Layton
2017-06-01 11:41       ` Jeff Layton
2017-06-01 11:49       ` Benjamin Coddington
2017-06-01 11:49         ` Benjamin Coddington
2017-06-01 12:59         ` Jeff Layton [this message]
2017-06-01 12:59           ` Jeff Layton
2017-06-01 15:14           ` J. Bruce Fields
2017-06-01 15:48             ` Jeff Layton
2017-06-01 15:48               ` Jeff Layton
2017-06-05 18:34               ` Benjamin Coddington
2017-06-05 18:34                 ` Benjamin Coddington
2017-06-05 22:02                 ` Jeff Layton
2017-06-05 22:02                   ` Jeff Layton
2017-06-06 13:00                   ` Benjamin Coddington
2017-06-06 13:00                     ` Benjamin Coddington
2017-06-06 13:15                     ` Jeff Layton
2017-06-06 13:15                       ` Jeff Layton
2017-06-06 13:21                       ` Benjamin Coddington
2017-06-06 13:21                         ` Benjamin Coddington
2017-05-26 20:14 ` [PATCH 3/3] fs/locks: Use fs-specific l_pid for remote locks Benjamin Coddington
2017-05-26 20:26 ` [PATCH 0/3] Fixups for l_pid Benjamin Coddington
2017-05-27 10:11 ` Jeff Layton

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=1496321961.2845.6.camel@redhat.com \
    --to=jlayton@redhat.com \
    --cc=bcodding@redhat.com \
    --cc=bfields@fieldses.org \
    --cc=hch@infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=lkp@01.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=xiaolong.ye@intel.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.