On 8/22/23, Vegard Nossum <vegard.nossum@xxxxxxxxxx> wrote:
>
> Testing out a review style with very detailed comments. Let me know if
> you hate it. Review notes:
>
I do, very noisy and I don't think it adds any value. ;)
If something like this becomes the norm I'm confident people are going
to start skimming responses to their mail, as opposed to reading them.
But then again, I had serious disagreement with review folk in the past, so...
> On 8/21/23 22:28, Mateusz Guzik wrote:
>> + counters = __alloc_percpu_gfp(sizeof(*counters) * count,
>> + sizeof(*counters), gfp);
>
> The second argument here is the alignment. I see other callers using
> __alignof__(type), which is what alloc_percpu_gfp() does as well. In
> practice I think it shouldn't matter, but for clarity/consistency maybe
> this should be __alignof__ as well?
>
Ye, I neglected to patch it up after a whipping out a PoC.
> Presumably multiplication overflow is not an issue here as it is with
> kmalloc and friends since the count can't be controlled by userspace.
>
I wanted to assert on the count being sensible to begin with, but
there is no general "only assert with debug enabled" macro. Perhaps a
warn_once + bail out would be preferred?
>> + if (!counters) {
>> + fbc[0].counters = NULL;
>> return -ENOMEM;
>> + }
>
> Checked that __alloc_percpu_gfp() returns NULL on failure.
>
> Checked that nothing else before this in the function needs cleanup.
>
> In the old code, fbc->count would have gotten initialized but it
> shouldn't matter here, I think, as long as the counter is never activated.
>
> I'm not sure why only fbc[0].counters is set to NULL, should this happen
> for all the "count" members? [PS: percpu_counter_destroy_many() below
> has a check for fbc[0].counters]
>
Consumers looked fishy to me with zeroing the counter prior to calling
the init func.
I added the NULL assignment so that a call to destroy does nothing.
> In summary, my only slight concern is sizeof(*counters) being passed as
> the alignment to __alloc_percpu_gfp() when maybe it would be more
> appropriate to pass __alignof__() -- not that it makes a difference at
> runtime since both are 4 for s32.
>
Agreed, will patch later.
> One other thing: I find it a bit odd that the "amount" parameter
> (initial value) is s64 when the counters themselves are s32. Maybe just
> a leftover from an old version?
>
I don't know the reasoning by the authors, but seems a clear case to
me that the per-CPU counts were left int-sized to reduce memory usage
and reduce deviation between the central counter and the real state.
--
Mateusz Guzik <mjguzik gmail.com>
