diff mbox series

xfs: Remove kmem_zalloc_large()

Message ID 20200825143458.41887-1-cmaiolino@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series xfs: Remove kmem_zalloc_large() | expand

Commit Message

Carlos Maiolino Aug. 25, 2020, 2:34 p.m. UTC 
This patch aims to replace kmem_zalloc_large() with global kernel memory
API. So, all its callers are now using kvzalloc() directly, so kmalloc()
fallsback to vmalloc() automatically.

__GFP_RETRY_MAYFAIL has been set because according to memory documentation,
it should be used in case kmalloc() is preferred over vmalloc().

Patch survives xfstests with large (32GiB) and small (4GiB) RAM memory amounts.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/kmem.h          | 6 ------
 fs/xfs/scrub/symlink.c | 4 +++-
 fs/xfs/xfs_acl.c       | 3 ++-
 fs/xfs/xfs_ioctl.c     | 5 +++--
 fs/xfs/xfs_rtalloc.c   | 3 ++-
 5 files changed, 10 insertions(+), 11 deletions(-)

I'm not entirely sure passing __GFP_RETRY_MAYFAIL is the right thing to do here,
but since current api attempts a kmalloc before falling back to vmalloc, it
seems to be correct to pass it.

Comments? Cheers

Comments

Dave Chinner Aug. 25, 2020, 10:37 p.m. UTC | #1 
On Tue, Aug 25, 2020 at 04:34:58PM +0200, Carlos Maiolino wrote:
> This patch aims to replace kmem_zalloc_large() with global kernel memory
> API. So, all its callers are now using kvzalloc() directly, so kmalloc()
> fallsback to vmalloc() automatically.
> 
> __GFP_RETRY_MAYFAIL has been set because according to memory documentation,
> it should be used in case kmalloc() is preferred over vmalloc().
> 
> Patch survives xfstests with large (32GiB) and small (4GiB) RAM memory amounts.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/kmem.h          | 6 ------
>  fs/xfs/scrub/symlink.c | 4 +++-
>  fs/xfs/xfs_acl.c       | 3 ++-
>  fs/xfs/xfs_ioctl.c     | 5 +++--
>  fs/xfs/xfs_rtalloc.c   | 3 ++-
>  5 files changed, 10 insertions(+), 11 deletions(-)
> 
> I'm not entirely sure passing __GFP_RETRY_MAYFAIL is the right thing to do here,
> but since current api attempts a kmalloc before falling back to vmalloc, it
> seems to be correct to pass it.

I don't think __GFP_RETRY_MAYFAIL is necessary. If the allocation is
larger than ALLOC_ORDER_COSTLY (8 pages, I think) then kmalloc()
will fail rather than retry forever and then it falls back to
vmalloc. Hence I don't think we need to tell the kmalloc() it needs
to fail large allocations if it can't make progress...

Cheers,

Dave.
Carlos Maiolino Aug. 26, 2020, 10:18 a.m. UTC | #2 
On Wed, Aug 26, 2020 at 08:37:06AM +1000, Dave Chinner wrote:
> On Tue, Aug 25, 2020 at 04:34:58PM +0200, Carlos Maiolino wrote:
> > This patch aims to replace kmem_zalloc_large() with global kernel memory
> > API. So, all its callers are now using kvzalloc() directly, so kmalloc()
> > fallsback to vmalloc() automatically.
> > 
> > __GFP_RETRY_MAYFAIL has been set because according to memory documentation,
> > it should be used in case kmalloc() is preferred over vmalloc().
> > 
> > Patch survives xfstests with large (32GiB) and small (4GiB) RAM memory amounts.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/kmem.h          | 6 ------
> >  fs/xfs/scrub/symlink.c | 4 +++-
> >  fs/xfs/xfs_acl.c       | 3 ++-
> >  fs/xfs/xfs_ioctl.c     | 5 +++--
> >  fs/xfs/xfs_rtalloc.c   | 3 ++-
> >  5 files changed, 10 insertions(+), 11 deletions(-)
> > 
> > I'm not entirely sure passing __GFP_RETRY_MAYFAIL is the right thing to do here,
> > but since current api attempts a kmalloc before falling back to vmalloc, it
> > seems to be correct to pass it.
> 
> I don't think __GFP_RETRY_MAYFAIL is necessary. If the allocation is
> larger than ALLOC_ORDER_COSTLY (8 pages, I think) then kmalloc()
> will fail rather than retry forever and then it falls back to
> vmalloc. Hence I don't think we need to tell the kmalloc() it needs
> to fail large allocations if it can't make progress...
> 
> Cheers,

Thanks Dave!

If nobody has any other comment, I'll submit a version without RETRY_MAYFAIL
later today.

cheers.

> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>
diff mbox series

Patch

diff --git a/fs/xfs/kmem.h b/fs/xfs/kmem.h
index fb1d066770723..38007117697ef 100644
--- a/fs/xfs/kmem.h
+++ b/fs/xfs/kmem.h
@@ -71,12 +71,6 @@  kmem_zalloc(size_t size, xfs_km_flags_t flags)
 	return kmem_alloc(size, flags | KM_ZERO);
 }
 
-static inline void *
-kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
-{
-	return kmem_alloc_large(size, flags | KM_ZERO);
-}
-
 /*
  * Zone interfaces
  */
diff --git a/fs/xfs/scrub/symlink.c b/fs/xfs/scrub/symlink.c
index 5641ae512c9ef..fe971eeb1123d 100644
--- a/fs/xfs/scrub/symlink.c
+++ b/fs/xfs/scrub/symlink.c
@@ -22,11 +22,13 @@  xchk_setup_symlink(
 	struct xfs_inode	*ip)
 {
 	/* Allocate the buffer without the inode lock held. */
-	sc->buf = kmem_zalloc_large(XFS_SYMLINK_MAXLEN + 1, 0);
+	sc->buf = kvzalloc(XFS_SYMLINK_MAXLEN + 1,
+			   GFP_KERNEL | __GFP_RETRY_MAYFAIL);
 	if (!sc->buf)
 		return -ENOMEM;
 
 	return xchk_setup_inode_contents(sc, ip, 0);
+
 }
 
 /* Symbolic links. */
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index d4c687b5cd067..2ac3016f36ff7 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -192,7 +192,8 @@  __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 
 	if (acl) {
 		args.valuelen = XFS_ACL_SIZE(acl->a_count);
-		args.value = kmem_zalloc_large(args.valuelen, 0);
+		args.value = kvzalloc(args.valuelen,
+				      GFP_KERNEL | __GFP_RETRY_MAYFAIL);
 		if (!args.value)
 			return -ENOMEM;
 		xfs_acl_to_disk(args.value, acl);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6f22a66777cd0..c3aa222960116 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -404,7 +404,7 @@  xfs_ioc_attr_list(
 	     context.cursor.offset))
 		return -EINVAL;
 
-	buffer = kmem_zalloc_large(bufsize, 0);
+	buffer = kvzalloc(bufsize, GFP_KERNEL | __GFP_RETRY_MAYFAIL);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -1690,7 +1690,8 @@  xfs_ioc_getbmap(
 	if (bmx.bmv_count > ULONG_MAX / recsize)
 		return -ENOMEM;
 
-	buf = kmem_zalloc_large(bmx.bmv_count * sizeof(*buf), 0);
+	buf = kvzalloc(bmx.bmv_count * sizeof(*buf),
+		       GFP_KERNEL | __GFP_RETRY_MAYFAIL);
 	if (!buf)
 		return -ENOMEM;
 
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 6209e7b6b895b..157ff4343c0f5 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -862,7 +862,8 @@  xfs_alloc_rsum_cache(
 	 * lower bound on the minimum level with any free extents. We can
 	 * continue without the cache if it couldn't be allocated.
 	 */
-	mp->m_rsum_cache = kmem_zalloc_large(rbmblocks, 0);
+	mp->m_rsum_cache = kvzalloc(rbmblocks,
+				    GFP_KERNEL | __GFP_RETRY_MAYFAIL);
 	if (!mp->m_rsum_cache)
 		xfs_warn(mp, "could not allocate realtime summary cache");
 }