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.
Please confirm if I correctly got your idea and I'll update this patch.
Thanks for the feedback!
>
