Date:   Sun, 15 Jan 2023 14:04:06 +0200
From:   Paul Blakey <paulb@...dia.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     netdev@...r.kernel.org, Saeed Mahameed <saeedm@...dia.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.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 0/4] net/sched: cls_api: Support hardware miss to
 tc action



On 12/01/2023 14:24, Jamal Hadi Salim wrote:
> On Thu, Jan 12, 2023 at 5:59 AM Paul Blakey <paulb@...dia.com> wrote:
>>
>> Hi,
>>
>> This series adds support for hardware miss to 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.
>>
>> This miss to action supports partial offload of a filter's action list,
>> and and let software continue processing where hardware left off.
>>
>> Example is the CT action, where new connections need to be handled in
>> software. And if there is a packet modifying action before the CT action,
>> then restoring only the chain on misses might cause the rule to not
>> re-execute the relevant filter in software.
>>
>> Consider the following scenario:
>>
>> $ tc filter add dev dev1 ingress chain 0 proto ip flower \
>>    ct_state -trk dst_mac fe:50:56:26:13:7d \
>>    action pedit ex munge eth dst aa:bb:cc:dd:ee:01 \
>>    action ct \
>>    action goto chain 1
>> $ tc filter add dev dev1 ingress chain 1 proto ip flower \
>>    ct_state +trk+est \
>>    action mirred egress redirect dev ...
>> $ tc filter add dev dev1 ingress chain 1 proto ip flower \
>>    ct_state +trk+new \
>>    action ct commit \
>>    action mirred egress redirect dev dev2
>>
>> $ tc filter add dev dev2 ingress chain 0 proto ip flower \
>>    action ct \
>>    action mirred egress redirect dev dev1
>>
>> A packet doing the pedit in hardware (setting dst_mac to aa:bb:cc:dd:ee:01),
>> missing in the ct action, and restarting in chain 0 in software will fail
>> matching the original dst_mac in the flower filter on chain 0.
> 
> 
> I had to read that a couple of times and didnt get it until i went and
> started looking at the patches and i am not 100% sure still.. IIUC, you have
> something like:
> match X action A action B action C
> You do action A in HW and then you want to continue with B and C in SW.
> If that is correct then the cover letter would be an easier read if it
> is laid out as
> follows:
> 
> -"consider the following scenario" (with examples you had above)....
> Actually those example were not clear, i think the reader is meant to
> assume lack of skip_sw means they are available both in hw and sw.
> 
> -Explain your goals and why it wont work (I think you did that well above)
> 
> -"and now with the changes in this patchset, the issue can be resolved
> with rearranging the policy setup as follows"...

Sure I can work on that for v2.

> 
> Having said that: Would this have been equally achieved with using skbmark?
> The advantage is removing the need to change the cls sw datapath.
> The HW doesnt have to support skb mark setting, just driver transform.
> i.e the driver can keep track of the action mapping and set the mark when
> it receives the packet from hw.
> You rearrange your rules to use cls fw and have action B and C  match
> an skb mark.
> 

The user would then need to be familiar with how it works for this 
specific vendor, of which actions are supported, and do so for all the 
rules that have such actions. He will also need to add a
skb mark action after each such successful execution that the driver 
should ignore.

Also with this patchset we actually support tc action continue, where 
with cls_fw it wont' be possible.



>> The above scenario is supported in mlx5 driver by reordering the actions
>> so ct will be done in hardware before the pedit action, but some packet
>> modifications can't be reordered in regards to the ct action. An example
>> of that is a modification to the tuple fields (e.g action pedit ex munge ip
>> dst 1.1.1.1) since it affects the ct action's result (as it does lookup based
>> on ip).
> 
> Also above text wasnt clear. It sounded like the driver would magically
> know that it has to continue action B and C in s/w because it knows the
> hardware wont be able to exec them?

It means that if MLX5 driver gets "action A, action CT, action B",
where action CT is only possible in hardware for offloaded established 
connections (offloaded via flow table).

We reorder the actions and parse it as if the action list was: "action 
CT, action A, action B" and then if we miss in action CT we didn't do 
any modifications (actions A/B, and counters) in hardware.

We can only do this if the actions are independent, and so to support
dependent actions (pedit for src ip followed by action CT), we have this 
patchset.



> 
> cheers,
> jamal

Powered by blists - more mailing lists