Re: [PATCH] mm/migrate: Fix NULL movable_ops if CONFIG_ZSMALLOC=m

[Huacai Chen]

On Sun, Aug 17, 2025 at 1:02 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 16.08.25 18:23, Matthew Wilcox wrote:
> > On Sat, Aug 16, 2025 at 12:54:52PM +0200, David Hildenbrand wrote:
> >> +++ b/mm/balloon_compaction.c
> >> @@ -256,8 +256,10 @@ const struct movable_operations balloon_mops = {
> >>   static int __init balloon_init(void)
> >>   {
> >> -    movable_ops[MOVABLE_BALLOON] = &balloon_mops;
> >> -    return 0;
> >> +    int rc;
> >> +
> >> +    rc = register_movable_ops(&balloon_mops, PGTY_offline);
> >> +    return rc;
> >
> > Using 'rc' as the name of this variable is an anti-pattern.  All it
> > tells you is "this is the return value".  Calling it 'err' is far
> > better because now we know it's an error number (or zero for success,
> > of course).
>
> I know, we all have our things to complain about. Some about Cc: above
> --, others about the name of error variables :P
>
> $ git grep "int rc" | wc -l
> 12730
> $ git grep "int ret" | wc -l
> 80386
> $ git grep "int error" | wc -l
> 4349
> $ git grep "int err " | wc -l
> 6117
>
> >
> > It seems to be a particularly IBM derived antipattern ;-)
>
> Careful miser :D
>
> > Some internal style guide, perhaps?
>
> Kernel-internal style guide maybe ;)
>
> >
> >> +void unregister_movable_ops(const struct movable_operations *ops, enum pagetype type)
> >> +{
> >> +    switch (type) {
> >> +    case PGTY_offline:
> >> +            WARN_ON_ONCE(offline_movable_ops != ops);
> >> +            offline_movable_ops = NULL;
> >> +            break;
> >> +    case PGTY_zsmalloc:
> >> +            WARN_ON_ONCE(zsmalloc_movable_ops != ops);
> >> +            zsmalloc_movable_ops = NULL;
> >> +            break;
> >
> > This might be a bit excessive ... just passing the pagetype and not
> > having the sanity checks should be enough for the tiny number of users
> > this interface will have.
>
> Yeah, no strong opinion, this was a 3 minute hack.
I have tested your code, everything works well. But if the checking is
too excessive, can I use a set_movalbe_ops() to replace both
register_movable_ops() and unregister_movable_ops()? Because register
and unregister are very similar without excessive checking, then we
can pass a meaningful pointer for register and a NULL for unregister
via set_movalbe_ops().

Huacai

>
> --
> Cheers
>
> David / dhildenb
>