On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty
[off-list ref] wrote:

On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob [off-list ref] wrote:

quoted

On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty
[off-list ref] wrote:

quoted

Hi

On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob [off-list ref] wrote:

quoted

On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
[off-list ref] wrote:

quoted

Hi, and thanks a lot for your RFC v1 comments.

RFC v2 emphasize the intent for sharing the flow action:
* The term 'action context' was unclear and replaced with
   'shared action'.
* RFC v2 subject became 'add flow shared action API'.
* all proposed APIs renamed according the above.

The new shared action is an independent entity decoupled from any flow
while any flow can reuse such an action. Please go over the RFC
description, it was almost entirely rewritten.

@Jerin Jacob:
Thanks again for your comments, it made me admit that v1 description was
incomplete & unclear.  I hope v2 will be better at least in terms of
clarity.

The public API and its usage is very clear. Thanks for this RFC.

My pleasure.

quoted

I think, RFC v2 still not addressing the concern raised in the
http://mails.dpdk.org/archives/dev/2020-June/169296.html.

Since MLX hardware has an HW based shared object it is fine to have
public API based on that level of abstraction.
But at the PMD driver level we need to choose the correct abstraction
to support all PMD and support shared object scheme if possible.

I purpose to introduce something below or similar
            int (*action_update)
                (struct rte_eth_dev *,
                  struct rte_flow *flow,
                 const struct rte_flow_action [],
                 struct rte_flow_error *);

Where this callback suppose to belong (struct rte_flow_ops)?

Yes.

quoted

How should it be implemented by PMD?

See below,

quoted

Is it about shared action and if "yes" why there is 'flow' argument?

flow holds the "pattern" and "action" data as PMD specific handle.
So PMD, implementation can just change that action if it gets the PMD
specific handle.

quoted

quoted

in addition to: shared_action_create, shared_action_destroy,
shared_action_update, shared_action_query

Have generic implementation of above, if action_update callback is not
NULL.

"is not NULL" -> "is NULL"?

Yes. When it is NULL.

Jerin, few clarifications regarding generic implementation of shared action:
Based on this conversation I'm assuming that generic implementation supposed to be something like:
For each flow using some shared action:
call ops-> action_update()
If the assumption above correct:
1. taking into account that shared_action_update() is atomic, how can this deal with partial success: some flows may fail validation - should it:
  1.1.lock all flows
  1.2.validate all flows
  1.3.update all flows
  1.4. unlock

Yes.

2. action_update callback is PMD specific & if it's unsupported there is no support for shared action any way

Yes.

Please address the issues above

quoted

quoted

quoted

So that, it can work all PMDs and to
avoid the duplication of "complex" shared session management code.

Do you mean shared action in use by multiple flows by "shared session"?

Yes.

Common 'shared session' management code:
- can be reduced to atomic usage counter
- maintaining list of flow using shared action expected to impact performance & not necessary for all PMD specific implementations
Access to other shared resources hard to generalize because:
- for some PMDs mutual exclusion is HW feature & no need to protect it in SW
- for others there may be multiple resources & access to each one protected by different mechanism

The general callback you can assume, it supports only action_update
based callback.
If PMD has mutual exclusion HW feature then it can override the
function pointers.

An observation related to action_update callback:
If replaced (updated) action was shared then the flow won't be influenced any more by updates or removed shared action.