Message-ID: <CAJuCfpEuLR8a2jrV4pUQL7emzjSrFa5JHC75PCBdLWRSL0SmmQ@mail.gmail.com>
Date: Fri, 22 Nov 2024 14:43:37 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: akpm@...ux-foundation.org, liam.howlett@...cle.com, 
	lorenzo.stoakes@...cle.com, mhocko@...e.com, vbabka@...e.cz, 
	hannes@...xchg.org, mjguzik@...il.com, oliver.sang@...el.com, 
	mgorman@...hsingularity.net, david@...hat.com, peterx@...hat.com, 
	oleg@...hat.com, dave@...olabs.net, paulmck@...nel.org, brauner@...nel.org, 
	dhowells@...hat.com, hdanton@...a.com, hughd@...gle.com, minchan@...gle.com, 
	jannh@...gle.com, shakeel.butt@...ux.dev, souravpanda@...gle.com, 
	pasha.tatashin@...een.com, corbet@....net, linux-doc@...r.kernel.org, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH v4 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

On Tue, Nov 19, 2024 at 10:37 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Tue, Nov 19, 2024 at 8:36 PM Matthew Wilcox <willy@...radead.org> wrote:
> >
> > On Tue, Nov 19, 2024 at 04:08:25PM -0800, Suren Baghdasaryan wrote:
> > > +static inline void vma_clear(struct vm_area_struct *vma)
> > > +{
> > > +     /* Preserve vma->vm_lock */
> > > +     memset(vma, 0, VMA_BEFORE_LOCK);
> > > +     memset(VMA_LOCK_END(vma), 0, VMA_AFTER_LOCK);
> > > +}
> >
> > This isn't how you're supposed to handle constructors.  You've fixed
> > the immediate problem rather than writing the code in the intended style.
>
> Yeah, I don't like this myself but the only alternative I can think of
> is to set the struct members individually.
>
> >
> > > +static void vm_area_ctor(void *data)
> > > +{
> > > +     vma_lock_init(data);
> > > +}
> >
> > After the ctor has run, the object should be in the same state as
> > it is after it's freed.  If you want to memset the entire thing
> > then you can do it in the ctor.  But there should be no need to
> > do it in vma_init().
>
> IIUC, your suggestion is to memset() the vma and initialize vm_lock
> inside the ctor. Then when it's time to free the vma, we reset all
> members except vm_lock before freeing the vma. As you mention later,
> members like anon_vma_chain, which are already clear, also won't need
> to be reset at this point. Am I understanding your proposal correctly?
>
> BTW, if so, then vma_copy() will have to also copy vma members individually.
>
> >
> > And there's lots of things you can move from vma_init() to the ctor.
> > For example, at free time, anon_vma_chain should be an empty list.
> > So if you init it in the ctor, you can avoid doing it in vma_init().
>
> True.
>
> > I'd suggest that vma_numab_state_free() should be the place which
> > sets vma->numab_state to NULL and we can delete vma_numab_state_init()
> > entirely.
>
> Sounds good to me.

I took a stab at it and the result does not look pretty...
Couple notes:
- vma_init() is used in other places to initialize VMAs allocated on
the stack, so I left it alone for such cases. VMAs like that are not
allocated from vm_area_cachep, can't be reused anyway, therefore we
can override their vm_lock.
- Since vma_init() has to stay, we can't retire vma_numab_state_init()
because it's used in vma_init().
- I think resetting members before freeing might not be such a good
idea because after resetting the object might not be reused at all.

Now, the main point:
I moved initializations of several members into ctor but even with
that the code looks roughly like this:

static void vm_area_ctor(void *data)
{
    struct vm_area_struct *vma = (struct vm_area_struct *)data;

    vma->detached = true;
    INIT_LIST_HEAD(&vma->anon_vma_chain);
    vma_lock_init(vma);
}

struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
{
    struct vm_area_struct *vma;

    vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
    if (!vma)
        return NULL;

    vma->vm_mm = mm;
    vma->vm_ops = &vma_dummy_vm_ops;
    vma->vm_start = 0;
    vma->vm_end = 0;
    memset(&vma->vm_page_prot, 0, sizeof(vma->vm_page_prot));
    vm_flags_init(vma, 0);
    vma_numab_state_init(vma);
    memset(&vma->shared, 0, sizeof(vma->shared));
    vma->anon_vma = NULL;
    vma->vm_pgoff = 0;
    vma->vm_file = NULL;
    vma->vm_private_data = NULL;
    memset(&vma->vm_userfaultfd_ctx, 0, sizeof(vma->vm_userfaultfd_ctx));
#ifdef CONFIG_ANON_VMA_NAME
    vma->anon_name = NULL;
#endif
#ifdef CONFIG_SWAP
    atomic_long_set(&vma->swap_readahead_info, 0);
#endif
#ifndef CONFIG_MMU
    vma->vm_region = NULL;
#endif
#ifdef CONFIG_NUMA
    vma->vm_policy = NULL;
#endif
#ifdef CONFIG_NUMA_BALANCING
    vma->numab_state = NULL;
#endif
    return vma;
}

I can of course add helper functions and get rid of the #ifdef's but still...

Matthew, want to double check if this looks like the solution you were
proposing or am I completely off the target?

>
> Please confirm if I correctly got your idea and I'll update this patch.
> Thanks for the feedback!
>
> >

Powered by blists - more mailing lists