Message-ID: <CALnP8ZbaT+jdBvaggAPW=yiW61fip6cjnZcU48tb2-5orqdeMg@mail.gmail.com>
Date: Mon, 27 Nov 2023 10:52:28 -0800
From: Marcelo Ricardo Leitner <mleitner@...hat.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Jiri Pirko <jiri@...nulli.us>, 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

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.

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