[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM0EoM=HDgawk5W70OxJThVsNvpyQ3npi_6Lai=nsk14SDM_xQ@mail.gmail.com>
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