On 20/01/2023 13:54, Edward Cree wrote:

On 17/01/2023 13:40, Jamal Hadi Salim wrote:

quoted

I agree that with your patch it will be operationally simpler. I hope other
vendors will be able to use this feature (and the only reason i am saying
this is because you are making core tc changes).

FTR at AMD/sfc we wouldn't use this as our HW has all action execution after
  all match stages in the pipeline (excepting actions that only control match
  behaviour, i.e. ct lookup), so users on ef100 HW (and I'd imagine probably
  some other vendors' products too) would still need to rewrite their rules
  with skbmark.
I mention this because this feature / patch series disconcerts me.  I wasn't
  even really happy about the 'miss to chain' feature, but even more so 'miss
  to action' feels like it makes the TC-driver offload interface more complex
  than it really ought to be.
Especially because the behaviour in some cases is already weird even with a
  fully offloadable rule; consider a match-all filter with 'action vlan push'
  and no further actions (specifically no redirect).  AIUI the HW will push
  the vlan, then deliver to the default destination (e.g. repr if the packet
  came from a VF), at which point TC SW will apply the same rule and perform
  the vlan push again, leading (incorrectly) to a double-tagged packet.

If I understand correctly, a "action vlan push" which in SW would return 
OK to end classification or CONTINUE to continue to next prio (not sure 
what the default for this action is), then we don't offload such rules, 
as we require a endpoint (goto/drop/mirred), but if we did, I guess we 
would have used this series to point to end of the last run action list 
continuing as software would (returning OK or CONTINUE in the right 
context) or not do the vlan push and jump right before it saving having 
to match again. This might need a small change to allow jumping to the 
end of the action list.

So it's not really about 'miss', there's a more fundamental issue with how
  HW offload and the SW path interact.  And I don't think it's possible to
  guaranteed-remove that issue without a performance cost (skb ext is
  expensive and we don't want it on *every* RX packet), so users will always
  need to consider this sort of thing when writing their TC rules.

We have the skb ext only when hardware already did some stuff (offloaded 
something), so what ever it did hopefully pays for using the skb ext, 
and I think this is the case. And of course we strive for full offload 
so this case would be an exception.

TBH doing an IP address pedit before a CT lookup seems like a fairly
  contrived use case to me.  Is there a pressing real-world need for it, or
  are mlx just adding this support because they can?
 > -ed

Of course, we see when users want to set up stateless NAT outside of 
action CT.

Paul.