Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko <jiri@resnulli.us>]

Thu, Nov 23, 2023 at 05:20:27PM CET, hadi@mojatatu.com wrote:
>On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
>> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> >> >> >This action takes advantage of the presence of tc block ports set in the
>> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
>> >> >> >broadcast the packet to a block, that is send to all members of the block except
>> >> >> >the port in which the packet arrived on. However, the user may specify
>> >> >> >the option "tx_type all", which will send the packet to all members of the
>> >> >> >block indiscriminately.
>> >> >> >
>> >> >> >Example usage:
>> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
>> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
>> >> >> >
>> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> >>
>> >> >> Name the arg "block" so it is consistent with "filter add block". Make
>> >> >> sure this is aligned netlink-wise as well.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >Or if we wish to send to all ports in the block:
>> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> >> >>
>> >> >> I read the discussion the the previous version again. I suggested this
>> >> >> to be part of mirred. Why exactly that was not addressed?
>> >> >>
>> >> >
>> >> >I am the one who pushed back (in that discussion). Actions should be
>> >> >small and specific. Like i had said in that earlier discussion it was
>> >> >a mistake to make mirred do both mirror and redirect - they should
>> >>
>> >> For mirror and redirect, I agree. For redirect and redirect, does not
>> >> make much sense. It's just confusing for the user.
>> >>
>> >
>> >Blockcast only emulates the mirror part. I agree redirect doesnt make
>> >any sense because once you redirect the packet is gone.
>>
>> How is it mirror? It is redirect to multiple, isn't it?
>
>mirror has been used (so far in mirred action and i believe in the
>industry in general) to mean  "send a copy of the packet" - meaning
>you can send to many ports and even when you are done sending to all
>those ports the packet is still in the pipeline and you can continue
>to execute other action on it. Whereas redirect means the packet is
>stolen from the pipeline i.e if you redirect to a port the packet is
>not available to redirect to the next port or for any other action
>after that.
>You could argue a loose interpretation of redirect to a block to mean
>"mirror to all ports on the block but on the last port redirect".

it's stolen from the pipeline, right? That would be redirect from my
perspective.


>
>>
>> >
>> >> >have been two actions. So i feel like adding a block to mirred is
>> >> >adding more knobs. We are also going to add dev->group as a way to
>> >> >select what devices to mirror to. Should that be in mirred as well?
>> >>
>> >> I need more details.
>> >>
>> >
>> >You set any port you want to be mirrored to using ip link, example:
>> >ip link set dev $DEV1 group 2
>> >ip link set dev $DEV2 group 2
>>
>> That does not looks correct at all. Do tc stuff in tc, no?
>>
>
>We could certainly annotate the dev group via tc but it seems odd ....
>
>cheers,
>jamal
>>
>> >...
>> >
>> >Then you can blockcast:
>> >tc filter add devx protocol ip pref 25 \
>> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>>
>> "blockcasting" to something that is not a block anymore. Not nice.
>>
>> >
>> >cheers,
>> >jamal
>> >
>> >>
>> >> >
>> >> >cheers,
>> >> >jamal
>> >> >
>> >> >> Instead of:
>> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> >> You'd have:
>> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >> >>
>> >> >> I don't see why we need special action for this.
>> >> >>
>> >> >> Regarding "tx_type all":
>> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> >> >> to have this as "no_src_skip" or some other similar arg, without value
>> >> >> acting as a bool (flag) on netlink level.
>> >> >>
>> >> >>