netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paul Blakey <paulb@nvidia.com>
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: netdev@vger.kernel.org, Saeed Mahameed <saeedm@nvidia.com>,
	Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Oz Shlomo <ozsh@nvidia.com>, Jiri Pirko <jiri@nvidia.com>,
	Roi Dayan <roid@nvidia.com>, Vlad Buslov <vladbu@nvidia.com>
Subject: Re: [PATCH net-next 0/4] net/sched: cls_api: Support hardware miss to tc action
Date: Wed, 18 Jan 2023 23:27:21 +0200	[thread overview]
Message-ID: <d7404d79-1d3b-9392-7911-1851f1c37cbf@nvidia.com> (raw)
In-Reply-To: <CAM0EoMm-YVTKWwTEEACjEuyh8C+tWiEWFurPB=F2JUT72nZp4A@mail.gmail.com>



On 18/01/2023 14:54, Jamal Hadi Salim wrote:
> On Tue, Jan 17, 2023 at 9:48 AM Paul Blakey <paulb@nvidia.com> wrote:
>>
>> On 17/01/2023 15:40, Jamal Hadi Salim wrote:
> 
> [..]
> 
>>> Question: How does someone adding these rules tell whether some of
>>> the actions are offloaded and some are not? If i am debugging this because
>>> something was wrong I would like to know.
>>
>> Currently by looking at the per action hw stats, and if they are
>> advancing. This is the same now with filters and the CT action for
>> new connections (driver reports offload, but it means that only for
>> established connections).
> 
> I think that may be sufficient given we use the same technique for
> filter offload.
> Can you maybe post an example of such a working example in your commit message
> with stats?
> You showed a candidate scenario that could be sorted but not a running example.
> 

Sure Ill give it as full example in v3.

>>> It will be an action continue for a scenario where (on ingress) you have
>>> action A from A,B,C being offloaded and B,C is in s/w - the fw filter
>>> will have the
>>> B,C and flower can have A offloaded.
>>> Yes, someone/thing programming these will have to know that only A can
>>> be offloaded
>>> in that graph.
>>
>> I meant using continue to go to next tc priority "as in "action A action
>> continue" but I'm not sure about the actual details of fully supporting
>> this as its not the purpose of this patch, but maybe will lead there.
> 
> Yeah, that was initially confusing when i read the commit log. It sounded
> like action continue == action pipe (because it continues to the next action
> in the action graph).
> Maybe fix the commit to be clearer.

I don't think I mentioned it in the cover letter/commits, or did I miss 
it ?

> 
>>> Ok, so would this work for the scenario I described above? i.e A,B, C where
>>> A is offloaded but not B, C?
>>
>> You mean the reorder? we reorder the CT action first if all other
>> actions are supported, so we do all or nothing.
> 
> Let me give a longer explanation.
> The key i believe is understanding the action dependency. In my mind
> there are 3 levels of
> complexity for assumed ordering of actions A, B, C:
> 
> 1) The simplest thing is to assume all-or-nothing (which is what we
> have done so far in tc);
> IOW if not all of A, B, C can be offloaded then we dont offload. >
> 2) next level of complexity is assuming that A MUST occur before B
> which MUST occur before C.
> Therefore on ingress you can offload part of that graph depending on
> your hardware capability.
> Example: On ingress A, B offloaded and then "continue" to C in s/w if
> your hardware supports
> only offloading A and B but not C. You do the reverse of that graph
> for egress offload.

This is actually the case we want support in this patchset.
Assuming a tc filter has action A , action CT, action B.
If hardware finds that it can't do CT action in hardware (for example 
for a new connection), but we already did action A, we want to continue
executing to "action CT, action B" in sw.

We can use it for partial offload of the action list, but for now it 
will be used for supporting tuple rewrite action followed by action ct 
such as in the example in the cover letter.

> 
> 3) And your case is even more complex because you have a lot more
> knowledge that infact
> there is no action dependency and you can offload something in the
> middle like B.
> So i believe you are solving a harder problem than #2 which is what
> was referring to in
> my earlier email.
> 


This is something we currently do but is "transparent" to the user.
If we have action A, action CT, action B, where A/B != CT and A doesn't 
affect CT result (no dependency), we reorder it internally as action CT, 
action A, action B. Then if we can't do CT in hardware, we didn't do A, 
and can continue in sw to re-execute the filter and actions.

If there is no action dependency then let driver take care of the 
details as we currently do we mlx5.


> The way these things are typically solved is to have a "dependency"
> graph that can be
> programmed depending on h/w offload capability and then you can make a decision
> whether (even in s/w) to allow A,B,C vs C,A,B for example.
> 
> Note: I am not asking for the change - but would be nice to have and I
> think over time
> generalize. I am not sure how other vendors would implement this today.
> 
> Note: if i have something smart in user space - which is what i was
> referring to earlier
> (with mention of skbmark) I can achieve these goals without any kernel
> code changes
> but like i said i understand the operational simplicity by putting
> things in the kernel.

Yeah that would work.


Thanks for the comments,
Paul.

>  > cheers,
> jamal

  reply	other threads:[~2023-01-18 21:28 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-12 10:58 [PATCH net-next 0/4] net/sched: cls_api: Support hardware miss to tc action Paul Blakey
2023-01-12 10:59 ` [PATCH net-next 1/6] " Paul Blakey
2023-01-12 15:45   ` kernel test robot
2023-01-12 10:59 ` [PATCH net-next 2/6] net/sched: flower: Move filter handle initialization earlier Paul Blakey
2023-01-12 10:59 ` [PATCH net-next 3/6] net/sched: flower: Support hardware miss to tc action Paul Blakey
2023-01-12 10:59 ` [PATCH net-next 4/6] net/mlx5: Refactor tc miss handling to a single function Paul Blakey
2023-01-12 10:59 ` [PATCH net-next 5/6] net/mlx5e: Rename CHAIN_TO_REG to MAPPED_OBJ_TO_REG Paul Blakey
2023-01-12 10:59 ` [PATCH net-next 6/6] net/mlx5: TC, Set CT miss to the specific ct action instance Paul Blakey
2023-01-12 12:24 ` [PATCH net-next 0/4] net/sched: cls_api: Support hardware miss to tc action Jamal Hadi Salim
2023-01-15 12:04   ` Paul Blakey
2023-01-17 13:40     ` Jamal Hadi Salim
2023-01-17 14:48       ` Paul Blakey
2023-01-18 12:54         ` Jamal Hadi Salim
2023-01-18 21:27           ` Paul Blakey [this message]
2023-01-23 20:00             ` Jamal Hadi Salim
2023-01-20 11:54       ` Edward Cree
2023-01-22  9:33         ` Paul Blakey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d7404d79-1d3b-9392-7911-1851f1c37cbf@nvidia.com \
    --to=paulb@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ozsh@nvidia.com \
    --cc=pabeni@redhat.com \
    --cc=roid@nvidia.com \
    --cc=saeedm@nvidia.com \
    --cc=vladbu@nvidia.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).