All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
To: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: "Ralph Campbell"
	<rcampbell-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>,
	"amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	"nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	"Felix Kuehling" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	"linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org"
	<linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org>,
	"Jérôme Glisse" <jglisse-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	"Jason Gunthorpe" <jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>,
	"Ben Skeggs" <bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	"Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.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-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

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.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

WARNING: multiple messages have this Message-ID (diff)
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.


  parent reply	other threads:[~2019-08-08  6:59 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-06 16:05 hmm cleanups, v2 Christoph Hellwig
2019-08-06 16:05 ` 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 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
     [not found]       ` <CAPcyv4hPCuHBLhSJgZZEh0CbuuJNPLFDA3f-79FX5uVOO0yubA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-08-08  6:59         ` Christoph Hellwig [this message]
2019-08-08  6:59           ` Christoph Hellwig
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
     [not found]                     ` <20190815180325.GA4920-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-08-15 19:22                       ` Jason Gunthorpe
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
     [not found]                                 ` <CAPcyv4j_Mxbw+T+yXTMdkrMoS_uxg+TXXgTM_EPBJ8XfXKxytA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-08-16  0:40                                   ` Jason Gunthorpe
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
     [not found]                                 ` <20190815205132.GC25517-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-08-16  0:43                                   ` Jason Gunthorpe
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
     [not found]                             ` <20190815203306.GB25517-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-08-16  4:41                               ` 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
     [not found] ` <20190806160554.14046-1-hch-jcswGhMUV9g@public.gmane.org>
2019-08-06 16:05   ` [PATCH 03/15] nouveau: pass struct nouveau_svmm to nouveau_range_fault Christoph Hellwig
2019-08-06 16:05     ` Christoph Hellwig
2019-08-06 18:02     ` Jason Gunthorpe
2019-08-06 16:05   ` [PATCH 07/15] mm: remove the page_shift member from struct hmm_range Christoph Hellwig
2019-08-06 16:05     ` Christoph Hellwig
     [not found]     ` <20190806160554.14046-8-hch-jcswGhMUV9g@public.gmane.org>
2019-08-07 17:51       ` Jason Gunthorpe
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 16:05     ` 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-06 16:05     ` 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     ` Christoph Hellwig
2019-08-06 16:05   ` [PATCH 12/15] mm: cleanup the hmm_vma_walk_hugetlb_entry stub Christoph Hellwig
2019-08-06 16:05     ` Christoph Hellwig
2019-08-06 16:05   ` [PATCH 14/15] mm: make HMM_MIRROR an implicit option Christoph Hellwig
2019-08-06 16:05     ` 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 16:05     ` 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
     [not found]             ` <20190806200356.GU11627-uk2M96/98Pc@public.gmane.org>
2019-08-07  6:57               ` Koenig, Christian
2019-08-07  6:57                 ` Koenig, Christian
2019-08-07 11:46                 ` Jason Gunthorpe
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 13/15] mm: allow HMM_MIRROR on all architectures with MMU Christoph Hellwig
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-jcswghmuv9g@public.gmane.org \
    --cc=Felix.Kuehling-5C7GfCeVMHo@public.gmane.org \
    --cc=amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
    --cc=bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
    --cc=jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org \
    --cc=jglisse-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org \
    --cc=nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
    --cc=rcampbell-DDmLM1+adcrQT0dZR+AlfA@public.gmane.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.