On Fri, Oct 6, 2023 at 12:50 PM Jiri Pirko [off-list ref] wrote:

Fri, Oct 06, 2023 at 05:37:41PM CEST, jhs@mojatatu.com wrote:

quoted

On Fri, Oct 6, 2023 at 8:59 AM Jiri Pirko [off-list ref] wrote:

quoted

Thu, Oct 05, 2023 at 08:42:25PM CEST, victor@mojatatu.com wrote:

quoted

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

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

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

quoted

Makes sense?

quoted

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 [off-list ref] (and horms@kernel.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