All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Jerome Glisse <jglisse@redhat.com>
Cc: "John Hubbard" <jhubbard@nvidia.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-xfs@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Christian König" <christian.koenig@amd.com>,
	"Jan Kara" <jack@suse.cz>
Subject: Re: [PATCH] Initialise mmu_notifier_range correctly
Date: Thu, 3 Jan 2019 06:39:08 -0800	[thread overview]
Message-ID: <20190103143908.GQ6310@bombadil.infradead.org> (raw)
In-Reply-To: <20190103142959.GA3395@redhat.com>

On Thu, Jan 03, 2019 at 09:29:59AM -0500, Jerome Glisse wrote:
> On Wed, Jan 02, 2019 at 08:18:33PM -0800, Matthew Wilcox wrote:
> > On Wed, Jan 02, 2019 at 07:32:08PM -0800, John Hubbard wrote:
> > > Having the range struct declared in separate places from the mmu_notifier_range_init()
> > > calls is not great. But I'm not sure I see a way to make it significantly cleaner, given
> > > that __follow_pte_pmd uses the range pointer as a way to decide to issue the mmn calls.
> > 
> > Yeah, I don't think there's anything we can do.  But I started reviewing
> > the comments, and they don't make sense together:
> > 
> >                 /*
> >                  * Note because we provide range to follow_pte_pmd it will
> >                  * call mmu_notifier_invalidate_range_start() on our behalf
> >                  * before taking any lock.
> >                  */
> >                 if (follow_pte_pmd(vma->vm_mm, address, &range,
> >                                    &ptep, &pmdp, &ptl))
> >                         continue;
> > 
> >                 /*
> >                  * No need to call mmu_notifier_invalidate_range() as we are
> >                  * downgrading page table protection not changing it to point
> >                  * to a new page.
> >                  *
> >                  * See Documentation/vm/mmu_notifier.rst
> >                  */
> > 
> > So if we don't call mmu_notifier_invalidate_range, why are we calling
> > mmu_notifier_invalidate_range_start and mmu_notifier_invalidate_range_end?
> > ie, why not this ...
> 
> Thus comments looks wrong to me ... we need to call
> mmu_notifier_invalidate_range() those are use by
> IOMMU. I might be to blame for those comments thought.

Yes, you're to blame for both of them.

a4d1a88525138 (Jérôme Glisse     2017-08-31 17:17:26 -0400  791)                 * Note because we provide start/end to follow_pte_pmd it will
a4d1a88525138 (Jérôme Glisse     2017-08-31 17:17:26 -0400  792)                 * call mmu_notifier_invalidate_range_start() on our behalf
a4d1a88525138 (Jérôme Glisse     2017-08-31 17:17:26 -0400  793)                 * before taking any lock.

0f10851ea475e (Jérôme Glisse     2017-11-15 17:34:07 -0800  794)                 * No need to call mmu_notifier_invalidate_range() as we are
0f10851ea475e (Jérôme Glisse     2017-11-15 17:34:07 -0800  795)                 * downgrading page table protection not changing it to point
0f10851ea475e (Jérôme Glisse     2017-11-15 17:34:07 -0800  796)                 * to a new page.


WARNING: multiple messages have this Message-ID (diff)
From: Matthew Wilcox <willy@infradead.org>
To: Jerome Glisse <jglisse@redhat.com>
Cc: "John Hubbard" <jhubbard@nvidia.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-xfs@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Christian König" <christian.koenig@amd.com>,
	"Jan Kara" <jack@suse.cz>
Subject: Re: [PATCH] Initialise mmu_notifier_range correctly
Date: Thu, 3 Jan 2019 06:39:08 -0800	[thread overview]
Message-ID: <20190103143908.GQ6310@bombadil.infradead.org> (raw)
In-Reply-To: <20190103142959.GA3395@redhat.com>

On Thu, Jan 03, 2019 at 09:29:59AM -0500, Jerome Glisse wrote:
> On Wed, Jan 02, 2019 at 08:18:33PM -0800, Matthew Wilcox wrote:
> > On Wed, Jan 02, 2019 at 07:32:08PM -0800, John Hubbard wrote:
> > > Having the range struct declared in separate places from the mmu_notifier_range_init()
> > > calls is not great. But I'm not sure I see a way to make it significantly cleaner, given
> > > that __follow_pte_pmd uses the range pointer as a way to decide to issue the mmn calls.
> > 
> > Yeah, I don't think there's anything we can do.  But I started reviewing
> > the comments, and they don't make sense together:
> > 
> >                 /*
> >                  * Note because we provide range to follow_pte_pmd it will
> >                  * call mmu_notifier_invalidate_range_start() on our behalf
> >                  * before taking any lock.
> >                  */
> >                 if (follow_pte_pmd(vma->vm_mm, address, &range,
> >                                    &ptep, &pmdp, &ptl))
> >                         continue;
> > 
> >                 /*
> >                  * No need to call mmu_notifier_invalidate_range() as we are
> >                  * downgrading page table protection not changing it to point
> >                  * to a new page.
> >                  *
> >                  * See Documentation/vm/mmu_notifier.rst
> >                  */
> > 
> > So if we don't call mmu_notifier_invalidate_range, why are we calling
> > mmu_notifier_invalidate_range_start and mmu_notifier_invalidate_range_end?
> > ie, why not this ...
> 
> Thus comments looks wrong to me ... we need to call
> mmu_notifier_invalidate_range() those are use by
> IOMMU. I might be to blame for those comments thought.

Yes, you're to blame for both of them.

a4d1a88525138 (J�r�me Glisse     2017-08-31 17:17:26 -0400  791)                 * Note because we provide start/end to follow_pte_pmd it will
a4d1a88525138 (J�r�me Glisse     2017-08-31 17:17:26 -0400  792)                 * call mmu_notifier_invalidate_range_start() on our behalf
a4d1a88525138 (J�r�me Glisse     2017-08-31 17:17:26 -0400  793)                 * before taking any lock.

0f10851ea475e (J�r�me Glisse     2017-11-15 17:34:07 -0800  794)                 * No need to call mmu_notifier_invalidate_range() as we are
0f10851ea475e (J�r�me Glisse     2017-11-15 17:34:07 -0800  795)                 * downgrading page table protection not changing it to point
0f10851ea475e (J�r�me Glisse     2017-11-15 17:34:07 -0800  796)                 * to a new page.

  reply	other threads:[~2019-01-03 14:39 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-03  0:21 [PATCH] Initialise mmu_notifier_range correctly Matthew Wilcox
2019-01-03  1:56 ` Jerome Glisse
2019-01-03  1:56   ` Jerome Glisse
2019-01-03  3:32   ` John Hubbard
2019-01-03  4:18     ` Matthew Wilcox
2019-01-03 14:29       ` Jerome Glisse
2019-01-03 14:39         ` Matthew Wilcox [this message]
2019-01-03 14:39           ` Matthew Wilcox
2019-01-03 14:59           ` Jerome Glisse
2019-01-03 14:59             ` Jerome Glisse
2019-01-03 14:31 ` Jerome Glisse
2019-01-03 14:36   ` Matthew Wilcox
2019-01-03 14:43   ` Matthew Wilcox
2019-01-03 15:05     ` Jerome Glisse
2019-01-03 15:05       ` Jerome Glisse

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=20190103143908.GQ6310@bombadil.infradead.org \
    --to=willy@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=christian.koenig@amd.com \
    --cc=jack@suse.cz \
    --cc=jglisse@redhat.com \
    --cc=jhubbard@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-xfs@vger.kernel.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.