LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Davidlohr Bueso <dave@stgolabs.net>
Cc: akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
	gorcunov@openvz.org, koct9i@gmail.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file serialization
Date: Sun, 15 Mar 2015 15:21:37 +0100	[thread overview]
Message-ID: <20150315142137.GA21741@redhat.com> (raw)
In-Reply-To: <1426372766-3029-1-git-send-email-dave@stgolabs.net>

I didn't even read this version, but honestly I don't like it anyway.

I leave the review to Cyrill and Konstantin though, If they like these
changes I won't argue.

But I simply can't understand why are you doing this.



Yes, this code needs cleanups, I agree. Does this series makes it better?
To me it doesn't, and the diffstat below shows that it blows the code.

In fact, to me it complicates this code. For example. Personally I think
that MMF_EXE_FILE_CHANGED should die. And currently we can just remove it.
Not after your patch which adds another dependency.



Or do you think this is performance improvement? I don't think so. Yes,
prctl() abuses mmap_sem, but this not a hot path and the task can only
abuse its own ->mm.

OK, I agree, dup_mm_exe_file() is horrible. But as I already said it can
simply die. We can move this code into dup_mmap() and avoid another
down_read/up_read.


Hmm. And this series is simply wrong without more changes in audit paths.
Unfortunately this is fixable, but let me NACK at least this version ;)


Speaking of cleanups... IIRC Konstantin suggested to rcuify this pointer
and I agree, this looks better than the new lock. But in fact I think
that the cleanups should start with s/get_mm_exe_file//get_mm_exe_path/.
Note that nobody actually needs "file *", every caller needs "struct path".
Plus kill dup_mm_exe_file().

Oleg.


On 03/14, Davidlohr Bueso wrote:
>
> This is a set I created on top of patch 1/4 which also includes mm_struct cleanups
> and dealing with prctl exe_file functionality. Specific details are in each patch.
> Patch 4 is an extra trivial one I found while going through the code.
>
> Applies on top of next-20150313.
>
> Thanks!
>
> Davidlohr Bueso (4):
>   mm: replace mmap_sem for mm->exe_file serialization
>   mm: introduce struct exe_file
>   prctl: move MMF_EXE_FILE_CHANGED into exe_file struct
>   kernel/fork: use pr_alert() for rss counter bugs
>
>  fs/exec.c                |   6 +++
>  include/linux/mm.h       |   4 ++
>  include/linux/mm_types.h |   8 +++-
>  include/linux/sched.h    |   5 +--
>  kernel/fork.c            |  72 ++++++++++++++++++++++++++------
>  kernel/sys.c             | 106 +++++++++++++++++++++++++++--------------------
>  6 files changed, 141 insertions(+), 60 deletions(-)
>
> --
> 2.1.4
>


  parent reply	other threads:[~2015-03-15 14:23 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-14 22:39 Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 1/4] " Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 2/4] mm: introduce struct exe_file Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 3/4] prctl: move MMF_EXE_FILE_CHANGED into exe_file struct Davidlohr Bueso
2015-03-15  2:13   ` Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 4/4] kernel/fork: use pr_alert() for rss counter bugs Davidlohr Bueso
2015-03-16 11:30   ` Konstantin Khlebnikov
2015-03-15 14:21 ` Oleg Nesterov [this message]
2015-03-15 14:54   ` [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file serialization Davidlohr Bueso
2015-03-15 15:26     ` Oleg Nesterov
2015-03-15 15:42       ` Davidlohr Bueso
2015-03-15 17:05         ` Cyrill Gorcunov
2015-03-15 17:34           ` Davidlohr Bueso
2015-03-16 22:08           ` Kees Cook
2015-03-20 16:09             ` Cyrill Gorcunov

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=20150315142137.GA21741@redhat.com \
    --to=oleg@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=dave@stgolabs.net \
    --cc=gorcunov@openvz.org \
    --cc=koct9i@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file serialization' \
    /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).