On Wed, Dec 13, 2017 at 07:14:19PM, Nelio Laranjeiro wrote:
Hi,
On Wed, Dec 13, 2017 at 07:23:19PM +0530, Anoob Joseph wrote:
Hi Nelio,
On 12/13/2017 06:23 PM, Nelio Laranjeiro wrote:
Hi Anoob,
On Wed, Dec 13, 2017 at 05:08:19PM +0530, Anoob Joseph wrote:
Hi Nelio,
On 12/13/2017 03:32 PM, Nelio Laranjeiro wrote:
Hi Anoob,
On Wed, Dec 13, 2017 at 12:11:18PM +0530, Anoob Joseph wrote:
Hi Nelio,
On 12/12/2017 08:08 PM, Nelio Laranjeiro wrote:
Hi Anoob,
On Tue, Dec 12, 2017 at 07:34:31PM +0530, Anoob Joseph wrote:
Hi Nelio,
On 12/12/2017 07:14 PM, Nelio Laranjeiro wrote:
Hi Anoob,
On Tue, Dec 12, 2017 at 06:13:08PM +0530, Anoob Joseph
wrote:
Hi Nelio,
On 12/11/2017 07:34 PM, Nelio Laranjeiro wrote:
Mellanox INNOVA NIC needs to have final target queue
actions to perform inline crypto.
Signed-off-by: Nelio Laranjeiro
<nelio.laranje...@6wind.com>
---
Changes in v3:
* removed PASSTHRU test for ingress.
* removed check on configured queues for the queue
action.
Changes in v2:
* Test the rule by PASSTHRU/RSS/QUEUE and apply the
first one validated.
---
examples/ipsec-secgw/ipsec.c | 57
++++++++++++++++++++++++++++++++++++++++++--
examples/ipsec-secgw/ipsec.h | 2 +-
2 files changed, 56 insertions(+), 3
deletions(-)
diff --git a/examples/ipsec-secgw/ipsec.c
b/examples/ipsec-secgw/ipsec.c index
17bd7620d..1b8b251c8 100644
--- a/examples/ipsec-secgw/ipsec.c
+++ b/examples/ipsec-secgw/ipsec.c
@@ -142,6 +142,7 @@ create_session(struct ipsec_ctx
*ipsec_ctx, struct ipsec_sa *sa)
rte_eth_dev_get_sec_ctx(
sa-
portid);
const struct rte_security_capability
*sec_cap;
+ int ret = 0;
sa->sec_session =
rte_security_session_create(ctx,
&sess_conf,
ipsec_ctx->session_pool); @@
-201,15 +202,67 @@ create_session(struct ipsec_ctx
*ipsec_ctx, struct ipsec_sa *sa)
sa->action[0].type =
RTE_FLOW_ACTION_TYPE_SECURITY;
sa->action[0].conf = sa->sec_session;
- sa->action[1].type =
RTE_FLOW_ACTION_TYPE_END;
-
sa->attr.egress = (sa->direction ==
RTE_SECURITY_IPSEC_SA_DIR_EGRESS);
sa->attr.ingress = (sa->direction ==
RTE_SECURITY_IPSEC_SA_DIR_INGRESS);
+ if (sa->attr.ingress) {
+ uint8_t rss_key[40];
+ struct rte_eth_rss_conf
rss_conf = {
+ .rss_key = rss_key,
+ .rss_key_len = 40,
+ };
+ struct rte_eth_dev
*eth_dev;
+ union {
+ struct
rte_flow_action_rss rss;
+ struct {
+ const struct
rte_eth_rss_conf *rss_conf;
+ uint16_t num;
+ uint16_t
queue[RTE_MAX_QUEUES_PER_PORT];
+ } local;
+ } action_rss;
+ unsigned int i;
+ unsigned int j;
+
+ sa->action[2].type =
RTE_FLOW_ACTION_TYPE_END;
+ /* Try RSS. */
+ sa->action[1].type =
RTE_FLOW_ACTION_TYPE_RSS;
+ sa->action[1].conf =
&action_rss;
+ eth_dev = ctx->device;
+
rte_eth_dev_rss_hash_conf_get(sa->portid,
+
&rss_conf);
+ for (i = 0, j = 0;
+ i < eth_dev->data-
nb_rx_queues; ++i)
+ if (eth_dev->data-
rx_queues[i])
+
action_rss.local.queue[j++] = i;
+ action_rss.local.num = j;
+ action_rss.local.rss_conf =
&rss_conf;
+ ret = rte_flow_validate(sa-
portid, &sa->attr,
+ sa-
pattern, sa->action,
+ &err);
+ if (!ret)
+ goto flow_create;
+ /* Try Queue. */
+ sa->action[1].type =
RTE_FLOW_ACTION_TYPE_QUEUE;
+ sa->action[1].conf =
+ &(struct
rte_flow_action_queue){
+ .index = 0,
+ };
+ ret = rte_flow_validate(sa-
portid, &sa->attr,
+ sa-
pattern, sa->action,
+ &err);
+ if (ret)
+ goto
flow_create_failure;
+ } else {
+ sa->action[1].type =
+
RTE_FLOW_ACTION_TYPE_PASSTHRU;
+ sa->action[2].type =
RTE_FLOW_ACTION_TYPE_END;
We would need flow validate here also. And, for
egress, the application will be able to set metadata
(set_pkt_metadata API) per packet. So flow may not be
required for such cases. But if the flow create fails,
the session create would also fail. It might be better if we
check whether the PMD would need metadata (part of the sec_cap-
ol_flags).
Seems what you are describing is outside of this scope
which is only related to correctly implement the generic
flow API with terminal actions.
Since SECURITY+PASSTHRU won't be terminal, this code
segment might be misleading.
Well, I don't mind adding an extra verification even if the
create should fail if the validate fails, as there is no
other option it is just like adding another if statement
considering the validate() cannot guarantee the flow will
be created(), other errors like ENOMEM are still possible in the
creation stage.
Good point. I was thinking of a scenario when flow for egress
itself would be optional.
I'll suggest to add it in another patch.
Anyway, the flow validate is useful in the ingress to
select the best behavior RSS/Queue, if the flow validate
may fail, the flow create should also fail for the same reasons.
If the driver doesn't need metadata and the flow
create fails, then the create session should fail. Any
thoughts?
How the create_session() can fail without having all the
informations (pattern, metadata, ...) the application wants to
offload?
Is flow mandatory for the egress traffic? My understanding is,
it's not.
"set_pkt_metadata" API gives application the ability to do
the lookup and pass the info along with the packet. In
such cases, flow creation is not necessary.
Some NIC need to apply a flow rule for Egress and they don't
need metadata for the packet.
Understood. In that case, what I proposed could be a separate
patch. The ingress path is proper with this patch, but we can
keep egress open for improvements.
What do you mean with "keep egrees open for improvements"?
In egress side, this set of flow actions won't be having any
terminating action. And addition of PASSTHRU won't be required, as it
will be implicit.
Flow API does not define any behavior on Egress. We have to define it.
Understood.
Creating flow for egress would allow hardware to perform the SA
lookup. But we cannot remove the lookup in application, as it's
the SA which has the information whether the packet need to be
completely offloaded. Once this lookup is done, this information
could be communicated to hardware using the set_pkt_metadata.
This will eliminate the second lookup in the hardware. So the flow
could be optional. The current patch assumes flow is mandatory for
egress as well.
For Cavium hardware, egress side flow is not required and we will
be using "set_pkt_metadata" API. May be Radu can give his thoughts
on this.
Got it, what is missing here is a verification on the sa->ol_flags
and only use the rte_flow for RTE_SECURITY_TX_HW_TRAILER_OFFLOAD
as
other NICs are using the RTE_SECURITY_TX_OLOAD_NEED_MDATA.
Precisely.
I'll make the changes discussed here, splitting this patch into ingress/Egress
and use the of_flags.
Do you know why such difference is not hidden by the library? It
won't help application which will need to have different
configuration path depending on the NIC capabilities.
I can only speculate the reasons. I think, application will have to
know the NIC capabilities as the usage of rte_flow would be a one time
configuration while using set_pkt_metadata would be per packet API
call. If library is to hide this, it would incur unwanted checks in the data
path.
Why not embedding all the configuration part necessary for all PMD (as this
example is making though this modifications) inside rte_security library and
in et_dev add a new Tx burst API with another array containing the metadata
for the each packet.
PMD who needs such metadata have it along with the packet they are
processing, others can ignore it.
From an application point of view, this become transparent and friendly, one
function to set the offload request, one function to send packets and
another one in Rx for the symmetry if necessary.
