Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> > Objects can be freed and reused and still be accessed from code that
> > thinks the object is the old and not the new object
>
> Yes, I know, that's the point of RCU typesafety.  My point is that an
> object *which has never been used* can't be accessed.  So you don't *need*
> a constructor.

But the object needs to have the proper contents after it was released and
re-allocated. Some objects may rely on contents (like list heads)
surviving the realloc process because access must always be possible.

validate_slab() checks on proper metadata content in a slab
although it does not access the payload. So that may work you separate
the payload init from the metadata init.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> > Objects can be freed and reused and still be accessed from code that
> > thinks the object is the old and not the new object
>
> Yes, I know, that's the point of RCU typesafety.  My point is that an
> object *which has never been used* can't be accessed.  So you don't *need*
> a constructor.

But the object needs to have the proper contents after it was released and
re-allocated. Some objects may rely on contents (like list heads)
surviving the realloc process because access must always be possible.

validate_slab() checks on proper metadata content in a slab
although it does not access the payload. So that may work you separate
the payload init from the metadata init.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 12:45:56PM -0500, Christopher Lameter wrote:
> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
> 
> > > How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
> > > Those must have a defined state of the objects at all times and a 
> > > constructor is
> > > required for that. And their use of RCU is required for numerous lockless
> > > lookup algorithms in the kernhel.
> >
> > Not at all times.  Only once they've been used.  Re-constructing them
> > once they've been used might break the rcu typesafety, I suppose ...
> > would need to examine the callers.
> 
> Objects can be freed and reused and still be accessed from code that
> thinks the object is the old and not the new object

Yes, I know, that's the point of RCU typesafety.  My point is that an
object *which has never been used* can't be accessed.  So you don't *need*
a constructor.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 12:45:56PM -0500, Christopher Lameter wrote:
> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
> 
> > > How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
> > > Those must have a defined state of the objects at all times and a 
> > > constructor is
> > > required for that. And their use of RCU is required for numerous lockless
> > > lookup algorithms in the kernhel.
> >
> > Not at all times.  Only once they've been used.  Re-constructing them
> > once they've been used might break the rcu typesafety, I suppose ...
> > would need to examine the callers.
> 
> Objects can be freed and reused and still be accessed from code that
> thinks the object is the old and not the new object

Yes, I know, that's the point of RCU typesafety.  My point is that an
object *which has never been used* can't be accessed.  So you don't *need*
a constructor.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> > How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
> > Those must have a defined state of the objects at all times and a 
> > constructor is
> > required for that. And their use of RCU is required for numerous lockless
> > lookup algorithms in the kernhel.
>
> Not at all times.  Only once they've been used.  Re-constructing them
> once they've been used might break the rcu typesafety, I suppose ...
> would need to examine the callers.

Objects can be freed and reused and still be accessed from code that
thinks the object is the old and not the new object

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> > How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
> > Those must have a defined state of the objects at all times and a 
> > constructor is
> > required for that. And their use of RCU is required for numerous lockless
> > lookup algorithms in the kernhel.
>
> Not at all times.  Only once they've been used.  Re-constructing them
> once they've been used might break the rcu typesafety, I suppose ...
> would need to examine the callers.

Objects can be freed and reused and still be accessed from code that
thinks the object is the old and not the new object

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 12:30:23PM -0500, Christopher Lameter wrote:
> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
> 
> > If we want to get rid of the concept of constructors, it's doable,
> > but somebody needs to do the work to show what the effects will be.
> 
> How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
> Those must have a defined state of the objects at all times and a constructor 
> is
> required for that. And their use of RCU is required for numerous lockless
> lookup algorithms in the kernhel.

Not at all times.  Only once they've been used.  Re-constructing them
once they've been used might break the rcu typesafety, I suppose ...
would need to examine the callers.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 12:30:23PM -0500, Christopher Lameter wrote:
> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
> 
> > If we want to get rid of the concept of constructors, it's doable,
> > but somebody needs to do the work to show what the effects will be.
> 
> How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
> Those must have a defined state of the objects at all times and a constructor 
> is
> required for that. And their use of RCU is required for numerous lockless
> lookup algorithms in the kernhel.

Not at all times.  Only once they've been used.  Re-constructing them
once they've been used might break the rcu typesafety, I suppose ...
would need to examine the callers.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> If we want to get rid of the concept of constructors, it's doable,
> but somebody needs to do the work to show what the effects will be.

How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
Those must have a defined state of the objects at all times and a constructor is
required for that. And their use of RCU is required for numerous lockless
lookup algorithms in the kernhel.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> If we want to get rid of the concept of constructors, it's doable,
> but somebody needs to do the work to show what the effects will be.

How do you envision dealing with the SLAB_TYPESAFE_BY_RCU slab caches?
Those must have a defined state of the objects at all times and a constructor is
required for that. And their use of RCU is required for numerous lockless
lookup algorithms in the kernhel.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> Are you willing to have this kind of bug go uncaught for a while?

There will be frequent allocations and this will show up at some point.

Also you could put this into the debug only portions somehwere so we
always catch it when debugging is on,
'

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> Are you willing to have this kind of bug go uncaught for a while?

There will be frequent allocations and this will show up at some point.

Also you could put this into the debug only portions somehwere so we
always catch it when debugging is on,
'

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 06:53:04AM -0700, Eric Dumazet wrote:
> On 04/10/2018 05:53 AM, Matthew Wilcox wrote:
> > From: Matthew Wilcox 
> > 
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
> > 
> > Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all 
> > allocators")
> > Signed-off-by: Matthew Wilcox 
> > Cc: sta...@vger.kernel.org
> 
> Since there are probably no bug to fix, what about adding the extra check
> only for some DEBUG option ?
> 
> How many caches are still using ctor these days ?

That's a really good question, and strangely hard to find out.  I settled
on "git grep -A4 kmem_cache_alloc" and then searching the 'less' output
with '[^L]);'.

--
arch/powerpc/kvm/book3s_64_mmu_radix.c: kvm_pte_cache = 
kmem_cache_create("kvm-pte", size, size, 0, pte_ctor);
--
arch/powerpc/mm/init-common.c:  new = kmem_cache_create(name, table_size, 
align, 0, ctor);
--
arch/powerpc/platforms/cell/spufs/inode.c:  spufs_inode_cache = 
kmem_cache_create("spufs_inode_cache",
arch/powerpc/platforms/cell/spufs/inode.c-  sizeof(struct 
spufs_inode_info), 0,
arch/powerpc/platforms/cell/spufs/inode.c-  
SLAB_HWCACHE_ALIGN|SLAB_ACCOUNT, spufs_init_once);
--
arch/sh/mm/pgtable.c:   pgd_cachep = kmem_cache_create("pgd_cache",
arch/sh/mm/pgtable.c-  PTRS_PER_PGD * 
(1<e_slab = kmem_cache_create(rp->slab_name,
drivers/usb/mon/mon_text.c- sizeof(struct mon_event_text), sizeof(long),
 0,
drivers/usb/mon/mon_text.c- mon_text_ctor);
--
fs/9p/v9fs.c:   v9fs_inode_cache = kmem_cache_create("v9fs_inode_cache",
fs/9p/v9fs.c- sizeof(struct v9fs_inode),
fs/9p/v9fs.c- 0, (SLAB_RECLAIM_ACCOUNT|
fs/9p/v9fs.c- 
SLAB_MEM_SPREAD|SLAB_ACCOUNT),
fs/9p/v9fs.c- v9fs_inode_init_once);
--
fs/adfs/super.c:adfs_inode_cachep = 
kmem_cache_create("adfs_inode_cache",
fs/adfs/super.c- sizeof(struct 
adfs_inode_info),
fs/adfs/super.c- 0, 
(SLAB_RECLAIM_ACCOUNT|
fs/adfs/super.c-
SLAB_MEM_SPREAD|SLAB_ACCOUNT),
fs/adfs/super.c- init_once);
... snip a huge number of filesystems ...
--
ipc/mqueue.c:   mqueue_inode_cachep = kmem_cache_create("mqueue_inode_cache",
ipc/mqueue.c-   sizeof(struct mqueue_inode_info), 0,
ipc/mqueue.c-   SLAB_HWCACHE_ALIGN|SLAB_ACCOUNT, 
init_once);
--
kernel/fork.c:  sighand_cachep = kmem_cache_create("sighand_cache",
kernel/fork.c-  sizeof(struct sighand_struct), 0,
kernel/fork.c-  SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_TYPESAFE_BY_R
CU|
kernel/fork.c-  SLAB_ACCOUNT, sighand_ctor);
--
lib/radix-tree.c:   radix_tree_node_cachep = kmem_cache_create("radix_tree_n
ode",
lib/radix-tree.c-   sizeof(struct radix_tree_node), 0,
lib/radix-tree.c-   SLAB_PANIC | SLAB_RECLAIM_ACCOUNT,
lib/radix-tree.c-   radix_tree_node_ctor);
--
mm/rmap.c:  anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct an
on_vma),
mm/rmap.c-  0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
mm/rmap.c-  anon_vma_ctor);
--
mm/shmem.c:  

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 06:53:04AM -0700, Eric Dumazet wrote:
> On 04/10/2018 05:53 AM, Matthew Wilcox wrote:
> > From: Matthew Wilcox 
> > 
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
> > 
> > Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all 
> > allocators")
> > Signed-off-by: Matthew Wilcox 
> > Cc: sta...@vger.kernel.org
> 
> Since there are probably no bug to fix, what about adding the extra check
> only for some DEBUG option ?
> 
> How many caches are still using ctor these days ?

That's a really good question, and strangely hard to find out.  I settled
on "git grep -A4 kmem_cache_alloc" and then searching the 'less' output
with '[^L]);'.

--
arch/powerpc/kvm/book3s_64_mmu_radix.c: kvm_pte_cache = 
kmem_cache_create("kvm-pte", size, size, 0, pte_ctor);
--
arch/powerpc/mm/init-common.c:  new = kmem_cache_create(name, table_size, 
align, 0, ctor);
--
arch/powerpc/platforms/cell/spufs/inode.c:  spufs_inode_cache = 
kmem_cache_create("spufs_inode_cache",
arch/powerpc/platforms/cell/spufs/inode.c-  sizeof(struct 
spufs_inode_info), 0,
arch/powerpc/platforms/cell/spufs/inode.c-  
SLAB_HWCACHE_ALIGN|SLAB_ACCOUNT, spufs_init_once);
--
arch/sh/mm/pgtable.c:   pgd_cachep = kmem_cache_create("pgd_cache",
arch/sh/mm/pgtable.c-  PTRS_PER_PGD * 
(1slab_name,
drivers/usb/mon/mon_text.c- sizeof(struct mon_event_text), sizeof(long),
 0,
drivers/usb/mon/mon_text.c- mon_text_ctor);
--
fs/9p/v9fs.c:   v9fs_inode_cache = kmem_cache_create("v9fs_inode_cache",
fs/9p/v9fs.c- sizeof(struct v9fs_inode),
fs/9p/v9fs.c- 0, (SLAB_RECLAIM_ACCOUNT|
fs/9p/v9fs.c- 
SLAB_MEM_SPREAD|SLAB_ACCOUNT),
fs/9p/v9fs.c- v9fs_inode_init_once);
--
fs/adfs/super.c:adfs_inode_cachep = 
kmem_cache_create("adfs_inode_cache",
fs/adfs/super.c- sizeof(struct 
adfs_inode_info),
fs/adfs/super.c- 0, 
(SLAB_RECLAIM_ACCOUNT|
fs/adfs/super.c-
SLAB_MEM_SPREAD|SLAB_ACCOUNT),
fs/adfs/super.c- init_once);
... snip a huge number of filesystems ...
--
ipc/mqueue.c:   mqueue_inode_cachep = kmem_cache_create("mqueue_inode_cache",
ipc/mqueue.c-   sizeof(struct mqueue_inode_info), 0,
ipc/mqueue.c-   SLAB_HWCACHE_ALIGN|SLAB_ACCOUNT, 
init_once);
--
kernel/fork.c:  sighand_cachep = kmem_cache_create("sighand_cache",
kernel/fork.c-  sizeof(struct sighand_struct), 0,
kernel/fork.c-  SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_TYPESAFE_BY_R
CU|
kernel/fork.c-  SLAB_ACCOUNT, sighand_ctor);
--
lib/radix-tree.c:   radix_tree_node_cachep = kmem_cache_create("radix_tree_n
ode",
lib/radix-tree.c-   sizeof(struct radix_tree_node), 0,
lib/radix-tree.c-   SLAB_PANIC | SLAB_RECLAIM_ACCOUNT,
lib/radix-tree.c-   radix_tree_node_ctor);
--
mm/rmap.c:  anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct an
on_vma),
mm/rmap.c-  0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
mm/rmap.c-  anon_vma_ctor);
--
mm/shmem.c: shmem_inode_cachep = kmem_cache_create("shmem_inode_cache",
mm/shmem.c- sizeof(struct shmem_inode_info),
mm/shmem.c- 0, SLAB_PANIC|SLAB_ACCOUNT, 
shmem_init_inode);
--
net/sunrpc/rpc_pipe.c:  rpc_inode_cachep = kmem_cache_create("rpc_inode_cache",
net/sunrpc/rpc_pipe.c-  sizeof(struct rpc_inode),
net/sunrpc/rpc_pipe.c-  0, (SLAB_HWCACHE_ALIGN|SLAB_RECL
AIM_ACCOUNT|
net/sunrpc/rpc_pipe.c-  SLAB_MEM_SPREAD|
SLAB_ACCOUNT),
net/sunrpc/rpc_pipe.c-  init_once);
--
security/integrity/iint.c:  kmem_cache_create("iint_cache", sizeof(struc
t integrity_iint_cache),
security/integrity/iint.c-0, SLAB_PANIC, init_once);

So aside from the filesystems, about fourteen places use it in the kernel.

If we want to get rid of the concept of constructors, it's doable,
but somebody needs to do the work to show what the effects will be.

For example, I took a quick look at the sighand_struct in kernel/fork.c.
That initialises the spinlock and waitqueue head which are at the end
of sighand_struct.  The caller who allocates sighand_struct touches
the head of the 

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 09:21:20AM -0500, Christopher Lameter wrote:
> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
> 
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
> 
> Can we move this check out of the critical paths and check for
> a ctor and GFP_ZERO when calling the page allocator? F.e. in
> allocate_slab()?

Are you willing to have this kind of bug go uncaught for a while?
In this specific case, __GFP_ZERO was only being passed on a few of the
calls to kmem_cache_alloc.  So we'd happily trash the constructed object
any time we didn't allocate a page.

I appreciate it's a tradeoff, and we don't want to clutter the critical
path unnecessarily.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 09:21:20AM -0500, Christopher Lameter wrote:
> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
> 
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
> 
> Can we move this check out of the critical paths and check for
> a ctor and GFP_ZERO when calling the page allocator? F.e. in
> allocate_slab()?

Are you willing to have this kind of bug go uncaught for a while?
In this specific case, __GFP_ZERO was only being passed on a few of the
calls to kmem_cache_alloc.  So we'd happily trash the constructed object
any time we didn't allocate a page.

I appreciate it's a tradeoff, and we don't want to clutter the critical
path unnecessarily.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Christopher Lameter wrote:

> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
>
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
>
> Can we move this check out of the critical paths and check for
> a ctor and GFP_ZERO when calling the page allocator? F.e. in
> allocate_slab()?

The ctor's are run at that point from setup_object() so it makes sense.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Christopher Lameter wrote:

> On Tue, 10 Apr 2018, Matthew Wilcox wrote:
>
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
>
> Can we move this check out of the critical paths and check for
> a ctor and GFP_ZERO when calling the page allocator? F.e. in
> allocate_slab()?

The ctor's are run at that point from setup_object() so it makes sense.

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.

Can we move this check out of the critical paths and check for
a ctor and GFP_ZERO when calling the page allocator? F.e. in
allocate_slab()?

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.

Can we move this check out of the critical paths and check for
a ctor and GFP_ZERO when calling the page allocator? F.e. in
allocate_slab()?

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Eric Dumazet]

On 04/10/2018 05:53 AM, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org


Since there are probably no bug to fix, what about adding the extra check
only for some DEBUG option ?

How many caches are still using ctor these days ?

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Eric Dumazet]

On 04/10/2018 05:53 AM, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org


Since there are probably no bug to fix, what about adding the extra check
only for some DEBUG option ?

How many caches are still using ctor these days ?

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Vlastimil Babka]

On 04/10/2018 02:53 PM, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

It doesn't fix any known problem, does it? Then the stable tag seems too
much IMHO. Especially for fixing a 2007 commit.

Otherwise
Acked-by: Vlastimil Babka 

> ---
>  mm/slab.c | 6 --
>  mm/slob.c | 4 +++-
>  mm/slub.c | 6 --
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 38d3f4fd17d7..8b2cb7db85db 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3313,8 +3313,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t 
> flags, int nodeid,
>   local_irq_restore(save_flags);
>   ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
>  
> - if (unlikely(flags & __GFP_ZERO) && ptr)
> - memset(ptr, 0, cachep->object_size);
> + if (unlikely(flags & __GFP_ZERO) && ptr) {
> + if (!WARN_ON_ONCE(cachep->ctor))
> + memset(ptr, 0, cachep->object_size);
> + }
>  
>   slab_post_alloc_hook(cachep, flags, 1, );
>   return ptr;
> diff --git a/mm/slob.c b/mm/slob.c
> index 1a46181b675c..958173fd7c24 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -556,8 +556,10 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t 
> flags, int node)
>   flags, node);
>   }
>  
> - if (b && c->ctor)
> + if (b && c->ctor) {
> + WARN_ON_ONCE(flags & __GFP_ZERO);
>   c->ctor(b);
> + }
>  
>   kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
>   return b;
> diff --git a/mm/slub.c b/mm/slub.c
> index 9e1100f9298f..0f55f0a0dcaa 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2714,8 +2714,10 @@ static __always_inline void *slab_alloc_node(struct 
> kmem_cache *s,
>   stat(s, ALLOC_FASTPATH);
>   }
>  
> - if (unlikely(gfpflags & __GFP_ZERO) && object)
> - memset(object, 0, s->object_size);
> + if (unlikely(gfpflags & __GFP_ZERO) && object) {
> + if (!WARN_ON_ONCE(s->ctor))
> + memset(object, 0, s->object_size);
> + }
>  
>   slab_post_alloc_hook(s, gfpflags, 1, );
>  
> 

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Vlastimil Babka]

On 04/10/2018 02:53 PM, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

It doesn't fix any known problem, does it? Then the stable tag seems too
much IMHO. Especially for fixing a 2007 commit.

Otherwise
Acked-by: Vlastimil Babka 

> ---
>  mm/slab.c | 6 --
>  mm/slob.c | 4 +++-
>  mm/slub.c | 6 --
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 38d3f4fd17d7..8b2cb7db85db 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3313,8 +3313,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t 
> flags, int nodeid,
>   local_irq_restore(save_flags);
>   ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
>  
> - if (unlikely(flags & __GFP_ZERO) && ptr)
> - memset(ptr, 0, cachep->object_size);
> + if (unlikely(flags & __GFP_ZERO) && ptr) {
> + if (!WARN_ON_ONCE(cachep->ctor))
> + memset(ptr, 0, cachep->object_size);
> + }
>  
>   slab_post_alloc_hook(cachep, flags, 1, );
>   return ptr;
> diff --git a/mm/slob.c b/mm/slob.c
> index 1a46181b675c..958173fd7c24 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -556,8 +556,10 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t 
> flags, int node)
>   flags, node);
>   }
>  
> - if (b && c->ctor)
> + if (b && c->ctor) {
> + WARN_ON_ONCE(flags & __GFP_ZERO);
>   c->ctor(b);
> + }
>  
>   kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
>   return b;
> diff --git a/mm/slub.c b/mm/slub.c
> index 9e1100f9298f..0f55f0a0dcaa 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2714,8 +2714,10 @@ static __always_inline void *slab_alloc_node(struct 
> kmem_cache *s,
>   stat(s, ALLOC_FASTPATH);
>   }
>  
> - if (unlikely(gfpflags & __GFP_ZERO) && object)
> - memset(object, 0, s->object_size);
> + if (unlikely(gfpflags & __GFP_ZERO) && object) {
> + if (!WARN_ON_ONCE(s->ctor))
> + memset(object, 0, s->object_size);
> + }
>  
>   slab_post_alloc_hook(s, gfpflags, 1, );
>  
> 

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Michal Hocko]

On Tue 10-04-18 05:53:50, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org
> ---
>  mm/slab.c | 6 --
>  mm/slob.c | 4 +++-
>  mm/slub.c | 6 --
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 38d3f4fd17d7..8b2cb7db85db 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3313,8 +3313,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t 
> flags, int nodeid,
>   local_irq_restore(save_flags);
>   ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
>  
> - if (unlikely(flags & __GFP_ZERO) && ptr)
> - memset(ptr, 0, cachep->object_size);
> + if (unlikely(flags & __GFP_ZERO) && ptr) {
> + if (!WARN_ON_ONCE(cachep->ctor))
> + memset(ptr, 0, cachep->object_size);
> + }
>  
>   slab_post_alloc_hook(cachep, flags, 1, );
>   return ptr;

Why don't we need to cover this in slab_alloc and kmem_cache_alloc_bulk as well?

Other than that this patch makes sense to me.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Michal Hocko]

On Tue 10-04-18 05:53:50, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org
> ---
>  mm/slab.c | 6 --
>  mm/slob.c | 4 +++-
>  mm/slub.c | 6 --
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 38d3f4fd17d7..8b2cb7db85db 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3313,8 +3313,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t 
> flags, int nodeid,
>   local_irq_restore(save_flags);
>   ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
>  
> - if (unlikely(flags & __GFP_ZERO) && ptr)
> - memset(ptr, 0, cachep->object_size);
> + if (unlikely(flags & __GFP_ZERO) && ptr) {
> + if (!WARN_ON_ONCE(cachep->ctor))
> + memset(ptr, 0, cachep->object_size);
> + }
>  
>   slab_post_alloc_hook(cachep, flags, 1, );
>   return ptr;

Why don't we need to cover this in slab_alloc and kmem_cache_alloc_bulk as well?

Other than that this patch makes sense to me.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Johannes Weiner]

On Tue, Apr 10, 2018 at 05:53:50AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

Acked-by: Johannes Weiner 

Re: [PATCH 1/2] slab: __GFP_ZERO is incompatible with a constructor

[Johannes Weiner]

On Tue, Apr 10, 2018 at 05:53:50AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

Acked-by: Johannes Weiner