Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree
On Mon, Nov 16, 2020 at 10:17:20PM -0800, Hugh Dickins wrote: > On Mon, 16 Nov 2020, a...@linux-foundation.org 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 > > 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 > > 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 >
Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree
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 > wrote: > > > > On Mon, 16 Nov 2020, a...@linux-foundation.org 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 > > > 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.
Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree
Hi Hugh, On Mon, 16 Nov 2020 22:17:20 -0800 (PST) Hugh Dickins wrote: > > On Mon, 16 Nov 2020, a...@linux-foundation.org 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 > > 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 pgpuNyVry7gja.pgp Description: OpenPGP digital signature
Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree
On Mon, 16 Nov 2020, a...@linux-foundation.org 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
> 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
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
> Signed-off-by: Andrew Morton
> ---
>
> 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 sh_...@163.com 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
