LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Yang Shi <yang.shi@linux.alibaba.com>
Cc: akpm@linux-foundation.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org,
	Ricardo Neri <ricardo.neri-calderon@linux.intel.com>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Borislav Petkov <bp@suse.de>,
	x86@kernel.org
Subject: Re: [RFC PATCH 7/8] x86: mpx: pass atomic parameter to do_munmap()
Date: Tue, 20 Mar 2018 23:35:30 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.1803202307330.1714@nanos.tec.linutronix.de> (raw)
In-Reply-To: <1521581486-99134-8-git-send-email-yang.shi@linux.alibaba.com>

On Wed, 21 Mar 2018, Yang Shi wrote:

Please CC everyone involved on the full patch set next time. I had to dig
the rest out from my lkml archive to get the context.

> Pass "true" to do_munmap() to not do unlock/relock to mmap_sem when
> manipulating mpx map.

> This is API change only.

This is wrong. You cannot change the function in one patch and then clean
up the users. That breaks bisectability.

Depending on the number of callers this wants to be a single patch changing
both the function and the callers or you need to create a new function
which has the extra argument and switch all users over to it and then
remove the old function.

> @@ -780,7 +780,7 @@ static int unmap_entire_bt(struct mm_struct *mm,
>  	 * avoid recursion, do_munmap() will check whether it comes
>  	 * from one bounds table through VM_MPX flag.
>  	 */
> -	return do_munmap(mm, bt_addr, mpx_bt_size_bytes(mm), NULL);
> +	return do_munmap(mm, bt_addr, mpx_bt_size_bytes(mm), NULL, true);

But looking at the full context this is the wrong approach.

First of all the name of that parameter 'atomic' is completely
misleading. It suggests that this happens in fully atomic context, which is
not the case.

Secondly, conditional locking is frowned upon in general and rightfully so.

So the right thing to do is to leave do_munmap() alone and add a new
function do_munmap_huge() or whatever sensible name you come up with. Then
convert the places which are considered to be safe one by one with a proper
changelog which explains WHY this is safe.

That way you avoid the chasing game of all existing do_munmap() callers and
just use the new 'free in chunks' approach where it is appropriate and
safe. No suprises, no bisectability issues....

While at it please add proper kernel doc documentation to both do_munmap()
and the new function which explains the intricacies.

Thanks,

	tglx

  reply	other threads:[~2018-03-20 22:35 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-20 21:31 [RFC PATCH 0/8] Drop mmap_sem during unmapping large map Yang Shi
2018-03-20 21:31 ` [RFC PATCH 1/8] mm: mmap: unmap large mapping by section Yang Shi
2018-03-21 13:08   ` Michal Hocko
2018-03-21 16:31     ` Yang Shi
2018-03-21 17:29       ` Matthew Wilcox
2018-03-21 21:45         ` Yang Shi
2018-03-21 22:15           ` Matthew Wilcox
2018-03-21 22:40             ` Yang Shi
2018-03-21 22:46           ` Matthew Wilcox
2018-03-22 15:32             ` Laurent Dufour
2018-03-22 15:40               ` Matthew Wilcox
2018-03-22 15:54                 ` Laurent Dufour
2018-03-22 16:05                   ` Matthew Wilcox
2018-03-22 16:18                     ` Laurent Dufour
2018-03-22 16:46                       ` Yang Shi
2018-03-23 13:03                         ` Laurent Dufour
2018-03-22 16:51                       ` Matthew Wilcox
2018-03-22 16:49                     ` Yang Shi
2018-03-22 17:34         ` Yang Shi
2018-03-22 18:48           ` Matthew Wilcox
2018-03-24 18:24         ` Jerome Glisse
2018-03-21 13:14   ` Michal Hocko
2018-03-21 16:50     ` Yang Shi
2018-03-21 17:16       ` Yang Shi
2018-03-21 21:23         ` Michal Hocko
2018-03-21 22:36           ` Yang Shi
2018-03-22  9:10             ` Michal Hocko
2018-03-22 16:06               ` Yang Shi
2018-03-22 16:12                 ` Michal Hocko
2018-03-22 16:13                 ` Matthew Wilcox
2018-03-22 16:28                   ` Laurent Dufour
2018-03-22 16:36                     ` David Laight
2018-03-20 21:31 ` [RFC PATCH 2/8] mm: mmap: pass atomic parameter to do_munmap() call sites Yang Shi
2018-03-20 21:31 ` [RFC PATCH 3/8] mm: mremap: pass atomic parameter to do_munmap() Yang Shi
2018-03-20 21:31 ` [RFC PATCH 4/8] mm: nommu: add " Yang Shi
2018-03-20 21:31 ` [RFC PATCH 5/8] ipc: shm: pass " Yang Shi
2018-03-20 21:31 ` [RFC PATCH 6/8] fs: proc/vmcore: " Yang Shi
2018-03-20 21:31 ` [RFC PATCH 7/8] x86: mpx: " Yang Shi
2018-03-20 22:35   ` Thomas Gleixner [this message]
2018-03-21 16:53     ` Yang Shi
2018-03-20 21:31 ` [RFC PATCH 8/8] x86: vma: " Yang Shi

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=alpine.DEB.2.21.1803202307330.1714@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=akpm@linux-foundation.org \
    --cc=bp@suse.de \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@redhat.com \
    --cc=ricardo.neri-calderon@linux.intel.com \
    --cc=x86@kernel.org \
    --cc=yang.shi@linux.alibaba.com \
    --subject='Re: [RFC PATCH 7/8] x86: mpx: pass atomic parameter to do_munmap()' \
    /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).