Re: [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table

[Daniel Borkmann <dborkman@redhat.com>]

On 05/01/2014 07:20 PM, Eric Dumazet wrote:
> On Thu, 2014-05-01 at 18:16 +0200, Daniel Borkmann wrote:
>> This patch simplifies label naming for the BPF jump-table.
>> When we define labels via DL(), we just concatenate/textify
>> the combination of instruction opcode which consists of the
>> class, subclass, word size, target register and so on. Each
>> time we leave BPF_ prefix intact, so that e.g. the preprocessor
>> generates a label BPF_ALU_BPF_ADD_BPF_X for DL(BPF_ALU, BPF_ADD,
>> BPF_X) whereas a label name of ALU_ADD_X is much more easy
>> to grasp. Pure cleanup only.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>> ---
>
> This exactly makes harder to use grep and future code reviews.
>
> # git grep -n BPF_XADD
> include/linux/filter.h:21:#define BPF_XADD      0xc0    /* exclusive add */
> net/core/filter.c:224:          DL(BPF_STX, BPF_XADD, BPF_W),
> net/core/filter.c:225:          DL(BPF_STX, BPF_XADD, BPF_DW),
> net/core/filter.c:481:  BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
> net/core/filter.c:485:  BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
>
> Compare now with :
>
> # git grep -n XADD
>
> So I am quite against this patch.

Well, if you review the code itself in filter.c it makes it hard
to read and review, with this patch, you'll immediately get the
label name and what it actually does, so I think it's quite convenient
and way more readable by itself. For grepping, you can always add
subdirectories you're searching for, besides that I don't think
that everything in the kernel needs to have unique names only so
that one can grep for it among the whole tree.