On Tue, Nov 19, 2024 at 10:37 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Tue, Nov 19, 2024 at 8:36 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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!
>
> >
