All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: Nicholas Piggin <npiggin@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: Randy Dunlap <rdunlap@infradead.org>,
	linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, linux-mm@kvack.org,
	Anton Blanchard <anton@ozlabs.org>
Subject: Re: [PATCH v4 2/4] lazy tlb: allow lazy tlb mm refcounting to be configurable
Date: Tue, 8 Jun 2021 09:20:05 -0700	[thread overview]
Message-ID: <8ac1d420-b861-f586-bacf-8c3949e9b5c4@kernel.org> (raw)
In-Reply-To: <20210605014216.446867-3-npiggin@gmail.com>

On 6/4/21 6:42 PM, Nicholas Piggin wrote:
> Add CONFIG_MMU_TLB_REFCOUNT which enables refcounting of the lazy tlb mm
> when it is context switched. This can be disabled by architectures that
> don't require this refcounting if they clean up lazy tlb mms when the
> last refcount is dropped. Currently this is always enabled, which is
> what existing code does, so the patch is effectively a no-op.
> 
> Rename rq->prev_mm to rq->prev_lazy_mm, because that's what it is.

I am in favor of this approach, but I would be a lot more comfortable
with the resulting code if task->active_mm were at least better
documented and possibly even guarded by ifdefs.

x86 bare metal currently does not need the core lazy mm refcounting, and
x86 bare metal *also* does not need ->active_mm.  Under the x86 scheme,
if lazy mm refcounting were configured out, ->active_mm could become a
dangling pointer, and this makes me extremely uncomfortable.

So I tend to think that, depending on config, the core code should
either keep ->active_mm [1] alive or get rid of it entirely.

[1] I don't really think it belongs in task_struct at all.  It's not a
property of the task.  It's the *per-cpu* mm that the core code is
keeping alive for lazy purposes.  How about consolidating it with the
copy in rq?

I guess the short summary of my opinion is that I like making this
configurable, but I do not like the state of the code.

--Andy

WARNING: multiple messages have this Message-ID (diff)
From: Andy Lutomirski <luto@kernel.org>
To: Nicholas Piggin <npiggin@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: linux-arch@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v4 2/4] lazy tlb: allow lazy tlb mm refcounting to be configurable
Date: Tue, 8 Jun 2021 09:20:05 -0700	[thread overview]
Message-ID: <8ac1d420-b861-f586-bacf-8c3949e9b5c4@kernel.org> (raw)
In-Reply-To: <20210605014216.446867-3-npiggin@gmail.com>

On 6/4/21 6:42 PM, Nicholas Piggin wrote:
> Add CONFIG_MMU_TLB_REFCOUNT which enables refcounting of the lazy tlb mm
> when it is context switched. This can be disabled by architectures that
> don't require this refcounting if they clean up lazy tlb mms when the
> last refcount is dropped. Currently this is always enabled, which is
> what existing code does, so the patch is effectively a no-op.
> 
> Rename rq->prev_mm to rq->prev_lazy_mm, because that's what it is.

I am in favor of this approach, but I would be a lot more comfortable
with the resulting code if task->active_mm were at least better
documented and possibly even guarded by ifdefs.

x86 bare metal currently does not need the core lazy mm refcounting, and
x86 bare metal *also* does not need ->active_mm.  Under the x86 scheme,
if lazy mm refcounting were configured out, ->active_mm could become a
dangling pointer, and this makes me extremely uncomfortable.

So I tend to think that, depending on config, the core code should
either keep ->active_mm [1] alive or get rid of it entirely.

[1] I don't really think it belongs in task_struct at all.  It's not a
property of the task.  It's the *per-cpu* mm that the core code is
keeping alive for lazy purposes.  How about consolidating it with the
copy in rq?

I guess the short summary of my opinion is that I like making this
configurable, but I do not like the state of the code.

--Andy

  parent reply	other threads:[~2021-06-08 16:20 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-05  1:42 [PATCH v4 0/4] shoot lazy tlbs Nicholas Piggin
2021-06-05  1:42 ` Nicholas Piggin
2021-06-05  1:42 ` [PATCH v4 1/4] lazy tlb: introduce lazy mm refcount helper functions Nicholas Piggin
2021-06-05  1:42   ` Nicholas Piggin
2021-06-07 23:49   ` Andrew Morton
2021-06-07 23:49     ` Andrew Morton
2021-06-08  1:39     ` Nicholas Piggin
2021-06-08  1:39       ` Nicholas Piggin
2021-06-08  1:48       ` Andrew Morton
2021-06-08  1:48         ` Andrew Morton
2021-06-08  4:11         ` Nicholas Piggin
2021-06-08  4:11           ` Nicholas Piggin
2021-06-05  1:42 ` [PATCH v4 2/4] lazy tlb: allow lazy tlb mm refcounting to be configurable Nicholas Piggin
2021-06-05  1:42   ` Nicholas Piggin
2021-06-08  3:11   ` Nicholas Piggin
2021-06-08  3:11     ` Nicholas Piggin
2021-06-08 16:20   ` Andy Lutomirski [this message]
2021-06-08 16:20     ` Andy Lutomirski
2021-06-14  0:45     ` Nicholas Piggin
2021-06-14  0:45       ` Nicholas Piggin
2021-06-14  3:52       ` Andy Lutomirski
2021-06-14  3:52         ` Andy Lutomirski
2021-06-14  4:14         ` Nicholas Piggin
2021-06-14  4:14           ` Nicholas Piggin
2021-06-14  4:47           ` Nicholas Piggin
2021-06-14  4:47             ` Nicholas Piggin
2021-06-14  5:21             ` Nicholas Piggin
2021-06-14  5:21               ` Nicholas Piggin
2021-06-14 16:20               ` Andy Lutomirski
2021-06-14 16:20                 ` Andy Lutomirski
2021-06-15  0:55                 ` Nicholas Piggin
2021-06-15  0:55                   ` Nicholas Piggin
2021-06-16  0:14                   ` Andy Lutomirski
2021-06-16  0:14                     ` Andy Lutomirski
2021-06-16  1:02                     ` Nicholas Piggin
2021-06-16  1:02                       ` Nicholas Piggin
2021-06-17  0:32                       ` Nicholas Piggin
2021-06-17  0:32                         ` Nicholas Piggin
2021-06-05  1:42 ` [PATCH v4 3/4] lazy tlb: shoot lazies, a non-refcounting lazy tlb option Nicholas Piggin
2021-06-05  1:42   ` Nicholas Piggin
2021-06-08  3:15   ` Nicholas Piggin
2021-06-08  3:15     ` Nicholas Piggin
2021-06-05  1:42 ` [PATCH v4 4/4] powerpc/64s: enable MMU_LAZY_TLB_SHOOTDOWN Nicholas Piggin
2021-06-05  1:42   ` Nicholas Piggin
2021-06-07 23:52   ` Andrew Morton
2021-06-07 23:52     ` Andrew Morton
2021-06-08  2:13     ` Nicholas Piggin
2021-06-08  2:13       ` Nicholas Piggin

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=8ac1d420-b861-f586-bacf-8c3949e9b5c4@kernel.org \
    --to=luto@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=anton@ozlabs.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=npiggin@gmail.com \
    --cc=rdunlap@infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.