Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou <akaraliou.dev@gmail.com>]

On 12/20/2017 02:57 PM, Michal Hocko wrote:
> On Wed 20-12-17 12:38:35, Michal Hocko wrote:
>> On Wed 20-12-17 20:05:35, Tetsuo Handa wrote:
>>> On 2017/12/20 18:25, Michal Hocko wrote:
>>>> On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
>>>>> On (12/20/17 10:08), Michal Hocko wrote:
>>>>> [..]
>>>>>>> let's keep void zs_register_shrinker() and just suppress the
>>>>>>> register_shrinker() must_check warning.
>>>>>> I would just hope we simply drop the must_check nonsense.
>>>>> agreed. given that unregister_shrinker() does not oops anymore,
>>>>> enforcing that check does not make that much sense.
>>>> Well, the registration failure is a failure like any others. Ignoring
>>>> the failure can have bad influence on the overal system behavior but
>>>> that is no different from thousands of other functions. must_check is an
>>>> overreaction here IMHO.
>>>>
>>> I don't think that must_check is an overreaction.
>>> As of linux-next-20171218, no patch is available for 10 locations.
>>>
>>> drivers/staging/android/ion/ion_heap.c:306:     register_shrinker(&heap->shrinker);
>>> drivers/staging/android/ashmem.c:857:   register_shrinker(&ashmem_shrinker);
>>> drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1185:  register_shrinker(&manager->mm_shrink);
>>> drivers/gpu/drm/ttm/ttm_page_alloc.c:484:       register_shrinker(&manager->mm_shrink);
>>> drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
>>> drivers/gpu/drm/msm/msm_gem_shrinker.c:154:     WARN_ON(register_shrinker(&priv->shrinker));
>>> drivers/md/dm-bufio.c:1756:     register_shrinker(&c->shrinker);
>>> drivers/android/binder_alloc.c:1012:    register_shrinker(&binder_shrinker);
>>> arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
>>> fs/xfs/xfs_qm.c:698:    register_shrinker(&qinf->qi_shrinker);
>> And how exactly has the must_check helped for those? Come on, start
>> being serious finally. This is a matter of fixing those. You have done
>> a good deal of work for some, it just takes to finish the rest. The
>> warning doesn't help on its own, it just makes people ignore it after
>> some time or make it silent in some way.
> Also have a look at how WARN_ON simply papers over the wrong code and
> must_check will not help you the slightest.

Regarding the other locations where return code is ignored, I think I will
try to fix them as I did in Lustre code recently.
However, it might be not straightforward and zsmalloc is good example -
we understand that failure is not critical and we can live without shrinker.

Locations specified by Michal are also different, for example:

drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
- this change is intentional.
arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
- was made before register_shrinker() became non-void.

and so on. The question is what to do in each particular case ?
Some people may consider wrapping it with WARN_ON to be rather good option too
while the others will prefer to consider it as a critical failure or at least
do their own logging, with still looks similar with WARN_ON for me imho.

For me, must_check looks like thing that works mostly for new code only,
but in this case it works like a trigger that forces people to act and fix
previously written code, but yes, all depends on attitude as Michal noticed.

Best regards,
	Aliaksei.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>