| Message ID | alpine.LSU.2.11.2008301401390.5954@eggly.anvils (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: fixes to past from future testing | expand |
On Sun, Aug 30, 2020 at 2:04 PM Hugh Dickins <hughd@google.com> wrote: > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c contains a shmem_writeback() > which calls shmem_writepage() from a shrinker: that usually works well > enough; but if /sys/kernel/mm/transparent_hugepage/shmem_enabled has > been set to "force" (documented as "Force the huge option on for all - > very useful for testing"), shmem_writepage() is surprised to be called > with a huge page, and crashes on the VM_BUG_ON_PAGE(PageCompound) (I > did not find out where the crash happens when CONFIG_DEBUG_VM is off). > > LRU page reclaim always splits the shmem huge page first: I'd prefer not > to demand that of i915, so check and split compound in shmem_writepage(). > > Fixes: 2d6692e642e7 ("drm/i915: Start writeback from the shrinker") > Signed-off-by: Hugh Dickins <hughd@google.com> > Cc: stable@vger.kernel.org # v5.3+ Reviewed-by: Shakeel Butt <shakeelb@google.com>
On Sun, Aug 30, 2020 at 2:04 PM Hugh Dickins <hughd@google.com> wrote: > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c contains a shmem_writeback() > which calls shmem_writepage() from a shrinker: that usually works well > enough; but if /sys/kernel/mm/transparent_hugepage/shmem_enabled has > been set to "force" (documented as "Force the huge option on for all - > very useful for testing"), shmem_writepage() is surprised to be called > with a huge page, and crashes on the VM_BUG_ON_PAGE(PageCompound) (I > did not find out where the crash happens when CONFIG_DEBUG_VM is off). > > LRU page reclaim always splits the shmem huge page first: I'd prefer not > to demand that of i915, so check and split compound in shmem_writepage(). > > Fixes: 2d6692e642e7 ("drm/i915: Start writeback from the shrinker") > Signed-off-by: Hugh Dickins <hughd@google.com> > Cc: stable@vger.kernel.org # v5.3+ > --- > I've marked this for stable just for the info, but the number of users > affected is very probably 1, so please feel free to delete that marking. > > mm/shmem.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > --- 5.9-rc2/mm/shmem.c 2020-08-16 17:32:50.693507198 -0700 > +++ linux/mm/shmem.c 2020-08-28 17:35:08.326024349 -0700 > @@ -1362,7 +1362,15 @@ static int shmem_writepage(struct page * > swp_entry_t swap; > pgoff_t index; > > - VM_BUG_ON_PAGE(PageCompound(page), page); > + /* > + * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "force", > + * then drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages, > + * and its shmem_writeback() needs them to be split when swapping. > + */ > + if (PageTransCompound(page)) > + if (split_huge_page(page) < 0) > + goto redirty; The change looks good to me. Acked-by: Yang Shi <shy828301@gmail.com> Just a nit: it may be better to move the spilte after the !PageLocked assertion? Split needs page locked too. > + > BUG_ON(!PageLocked(page)); > mapping = page->mapping; > index = page->index; >
On Tue, 1 Sep 2020, Yang Shi wrote: > On Sun, Aug 30, 2020 at 2:04 PM Hugh Dickins <hughd@google.com> wrote: > > > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c contains a shmem_writeback() > > which calls shmem_writepage() from a shrinker: that usually works well > > enough; but if /sys/kernel/mm/transparent_hugepage/shmem_enabled has > > been set to "force" (documented as "Force the huge option on for all - > > very useful for testing"), shmem_writepage() is surprised to be called > > with a huge page, and crashes on the VM_BUG_ON_PAGE(PageCompound) (I > > did not find out where the crash happens when CONFIG_DEBUG_VM is off). > > > > LRU page reclaim always splits the shmem huge page first: I'd prefer not > > to demand that of i915, so check and split compound in shmem_writepage(). > > > > Fixes: 2d6692e642e7 ("drm/i915: Start writeback from the shrinker") > > Signed-off-by: Hugh Dickins <hughd@google.com> > > Cc: stable@vger.kernel.org # v5.3+ > > --- > > I've marked this for stable just for the info, but the number of users > > affected is very probably 1, so please feel free to delete that marking. > > > > mm/shmem.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > --- 5.9-rc2/mm/shmem.c 2020-08-16 17:32:50.693507198 -0700 > > +++ linux/mm/shmem.c 2020-08-28 17:35:08.326024349 -0700 > > @@ -1362,7 +1362,15 @@ static int shmem_writepage(struct page * > > swp_entry_t swap; > > pgoff_t index; > > > > - VM_BUG_ON_PAGE(PageCompound(page), page); > > + /* > > + * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "force", > > + * then drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages, > > + * and its shmem_writeback() needs them to be split when swapping. > > + */ > > + if (PageTransCompound(page)) > > + if (split_huge_page(page) < 0) > > + goto redirty; > > The change looks good to me. Acked-by: Yang Shi <shy828301@gmail.com> Thanks. > > Just a nit: it may be better to move the spilte after the !PageLocked > assertion? Split needs page locked too. I hadn't considered that, but I think it's best left as is: split_huge_page_to_list() has its own VM_BUG_ON_PAGE(!PageLocked(head), head); to enforce its needs: think of the old BUG_ON(!PageLocked(page)) below as enforcing shmem's needs, checking that split_huge_page() did not unlock it :) > > > + > > BUG_ON(!PageLocked(page)); > > mapping = page->mapping; > > index = page->index; > >
On Tue, Sep 1, 2020 at 10:39 AM Hugh Dickins <hughd@google.com> wrote: > > On Tue, 1 Sep 2020, Yang Shi wrote: > > On Sun, Aug 30, 2020 at 2:04 PM Hugh Dickins <hughd@google.com> wrote: > > > > > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c contains a shmem_writeback() > > > which calls shmem_writepage() from a shrinker: that usually works well > > > enough; but if /sys/kernel/mm/transparent_hugepage/shmem_enabled has > > > been set to "force" (documented as "Force the huge option on for all - > > > very useful for testing"), shmem_writepage() is surprised to be called > > > with a huge page, and crashes on the VM_BUG_ON_PAGE(PageCompound) (I > > > did not find out where the crash happens when CONFIG_DEBUG_VM is off). > > > > > > LRU page reclaim always splits the shmem huge page first: I'd prefer not > > > to demand that of i915, so check and split compound in shmem_writepage(). > > > > > > Fixes: 2d6692e642e7 ("drm/i915: Start writeback from the shrinker") > > > Signed-off-by: Hugh Dickins <hughd@google.com> > > > Cc: stable@vger.kernel.org # v5.3+ > > > --- > > > I've marked this for stable just for the info, but the number of users > > > affected is very probably 1, so please feel free to delete that marking. > > > > > > mm/shmem.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > --- 5.9-rc2/mm/shmem.c 2020-08-16 17:32:50.693507198 -0700 > > > +++ linux/mm/shmem.c 2020-08-28 17:35:08.326024349 -0700 > > > @@ -1362,7 +1362,15 @@ static int shmem_writepage(struct page * > > > swp_entry_t swap; > > > pgoff_t index; > > > > > > - VM_BUG_ON_PAGE(PageCompound(page), page); > > > + /* > > > + * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "force", > > > + * then drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages, > > > + * and its shmem_writeback() needs them to be split when swapping. > > > + */ > > > + if (PageTransCompound(page)) > > > + if (split_huge_page(page) < 0) > > > + goto redirty; > > > > The change looks good to me. Acked-by: Yang Shi <shy828301@gmail.com> > > Thanks. > > > > > Just a nit: it may be better to move the spilte after the !PageLocked > > assertion? Split needs page locked too. > > I hadn't considered that, but I think it's best left as is: > split_huge_page_to_list() has its own > VM_BUG_ON_PAGE(!PageLocked(head), head); > to enforce its needs: think of the old BUG_ON(!PageLocked(page)) > below as enforcing shmem's needs, checking that split_huge_page() > did not unlock it :) Yes, it is definitely fine to keep it as is. I just thought we could bailout earlier if the page is not locked. > > > > > > + > > > BUG_ON(!PageLocked(page)); > > > mapping = page->mapping; > > > index = page->index; > > >
--- 5.9-rc2/mm/shmem.c 2020-08-16 17:32:50.693507198 -0700 +++ linux/mm/shmem.c 2020-08-28 17:35:08.326024349 -0700 @@ -1362,7 +1362,15 @@ static int shmem_writepage(struct page * swp_entry_t swap; pgoff_t index; - VM_BUG_ON_PAGE(PageCompound(page), page); + /* + * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "force", + * then drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages, + * and its shmem_writeback() needs them to be split when swapping. + */ + if (PageTransCompound(page)) + if (split_huge_page(page) < 0) + goto redirty; + BUG_ON(!PageLocked(page)); mapping = page->mapping; index = page->index;
drivers/gpu/drm/i915/gem/i915_gem_shmem.c contains a shmem_writeback() which calls shmem_writepage() from a shrinker: that usually works well enough; but if /sys/kernel/mm/transparent_hugepage/shmem_enabled has been set to "force" (documented as "Force the huge option on for all - very useful for testing"), shmem_writepage() is surprised to be called with a huge page, and crashes on the VM_BUG_ON_PAGE(PageCompound) (I did not find out where the crash happens when CONFIG_DEBUG_VM is off). LRU page reclaim always splits the shmem huge page first: I'd prefer not to demand that of i915, so check and split compound in shmem_writepage(). Fixes: 2d6692e642e7 ("drm/i915: Start writeback from the shrinker") Signed-off-by: Hugh Dickins <hughd@google.com> Cc: stable@vger.kernel.org # v5.3+ --- I've marked this for stable just for the info, but the number of users affected is very probably 1, so please feel free to delete that marking. mm/shmem.c | 9 +++++++++ 1 file changed, 9 insertions(+)