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 03:29:00PM CET, mleitner@redhat.com wrote:
>On Thu, Nov 23, 2023 at 08:37:13AM -0500, Jamal Hadi Salim 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
>
>Me too, and actually I thought Jiri had agreed to it with some
>remarks to be addressed (which I don't know if there were, I didn't
>read this version yet).

I looked today and didn't' find it. If I missed it, sorry. I still don't
understand why new action is needed for redirect to X instead of Y.

>
>> 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
>> 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?
>>
>> 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.
>> >
>> >
>>
>