On Wed, Nov 13, 2019 at 10:23:43AM -0800, Darrick J. Wong wrote:
> On Wed, Nov 13, 2019 at 03:23:35PM +0100, Carlos Maiolino wrote:
> > Getting rid of these functions, is a bit more complicated, giving the
> > fact they use a vmalloc fallback, and (in case of _io version) uses an
> > alignment check, so they have their useness.
> >
> > Instead of keeping both of them, I think sharing the same function for
> > both cases is a more interesting idea, giving the fact they both have
> > the same purpose, with the only difference being the alignment check,
> > which can be selected by using a flag.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > ---
> > fs/xfs/kmem.c | 39 +++++++++++------------------------
> > fs/xfs/kmem.h | 10 +--------
> > fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> > fs/xfs/scrub/attr.c | 2 +-
> > fs/xfs/scrub/symlink.c | 3 ++-
> > fs/xfs/xfs_acl.c | 3 ++-
> > fs/xfs/xfs_buf.c | 4 ++--
> > fs/xfs/xfs_ioctl.c | 8 ++++---
> > fs/xfs/xfs_ioctl32.c | 3 ++-
> > fs/xfs/xfs_log.c | 5 +++--
> > fs/xfs/xfs_log_cil.c | 2 +-
> > fs/xfs/xfs_log_recover.c | 4 ++--
> > fs/xfs/xfs_rtalloc.c | 3 ++-
> > 13 files changed, 36 insertions(+), 52 deletions(-)
> >
> > diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c
> > index 44145293cfc9..bb4990970647 100644
> > --- a/fs/xfs/kmem.c
> > +++ b/fs/xfs/kmem.c
> > @@ -8,40 +8,25 @@
> > #include "xfs_message.h"
> > #include "xfs_trace.h"
> >
> > -/*
> > - * Same as kmem_alloc_large, except we guarantee the buffer returned is aligned
> > - * to the @align_mask. We only guarantee alignment up to page size, we'll clamp
> > - * alignment at page size if it is larger. vmalloc always returns a PAGE_SIZE
> > - * aligned region.
> > - */
> > void *
> > -kmem_alloc_io(size_t size, int align_mask, gfp_t flags)
> > +xfs_kmem_alloc(size_t size, gfp_t flags, bool align, int align_mask)
>
> A boolean for the align /and/ an alignment mask? Yuck.
>
> I think I'd rather have:
>
> void *
> kmem_alloc(
> size_t size,
> gfp_t flags,
> unsigned int align_mask)
> {
> ... allocation logic ...
> }
If you avoid changing the order of the flags/alignmask parameters,
most of the churn in this patch goes away.
>
> and in kmem.h:
>
> static inline void *
> kmem_alloc_io(
> size_t size,
> gfp_t flags,
> unsigned int align_mask)
> {
> trace_kmem_alloc_io(size, flags, align_mask, _RET_IP_);
> return kmem_alloc(size, flags, align_mask);
> }
This should be able to go away soon, because the heap allocator will
guarantee alignment soon. That means kmem.c is a single function,
and kmem.h is a single function. I'd be looking to move the two
helper functions into some other utility file at that point
(fsops?)...
ANother question: how much work is there to be done on the userspace
side of things?
> > */
> >
> > -extern void *kmem_alloc_io(size_t size, int align_mask, gfp_t flags);
> > -extern void *kmem_alloc_large(size_t size, gfp_t);
> > +extern void *xfs_kmem_alloc(size_t, gfp_t, bool, int);
> > static inline void kmem_free(const void *ptr)
> > {
> > kvfree(ptr);
> > }
Didn't an earlier patch get rid of kmem_free(), or am I just
imagining this? Seems silly to leave this behind, now that the
only place that needs kvfree() is the callers to kmem_alloc_io and
kmem_alloc_large...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
