Date: Tue, 5 Dec 2023 09:41:02 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Marcelo Ricardo Leitner <mleitner@...hat.com>,
	Jamal Hadi Salim <hadi@...atatu.com>,
	Victor Nogueira <victor@...atatu.com>, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	xiyou.wangcong@...il.com, vladbu@...dia.com, paulb@...dia.com,
	pctammela@...atatu.com, netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce
 blockcast tc action

Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@...atatu.com wrote:
>On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@...atatu.com wrote:
>> >On Mon, Nov 27, 2023 at 1:52 PM Marcelo Ricardo Leitner
>> ><mleitner@...hat.com> wrote:
>> >>
>> >> On Mon, Nov 27, 2023 at 10:50:48AM -0500, Jamal Hadi Salim wrote:
>> >> > On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@...nulli.us> wrote:
>> >> > >
>> >> > > Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@...atatu.com wrote:
>> >> > > >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@...nulli.us> wrote:
>> >> > > >>
>> >> > > >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@...atatu.com wrote:
>> >> > > >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@...nulli.us> wrote:
>> >> > > >> >>
>> >> > > >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@...atatu.com wrote:
>> >> > > >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@...nulli.us> wrote:
>> >> > > >> >> >>
>> >> > > >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@...atatu.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?
>> >> > > >>
>> >> > > >>
>> >> > > >> >
>> >> > > >> >> >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?
>> >> > > >>
>> >> > > >>
>> >> > > >> >...
>> >> > > >> >
>> >> > > >> >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.
>> >>
>> >> +1
>> >>
>> >> > > >>
>> >> > > >
>> >> > > >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
>> >> > > >perhaps a different action altogether.
>> >> > >
>> >> > > mirret redirect? :)
>> >> > >
>> >> > > With target of:
>> >> > > 1) dev (the current one)
>> >> > > 2) block
>> >> > > 3) group
>> >> > > ?
>> >> >
>> >> > tbh, I dont like it - but we need to make progress. I will defer to Marcelo.
>> >>
>> >> With the addition of a new output type that I didn't foresee, that
>> >> AFAICS will use the same parameters as the block output, creating a
>> >> new action for it is a lot of boilerplate for just having a different
>> >> name. If these new two actions can share parsing code and everything,
>> >> then it's not too far for mirred also use. And if we stick to the
>> >> concept of one single action for outputting to multiple interfaces,
>> >> even just deciding on the new name became quite challenging now.
>> >> "groupcast" is misleading. "multicast" no good, "multimirred" not
>> >> intuitive, "supermirred" what? and so on..
>> >>
>> >> I still think that it will become a very complex action, but well,
>> >> hopefully the man page can be updated in a way to minimize the
>> >> confusion.
>> >
>> >Ok, so we are moving forward with mirred "mirror" option only for this then...
>>
>> Could you remind me why mirror and not redirect? Does the packet
>> continue through the stack?
>
>For mirror it is _a copy_ of the packet so it continues up the stack
>and you can have other actions follow it (including multiple mirrors
>after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
>so removed from the stack processing (and cant be sent to more ports).
>That is how mirred has always worked and i believe thats how most
>hardware works as well.
>So sending to multiple ports has to be mirroring semantics (most
>hardware assumes the same semantics).

You assume cloning (sending to multiple ports) means mirror,
that is I believe a mistake. Look at it from the perspective of
replacing device by target for each action. Currently we have:

1) mirred mirror TARGET_DEVICE
   Clones, sends to TARGET_DEVICE and continues up the stack
2) mirred redirect TARGET_DEVICE
   Sends to TARGET_DEVICE, nothing is sent up the stack

For block target, there should be exacly the same semantics:

1) mirred mirror TARGET_BLOCK
   Clones (multiple times, for each block member), sends to TARGET_BLOCK
   and continues up the stack
2) mirred redirect TARGET_BLOCK
   Clones (multiple times, for each block member - 1), sends to
   TARGET_BLOCK, nothing is sent up the stack



>
>cheers,
>jamal
>
>>
>> >
>> >cheers,
>> >jamal
>> >
>> >> Cheers,
>> >> Marcelo
>> >>
>> >> >
>> >> > cheers,
>> >> > jamal
>> >> >
>> >> > >
>> >> > > >
>> >> > > >cheers,
>> >> > > >jamal
>> >> > > >> >
>> >> > > >> >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.
>> >> > > >> >> >>
>> >> > > >> >> >>
>> >> >
>> >>

Powered by blists - more mailing lists