Date: Fri, 6 Oct 2023 11:37:41 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Victor Nogueira <victor@...atatu.com>, xiyou.wangcong@...il.com, davem@...emloft.net, 
	pabeni@...hat.com, edumazet@...gle.com, kuba@...nel.org, mleitner@...hat.com, 
	vladbu@...dia.com, simon.horman@...igine.com, pctammela@...atatu.com, 
	netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next v4 0/3] net/sched: Introduce tc block ports
 tracking and use

On Fri, Oct 6, 2023 at 8:59 AM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Thu, Oct 05, 2023 at 08:42:25PM CEST, victor@...atatu.com wrote:
> >__Context__
> >The "tc block" is a collection of netdevs/ports which allow qdiscs to share
> >match-action block instances (as opposed to the traditional tc filter per
> >netdev/port)[1].
> >
> >Example setup:
> >$ tc qdisc add dev ens7 ingress block 22
> >$ tc qdisc add dev ens8 ingress block 22
> >
> >Once the block is created we can add a filter using the block index:
> >$ tc filter add block 22 protocol ip pref 25 \
> >  flower dst_ip 192.168.0.0/16 action drop
> >
> >A packet with dst IP matching 192.168.0.0/16 arriving on the ingress of
> >either ens7 or ens8 is dropped.
> >
> >__This patchset__
> >Up to this point in the implementation, the block is unaware of its ports.
> >This patch fixes that and makes the tc block ports available to the
>
> Odd. You fix a bug. Is there a bug? If yes, you need to describe it. If
> no, don't use "fix".

Ok, Jiri;->  we will change the language.

>
> >datapath.
> >
> >For the datapath we provide a use case of the tc block in an action
> >we call "blockcast" in patch 3. This action can be used in an example as
> >such:
> >
> >$ tc qdisc add dev ens7 ingress block 22
> >$ tc qdisc add dev ens8 ingress block 22
> >$ tc qdisc add dev ens9 ingress block 22
> >$ tc filter add block 22 protocol ip pref 25 \
> >  flower dst_ip 192.168.0.0/16 action blockcast
>
> Seems to me a bit odd that the action works with the entity (block) is
> is connected to. I would expect rather to give the action configuration:
>
> $ tc filter add block 22 protocol ip pref 25 \
>   flower dst_ip 192.168.0.0/16 action blockcast block 22
>                                                 ^^^^^^^^

We are currently passing the blockid in the skb cb field so it is
configuration-less. I suppose we could add this as an optional field
and use it when specified.

> Then this is more flexible and allows user to use this action for any
> packet, no matter from where it was received.
>
> Looks like this is functionality-wise similar to mirred redirect. Why
> can't we have that action extended to accept block number instead of
> netdev and have something like:
>
> $ tc filter add block 22 protocol ip pref 25 \
>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>
> This would be very much alike we do either "tc filter add dev X" or "tc
> filter add block Y".
>

We did consider it but concluded it is a lot of work to get it done on
mirred - just take a look at mirred and you'll see what i mean;->
Based on that review we came to the conclusion that at some point it
would be safer to separate mirred's mirror from redirect; there are
too many checks to avoid one or the other based on whether you are
coming from egress vs ingress etc. This one is simple, it is just a
broadcast.


> Regarding the filtering, that could be a simple flag config of mirred
> action:
>
> $ tc filter add block 22 protocol ip pref 25 \
>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>   srcfilter
>
> Or something like that.
>

See my comment above.

cheers,
jamal
> Makes sense?
>
>
>
> >
> >When a packet(matching dst IP 192.168.0.0/16) arrives on the ingress of any
> >of ens7, ens8 or ens9 it will be copied to all ports other than itself.
> >For example, if it arrives on ens8 then a copy of the packet will be
> >"blockcasted";-> to both ens7 and ens9 (unmodified), but not to ens8.
> >
> >Patch 1 introduces the required infra. Patch 2 exposes the tc block to the
> >tc datapath and patch 3 implements datapath usage via a new tc action
> >"blockcast".
> >
> >__Acknowledgements__
> >Suggestions from Vlad Buslov and Marcelo Ricardo Leitner made this patchset
> >better. The idea of integrating the ports into the tc block was suggested
> >by Jiri Pirko.
> >
> >[1] See commit ca46abd6f89f ("Merge branch 'net-sched-allow-qdiscs-to-share-filter-block-instances'")
> >
> >Changes in v2:
> >  - Remove RFC tag
> >  - Add more details in patch 0(Jiri)
> >  - When CONFIG_NET_TC_SKB_EXT is selected we have unused qdisc_cb
> >    Reported-by: kernel test robot <lkp@...el.com> (and horms@...nel.org)
> >  - Fix bad dev dereference in printk of blockcast action (Simon)
> >
> >Changes in v3:
> >  - Add missing xa_destroy (pointed out by Vlad)
> >  - Remove bugfix pointed by Vlad (will send in separate patch)
> >  - Removed ports from subject in patch #2 and typos (suggested by Marcelo)
> >  - Remove net_notice_ratelimited debug messages in error
> >    cases (suggested by Marcelo)
> >  - Minor changes to appease sparse's lock context warning
> >
> >Changes in v4:
> >  - Avoid code repetition using gotos in cast_one (suggested by Paolo)
> >  - Fix typo in cover letter (pointed out by Paolo)
> >  - Create a module description for act_blockcast
> >    (reported by Paolo and CI)
> >
> >Victor Nogueira (3):
> >  net/sched: Introduce tc block netdev tracking infra
> >  net/sched: cls_api: Expose tc block to the datapath
> >  net/sched: act_blockcast: Introduce blockcast tc action
> >
> > include/net/sch_generic.h    |   8 +
> > include/net/tc_wrapper.h     |   5 +
> > include/uapi/linux/pkt_cls.h |   1 +
> > net/sched/Kconfig            |  13 ++
> > net/sched/Makefile           |   1 +
> > net/sched/act_blockcast.c    | 297 +++++++++++++++++++++++++++++++++++
> > net/sched/cls_api.c          |  12 +-
> > net/sched/sch_api.c          |  58 +++++++
> > net/sched/sch_generic.c      |  34 +++-
> > 9 files changed, 426 insertions(+), 3 deletions(-)
> > create mode 100644 net/sched/act_blockcast.c
> >
> >--
> >2.25.1
> >

Powered by blists - more mailing lists