On Mon, 16 Nov 2020, [email protected] wrote:
>
> The patch titled
> Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
> has been added to the -mm tree. Its filename is
> mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
>
> This patch should soon appear at
> https://ozlabs.org/~akpm/mmots/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> and later at
> https://ozlabs.org/~akpm/mmotm/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
>
> Before you just go and hit "reply", please:
> a) Consider who else should be cc'ed
> b) Prefer to cc a suitable mailing list as well
> c) Ideally: find the original patch on the mailing list and do a
> reply-to-all to that, adding suitable additional cc's
>
> *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
>
> The -mm tree is included into linux-next and is updated
> there every 3-4 working days
>
> ------------------------------------------------------
> From: Hui Su <[email protected]>
> Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
Andrew, Stephen, please revert this untested "cleanup" from your
trees a.s.a.p: it's a disaster for anyone using shmem/tmpfs.
>
> in shmem_get_inode():
> new_inode();
> new_inode_pseudo();
> alloc_inode();
> ops->alloc_inode(); -> shmem_alloc_inode()
> kmem_cache_alloc();
>
> memset(info, 0, (char *)inode - (char *)info);
>
> So use kmem_cache_zalloc() in shmem_alloc_inode(),
> and remove the memset in shmem_get_inode().
I could not follow that argument at all. The shmem_inode_cachep
uses a constructor, and the memset shown is of only a portion of
the whole inode. zeroing the entire inode quickly crashes the
kernel, after showing errors.
(If you're lucky enough to have a readable display at that point:
I did not, but got on better with framebuffer than drm/i915; and
I wonder if there's a separate bug in that area too, because fixing
this shmem issue is not enough to get my drm/i915 rc4-mm1 booting.)
>
> Link: https://lkml.kernel.org/r/20201115174026.GA365412@rlk
> Signed-off-by: Hui Su <[email protected]>
NAK. Hui Su, please test your "cleanups" before sending them.
I'm sorry for being slow to respond, but the priority appeared
to be to get Matthew Wilcox's series running reliably, so I had not
got around to checking the less urgent shmem patches before they
slipped into mmotm - there may well be more that I want to NAK,
but this is the dangerous one.
Thanks,
Hugh
> Cc: Hugh Dickins <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> mm/shmem.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> --- a/mm/shmem.c~mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode
> +++ a/mm/shmem.c
> @@ -2331,7 +2331,6 @@ static struct inode *shmem_get_inode(str
> inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> inode->i_generation = prandom_u32();
> info = SHMEM_I(inode);
> - memset(info, 0, (char *)inode - (char *)info);
> spin_lock_init(&info->lock);
> atomic_set(&info->stop_eviction, 0);
> info->seals = F_SEAL_SEAL;
> @@ -3851,7 +3850,7 @@ static struct kmem_cache *shmem_inode_ca
> static struct inode *shmem_alloc_inode(struct super_block *sb)
> {
> struct shmem_inode_info *info;
> - info = kmem_cache_alloc(shmem_inode_cachep, GFP_KERNEL);
> + info = kmem_cache_zalloc(shmem_inode_cachep, GFP_KERNEL);
> if (!info)
> return NULL;
> return &info->vfs_inode;
> _
>
> Patches currently in -mm which might be from [email protected] are
>
> mmslab_common-use-list_for_each_entry-in-dump_unreclaimable_slab.patch
> mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> mm-page_counter-use-page_counter_read-in-page_counter_set_max.patch
> mm-hugetlbc-just-use-put_page_testzero-instead-of-page_count.patch
> mm-compaction-move-compaction_suitables-comment-to-right-place.patch
> mm-oom_kill-change-comment-and-rename-is_dump_unreclaim_slabs.patch
> acctc-use-elif-instead-of-end-and-elif.patch
> mm-memcontrol-rewrite-mem_cgroup_page_lruvec.patch
Hi Hugh,
On Mon, 16 Nov 2020 22:17:20 -0800 (PST) Hugh Dickins <[email protected]> wrote:
>
> On Mon, 16 Nov 2020, [email protected] wrote:
> >
> > The patch titled
> > Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
> > has been added to the -mm tree. Its filename is
> > mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> >
> > This patch should soon appear at
> > https://ozlabs.org/~akpm/mmots/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> > and later at
> > https://ozlabs.org/~akpm/mmotm/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> >
> > Before you just go and hit "reply", please:
> > a) Consider who else should be cc'ed
> > b) Prefer to cc a suitable mailing list as well
> > c) Ideally: find the original patch on the mailing list and do a
> > reply-to-all to that, adding suitable additional cc's
> >
> > *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> >
> > The -mm tree is included into linux-next and is updated
> > there every 3-4 working days
> >
> > ------------------------------------------------------
> > From: Hui Su <[email protected]>
> > Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
>
> Andrew, Stephen, please revert this untested "cleanup" from your
> trees a.s.a.p: it's a disaster for anyone using shmem/tmpfs.
Thanks for that. I have also bisected my boot failures to that commit
and so have reverted it from linux-next today.
--
Cheers,
Stephen Rothwell
On Tue, Nov 17, 2020 at 06:12:46PM +1100, Stephen Rothwell wrote:
> Hi Hugh,
>
> On Mon, 16 Nov 2020 22:17:20 -0800 (PST) Hugh Dickins <[email protected]> wrote:
> >
> > On Mon, 16 Nov 2020, [email protected] wrote:
> > >
> > > The patch titled
> > > Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
> > > has been added to the -mm tree. Its filename is
> > > mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> > >
> > > This patch should soon appear at
> > > https://ozlabs.org/~akpm/mmots/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> > > and later at
> > > https://ozlabs.org/~akpm/mmotm/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> > >
> > > Before you just go and hit "reply", please:
> > > a) Consider who else should be cc'ed
> > > b) Prefer to cc a suitable mailing list as well
> > > c) Ideally: find the original patch on the mailing list and do a
> > > reply-to-all to that, adding suitable additional cc's
> > >
> > > *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> > >
> > > The -mm tree is included into linux-next and is updated
> > > there every 3-4 working days
> > >
> > > ------------------------------------------------------
> > > From: Hui Su <[email protected]>
> > > Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
> >
> > Andrew, Stephen, please revert this untested "cleanup" from your
> > trees a.s.a.p: it's a disaster for anyone using shmem/tmpfs.
>
> Thanks for that. I have also bisected my boot failures to that commit
> and so have reverted it from linux-next today.
>
> --
> Cheers,
> Stephen Rothwell
Sorry for not full testing the change.
Please ignore this change.
Thanks.
On Mon, Nov 16, 2020 at 10:17:20PM -0800, Hugh Dickins wrote:
> On Mon, 16 Nov 2020, [email protected] wrote:
> >
> > The patch titled
> > Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
> > has been added to the -mm tree. Its filename is
> > mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> >
> > This patch should soon appear at
> > https://ozlabs.org/~akpm/mmots/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> > and later at
> > https://ozlabs.org/~akpm/mmotm/broken-out/mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch
> >
> > Before you just go and hit "reply", please:
> > a) Consider who else should be cc'ed
> > b) Prefer to cc a suitable mailing list as well
> > c) Ideally: find the original patch on the mailing list and do a
> > reply-to-all to that, adding suitable additional cc's
> >
> > *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> >
> > The -mm tree is included into linux-next and is updated
> > there every 3-4 working days
> >
> > ------------------------------------------------------
> > From: Hui Su <[email protected]>
> > Subject: mm/shmem: use kmem_cache_zalloc in shmem_alloc_inode()
>
> Andrew, Stephen, please revert this untested "cleanup" from your
> trees a.s.a.p: it's a disaster for anyone using shmem/tmpfs.
>
> >
> > in shmem_get_inode():
> > new_inode();
> > new_inode_pseudo();
> > alloc_inode();
> > ops->alloc_inode(); -> shmem_alloc_inode()
> > kmem_cache_alloc();
> >
> > memset(info, 0, (char *)inode - (char *)info);
> >
> > So use kmem_cache_zalloc() in shmem_alloc_inode(),
> > and remove the memset in shmem_get_inode().
>
> I could not follow that argument at all. The shmem_inode_cachep
> uses a constructor, and the memset shown is of only a portion of
> the whole inode. zeroing the entire inode quickly crashes the
> kernel, after showing errors.
>
> (If you're lucky enough to have a readable display at that point:
> I did not, but got on better with framebuffer than drm/i915; and
> I wonder if there's a separate bug in that area too, because fixing
> this shmem issue is not enough to get my drm/i915 rc4-mm1 booting.)
>
Sorry, I thought it was a small change before, so I forgot to test it.
It's SO STUPID. I'm really sorry.
> >
> > Link: https://lkml.kernel.org/r/20201115174026.GA365412@rlk
> > Signed-off-by: Hui Su <[email protected]>
>
> NAK. Hui Su, please test your "cleanups" before sending them.
>
I will remember it, thanks.
I have spent two evenings building a patch automated test platform
using qemu and jenkins.
> I'm sorry for being slow to respond, but the priority appeared
> to be to get Matthew Wilcox's series running reliably, so I had not
> got around to checking the less urgent shmem patches before they
> slipped into mmotm - there may well be more that I want to NAK,
> but this is the dangerous one.
>
> Thanks,
> Hugh
>