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

[David Hildenbrand <david@redhat.com>]

On 17.08.25 03:37, Huacai Chen wrote:
> On Sun, Aug 17, 2025 at 1:02 AM David Hildenbrand <david@redhat.com> 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()? 

Makes sense, given that I don't expect us to have multiple handlers for 
the same type in the near future.

Please do keep a sanity check for replacing valid pointers

if (old_ops && new_ops)
	return -EBUSY;

Thanks!

-- 
Cheers

David / dhildenb