Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree

Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree

[Hugh Dickins]

On Mon, 16 Nov 2020, akpm@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 <sh_def@163.com>
> 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 <sh_def@163.com>

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 <hughd@google.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  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_def@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

Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

Hi Hugh,

On Mon, 16 Nov 2020 22:17:20 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
>
> On Mon, 16 Nov 2020, akpm@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 <sh_def@163.com>
> > 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: + mm-shmem-use-kmem_cache_zalloc-in-shmem_alloc_inode.patch added to -mm tree

[Hui Su]

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 <hughd@google.com> wrote:
> >
> > On Mon, 16 Nov 2020, akpm@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 <sh_def@163.com>
> > > 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

[Hui Su]

On Mon, Nov 16, 2020 at 10:17:20PM -0800, Hugh Dickins wrote:
> On Mon, 16 Nov 2020, akpm@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 <sh_def@163.com>
> > 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 <sh_def@163.com>
> 
> 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
>