Message-ID: <CALnP8ZaEFnd=N_oFar+8hBF=XukRis92cnW4KBtywxnO4u9=zQ@mail.gmail.com>
Date:   Mon, 6 Feb 2023 21:03:15 -0800
From:   Marcelo Leitner <mleitner@...hat.com>
To:     Ilya Maximets <i.maximets@....org>
Cc:     Paul Blakey <paulb@...dia.com>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Oz Shlomo <ozsh@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Roi Dayan <roid@...dia.com>, Vlad Buslov <vladbu@...dia.com>
Subject: Re: [PATCH net-next v8 0/7] net/sched: cls_api: Support hardware miss
 to tc action

On Tue, Feb 07, 2023 at 01:20:55AM +0100, Ilya Maximets wrote:
> On 2/6/23 18:14, Paul Blakey wrote:
> >
> >
> > On 06/02/2023 14:34, Ilya Maximets wrote:
> >> On 2/5/23 16:49, Paul Blakey wrote:
> >>> Hi,
> >>>
> >>> This series adds support for hardware miss to instruct tc to continue execution
> >>> in a specific tc action instance on a filter's action list. The mlx5 driver patch
> >>> (besides the refactors) shows its usage instead of using just chain restore.
> >>>
> >>> Currently a filter's action list must be executed all together or
> >>> not at all as driver are only able to tell tc to continue executing from a
> >>> specific tc chain, and not a specific filter/action.
> >>>
> >>> This is troublesome with regards to action CT, where new connections should
> >>> be sent to software (via tc chain restore), and established connections can
> >>> be handled in hardware.
> >>>
> >>> Checking for new connections is done when executing the ct action in hardware
> >>> (by checking the packet's tuple against known established tuples).
> >>> But if there is a packet modification (pedit) action before action CT and the
> >>> checked tuple is a new connection, hardware will need to revert the previous
> >>> packet modifications before sending it back to software so it can
> >>> re-match the same tc filter in software and re-execute its CT action.
> >>>
> >>> The following is an example configuration of stateless nat
> >>> on mlx5 driver that isn't supported before this patchet:
> >>>
> >>>   #Setup corrosponding mlx5 VFs in namespaces
> >>>   $ ip netns add ns0
> >>>   $ ip netns add ns1
> >>>   $ ip link set dev enp8s0f0v0 netns ns0
> >>>   $ ip netns exec ns0 ifconfig enp8s0f0v0 1.1.1.1/24 up
> >>>   $ ip link set dev enp8s0f0v1 netns ns1
> >>>   $ ip netns exec ns1 ifconfig enp8s0f0v1 1.1.1.2/24 up
> >>>
> >>>   #Setup tc arp and ct rules on mxl5 VF representors
> >>>   $ tc qdisc add dev enp8s0f0_0 ingress
> >>>   $ tc qdisc add dev enp8s0f0_1 ingress
> >>>   $ ifconfig enp8s0f0_0 up
> >>>   $ ifconfig enp8s0f0_1 up
> >>>
> >>>   #Original side
> >>>   $ tc filter add dev enp8s0f0_0 ingress chain 0 proto ip flower \
> >>>      ct_state -trk ip_proto tcp dst_port 8888 \
> >>>        action pedit ex munge tcp dport set 5001 pipe \
> >>>        action csum ip tcp pipe \
> >>>        action ct pipe \
> >>>        action goto chain 1
> >>>   $ tc filter add dev enp8s0f0_0 ingress chain 1 proto ip flower \
> >>>      ct_state +trk+est \
> >>>        action mirred egress redirect dev enp8s0f0_1
> >>>   $ tc filter add dev enp8s0f0_0 ingress chain 1 proto ip flower \
> >>>      ct_state +trk+new \
> >>>        action ct commit pipe \
> >>>        action mirred egress redirect dev enp8s0f0_1
> >>>   $ tc filter add dev enp8s0f0_0 ingress chain 0 proto arp flower \
> >>>        action mirred egress redirect dev enp8s0f0_1
> >>>
> >>>   #Reply side
> >>>   $ tc filter add dev enp8s0f0_1 ingress chain 0 proto arp flower \
> >>>        action mirred egress redirect dev enp8s0f0_0
> >>>   $ tc filter add dev enp8s0f0_1 ingress chain 0 proto ip flower \
> >>>      ct_state -trk ip_proto tcp \
> >>>        action ct pipe \
> >>>        action pedit ex munge tcp sport set 8888 pipe \
> >>>        action csum ip tcp pipe \
> >>>        action mirred egress redirect dev enp8s0f0_0
> >>>
> >>>   #Run traffic
> >>>   $ ip netns exec ns1 iperf -s -p 5001&
> >>>   $ sleep 2 #wait for iperf to fully open
> >>>   $ ip netns exec ns0 iperf -c 1.1.1.2 -p 8888
> >>>
> >>>   #dump tc filter stats on enp8s0f0_0 chain 0 rule and see hardware packets:
> >>>   $ tc -s filter show dev enp8s0f0_0 ingress chain 0 proto ip | grep "hardware.*pkt"
> >>>          Sent hardware 9310116832 bytes 6149672 pkt
> >>>          Sent hardware 9310116832 bytes 6149672 pkt
> >>>          Sent hardware 9310116832 bytes 6149672 pkt
> >>>
> >>> A new connection executing the first filter in hardware will first rewrite
> >>> the dst port to the new port, and then the ct action is executed,
> >>> because this is a new connection, hardware will need to be send this back
> >>> to software, on chain 0, to execute the first filter again in software.
> >>> The dst port needs to be reverted otherwise it won't re-match the old
> >>> dst port in the first filter. Because of that, currently mlx5 driver will
> >>> reject offloading the above action ct rule.
> >>>
> >>> This series adds supports partial offload of a filter's action list,
> >>> and letting tc software continue processing in the specific action instance
> >>> where hardware left off (in the above case after the "action pedit ex munge tcp
> >>> dport... of the first rule") allowing support for scenarios such as the above.
> >>
> >>
> >> Hi, Paul.  Not sure if this was discussed before, but don't we also need
> >> a new TCA_CLS_FLAGS_IN_HW_PARTIAL flag or something like this?
> >>
> >> Currently the in_hw/not_in_hw flags are reported per filter, i.e. these
> >> flags are not per-action.  This may cause confusion among users, if flows
> >> are reported as in_hw, while they are actually partially or even mostly
> >> processed in SW.
> >>
> >> What do you think?
> >>
> >> Best regards, Ilya Maximets.
> >
> > I think its a good idea, and I'm fine with proposing something like this in a
> > different series, as this isn't a new problem from this series and existed before
> > it, at least with CT rules.
>
> Hmm, I didn't realize the issue already exists.

Maintainers: please give me up to Friday to review this patchset.

Disclaimer: I had missed this patchset, and I didn't even read it yet.

I don't follow. Can someone please rephase the issue please?
AFAICT, it is not that the NIC is offloading half of the action list
and never executing a part of it. Instead, for established connections
the rule will work fully offloaded. While for misses in the CT action,
it will simply trigger a miss, like it already does today.

>
> >
> > So how about I'll propose it in a different series and we continue with this first?

So I'm not sure either on what's the idea here.

Thanks,
Marcelo

>
> Sounds fine to me.  Thanks!
>
> Best regards, Ilya Maximets.
>

Powered by blists - more mailing lists