Re: [PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[Alexei Starovoitov <ast@plumgrid.com>]

On 5/29/15 2:10 AM, Daniel Borkmann wrote:
>>
>> +static void __prog_put_rcu(struct rcu_head *rcu)
>> +{
>> +    struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux,
>> rcu);
>> +
>> +    free_used_maps(aux);
>> +    bpf_prog_free(aux->prog);
>
> Not sure if it's worth it to move these two into a common helper shared
> with bpf_prog_put()? Probably only in case that code should get further
> extended.

I though about it too, but my recent re-reading of net/core/filter.c
taught me otherwise. We have too many tiny helper functions that
are hiding meaning instead of helping.
Like instead of having two pieces of the code:
do1(); do2(); do3(); and do1(); do2();
if we introduce a helper foo() { do1(); do2(); } and the code will do:
foo(), do3() and foo()
when the helper is close enough to invocation it's still easy to read,
but overtime the whole thing, imo, will become a mess. For example,
we have prog_release, prog_free, filter_release and all combinations
with and without __ prefix and _rcu suffix.
I think some of this stuff should be 'unhelpered'.
Like __sk_filter_release() and __bpf_prog_release() should be removed.
Of course, it's a grey line when to introduce a helper and when not to,
but just because two lines are close enough between two functions it
doesn't mean that helper is warranted. In this bpf_prog_put() case
I think helper is not needed _today_. If it grows, we'll reconsider.