Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1218680 > unrolled thread

[4.2, Regression] Queued spinlocks cause major XFS performance regression

Back to article view | Back to linux.kernel

Contents

  [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-04 07:50 +0200
    Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 08:40 +0200
      Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-04 09:20 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Juergen Gross <jgross@suse.com> - 2015-09-04 09:40 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 10:00 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-04 10:40 +0200
          Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 17:10 +0200
            Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 17:20 +0200
              Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 17:30 +0200
                Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-07 01:40 +0200
                Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Davidlohr Bueso <dave@stgolabs.net> - 2015-09-07 02:10 +0200
                  Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-07 09:00 +0200
              Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 17:30 +0200
                Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 17:40 +0200
                  Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 18:00 +0200
                  Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-04 18:00 +0200
                    Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-05 19:50 +0200
      Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 09:40 +0200
        Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Peter Zijlstra <peterz@infradead.org> - 2015-09-04 13:40 +0200
          Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-05 00:10 +0200
          Re: [4.2, Regression] Queued spinlocks cause major XFS performance  regression Dave Chinner <david@fromorbit.com> - 2015-09-07 01:50 +0200

Page 1 of 2  [1] 2  Next page →

#1218680 — [4.2, Regression] Queued spinlocks cause major XFS performance regression

Hi Waiman,

For the first time in months I just turned of spinlock debugging on
my performance test machine and I just got an unpleasant surprise on
my standard inode allocation and reclaim test.  I've described this
test to you before, because it's found regressions in your previous
lock scaling changes:

http://permalink.gmane.org/gmane.linux.kernel/1768786

This time it is the fsmark run that I use to populate the filesystem
that is demonstrating a locking regression. I'll asked you before
if you could add this test to your lock scaling regression test
suite; please do it this time.

Now, the regression.  With spinlock debugging turned on, the
performance of my usual XFS inode allocation benchmark using fsmark
reports performance like this:

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     312594.0          9944159
     0      3200000            0     295668.6         10399679
     0      4800000            0     279026.1         11397617
.....

This has been pretty stable for several releases - it varies +/- a
few percent, but it's pretty much been like this since about 3.2
when CONFIG_XFS_DEBUG=n, with or without basic spinlock debugging.

When I turned spinlock debugging off on 4.2 to get some perf numbers
a request from Linus, I got this:

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     114143.9          9597599
     0      3200000            0      95486.9          9460413
     0      4800000            0      93918.2          9784699
....

All 16 CPUs were pegged at 100% cpu usage. I took a quick look at
perf:

  67.32%  [kernel]       [k] queued_spin_lock_slowpath
   5.17%  [kernel]       [k] xfs_log_commit_cil
   2.47%  [kernel]       [k] _xfs_buf_find
   1.37%  [kernel]       [k] _raw_spin_lock
   ....

And then a quick call graph sample to find the lock:

   37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
   - queued_spin_lock_slowpath
      - 99.98% _raw_spin_lock
         - 89.16% xfs_log_commit_cil
            - __xfs_trans_commit
               - 98.48% xfs_trans_commit
                    xfs_create
                    xfs_generic_create
                    xfs_vn_mknod
                    xfs_vn_create
                    vfs_create
                    path_openat
                    do_filp_open
                    do_sys_open
                    sys_open
                    entry_SYSCALL_64_fastpath
                  + __GI___libc_open
               + 1.52% __xfs_trans_roll

This shows that we have catastrophic spinlock contention in the
transaction commit path. The cil->xc_cil_lock spin lock as it's the
only spinlock in that path. And while it's the hot lock in the
commit path, turning spinlock debugging back on (and no other
changes) shows that it shouldn't be contended:

   8.92%  [kernel]  [k] _xfs_buf_find
   5.51%  [kernel]  [k] xfs_dir2_node_addname
   3.49%  [kernel]  [k] xfs_dir3_free_hdr_from_disk
   3.45%  [kernel]  [k] do_raw_spin_lock
   3.06%  [kernel]  [k] kmem_cache_alloc
   2.99%  [kernel]  [k] __memcpy
   2.97%  [kernel]  [k] xfs_log_commit_cil
   .....

And the call graph:

    4.52%     0.24%  [kernel]            [k] _raw_spin_lock
   - _raw_spin_lock
      + 43.63% xfs_log_commit_cil
      + 9.76% inode_sb_list_add
      + 8.57% list_lru_add
      + 6.37% _xfs_buf_find
      + 6.08% d_alloc
      + 4.18% dput
      + 4.08% __d_instantiate
...

IOWs, about 2% of the cpu usage is lock traffic through
cil->xc_cil_lock, and that's 3x the locking rate where the queued
spinlock becomes catastrophically contention bound.

To confirm that this is indeed caused by the queued spinlocks, I
removed the the spinlock debugging and did this to arch/x86/Kconfig:

-       select ARCH_USE_QUEUED_SPINLOCK

And the results are:

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     329310.4          9727415
     0      3200000            0     305421.4         10421358
     0      4800000            0     294593.3         11007112
     0      6400000            0     283863.7         12557190
....

So the problem is definitely the queued spinlock...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1218686 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> When I turned spinlock debugging off on 4.2 to get some perf numbers
> a request from Linus, I got this:

[ ugly numbers deleted ]

> And then a quick call graph sample to find the lock:
>
>    37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
>    - queued_spin_lock_slowpath
>       - 99.98% _raw_spin_lock
>          - 89.16% xfs_log_commit_cil
[ snip ]
>
> This shows that we have catastrophic spinlock contention in the
> transaction commit path. The cil->xc_cil_lock spin lock as it's the
> only spinlock in that path. And while it's the hot lock in the
> commit path, turning spinlock debugging back on (and no other
> changes) shows that it shouldn't be contended:
>
>    8.92%  [kernel]  [k] _xfs_buf_find
[ snip ]

So you basically have almost no spinlock overhead at all even when
debugging is on.

That's unusual, as usually the debug code makes the contention much much worse.

> To confirm that this is indeed caused by the queued spinlocks, I
> removed the the spinlock debugging and did this to arch/x86/Kconfig:
>
> -       select ARCH_USE_QUEUED_SPINLOCK
>
> And the results are:

Ok, that's pretty conclusive. It doesn't seem to make much _sense_,
but numbers talk, BS walks.

If I read things right, the actual spinlock is the "cil->xc_cil_lock"
that is taken in xlog_cil_insert_items(), and it justr shows up in
xfs_log_commit_cil() in the call graph due to inlining. Correct?

There doesn't seem to be anything even remotely strange going on in that area.

Is this a PARAVIRT configuration? There were issues with PV
interaction at some point. If it is PV, and you don't actually use PV,
can you test with PV support disabled?

Also, if you look at the instruction-level profile for
queued_spin_lock_slowpath itself, does anything stand out? For
example, I note that the for-loop with the atomic_cmpxchg() call in it
doesn't ever do a cpu_relax(). It doesn't look like that should
normally loop, but obviously that function also shouldn't normally use
2/3rds of the cpu, so.. Maybe some part of queued_spin_lock_slowpath()
stands out as "it's spending 99% of the time in _that_ particular
part, and it gives some clue what goes wrong.

Ugh. Added Ingo to the cc. I *hate* locking problems, to the point
where I think we need to seriously consider undoing the queued
spinlocks if they have these kinds of odd issues..

Anybody have any ideas?

                         Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218698

On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
> On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > When I turned spinlock debugging off on 4.2 to get some perf numbers
> > a request from Linus, I got this:
> 
> [ ugly numbers deleted ]
> 
> > And then a quick call graph sample to find the lock:
> >
> >    37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
> >    - queued_spin_lock_slowpath
> >       - 99.98% _raw_spin_lock
> >          - 89.16% xfs_log_commit_cil
> [ snip ]
> >
> > This shows that we have catastrophic spinlock contention in the
> > transaction commit path. The cil->xc_cil_lock spin lock as it's the
> > only spinlock in that path. And while it's the hot lock in the
> > commit path, turning spinlock debugging back on (and no other
> > changes) shows that it shouldn't be contended:
> >
> >    8.92%  [kernel]  [k] _xfs_buf_find
> [ snip ]
> 
> So you basically have almost no spinlock overhead at all even when
> debugging is on.

*nod*

> That's unusual, as usually the debug code makes the contention much much worse.

Right. The debug behaviour is completely unchanged, that's why I
didn't notice this earlier. And it's not until I scale this workload
to >32p that is tend to see and significant level of contention on
the cil->xc_cil_lock when the basic spin lock debugging is enabled.

> > To confirm that this is indeed caused by the queued spinlocks, I
> > removed the the spinlock debugging and did this to arch/x86/Kconfig:
> >
> > -       select ARCH_USE_QUEUED_SPINLOCK
> >
> > And the results are:
> 
> Ok, that's pretty conclusive. It doesn't seem to make much _sense_,
> but numbers talk, BS walks.
> 
> If I read things right, the actual spinlock is the "cil->xc_cil_lock"
> that is taken in xlog_cil_insert_items(), and it justr shows up in
> xfs_log_commit_cil() in the call graph due to inlining. Correct?

Yup, that's how I read it, too. 

> There doesn't seem to be anything even remotely strange going on in that area.
> 
> Is this a PARAVIRT configuration? There were issues with PV
> interaction at some point. If it is PV, and you don't actually use PV,
> can you test with PV support disabled?

$ grep PARAVIRT .config
CONFIG_PARAVIRT=y
# CONFIG_PARAVIRT_DEBUG is not set
# CONFIG_PARAVIRT_SPINLOCKS is not set
CONFIG_PARAVIRT_TIME_ACCOUNTING=y
CONFIG_PARAVIRT_CLOCK=y
$

I'll retest with CONFIG_PARAVIRT=n....

> Also, if you look at the instruction-level profile for
> queued_spin_lock_slowpath itself, does anything stand out? For
> example, I note that the for-loop with the atomic_cmpxchg() call in it
> doesn't ever do a cpu_relax(). It doesn't look like that should
> normally loop, but obviously that function also shouldn't normally use
> 2/3rds of the cpu, so.. Maybe some part of queued_spin_lock_slowpath()
> stands out as "it's spending 99% of the time in _that_ particular
> part, and it gives some clue what goes wrong.

I'll have a look when the current tests on that machine have
finished running.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218704

On 09/04/2015 09:11 AM, Dave Chinner wrote:
> On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
>> On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
>>>
>>> When I turned spinlock debugging off on 4.2 to get some perf numbers
>>> a request from Linus, I got this:
>>
>> [ ugly numbers deleted ]
>>
>>> And then a quick call graph sample to find the lock:
>>>
>>>     37.19%    37.19%  [kernel]         [k] queued_spin_lock_slowpath
>>>     - queued_spin_lock_slowpath
>>>        - 99.98% _raw_spin_lock
>>>           - 89.16% xfs_log_commit_cil
>> [ snip ]
>>>
>>> This shows that we have catastrophic spinlock contention in the
>>> transaction commit path. The cil->xc_cil_lock spin lock as it's the
>>> only spinlock in that path. And while it's the hot lock in the
>>> commit path, turning spinlock debugging back on (and no other
>>> changes) shows that it shouldn't be contended:
>>>
>>>     8.92%  [kernel]  [k] _xfs_buf_find
>> [ snip ]
>>
>> So you basically have almost no spinlock overhead at all even when
>> debugging is on.
>
> *nod*
>
>> That's unusual, as usually the debug code makes the contention much much worse.
>
> Right. The debug behaviour is completely unchanged, that's why I
> didn't notice this earlier. And it's not until I scale this workload
> to >32p that is tend to see and significant level of contention on
> the cil->xc_cil_lock when the basic spin lock debugging is enabled.
>
>>> To confirm that this is indeed caused by the queued spinlocks, I
>>> removed the the spinlock debugging and did this to arch/x86/Kconfig:
>>>
>>> -       select ARCH_USE_QUEUED_SPINLOCK
>>>
>>> And the results are:
>>
>> Ok, that's pretty conclusive. It doesn't seem to make much _sense_,
>> but numbers talk, BS walks.
>>
>> If I read things right, the actual spinlock is the "cil->xc_cil_lock"
>> that is taken in xlog_cil_insert_items(), and it justr shows up in
>> xfs_log_commit_cil() in the call graph due to inlining. Correct?
>
> Yup, that's how I read it, too.
>
>> There doesn't seem to be anything even remotely strange going on in that area.
>>
>> Is this a PARAVIRT configuration? There were issues with PV
>> interaction at some point. If it is PV, and you don't actually use PV,
>> can you test with PV support disabled?
>
> $ grep PARAVIRT .config
> CONFIG_PARAVIRT=y
> # CONFIG_PARAVIRT_DEBUG is not set
> # CONFIG_PARAVIRT_SPINLOCKS is not set
> CONFIG_PARAVIRT_TIME_ACCOUNTING=y
> CONFIG_PARAVIRT_CLOCK=y
> $
>
> I'll retest with CONFIG_PARAVIRT=n....

Shouldn't matter at all. CONFIG_PARAVIRT_SPINLOCKS isn't set, so the
locks aren't para-virtualized.


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218715

On Fri, Sep 04, 2015 at 05:11:43PM +1000, Dave Chinner wrote:
> $ grep PARAVIRT .config
> CONFIG_PARAVIRT=y
> # CONFIG_PARAVIRT_DEBUG is not set
> # CONFIG_PARAVIRT_SPINLOCKS is not set
> CONFIG_PARAVIRT_TIME_ACCOUNTING=y
> CONFIG_PARAVIRT_CLOCK=y
> $

Curious, with PARAVIRT_SPINLOCKS=n we _should_ end up in
virt_queued_spin_lock() which is a simple test-and-set lock, much like
the lock debugging code should end up being.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218762

On Fri, Sep 04, 2015 at 05:11:43PM +1000, Dave Chinner wrote:
> On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
> > There doesn't seem to be anything even remotely strange going on in that area.
> > 
> > Is this a PARAVIRT configuration? There were issues with PV
> > interaction at some point. If it is PV, and you don't actually use PV,
> > can you test with PV support disabled?
> 
> $ grep PARAVIRT .config
> CONFIG_PARAVIRT=y
> # CONFIG_PARAVIRT_DEBUG is not set
> # CONFIG_PARAVIRT_SPINLOCKS is not set
> CONFIG_PARAVIRT_TIME_ACCOUNTING=y
> CONFIG_PARAVIRT_CLOCK=y
> $
> 
> I'll retest with CONFIG_PARAVIRT=n....

$ grep PARAVIRT .config
# CONFIG_PARAVIRT is not set
$


FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     123407.7          9202289
     0      3200000            0      97271.9          9187905
     0      4800000            0     101010.3         11246527
....

So, no, that doesn't affect the queued spinlock performance at all.

> > Also, if you look at the instruction-level profile for
> > queued_spin_lock_slowpath itself, does anything stand out? For
> > example, I note that the for-loop with the atomic_cmpxchg() call in it
> > doesn't ever do a cpu_relax(). It doesn't look like that should
> > normally loop, but obviously that function also shouldn't normally use
> > 2/3rds of the cpu, so.. Maybe some part of queued_spin_lock_slowpath()
> > stands out as "it's spending 99% of the time in _that_ particular
> > part, and it gives some clue what goes wrong.
> 
> I'll have a look when the current tests on that machine have
> finished running.

      ¿     Disassembly of section load2:
       ¿
       ¿     ffffffff810e0f30 <load2+0xe0f30>:
  0.00 ¿       nop
       ¿       push   %rbp
  0.00 ¿       mov    %rsp,%rbp
  0.00 ¿       xchg   %ax,%ax
       ¿       xor    %eax,%eax
  0.00 ¿       mov    $0x1,%edx
       ¿       lock   cmpxchg %edx,(%rdi)
  0.33 ¿       xor    %ecx,%ecx
       ¿       test   %eax,%eax
       ¿     ¿ je     28
  0.02 ¿ 1c:   pause
  4.45 ¿       mov    %ecx,%eax
  0.00 ¿       lock   cmpxchg %edx,(%rdi)
 95.18 ¿       test   %eax,%eax
       ¿     ¿ jne    1c
  0.01 ¿ 28:   pop    %rbp
  0.01 ¿     ¿ retq

.....

It looks like it's spending all it's time looping around the cmpxchg.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219103 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

On Fri, Sep 4, 2015 at 1:29 AM, Dave Chinner <david@fromorbit.com> wrote:
>
...
>   0.02  1c:   pause
>   4.45        mov    %ecx,%eax
>   0.00        lock   cmpxchg %edx,(%rdi)
>  95.18        test   %eax,%eax
>                jne    1c
...
> It looks like it's spending all it's time looping around the cmpxchg.

That code sequence doesn't look sensible. Busy-looping on a cmpxchg is
insane - if you are busy-looping, you should always make sure the
inner tight loop is done while waiting for the value.

It seems to come from virt_queued_spin_lock(), and that just looks
like completely bogus crap.

PeterZ, this is your magical hypervisor thing, and I get the feeling
that that explains why Dave sees nasty performance: most people have
tested either on raw hardware or using the actual paravirtualized
ones, but this is the case for "we're running with a hypervisor, but
not paravirtualized".

So virt_queued_spin_lock() for the hypervisor case looks completely
buggered to me for several reasons:

 - it doesn't actually ever use any queueing, since it always returns true

   so the "queued spinlocks" in this case aren't actually queued, and
they aren't even ticket-locks, they are just plain 0/1 values if I
read things right.

 - the busy-loop to set the queued spinlock uses that cmpxchg in a
tight loop, which kills any memory subsystem. That's unacceptable.

So at the very *minimum*, that second issue should be fixed, and the
loop in virt_queued_spin_lock() should look something like

    do {
        while (READ_ONCE(lock->val) != 0)
            cpu_relax();
    } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);

which at least has a chance in hell of behaving well on the bus and in
a HT environment.

But I suspect that it would be even better for Dave to just disable
the whole thing, and see how the queued locks actually work. Dave, can
you turn that virt_queued_spin_lock() into just "return false"? In
fact, I would almost _insist_ we do this when CONFIG_PARAVIRT_SPINLOCK
isn't set, isn't that what our old ticket-spinlocks did? They didn't
screw up and degrade to a test-and-set lock just because they saw a
hypervisor - that only happened when things were paravirt-aware. No?

Dave, if you have the energy, try it both ways. But the code as-is for
"I'm running in a hypervisor" looks just terminally broken. People who
didn't run in hypervisors just never saw the breakage.

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219109

On Fri, Sep 04, 2015 at 08:05:16AM -0700, Linus Torvalds wrote:
> So at the very *minimum*, that second issue should be fixed, and the
> loop in virt_queued_spin_lock() should look something like
> 
>     do {
>         while (READ_ONCE(lock->val) != 0)
>             cpu_relax();
>     } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> 
> which at least has a chance in hell of behaving well on the bus and in
> a HT environment.

True.

> But I suspect that it would be even better for Dave to just disable
> the whole thing, and see how the queued locks actually work. Dave, can
> you turn that virt_queued_spin_lock() into just "return false"? In
> fact, I would almost _insist_ we do this when CONFIG_PARAVIRT_SPINLOCK
> isn't set, isn't that what our old ticket-spinlocks did? They didn't
> screw up and degrade to a test-and-set lock just because they saw a
> hypervisor - that only happened when things were paravirt-aware. No?

The reason we chose to revert to a test-and-set is because regular fair
locks, like the ticket and the queue thing, have horrible behaviour
under vcpu preemption.

> Dave, if you have the energy, try it both ways. But the code as-is for
> "I'm running in a hypervisor" looks just terminally broken. People who
> didn't run in hypervisors just never saw the breakage.

He did, it mostly restores performance, but was quite erratic. Lock
holder preemption problems get much worse with strict queueing. So even
though he's typically not overloaded, any vcpu preemption can ripple
through and create noise.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219116

On Fri, Sep 04, 2015 at 05:14:27PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 04, 2015 at 08:05:16AM -0700, Linus Torvalds wrote:
> > So at the very *minimum*, that second issue should be fixed, and the
> > loop in virt_queued_spin_lock() should look something like
> > 
> >     do {
> >         while (READ_ONCE(lock->val) != 0)
> >             cpu_relax();
> >     } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> > 
> > which at least has a chance in hell of behaving well on the bus and in
> > a HT environment.
> 
> True.

Something like so...

---
Subject: locking: Fix virt test-and-set lock implementation

Dave ran into horrible performance on a VM without PARAVIRT_SPINLOCKS
set and Linus noted that the test-and-set implementation was retarded.

One should spin on the variable with a load, not a rmw.

While there, remove the queued from the name, as the lock isn't queued
at all, but a simple test-and-set.

Reported-by: Dave Chinner <david@fromorbit.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/qspinlock.h | 16 ++++++++++++----
 include/asm-generic/qspinlock.h  |  4 ++--
 kernel/locking/qspinlock.c       |  2 +-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 9d51fae1cba3..8dde3bdc4a05 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -39,15 +39,23 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 #endif
 
-#define virt_queued_spin_lock virt_queued_spin_lock
+#define virt_spin_lock virt_spin_lock
 
-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
+static inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
 		return false;
 
-	while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
-		cpu_relax();
+	/*
+	 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
+	 * back to a Test-and-Set spinlock, because fair locks have
+	 * horrible lock 'holder' preemption issues.
+	 */
+
+	do {
+		while (atomic_read(&lock->val) != 0)
+			cpu_relax();
+	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
 
 	return true;
 }
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index 83bfb87f5bf1..e2aadbc7151f 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -111,8 +111,8 @@ static inline void queued_spin_unlock_wait(struct qspinlock *lock)
 		cpu_relax();
 }
 
-#ifndef virt_queued_spin_lock
-static __always_inline bool virt_queued_spin_lock(struct qspinlock *lock)
+#ifndef virt_spin_lock
+static __always_inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	return false;
 }
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 337c8818541d..87e9ce6a63c5 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -289,7 +289,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
 	if (pv_enabled())
 		goto queue;
 
-	if (virt_queued_spin_lock(lock))
+	if (virt_spin_lock(lock))
 		return;
 
 	/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219874

On Fri, Sep 04, 2015 at 05:25:23PM +0200, Peter Zijlstra wrote:
> Something like so...
> 
> ---
> Subject: locking: Fix virt test-and-set lock implementation
> 
> Dave ran into horrible performance on a VM without PARAVIRT_SPINLOCKS
> set and Linus noted that the test-and-set implementation was retarded.
> 
> One should spin on the variable with a load, not a rmw.
> 
> While there, remove the queued from the name, as the lock isn't queued
> at all, but a simple test-and-set.
> 
> Reported-by: Dave Chinner <david@fromorbit.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

FSUse%        Count         Size    Files/sec     App Overhead
     0      1600000            0     319511.8          9398733
     0      3200000            0     301517.9          9516230
     0      4800000            0     300357.6          9676651
     0      6400000            0     282167.8         11513255

Looks OK.

Tested-by: Dave Chinner <david@fromorbit.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219880

On Fri, 04 Sep 2015, Peter Zijlstra wrote:

>-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
>+static inline bool virt_spin_lock(struct qspinlock *lock)

Given that we fall back to the cmpxchg loop even when PARAVIRT is not in the
picture, I believe this function is horribly misnamed.

> {
> 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> 		return false;
>
>-	while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0)
>-		cpu_relax();
>+	/*
>+	 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>+	 * back to a Test-and-Set spinlock, because fair locks have
>+	 * horrible lock 'holder' preemption issues.
>+	 */
>+

This comment is also misleading... but if you tuck the whole function
under some PARAVIRT option, it obviously makes sense to just leave as is.
And let native actually _use_ qspinlocks.

>+	do {
>+		while (atomic_read(&lock->val) != 0)
>+			cpu_relax();
>+	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);

CCAS to the rescue again.

Thanks,
Davidlohr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219954

On Sun, Sep 06, 2015 at 05:05:46PM -0700, Davidlohr Bueso wrote:
> On Fri, 04 Sep 2015, Peter Zijlstra wrote:
> 
> >-static inline bool virt_queued_spin_lock(struct qspinlock *lock)
> >+static inline bool virt_spin_lock(struct qspinlock *lock)
> 
> Given that we fall back to the cmpxchg loop even when PARAVIRT is not in the
> picture, I believe this function is horribly misnamed.

Just to continue the argument for arguments sake, the function is named
'virt' (not paravirt) and tests the HYPERVISOR CPUID bit. How is that
not appropriately named?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219120 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

On Fri, Sep 4, 2015 at 8:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> The reason we chose to revert to a test-and-set is because regular fair
> locks, like the ticket and the queue thing, have horrible behaviour
> under vcpu preemption.

Right. However, with our old ticket locks, that's what we got when you
didn't ask for paravirt support. No?

So this seems to be a misfeature - you made the hypervisor "support"
unconditional.  Even a kernel compiled for raw hardware now does that
"let's act differently under a hypervisor", which I think is quite
debatable to begin with, but when that "act differently" is then
complete garbage, it's a disaster.

And even ignoring the "implementation was crap" issue, some people may
well want their kernels to be "bare hardware" kernels even under a
hypervisor. It may be a slim hypervisor that gives you all the cpus,
or it may just be a system that is just sufficiently overprovisioned,
so you don't get vcpu preemption in practice.

But it would be interesting to hear if just fixing the busy-looping to
not pound the lock with a constant stream of cmpxchg's is already
sufficient to fix the big picture problem.

                    Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219122

On Fri, Sep 04, 2015 at 08:21:28AM -0700, Linus Torvalds wrote:
> On Fri, Sep 4, 2015 at 8:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > The reason we chose to revert to a test-and-set is because regular fair
> > locks, like the ticket and the queue thing, have horrible behaviour
> > under vcpu preemption.
> 
> Right. However, with our old ticket locks, that's what we got when you
> didn't ask for paravirt support. No?

Indeed.

> And even ignoring the "implementation was crap" issue, some people may
> well want their kernels to be "bare hardware" kernels even under a
> hypervisor. It may be a slim hypervisor that gives you all the cpus,
> or it may just be a system that is just sufficiently overprovisioned,
> so you don't get vcpu preemption in practice.

Fair enough; I had not considered the slim hypervisor case.

Should I place the virt_spin_lock() thing under CONFIG_PARAVIRT (maybe
even _SPINLOCKS) such that only paravirt enabled kernels when ran on a
hypervisor that does not support paravirt patching (HyperV, VMware,
etc..) revert to the test-and-set?

> But it would be interesting to hear if just fixing the busy-looping to
> not pound the lock with a constant stream of cmpxchg's is already
> sufficient to fix the big picture problem.

Dave replaced the cpu_relax() with a __delay(1) to match what
spinlock-debug does and that fixed things for him.

Of course, it would be good if he can try the proposed patch too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219134

On Fri, Sep 04, 2015 at 05:30:35PM +0200, Peter Zijlstra wrote:
> Should I place the virt_spin_lock() thing under CONFIG_PARAVIRT (maybe
> even _SPINLOCKS) such that only paravirt enabled kernels when ran on a
> hypervisor that does not support paravirt patching (HyperV, VMware,
> etc..) revert to the test-and-set?

Ah, CONFIG_HYPERVISOR_GUEST seems fitting, that's a prerequisite for all
the PARAVIRT options too.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219139 — Re: [4.2, Regression] Queued spinlocks cause major XFS performance regression

On Fri, Sep 4, 2015 at 8:30 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> And even ignoring the "implementation was crap" issue, some people may
>> well want their kernels to be "bare hardware" kernels even under a
>> hypervisor. It may be a slim hypervisor that gives you all the cpus,
>> or it may just be a system that is just sufficiently overprovisioned,
>> so you don't get vcpu preemption in practice.
>
> Fair enough; I had not considered the slim hypervisor case.
>
> Should I place the virt_spin_lock() thing under CONFIG_PARAVIRT (maybe
> even _SPINLOCKS) such that only paravirt enabled kernels when ran on a
> hypervisor that does not support paravirt patching (HyperV, VMware,
> etc..) revert to the test-and-set?

My gut feel would be to try to match out old paravirt setup, which
similarly replaced the ticket locks with the test-and-set lock, and
try to match the situation where that happened?

Looking at 4.1, back then we very statically just based on
CONFIG_PARAVIRT_SPINLOCKS switched between the ticket lock behavior
and the test-and-set lock. I think we should aim for matching that for
now.

Which is not to say that we can't tune this if somebody comes up with
a better model. For example, the "test hypervisor bit" thing might
still be a good idea: even *if* you have CONFIG_PARAVIRT_SPINLOCKS,
maybe we can do the queued locks if we don't seem to be running under
a hypervisor? Our old model was entirely static, the new queued
spinlock slowpath could clearly be a *bit* more dynamic.

But as a first rough draft, I think "replace ticket locks with queued
locks, leave test-and-set lock condition the way it was" is the way to
go. Particularly since clearly the virtualized behavior had not gotten
enough testing..

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219590

On Fri, Sep 04, 2015 at 08:58:38AM -0700, Linus Torvalds wrote:
> On Fri, Sep 4, 2015 at 8:30 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> And even ignoring the "implementation was crap" issue, some people may
> >> well want their kernels to be "bare hardware" kernels even under a
> >> hypervisor. It may be a slim hypervisor that gives you all the cpus,
> >> or it may just be a system that is just sufficiently overprovisioned,
> >> so you don't get vcpu preemption in practice.
> >
> > Fair enough; I had not considered the slim hypervisor case.
> >
> > Should I place the virt_spin_lock() thing under CONFIG_PARAVIRT (maybe
> > even _SPINLOCKS) such that only paravirt enabled kernels when ran on a
> > hypervisor that does not support paravirt patching (HyperV, VMware,
> > etc..) revert to the test-and-set?
> 
> My gut feel would be to try to match out old paravirt setup, which
> similarly replaced the ticket locks with the test-and-set lock, and
> try to match the situation where that happened?

I'm not sure there was a test-and-set option in 4.1.

Either the hypervisor layer implemented paravirt spinlocks (Xen, KVM)
(and you selected CONFIG_PARAVIRT_SPINLOCKS, which had a fairly large
negative impact on native code), or you got our native locking.

So if you want I can simply remove the whole test-and-set thing, but I'd
rather fix it and put it under one of the PARAVIRT options.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218706

On Thu, Sep 03, 2015 at 11:39:21PM -0700, Linus Torvalds wrote:
> On Thu, Sep 3, 2015 at 10:48 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > When I turned spinlock debugging off on 4.2 to get some perf numbers
> > a request from Linus, I got this:

> > To confirm that this is indeed caused by the queued spinlocks, I
> > removed the the spinlock debugging and did this to arch/x86/Kconfig:
> >
> > -       select ARCH_USE_QUEUED_SPINLOCK
> >
> > And the results are:

> Is this a PARAVIRT configuration? There were issues with PV
> interaction at some point. If it is PV, and you don't actually use PV,
> can you test with PV support disabled?

His earlier instructions on how to reproduce do indeed have the workload
running inside a VM.

Dave, can you confirm you're still running this in a VM, and can you
provide your full guest .config?  In particular the PARAVIRT_SPINLOCKS
option is interesting.

Also, are you doing anything special with the host?

I'll try and poke at this, but I'm not much into this whole VM thing so
it'll probably be a while before I even have a simple guest running :/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218845

On Fri, Sep 04, 2015 at 06:12:34PM +1000, Dave Chinner wrote:
> You probably don't even need a VM to reproduce it - that would
> certainly be an interesting counterpoint if it didn't....

Even though you managed to restore your DEBUG_SPINLOCK performance by
changing virt_queued_spin_lock() to use __delay(1), I ran the thing on
actual hardware just to test.

[ Note: In any case, I would recommend you use (or at least try)
  PARAVIRT_SPINLOCKS if you use VMs, as that is where we were looking for
  performance, the test-and-set fallback really wasn't meant as a
  performance option (although it clearly sucks worse than expected).

  Pre qspinlock, your setup would have used regular ticket locks on
  vCPUs, which mostly works as long as there is almost no vCPU
  preemption, if you overload your machine such that the vCPU threads
  get preempted that will implode into silly-land. ]

So on to native performance:

 - IVB-EX, 4-socket, 15 core, hyperthreaded, for a total of 120 CPUs
 - 1.1T of md-stripe (5x200GB) SSDs
 - Linux v4.2 (distro style .config)
 - Debian "testing" base system
 - xfsprogs v3.2.1


# mkfs.xfs -f -m "crc=1,finobt=1" /dev/md0
log stripe unit (524288 bytes) is too large (maximum is 256KiB)
log stripe unit adjusted to 32KiB
meta-data=/dev/md0               isize=512    agcount=32, agsize=9157504 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1
data     =                       bsize=4096   blocks=293038720, imaxpct=5
         =                       sunit=128    swidth=640 blks
naming   =version 2              bsize=4096   ascii-ci=0 ftype=1
log      =internal log           bsize=4096   blocks=143088, version=2
         =                       sectsz=512   sunit=8 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0

# mount -o logbsize=262144,nobarrier /dev/md0 /mnt/scratch

# ./fs_mark  -D  10000  -S0  -n  50000  -s  0  -L  32 \
         -d  /mnt/scratch/0  -d  /mnt/scratch/1 \
         -d  /mnt/scratch/2  -d  /mnt/scratch/3 \
         -d  /mnt/scratch/4  -d  /mnt/scratch/5 \
         -d  /mnt/scratch/6  -d  /mnt/scratch/7 \
         -d  /mnt/scratch/8  -d  /mnt/scratch/9 \
         -d  /mnt/scratch/10  -d  /mnt/scratch/11 \
         -d  /mnt/scratch/12  -d  /mnt/scratch/13 \
         -d  /mnt/scratch/14  -d  /mnt/scratch/15 \


Regular v4.2 (qspinlock) does:

     0      6400000            0     286491.9          3500179
     0      7200000            0     293229.5          3963140
     0      8000000            0     271182.4          3708212
     0      8800000            0     300592.0          3595722

Modified v4.2 (ticket) does:

     0      6400000            0     310419.6          3343821
     0      7200000            0     348346.5          4721133
     0      8000000            0     328098.2          3235753
     0      8800000            0     316765.3          3238971


Which shows that qspinlock is clearly slower, even for these large-ish
NUMA boxes where it was supposed to be better.

Clearly our benchmarks used before this were not sufficient, and more
works needs to be done.


Also, I note that after running to completion, there is only 14G of
actual data on the device, so you don't need silly large storage to run
this -- I expect your previous 275G quote was due to XFS populating the
sparse file with meta-data or something along those lines.

Further note, rm -rf /mnt/scratch0/*, takes for bloody ever :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219342

On Fri, Sep 04, 2015 at 01:32:33PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 04, 2015 at 06:12:34PM +1000, Dave Chinner wrote:
> > You probably don't even need a VM to reproduce it - that would
> > certainly be an interesting counterpoint if it didn't....
> 
> Even though you managed to restore your DEBUG_SPINLOCK performance by
> changing virt_queued_spin_lock() to use __delay(1), I ran the thing on
> actual hardware just to test.
> 
> [ Note: In any case, I would recommend you use (or at least try)
>   PARAVIRT_SPINLOCKS if you use VMs, as that is where we were looking for
>   performance, the test-and-set fallback really wasn't meant as a
>   performance option (although it clearly sucks worse than expected).

I will try it, but that can happen when I've got a bit of spare
time...

>   Pre qspinlock, your setup would have used regular ticket locks on
>   vCPUs, which mostly works as long as there is almost no vCPU
>   preemption, if you overload your machine such that the vCPU threads
>   get preempted that will implode into silly-land. ]

I don't tend to overload the host CPUs - all my test loads are IO
bound - so this has never really been a problem I've noticed
in the past.

> So on to native performance:
> 
>  - IVB-EX, 4-socket, 15 core, hyperthreaded, for a total of 120 CPUs
>  - 1.1T of md-stripe (5x200GB) SSDs
>  - Linux v4.2 (distro style .config)
>  - Debian "testing" base system
>  - xfsprogs v3.2.1
> 
> 
> # mkfs.xfs -f -m "crc=1,finobt=1" /dev/md0

If you use xfsprogs v3.2.4 (current debian unstable) these are the
default options.

> log stripe unit (524288 bytes) is too large (maximum is 256KiB)
> log stripe unit adjusted to 32KiB
> meta-data=/dev/md0               isize=512    agcount=32, agsize=9157504 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=1        finobt=1
> data     =                       bsize=4096   blocks=293038720, imaxpct=5
>          =                       sunit=128    swidth=640 blks
> naming   =version 2              bsize=4096   ascii-ci=0 ftype=1
> log      =internal log           bsize=4096   blocks=143088, version=2
>          =                       sectsz=512   sunit=8 blks, lazy-count=1
> realtime =none                   extsz=4096   blocks=0, rtextents=0
> 
> # mount -o logbsize=262144,nobarrier /dev/md0 /mnt/scratch
> 
> # ./fs_mark  -D  10000  -S0  -n  50000  -s  0  -L  32 \
>          -d  /mnt/scratch/0  -d  /mnt/scratch/1 \
>          -d  /mnt/scratch/2  -d  /mnt/scratch/3 \
>          -d  /mnt/scratch/4  -d  /mnt/scratch/5 \
>          -d  /mnt/scratch/6  -d  /mnt/scratch/7 \
>          -d  /mnt/scratch/8  -d  /mnt/scratch/9 \
>          -d  /mnt/scratch/10  -d  /mnt/scratch/11 \
>          -d  /mnt/scratch/12  -d  /mnt/scratch/13 \
>          -d  /mnt/scratch/14  -d  /mnt/scratch/15 \
> 
> 
> Regular v4.2 (qspinlock) does:
> 
>      0      6400000            0     286491.9          3500179
>      0      7200000            0     293229.5          3963140
>      0      8000000            0     271182.4          3708212
>      0      8800000            0     300592.0          3595722
> 
> Modified v4.2 (ticket) does:
> 
>      0      6400000            0     310419.6          3343821
>      0      7200000            0     348346.5          4721133
>      0      8000000            0     328098.2          3235753
>      0      8800000            0     316765.3          3238971
> 
> 
> Which shows that qspinlock is clearly slower, even for these large-ish
> NUMA boxes where it was supposed to be better.

Be careful just reading the throughput numbers like that. You can
have the files/s number go down, but the benchmark wall time get
faster because the userspace portion runs faster (i.e. CPU cache
residency effects).  In this case, however, both the userspace time
is down by 5-10% and the files/s is up by 5-10%, so (without knowing
the wall time) I'd say that there is significance in these
numbers....

FWIW. you've got a lot more CPUs than I have - you can scale up the
parallelism of the workload by increasing the number of working
directories (i.e. -d <dir> options). You'd also need to scale up the
amount of allocation concurrency in XFS - 32 AGs will be the
limiting factor for any more workload concurrency. i.e. use "-d
agcount=<xxx>" on the mkfs.xfs command line to increase the AG
count. For artificial scalability testing like this, you want the AG
count ot be at least 2x the number of directories you are working in
concurrently.

> Clearly our benchmarks used before this were not sufficient, and more
> works needs to be done.
> 
> 
> Also, I note that after running to completion, there is only 14G of
> actual data on the device, so you don't need silly large storage to run
> this -- I expect your previous 275G quote was due to XFS populating the
> sparse file with meta-data or something along those lines.

Yeah, that would have been after lots of other work being done on
the sparse file I use to back the 500TB filesystem I test on in the
VM. Currently:

$ ls -lh /mnt/fast-ssd
total 61G
-rw------- 1 root root 500T Sep  4 19:36 vm-500t.img
$ df -h /mnt/fast-ssd
Filesystem      Size  Used Avail Use% Mounted on
/dev/sdb        400G   61G  340G  16% /mnt/fast-ssd
$

I'm using 61GB of space in the file that backs the 500TB device I'm
testing against. Every so often I punch out the file so that it gets
laid out again- I usually do that after running btrfs testing as
btrfs fragments the crap out of the backing file, even with extent
size hints set to minimise the fragmentation...

> Further note, rm -rf /mnt/scratch0/*, takes for bloody ever :-)

That's why I do it in parallel - step 6 of my test script is:

echo removing files
for f in /mnt/scratch/* ; do time rm -rf $f &  done
wait

And so:

.....
removing files

real    4m2.752s
user    0m3.387s
sys     2m56.801s
....
real    4m17.326s
user    0m3.333s
sys     2m57.831s
$

It takes a lot less than forever :)

Really, the fsmark run is just the part of my concurrent XFS inode
test script that takes about 20 minutes to run. It does:

	Prep: mkfs, mount
	1. run fsmark to create inodes in parallel
	2. run xfs_repair with maximum concurrency
	3. run multi-threaded bulkstat
	4. run concurrent find+stat
	5. run concurrent ls -R
	6. run concurrent rm -rf

It stresses all sorts of stuff:

	- steps 1 and 6 stress the XFS inode allocation and
	  transaction subsystems - it runs at about 4-500,000
	  transaction commits a second here.

	- Step 2 absolutely thrashes the mmap_sem from userspace due
	  to the memory demand and concurrent access patterns of
	  xfs_repair.

	- Step 3 is a cold cache inode traversal - it pushes close
	  to a million inodes/second through the slab caches.  It
	  puts a hell of a lot of load on the inode and xfs_buf slab
	  cache, the xfs_buf slab shrinker and all the VFS inode
	  instantiation and teardown paths. It is currently limited
	  in scalability by the inode_sb_list_lock contention.

	- Step 4 and 5 do different types of directory traversal,
	  putting heavy demand on the XFS buffer cache and inode
	  cache shrinkers to work effectively.

I have several variants - small files, different filesystems,
different directory structures, etc - because they all stress
different aspects of filesystem and core infrastructure. It's found
locking regressions. It's found mm/ subsystem regressions. It's
found writeback regressions. It's found all sorts of bugs in my code
over the years - it's a very useful test, so I keep using it. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web