LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Davidlohr Bueso <dave@stgolabs.net>
To: Oleg Nesterov <oleg@redhat.com>
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 07:54:30 -0700	[thread overview]
Message-ID: <1426431270.28068.92.camel@stgolabs.net> (raw)
In-Reply-To: <20150315142137.GA21741@redhat.com>

On Sun, 2015-03-15 at 15:21 +0100, Oleg Nesterov wrote:
> 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.

Looking at some of the caller paths now, I have to disagree.

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

How could you remove this? I mean it's user functionality, so you need
some way of keeping track of a changed file. But you might be talking
about something else.

> Not after your patch which adds another dependency.

I don't add another dependency, I just replace the current one.

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

I've tried to make it as clear as possible this is a not performance
patch. I guess I've failed. Let me repeat it again: this is *not*
performance motivated ;) This kind of things under mmap_sem prevents
lock breakup.

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

If this series goes to the dumpster then ok I'll send a patch for this,
I have no objection.

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

Could you explain this? Are you referring to the audit.c user? If so
that caller has already been updated.

> 
> 
> Speaking of cleanups... IIRC Konstantin suggested to rcuify this pointer
> and I agree, this looks better than the new lock.

Yes, I can do that in patch 1, but as mentioned, rcu is not really the
question to me, it's the lock for when we change the exe file, so if
it's not mmap_sem we'd still need another lock. If mmap_sem is kept, yes
we can use the read lock in things like get_mm_exe_file() and still rely
on the file ref counting so we wouldn't need to do everything under rcu,
which was a though I originally had.

Thanks,
Davidlohr


  reply	other threads:[~2015-03-15 14:54 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 ` [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file serialization Oleg Nesterov
2015-03-15 14:54   ` Davidlohr Bueso [this message]
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=1426431270.28068.92.camel@stgolabs.net \
    --to=dave@stgolabs.net \
    --cc=akpm@linux-foundation.org \
    --cc=gorcunov@openvz.org \
    --cc=koct9i@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=oleg@redhat.com \
    --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).