Re: [v5 PATCH 04/11] mm: vmscan: remove memcg_shrinker_map_size

[Vlastimil Babka <vbabka@suse.cz>]

On 1/28/21 10:22 PM, Yang Shi wrote:
>> > @@ -266,12 +265,13 @@ int alloc_shrinker_maps(struct mem_cgroup *memcg)
>> >  static int expand_shrinker_maps(int new_id)
>> >  {
>> >       int size, old_size, ret = 0;
>> > +     int new_nr_max = new_id + 1;
>> >       struct mem_cgroup *memcg;
>> >
>> > -     size = DIV_ROUND_UP(new_id + 1, BITS_PER_LONG) * sizeof(unsigned long);
>> > -     old_size = memcg_shrinker_map_size;
>> > +     size = (new_nr_max / BITS_PER_LONG + 1) * sizeof(unsigned long);
>> > +     old_size = (shrinker_nr_max / BITS_PER_LONG + 1) * sizeof(unsigned long);
>>
>> What's wrong with using DIV_ROUND_UP() here?
> 
> I don't think there is anything wrong with DIV_ROUND_UP. Should be
> just different taste and result in shorter statement.

IMHO it's not just taste. DIV_ROUND_UP() says what it does and you don't need to
guess it from the math expression. Also your expression is shorter as it simply
adds + 1, so if shrinker_nr_max is a multiple of BITS_PER_LONG, there's an extra
unsigned long that shouldn't be needed. People reading that code will wonder
whether there was some non-obvious intention behind that, and possibly send
cleanup patches.

>>
>> >       if (size <= old_size)
>> > -             return 0;
>> > +             goto out;
>>
>> Can this even happen? Seems to me it can't, so just remove this?
> 
> Yes, it can. The maps use unsigned long value for bitmap, so any
> shrinker ID < 31 would fall into the same unsigned long, so we may see
> size <= old_size, but we need increase shrinker_nr_max since
> expand_shrinker_maps() is called iff id >= shrinker_nr_max.

Ah, good point.