Re: [PATCH net-next 1/1] net_sched: Introduce skbmod action

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 16-07-18 12:19 AM, Alexei Starovoitov wrote:
> On Sun, Jul 17, 2016 at 04:41:24AM -0400, Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> This action is intended to be an upgrade from a usability perspective
>> from pedit (as well as operational debugability).
>> Compare this:
>>
>> sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
>> u32 match ip protocol 1 0xff flowid 1:2 \
>> action pedit munge offset -14 u8 set 0x02 \
>>      munge offset -13 u8 set 0x15 \
>>      munge offset -12 u8 set 0x15 \
>>      munge offset -11 u8 set 0x15 \
>>      munge offset -10 u16 set 0x1515 \
>>      pipe
>>
>> to:
>>
>> sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
>> u32 match ip protocol 1 0xff flowid 1:2 \
>> action skbmod dmac 02:15:15:15:15:15
>>
>> Or worse, try to debug a policy with destination mac, source mac and
>> etherype. Then make that a hundred rules and you'll get my point.
>>
>> In the future common use cases on pedit can be migrated to this action
>> (as an example different fields in ip v4/6, transports like tcp/udp/sctp
>> etc). For this first cut, this allows modifying basic ethernet header.
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> ---
>>   include/net/tc_act/tc_skbmod.h        |  35 +++++
>>   include/uapi/linux/tc_act/tc_skbmod.h |  47 +++++++
>>   net/sched/Kconfig                     |  11 ++
>>   net/sched/Makefile                    |   1 +
>>   net/sched/act_skbmod.c                | 245 ++++++++++++++++++++++++++++++++++
>>   5 files changed, 339 insertions(+)
>>   create mode 100644 include/net/tc_act/tc_skbmod.h
>>   create mode 100644 include/uapi/linux/tc_act/tc_skbmod.h
>>   create mode 100644 net/sched/act_skbmod.c
>
> I agree with Cong's point that this new action adds 339 lines of kernel
> code without adding any new functionality.

Come on Alexei.
You cant be serious saying that with all the ebpf code being added now
to the driver level.

> It is suppose to solve debugging issues, but it's hard to understand
> from commit log.

It is. We have deployed hundreds of actions which do pedit. They
get harder to debug as the number goes up.

> I can imagine new tc command 'action skbmod dmac 02:15:15:15:15:15' that
> uses kernel pedit action undercover, so from user space point of view
> the same effect can be achieved by extending iproute2.

Of course - and i pointed to Cong it has been tried before. I should
know because i wrote that code (just look at the pedit code in
iproute2 for udp/icmp/ip type overlays).
It gets tiresome at some point - Ive reached that point. I dont just
sit here and whip features off my ass because it feels great to add 300
lines to the kernel today. We have real need for this.
And as i keep looking closer i see that ebtables has this feature
already. And someone else pointed that openvswitch has it. So precedence
exists.

cheers,
jamal

> What is the reason to add this action to the kernel?
> By debug you mean to dump kernel action list and multiple pedits
> are hard to decipher? Anything else I'm missing?
>