Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-arch@vger.kernel.org, Kees Cook <keescook@chromium.org>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: remove the last set_fs() in common code, and remove it for x86 and powerpc
Date: Wed, 19 Aug 2020 09:16:59 +0200	[thread overview]
Message-ID: <f2e31c89-dd9e-f0f8-ef5c-e930d01a3b65@csgroup.eu> (raw)
In-Reply-To: <e3781661-2e13-4f46-d892-181907a2e768@csgroup.eu>



Le 18/08/2020 à 20:23, Christophe Leroy a écrit :
> 
> 
> Le 18/08/2020 à 20:05, Christoph Hellwig a écrit :
>> On Tue, Aug 18, 2020 at 07:46:22PM +0200, Christophe Leroy wrote:
>>> I gave it a go on my powerpc mpc832x. I tested it on top of my newest
>>> series that reworks the 32 bits signal handlers (see
>>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278) with 
>>>
>>> the microbenchmark test used is that series.
>>>
>>> With KUAP activated, on top of signal32 rework, performance is 
>>> boosted as
>>> system time for the microbenchmark goes from 1.73s down to 1.56s, 
>>> that is
>>> 10% quicker
>>>
>>> Surprisingly, with the kernel as is today without my signal's series, 
>>> your
>>> series degrades performance slightly (from 2.55s to 2.64s ie 3.5% 
>>> slower).
>>>
>>>
>>> I also observe, in both cases, a degradation on
>>>
>>>     dd if=/dev/zero of=/dev/null count=1M
>>>
>>> Without your series, it runs in 5.29 seconds.
>>> With your series, it runs in 5.82 seconds, that is 10% more time.
>>
>> That's pretty strage, I wonder if some kernel text cache line
>> effects come into play here?
>>
>> The kernel access side is only used in slow path code, so it should
>> not make a difference, and the uaccess code is simplified and should be
>> (marginally) faster.
>>
>> Btw, was this with the __{get,put}_user_allowed cockup that you noticed
>> fixed?
>>
> 
> Yes it is with the __get_user_size() replaced by __get_user_size_allowed().

I made a test with only the first patch of your series: That's 
definitely the culprit. With only that patch applies, the duration is 
6.64 seconds, that's a 25% degradation.

A perf record provides the following without the patch:
     41.91%  dd       [kernel.kallsyms]  [k] __arch_clear_user
      7.02%  dd       [kernel.kallsyms]  [k] vfs_read
      6.86%  dd       [kernel.kallsyms]  [k] new_sync_read
      6.68%  dd       [kernel.kallsyms]  [k] iov_iter_zero
      6.03%  dd       [kernel.kallsyms]  [k] transfer_to_syscall
      3.39%  dd       [kernel.kallsyms]  [k] memset
      3.07%  dd       [kernel.kallsyms]  [k] __fsnotify_parent
      2.68%  dd       [kernel.kallsyms]  [k] ksys_read
      2.09%  dd       [kernel.kallsyms]  [k] read_iter_zero
      2.01%  dd       [kernel.kallsyms]  [k] __fget_light
      1.84%  dd       [kernel.kallsyms]  [k] __fdget_pos
      1.35%  dd       [kernel.kallsyms]  [k] rw_verify_area
      1.32%  dd       libc-2.23.so       [.] __GI___libc_write
      1.21%  dd       [kernel.kallsyms]  [k] vfs_write
...
      0.03%  dd       [kernel.kallsyms]  [k] write_null

And the following with the patch:

     15.54%  dd       [kernel.kallsyms]  [k] __arch_clear_user
      9.17%  dd       [kernel.kallsyms]  [k] vfs_read
      6.54%  dd       [kernel.kallsyms]  [k] new_sync_write
      6.31%  dd       [kernel.kallsyms]  [k] transfer_to_syscall
      6.29%  dd       [kernel.kallsyms]  [k] __fsnotify_parent
      6.20%  dd       [kernel.kallsyms]  [k] new_sync_read
      5.47%  dd       [kernel.kallsyms]  [k] memset
      5.13%  dd       [kernel.kallsyms]  [k] vfs_write
      4.44%  dd       [kernel.kallsyms]  [k] iov_iter_zero
      2.95%  dd       [kernel.kallsyms]  [k] write_iter_null
      2.82%  dd       [kernel.kallsyms]  [k] ksys_read
      2.46%  dd       [kernel.kallsyms]  [k] __fget_light
      2.34%  dd       libc-2.23.so       [.] __GI___libc_read
      1.89%  dd       [kernel.kallsyms]  [k] iov_iter_advance
      1.76%  dd       [kernel.kallsyms]  [k] __fdget_pos
      1.65%  dd       [kernel.kallsyms]  [k] rw_verify_area
      1.63%  dd       [kernel.kallsyms]  [k] read_iter_zero
      1.60%  dd       [kernel.kallsyms]  [k] iov_iter_init
      1.22%  dd       [kernel.kallsyms]  [k] ksys_write
      1.14%  dd       libc-2.23.so       [.] __GI___libc_write

Christophe

> 
> Christophe

  reply	other threads:[~2020-08-19  7:17 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-17  7:32 Christoph Hellwig
2020-08-17  7:32 ` [PATCH 01/11] mem: remove duplicate ops for /dev/zero and /dev/null Christoph Hellwig
2020-08-18 19:33   ` Kees Cook
2020-08-17  7:32 ` [PATCH 02/11] fs: don't allow kernel reads and writes without iter ops Christoph Hellwig
2020-08-18 19:34   ` Kees Cook
2020-08-17  7:32 ` [PATCH 03/11] fs: don't allow splice read/write without explicit ops Christoph Hellwig
2020-08-18 19:39   ` Kees Cook
2020-08-18 19:54     ` Christoph Hellwig
2020-08-18 19:58       ` Kees Cook
2020-08-18 20:07         ` Christoph Hellwig
2020-08-17  7:32 ` [PATCH 04/11] uaccess: add infrastructure for kernel builds with set_fs() Christoph Hellwig
2020-08-18 19:40   ` Kees Cook
2020-08-17  7:32 ` [PATCH 05/11] test_bitmap: skip user bitmap tests for !CONFIG_SET_FS Christoph Hellwig
2020-08-17  7:50   ` Christophe Leroy
2020-08-17  7:52     ` Christoph Hellwig
2020-08-18 19:43   ` Kees Cook
2020-08-17  7:32 ` [PATCH 06/11] lkdtm: disable set_fs-based " Christoph Hellwig
2020-08-18 19:32   ` Kees Cook
2020-08-17  7:32 ` [PATCH 07/11] x86: move PAGE_OFFSET, TASK_SIZE & friends to page_{32,64}_types.h Christoph Hellwig
2020-08-18 19:27   ` Kees Cook
2020-08-17  7:32 ` [PATCH 08/11] x86: make TASK_SIZE_MAX usable from assembly code Christoph Hellwig
2020-08-18 19:44   ` Kees Cook
2020-08-18 19:55     ` Christoph Hellwig
2020-08-18 19:59       ` Kees Cook
2020-08-18 20:00         ` Christoph Hellwig
2020-08-18 20:08           ` Kees Cook
2020-08-17  7:32 ` [PATCH 09/11] x86: remove address space overrides using set_fs() Christoph Hellwig
2020-08-17  8:23   ` David Laight
2020-08-27  9:37     ` 'Christoph Hellwig'
2020-08-18 19:46   ` Kees Cook
2020-08-17  7:32 ` [PATCH 10/11] powerpc: use non-set_fs based maccess routines Christoph Hellwig
2020-08-17 15:47   ` Christophe Leroy
2020-08-17  7:32 ` [PATCH 11/11] powerpc: remove address space overrides using set_fs() Christoph Hellwig
2020-08-17  7:39 ` remove the last set_fs() in common code, and remove it for x86 and powerpc Christoph Hellwig
2020-08-18 17:46 ` Christophe Leroy
2020-08-18 18:05   ` Christoph Hellwig
2020-08-18 18:23     ` Christophe Leroy
2020-08-19  7:16       ` Christophe Leroy [this message]
2020-08-19  7:22         ` iter and normal ops on /dev/zero & co, was " Christoph Hellwig

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=f2e31c89-dd9e-f0f8-ef5c-e930d01a3b65@csgroup.eu \
    --to=christophe.leroy@csgroup.eu \
    --cc=hch@lst.de \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=x86@kernel.org \
    --subject='Re: remove the last set_fs() in common code, and remove it for x86 and powerpc' \
    /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

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).