| Message ID | 20190613094326.24093-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/22] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option | expand |
On Thu, Jun 13, 2019 at 11:43:07AM +0200, Christoph Hellwig wrote: > ->mapping isn't even used by HMM users, and the field at the same offset > in the zone_device part of the union is declared as pad. (Which btw is > rather confusing, as DAX uses ->pgmap and ->mapping from two different > sides of the union, but DAX doesn't use hmm_devmem_free). > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > mm/hmm.c | 2 -- > 1 file changed, 2 deletions(-) Hurm, is hmm following this comment from mm_types.h? * If you allocate the page using alloc_pages(), you can use some of the * space in struct page for your own purposes. The five words in the main * union are available, except for bit 0 of the first word which must be * kept clear. Many users use this word to store a pointer to an object * which is guaranteed to be aligned. If you use the same storage as * page->mapping, you must restore it to NULL before freeing the page. Maybe the assumption was that a driver is using ->mapping ? However, nouveau is the only driver that uses this path, and it never touches page->mapping either (nor in -next). It looks like if a driver were to start using mapping then the driver should be responsible to set it back to NULL before being done with the page. Reviewed-by: Jason Gunthorpe <jgg@mellanox.com> Jason
On 6/13/19 2:43 AM, Christoph Hellwig wrote: > ->mapping isn't even used by HMM users, and the field at the same offset > in the zone_device part of the union is declared as pad. (Which btw is > rather confusing, as DAX uses ->pgmap and ->mapping from two different > sides of the union, but DAX doesn't use hmm_devmem_free). > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > mm/hmm.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 0c62426d1257..e1dc98407e7b 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -1347,8 +1347,6 @@ static void hmm_devmem_free(struct page *page, void *data) > { > struct hmm_devmem *devmem = data; > > - page->mapping = NULL; > - > devmem->ops->free(devmem, page); > } > > Yes, I think that line was unnecessary. I see from git history that it was originally being set to NULL from within __put_devmap_managed_page(), and then in commit 2fa147bdbf672c53386a8f5f2c7fe358004c3ef8, Dan moved it out of there, and stashed in specifically here. But it appears to have been unnecessary from the beginning. Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
On Thu, Jun 13, 2019 at 07:05:07PM +0000, Jason Gunthorpe wrote: > Hurm, is hmm following this comment from mm_types.h? > > * If you allocate the page using alloc_pages(), you can use some of the > * space in struct page for your own purposes. The five words in the main > * union are available, except for bit 0 of the first word which must be > * kept clear. Many users use this word to store a pointer to an object > * which is guaranteed to be aligned. If you use the same storage as > * page->mapping, you must restore it to NULL before freeing the page. > > Maybe the assumption was that a driver is using ->mapping ? Maybe. The union layou in struct page certainly doesn't help..
On Thu 13-06-19 11:43:07, Christoph Hellwig wrote: > ->mapping isn't even used by HMM users, and the field at the same offset > in the zone_device part of the union is declared as pad. (Which btw is > rather confusing, as DAX uses ->pgmap and ->mapping from two different > sides of the union, but DAX doesn't use hmm_devmem_free). > > Signed-off-by: Christoph Hellwig <hch@lst.de> I cannot really judge here but setting mapping here without any comment is quite confusing. So if this is safe to remove then it is certainly an improvement. > --- > mm/hmm.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 0c62426d1257..e1dc98407e7b 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -1347,8 +1347,6 @@ static void hmm_devmem_free(struct page *page, void *data) > { > struct hmm_devmem *devmem = data; > > - page->mapping = NULL; > - > devmem->ops->free(devmem, page); > } > > -- > 2.20.1
diff --git a/mm/hmm.c b/mm/hmm.c index 0c62426d1257..e1dc98407e7b 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -1347,8 +1347,6 @@ static void hmm_devmem_free(struct page *page, void *data) { struct hmm_devmem *devmem = data; - page->mapping = NULL; - devmem->ops->free(devmem, page); }
->mapping isn't even used by HMM users, and the field at the same offset in the zone_device part of the union is declared as pad. (Which btw is rather confusing, as DAX uses ->pgmap and ->mapping from two different sides of the union, but DAX doesn't use hmm_devmem_free). Signed-off-by: Christoph Hellwig <hch@lst.de> --- mm/hmm.c | 2 -- 1 file changed, 2 deletions(-)