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.
