On Thu, Jul 27, 2023 at 07:20:46PM +0900, Damien Le Moal wrote:

On 7/27/23 17:55, Qi Zheng wrote:

quoted

quoted

quoted

          goto err;
      }
  +    zmd->mblk_shrinker->count_objects = dmz_mblock_shrinker_count;
+    zmd->mblk_shrinker->scan_objects = dmz_mblock_shrinker_scan;
+    zmd->mblk_shrinker->seeks = DEFAULT_SEEKS;
+    zmd->mblk_shrinker->private_data = zmd;
+
+    shrinker_register(zmd->mblk_shrinker);

I fail to see how this new shrinker API is better... Why isn't there a
shrinker_alloc_and_register() function ? That would avoid adding all this code
all over the place as the new API call would be very similar to the current
shrinker_register() call with static allocation.

In some registration scenarios, memory needs to be allocated in advance.
So we continue to use the previous prealloc/register_prepared()
algorithm. The shrinker_alloc_and_register() is just a helper function
that combines the two, and this increases the number of APIs that
shrinker exposes to the outside, so I choose not to add this helper.

And that results in more code in many places instead of less code + a simple
inline helper in the shrinker header file...

It's not just a "simple helper" - it's a function that has to take 6
or 7 parameters with a return value that must be checked and
handled.

This was done in the first versions of the patch set - the amount of
code in each caller does not go down and, IMO, was much harder to
read and determine "this is obviously correct" that what we have
now.

So not adding that super simple
helper is not exactly the best choice in my opinion.

Each to their own - I much prefer the existing style/API over having
to go look up a helper function every time I want to check some
random shrinker has been set up correctly....

-Dave.
-- 
Dave Chinner
david@fromorbit.com