On Mon, Jul 23, 2018 at 1:55 AM Kirill A. Shutemov
<kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
>
> I also would rather keep ->vm_ops non-NULL for anonymous VMAs. This way it's
> easier to catch broken drivers: place a WARN() in __vma_link_rb().
>
> With non-NULL vm_ops we can drop all vma->vm_ops checks. There's patch in
> -mm tree doing this[1].
Yeah, that patch is completely broken. See the separate email where I
pointed out how there are drivers that explicitly set vm_ops to NULL
while the vma is still live.
So no. That kind of stuff isn't even an option unless everything else
is cleaned up.
So I really think there's a lot more cleanup here before
'&anon_vm_ops" would make any sense what-so-ever.
But if we had a wrapper function for it, I'd at least find it more
palatable. Something simple like
static inline void vma_set_anonymous(struct vm_area_struct *vma)
{
static const struct vm_operations_struct anon_vm_ops = {};
vma->vm_ops = &anon_vm_ops;
vma->vm_flags |= VM_ANONYMOUS;
}
would look fine to me. And I'd rather have it in a flag than anywhere
else. That's what those flags *are* for.
In fact, I think one cleanup we should do on the way to this is to fix
the current "vm_flags". It's insanely different sizes on 32-bit and
64-bit, because we use 'unsigned long' for it. And it's a big
inconvenience, because we're tight on flags.
So we could make it a u64, which would get us reliably more flags. Or
even make a separate field for some of the more specialized flags, and
have two (separate) flag fields instead. Right now we do some odd
things due to packing (not as bad as the page flags, but not pretty).
Linus
