From: Christoph Hellwig <hch@lst.de>
To: Dan Williams <dan.j.williams@intel.com>
Cc: "Jason Gunthorpe" <jgg@mellanox.com>,
"Christoph Hellwig" <hch@lst.de>,
"Jérôme Glisse" <jglisse@redhat.com>,
"Ben Skeggs" <bskeggs@redhat.com>,
"Felix Kuehling" <Felix.Kuehling@amd.com>,
"Ralph Campbell" <rcampbell@nvidia.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 04/15] mm: remove the pgmap field from struct hmm_vma_walk
Date: Thu, 8 Aug 2019 08:59:33 +0200 [thread overview]
Message-ID: <20190808065933.GA29382@lst.de> (raw)
In-Reply-To: <CAPcyv4hPCuHBLhSJgZZEh0CbuuJNPLFDA3f-79FX5uVOO0yubA@mail.gmail.com>
On Wed, Aug 07, 2019 at 11:47:22AM -0700, Dan Williams wrote:
> > Unrelated to this patch, but what is the point of getting checking
> > that the pgmap exists for the page and then immediately releasing it?
> > This code has this pattern in several places.
> >
> > It feels racy
>
> Agree, not sure what the intent is here. The only other reason call
> get_dev_pagemap() is to just check in general if the pfn is indeed
> owned by some ZONE_DEVICE instance, but if the intent is to make sure
> the device is still attached/enabled that check is invalidated at
> put_dev_pagemap().
>
> If it's the former case, validating ZONE_DEVICE pfns, I imagine we can
> do something cheaper with a helper that is on the order of the same
> cost as pfn_valid(). I.e. replace PTE_DEVMAP with a mem_section flag
> or something similar.
The hmm literally never dereferences the pgmap, so validity checking is
the only explanation for it.
> > + /*
> > + * We do put_dev_pagemap() here so that we can leverage
> > + * get_dev_pagemap() optimization which will not re-take a
> > + * reference on a pgmap if we already have one.
> > + */
> > + if (hmm_vma_walk->pgmap)
> > + put_dev_pagemap(hmm_vma_walk->pgmap);
> > +
>
> Seems ok, but only if the caller is guaranteeing that the range does
> not span outside of a single pagemap instance. If that guarantee is
> met why not just have the caller pass in a pinned pagemap? If that
> guarantee is not met, then I think we're back to your race concern.
It iterates over multiple ptes in a non-huge pmd. Is there any kind of
limitations on different pgmap instances inside a pmd? I can't think
of one, so this might actually be a bug.
next prev parent reply other threads:[~2019-08-08 6:59 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-06 16:05 hmm cleanups, v2 Christoph Hellwig
2019-08-06 16:05 ` [PATCH 01/15] amdgpu: remove -EAGAIN handling for hmm_range_fault Christoph Hellwig
2019-08-06 16:05 ` [PATCH 02/15] amdgpu: don't initialize range->list in amdgpu_hmm_init_range Christoph Hellwig
2019-08-06 16:05 ` [PATCH 03/15] nouveau: pass struct nouveau_svmm to nouveau_range_fault Christoph Hellwig
2019-08-06 18:02 ` Jason Gunthorpe
2019-08-06 16:05 ` [PATCH 04/15] mm: remove the pgmap field from struct hmm_vma_walk Christoph Hellwig
2019-08-07 17:45 ` Jason Gunthorpe
2019-08-07 18:47 ` Dan Williams
2019-08-08 6:59 ` Christoph Hellwig [this message]
2019-08-14 1:36 ` Dan Williams
2019-08-14 7:38 ` Christoph Hellwig
2019-08-14 13:27 ` Jason Gunthorpe
2019-08-14 14:48 ` Dan Williams
2019-08-15 18:03 ` Jerome Glisse
2019-08-15 19:22 ` Jason Gunthorpe
2019-08-15 19:36 ` Dan Williams
2019-08-15 19:43 ` Jerome Glisse
2019-08-15 20:12 ` Dan Williams
2019-08-15 20:33 ` Jerome Glisse
2019-08-15 20:41 ` Jason Gunthorpe
2019-08-15 20:47 ` Dan Williams
2019-08-16 0:40 ` Jason Gunthorpe
2019-08-16 3:54 ` Dan Williams
2019-08-16 12:24 ` Jason Gunthorpe
2019-08-16 17:21 ` Dan Williams
2019-08-16 17:28 ` Jason Gunthorpe
2019-08-16 21:10 ` Ralph Campbell
2019-08-15 20:51 ` Jerome Glisse
2019-08-16 0:43 ` Jason Gunthorpe
2019-08-16 4:44 ` Christoph Hellwig
2019-08-16 12:30 ` Jason Gunthorpe
2019-08-16 12:34 ` Christoph Hellwig
2019-08-16 4:41 ` Christoph Hellwig
2019-08-06 16:05 ` [PATCH 05/15] mm: remove the unused vma argument to hmm_range_dma_unmap Christoph Hellwig
2019-08-06 16:05 ` [PATCH 06/15] mm: remove superflous arguments from hmm_range_register Christoph Hellwig
2019-08-06 16:05 ` [PATCH 07/15] mm: remove the page_shift member from struct hmm_range Christoph Hellwig
2019-08-07 17:51 ` Jason Gunthorpe
2019-08-06 16:05 ` [PATCH 08/15] mm: remove the mask variable in hmm_vma_walk_hugetlb_entry Christoph Hellwig
2019-08-06 18:02 ` Jason Gunthorpe
2019-08-06 16:05 ` [PATCH 09/15] mm: don't abuse pte_index() in hmm_vma_handle_pmd Christoph Hellwig
2019-08-07 17:18 ` Jason Gunthorpe
2019-08-06 16:05 ` [PATCH 10/15] mm: only define hmm_vma_walk_pud if needed Christoph Hellwig
2019-08-06 16:05 ` [PATCH 11/15] mm: cleanup the hmm_vma_handle_pmd stub Christoph Hellwig
2019-08-06 18:00 ` Jason Gunthorpe
2019-08-06 16:05 ` [PATCH 12/15] mm: cleanup the hmm_vma_walk_hugetlb_entry stub Christoph Hellwig
2019-08-06 16:05 ` [PATCH 13/15] mm: allow HMM_MIRROR on all architectures with MMU Christoph Hellwig
2019-08-06 16:05 ` [PATCH 14/15] mm: make HMM_MIRROR an implicit option Christoph Hellwig
2019-08-06 17:44 ` Jason Gunthorpe
2019-08-06 16:05 ` [PATCH 15/15] amdgpu: remove CONFIG_DRM_AMDGPU_USERPTR Christoph Hellwig
2019-08-06 17:44 ` Jason Gunthorpe
2019-08-06 17:51 ` Kuehling, Felix
2019-08-06 18:58 ` Alex Deucher
2019-08-06 20:03 ` Jason Gunthorpe
2019-08-07 6:57 ` Koenig, Christian
2019-08-07 11:46 ` Jason Gunthorpe
2019-08-07 18:17 ` hmm cleanups, v2 Jason Gunthorpe
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=20190808065933.GA29382@lst.de \
--to=hch@lst.de \
--cc=Felix.Kuehling@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=bskeggs@redhat.com \
--cc=dan.j.williams@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=jgg@mellanox.com \
--cc=jglisse@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=nouveau@lists.freedesktop.org \
--cc=rcampbell@nvidia.com \
/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 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).