| Message ID | 20190626122724.13313-17-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [01/25] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option | expand |
On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > The functionality is identical to the one currently open coded in > device-dax. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/dax/dax-private.h | 4 ---- > drivers/dax/device.c | 43 --------------------------------------- > 2 files changed, 47 deletions(-) > > diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h > index b4177aafbbd1..c915889d1769 100644 > --- a/drivers/dax/dax-private.h > +++ b/drivers/dax/dax-private.h > @@ -43,8 +43,6 @@ struct dax_region { > * @target_node: effective numa node if dev_dax memory range is onlined > * @dev - device core > * @pgmap - pgmap for memmap setup / lifetime (driver owned) > - * @ref: pgmap reference count (driver owned) > - * @cmp: @ref final put completion (driver owned) > */ > struct dev_dax { > struct dax_region *region; > @@ -52,8 +50,6 @@ struct dev_dax { > int target_node; > struct device dev; > struct dev_pagemap pgmap; > - struct percpu_ref ref; > - struct completion cmp; > }; > > static inline struct dev_dax *to_dev_dax(struct device *dev) > diff --git a/drivers/dax/device.c b/drivers/dax/device.c > index b5257038c188..1af823b2fe6b 100644 > --- a/drivers/dax/device.c > +++ b/drivers/dax/device.c > @@ -14,36 +14,6 @@ > #include "dax-private.h" > #include "bus.h" > > -static struct dev_dax *ref_to_dev_dax(struct percpu_ref *ref) > -{ > - return container_of(ref, struct dev_dax, ref); > -} > - > -static void dev_dax_percpu_release(struct percpu_ref *ref) > -{ > - struct dev_dax *dev_dax = ref_to_dev_dax(ref); > - > - dev_dbg(&dev_dax->dev, "%s\n", __func__); > - complete(&dev_dax->cmp); > -} > - > -static void dev_dax_percpu_exit(struct dev_pagemap *pgmap) > -{ > - struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap); > - > - dev_dbg(&dev_dax->dev, "%s\n", __func__); > - wait_for_completion(&dev_dax->cmp); > - percpu_ref_exit(pgmap->ref); > -} > - > -static void dev_dax_percpu_kill(struct dev_pagemap *pgmap) > -{ > - struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap); > - > - dev_dbg(&dev_dax->dev, "%s\n", __func__); > - percpu_ref_kill(pgmap->ref); > -} > - > static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, > const char *func) > { > @@ -441,11 +411,6 @@ static void dev_dax_kill(void *dev_dax) > kill_dev_dax(dev_dax); > } > > -static const struct dev_pagemap_ops dev_dax_pagemap_ops = { > - .kill = dev_dax_percpu_kill, > - .cleanup = dev_dax_percpu_exit, > -}; > - > int dev_dax_probe(struct device *dev) > { > struct dev_dax *dev_dax = to_dev_dax(dev); > @@ -463,15 +428,7 @@ int dev_dax_probe(struct device *dev) > return -EBUSY; > } > > - init_completion(&dev_dax->cmp); > - rc = percpu_ref_init(&dev_dax->ref, dev_dax_percpu_release, 0, > - GFP_KERNEL); > - if (rc) > - return rc; > - > - dev_dax->pgmap.ref = &dev_dax->ref; > dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX; > - dev_dax->pgmap.ops = &dev_dax_pagemap_ops; > addr = devm_memremap_pages(dev, &dev_dax->pgmap); > if (IS_ERR(addr)) > return PTR_ERR(addr); > -- > 2.20.1 > > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > The functionality is identical to the one currently open coded in > device-dax. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/dax/dax-private.h | 4 ---- > drivers/dax/device.c | 43 --------------------------------------- > 2 files changed, 47 deletions(-) DanW: I think this series has reached enough review, did you want to ack/test any further? This needs to land in hmm.git soon to make the merge window. Thanks, Jason
On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > > The functionality is identical to the one currently open coded in > > device-dax. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > --- > > drivers/dax/dax-private.h | 4 ---- > > drivers/dax/device.c | 43 --------------------------------------- > > 2 files changed, 47 deletions(-) > > DanW: I think this series has reached enough review, did you want > to ack/test any further? > > This needs to land in hmm.git soon to make the merge window. I was awaiting a decision about resolving the collision with Ira's patch before testing the final result again [1]. You can go ahead and add my reviewed-by for the series, but my tested-by should be on the final state of the series. [1]: https://lore.kernel.org/lkml/CAPcyv4gTOf+EWzSGrFrh2GrTZt5HVR=e+xicUKEpiy57px8J+w@mail.gmail.com/
On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote: > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > > > The functionality is identical to the one currently open coded in > > > device-dax. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > drivers/dax/dax-private.h | 4 ---- > > > drivers/dax/device.c | 43 --------------------------------------- > > > 2 files changed, 47 deletions(-) > > > > DanW: I think this series has reached enough review, did you want > > to ack/test any further? > > > > This needs to land in hmm.git soon to make the merge window. > > I was awaiting a decision about resolving the collision with Ira's > patch before testing the final result again [1]. You can go ahead and > add my reviewed-by for the series, but my tested-by should be on the > final state of the series. The conflict looks OK to me, I think we can let Andrew and Linus resolve it. Thanks, Jason
On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote: > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > > > > The functionality is identical to the one currently open coded in > > > > device-dax. > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > drivers/dax/dax-private.h | 4 ---- > > > > drivers/dax/device.c | 43 --------------------------------------- > > > > 2 files changed, 47 deletions(-) > > > > > > DanW: I think this series has reached enough review, did you want > > > to ack/test any further? > > > > > > This needs to land in hmm.git soon to make the merge window. > > > > I was awaiting a decision about resolving the collision with Ira's > > patch before testing the final result again [1]. You can go ahead and > > add my reviewed-by for the series, but my tested-by should be on the > > final state of the series. > > The conflict looks OK to me, I think we can let Andrew and Linus > resolve it. > Andrew's tree effectively always rebases since it's a quilt series. I'd recommend pulling Ira's patch out of -mm and applying it with the rest of hmm reworks. Any other git tree I'd agree with just doing the late conflict resolution, but I'm not clear on what's the best practice when conflicting with -mm.
On Fri, Jun 28, 2019 at 10:08 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote: > > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > > > > > The functionality is identical to the one currently open coded in > > > > > device-dax. > > > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > > drivers/dax/dax-private.h | 4 ---- > > > > > drivers/dax/device.c | 43 --------------------------------------- > > > > > 2 files changed, 47 deletions(-) > > > > > > > > DanW: I think this series has reached enough review, did you want > > > > to ack/test any further? > > > > > > > > This needs to land in hmm.git soon to make the merge window. > > > > > > I was awaiting a decision about resolving the collision with Ira's > > > patch before testing the final result again [1]. You can go ahead and > > > add my reviewed-by for the series, but my tested-by should be on the > > > final state of the series. > > > > The conflict looks OK to me, I think we can let Andrew and Linus > > resolve it. > > > > Andrew's tree effectively always rebases since it's a quilt series. > I'd recommend pulling Ira's patch out of -mm and applying it with the > rest of hmm reworks. Any other git tree I'd agree with just doing the > late conflict resolution, but I'm not clear on what's the best > practice when conflicting with -mm. Regardless the patch is buggy. If you want to do the conflict resolution it should be because the DEVICE_PUBLIC removal effectively does the same fix otherwise we're knowingly leaving a broken point in the history.
On Fri, Jun 28, 2019 at 10:10:12AM -0700, Dan Williams wrote: > On Fri, Jun 28, 2019 at 10:08 AM Dan Williams <dan.j.williams@intel.com> wrote: > > > > On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote: > > > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > > > > > > The functionality is identical to the one currently open coded in > > > > > > device-dax. > > > > > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > > > drivers/dax/dax-private.h | 4 ---- > > > > > > drivers/dax/device.c | 43 --------------------------------------- > > > > > > 2 files changed, 47 deletions(-) > > > > > > > > > > DanW: I think this series has reached enough review, did you want > > > > > to ack/test any further? > > > > > > > > > > This needs to land in hmm.git soon to make the merge window. > > > > > > > > I was awaiting a decision about resolving the collision with Ira's > > > > patch before testing the final result again [1]. You can go ahead and > > > > add my reviewed-by for the series, but my tested-by should be on the > > > > final state of the series. > > > > > > The conflict looks OK to me, I think we can let Andrew and Linus > > > resolve it. > > > > Andrew's tree effectively always rebases since it's a quilt series. > > I'd recommend pulling Ira's patch out of -mm and applying it with the > > rest of hmm reworks. Any other git tree I'd agree with just doing the > > late conflict resolution, but I'm not clear on what's the best > > practice when conflicting with -mm. What happens depends on timing as things arrive to Linus. I promised to send hmm.git early, so I understand that Andrew will quilt rebase his tree to Linus's and fix the conflict in Ira's patch before he sends it. > Regardless the patch is buggy. If you want to do the conflict > resolution it should be because the DEVICE_PUBLIC removal effectively > does the same fix otherwise we're knowingly leaving a broken point in > the history. I'm not sure I understand your concern, is there something wrong with CH's series as it stands? hmm is a non-rebasing git tree, so as long as the series is correct *when I apply it* there is no broken history. I assumed the conflict resolution for Ira's patch was to simply take the deletion of the if block from CH's series - right? If we do need to take Ira's patch into hmm.git it will go after CH's series (and Ira will have to rebase/repost it), so I think there is nothing to do at this moment - unless you are saying there is a problem with the series in CH's git tree? Regards, Jason
On Fri, Jun 28, 2019 at 11:29 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > On Fri, Jun 28, 2019 at 10:10:12AM -0700, Dan Williams wrote: > > On Fri, Jun 28, 2019 at 10:08 AM Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > > > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote: > > > > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > > > > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote: > > > > > > > The functionality is identical to the one currently open coded in > > > > > > > device-dax. > > > > > > > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > > > > > > drivers/dax/dax-private.h | 4 ---- > > > > > > > drivers/dax/device.c | 43 --------------------------------------- > > > > > > > 2 files changed, 47 deletions(-) > > > > > > > > > > > > DanW: I think this series has reached enough review, did you want > > > > > > to ack/test any further? > > > > > > > > > > > > This needs to land in hmm.git soon to make the merge window. > > > > > > > > > > I was awaiting a decision about resolving the collision with Ira's > > > > > patch before testing the final result again [1]. You can go ahead and > > > > > add my reviewed-by for the series, but my tested-by should be on the > > > > > final state of the series. > > > > > > > > The conflict looks OK to me, I think we can let Andrew and Linus > > > > resolve it. > > > > > > Andrew's tree effectively always rebases since it's a quilt series. > > > I'd recommend pulling Ira's patch out of -mm and applying it with the > > > rest of hmm reworks. Any other git tree I'd agree with just doing the > > > late conflict resolution, but I'm not clear on what's the best > > > practice when conflicting with -mm. > > What happens depends on timing as things arrive to Linus. I promised > to send hmm.git early, so I understand that Andrew will quilt rebase > his tree to Linus's and fix the conflict in Ira's patch before he > sends it. > > > Regardless the patch is buggy. If you want to do the conflict > > resolution it should be because the DEVICE_PUBLIC removal effectively > > does the same fix otherwise we're knowingly leaving a broken point in > > the history. > > I'm not sure I understand your concern, is there something wrong with > CH's series as it stands? hmm is a non-rebasing git tree, so as long > as the series is correct *when I apply it* there is no broken history. > > I assumed the conflict resolution for Ira's patch was to simply take > the deletion of the if block from CH's series - right? > > If we do need to take Ira's patch into hmm.git it will go after CH's > series (and Ira will have to rebase/repost it), so I think there is > nothing to do at this moment - unless you are saying there is a > problem with the series in CH's git tree? There is a problem with the series in CH's tree. It removes the ->page_free() callback from the release_pages() path because it goes too far and removes the put_devmap_managed_page() call.
On Fri, Jun 28, 2019 at 11:44:35AM -0700, Dan Williams wrote: > There is a problem with the series in CH's tree. It removes the > ->page_free() callback from the release_pages() path because it goes > too far and removes the put_devmap_managed_page() call. release_pages only called put_devmap_managed_page for device public pages. So I can't see how that is in any way a problem.
On Fri, Jun 28, 2019 at 11:52 AM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Jun 28, 2019 at 11:44:35AM -0700, Dan Williams wrote: > > There is a problem with the series in CH's tree. It removes the > > ->page_free() callback from the release_pages() path because it goes > > too far and removes the put_devmap_managed_page() call. > > release_pages only called put_devmap_managed_page for device public > pages. So I can't see how that is in any way a problem. It's a bug that the call to put_devmap_managed_page() was gated by MEMORY_DEVICE_PUBLIC in release_pages(). That path is also applicable to MEMORY_DEVICE_FSDAX because it needs to trigger the ->page_free() callback to wake up wait_on_var() via fsdax_pagefree(). So I guess you could argue that the MEMORY_DEVICE_PUBLIC removal patch left the original bug in place. In that sense we're no worse off, but since we know about the bug, the fix and the patches have not been applied yet, why not fix it now?
On Fri, Jun 28, 2019 at 11:59:19AM -0700, Dan Williams wrote: > It's a bug that the call to put_devmap_managed_page() was gated by > MEMORY_DEVICE_PUBLIC in release_pages(). That path is also applicable > to MEMORY_DEVICE_FSDAX because it needs to trigger the ->page_free() > callback to wake up wait_on_var() via fsdax_pagefree(). > > So I guess you could argue that the MEMORY_DEVICE_PUBLIC removal patch > left the original bug in place. In that sense we're no worse off, but > since we know about the bug, the fix and the patches have not been > applied yet, why not fix it now? The fix it now would simply be to apply Ira original patch now, but given that we are at -rc6 is this really a good time? And if we don't apply it now based on the quilt based -mm worflow it just seems a lot easier to apply it after my series. Unless we want to include it in the series, in which case I can do a quick rebase, we'd just need to make sure Andrew pulls it from -mm.
On Fri, Jun 28, 2019 at 12:02 PM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Jun 28, 2019 at 11:59:19AM -0700, Dan Williams wrote: > > It's a bug that the call to put_devmap_managed_page() was gated by > > MEMORY_DEVICE_PUBLIC in release_pages(). That path is also applicable > > to MEMORY_DEVICE_FSDAX because it needs to trigger the ->page_free() > > callback to wake up wait_on_var() via fsdax_pagefree(). > > > > So I guess you could argue that the MEMORY_DEVICE_PUBLIC removal patch > > left the original bug in place. In that sense we're no worse off, but > > since we know about the bug, the fix and the patches have not been > > applied yet, why not fix it now? > > The fix it now would simply be to apply Ira original patch now, but > given that we are at -rc6 is this really a good time? And if we don't > apply it now based on the quilt based -mm worflow it just seems a lot > easier to apply it after my series. Unless we want to include it in > the series, in which case I can do a quick rebase, we'd just need to > make sure Andrew pulls it from -mm. I believe -mm auto drops patches when they appear in the -next baseline. So it should "just work" to pull it into the series and send it along for -next inclusion.
On Fri, 28 Jun 2019 12:14:44 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > I believe -mm auto drops patches when they appear in the -next > baseline. So it should "just work" to pull it into the series and send > it along for -next inclusion. Yup. Although it isn't very "auto" - I manually check that the patch which turned up in -next was identical to the version which I had. If not, I go find out why...
diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h index b4177aafbbd1..c915889d1769 100644 --- a/drivers/dax/dax-private.h +++ b/drivers/dax/dax-private.h @@ -43,8 +43,6 @@ struct dax_region { * @target_node: effective numa node if dev_dax memory range is onlined * @dev - device core * @pgmap - pgmap for memmap setup / lifetime (driver owned) - * @ref: pgmap reference count (driver owned) - * @cmp: @ref final put completion (driver owned) */ struct dev_dax { struct dax_region *region; @@ -52,8 +50,6 @@ struct dev_dax { int target_node; struct device dev; struct dev_pagemap pgmap; - struct percpu_ref ref; - struct completion cmp; }; static inline struct dev_dax *to_dev_dax(struct device *dev) diff --git a/drivers/dax/device.c b/drivers/dax/device.c index b5257038c188..1af823b2fe6b 100644 --- a/drivers/dax/device.c +++ b/drivers/dax/device.c @@ -14,36 +14,6 @@ #include "dax-private.h" #include "bus.h" -static struct dev_dax *ref_to_dev_dax(struct percpu_ref *ref) -{ - return container_of(ref, struct dev_dax, ref); -} - -static void dev_dax_percpu_release(struct percpu_ref *ref) -{ - struct dev_dax *dev_dax = ref_to_dev_dax(ref); - - dev_dbg(&dev_dax->dev, "%s\n", __func__); - complete(&dev_dax->cmp); -} - -static void dev_dax_percpu_exit(struct dev_pagemap *pgmap) -{ - struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap); - - dev_dbg(&dev_dax->dev, "%s\n", __func__); - wait_for_completion(&dev_dax->cmp); - percpu_ref_exit(pgmap->ref); -} - -static void dev_dax_percpu_kill(struct dev_pagemap *pgmap) -{ - struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap); - - dev_dbg(&dev_dax->dev, "%s\n", __func__); - percpu_ref_kill(pgmap->ref); -} - static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, const char *func) { @@ -441,11 +411,6 @@ static void dev_dax_kill(void *dev_dax) kill_dev_dax(dev_dax); } -static const struct dev_pagemap_ops dev_dax_pagemap_ops = { - .kill = dev_dax_percpu_kill, - .cleanup = dev_dax_percpu_exit, -}; - int dev_dax_probe(struct device *dev) { struct dev_dax *dev_dax = to_dev_dax(dev); @@ -463,15 +428,7 @@ int dev_dax_probe(struct device *dev) return -EBUSY; } - init_completion(&dev_dax->cmp); - rc = percpu_ref_init(&dev_dax->ref, dev_dax_percpu_release, 0, - GFP_KERNEL); - if (rc) - return rc; - - dev_dax->pgmap.ref = &dev_dax->ref; dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX; - dev_dax->pgmap.ops = &dev_dax_pagemap_ops; addr = devm_memremap_pages(dev, &dev_dax->pgmap); if (IS_ERR(addr)) return PTR_ERR(addr);
The functionality is identical to the one currently open coded in device-dax. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/dax/dax-private.h | 4 ---- drivers/dax/device.c | 43 --------------------------------------- 2 files changed, 47 deletions(-)