Date: Fri, 6 Oct 2023 15:06:45 -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 12:50 PM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Fri, Oct 06, 2023 at 05:37:41PM CEST, jhs@...atatu.com wrote:
> >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.
>
> I don't understand the need for configuration less here. You don't have
> it for the rest of the actions. Why this is special?

It is not needed really. Think of an L2 switch - the broadcast action
is to send to all ports but self.

>
> >
> >> 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.
>
> Perhaps it is a nice opportunity to do such mirred cleanup, prepare the
> code and implement block send afterwards?

I was worried about breaking some existing use cases - the code has
got too clever.
But probably it is time to show it some love, one of us will invest
time into it.

> If I omit the code for now, from user perspective, this functionality
> belongs into mirred, don't you think? Just replace "dev" by "block" and
> you got what you need.

If we can adequately cleanup mirred,  then we can put it there but
certainly now we are adding more buttons to click on mirred. It may
make sense to refactor the mirred code then reuse the refactored code
in a new action.

cheers,
jamal

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