Hi Sowmini,
> -----Original Message-----
> From: Sowmini Varadhan <sovar...@linux.microsoft.com>
> Sent: Wednesday, August 18, 2021 4:01 PM
> To: sowmin...@gmail.com; Iremonger, Bernard
> <bernard.iremon...@intel.com>; dev@dpdk.org;
> sovar...@linux.microsoft.com
> Cc: tho...@monjalon.net
> Subject: [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp
>
> v2 updates: typo fixes for checkpatch, bernard.iremonger comments
The above line should not be added to the commit message.
~/dpdk/devtools/check-git-log.sh -1
Wrong tag:
v2 fixes: typo fixes, get_tcp_flags returns -EINVAL
The check-git-log.sh script is in your dpdk checkout, best to run this script
before sending patches
>
> The struct rte_flow_classifier can have up to
> RTE_FLOW_CLASSIFY_TABLE_MAX
> (32) classifier tables, but the existing flow_classify examples only adds a
> single table for the L4 5-tuple.
>
> When dealing with tcp flows, we frequently want to add ACLs and filters to
> filter based on the state of the TCP connection, e.g., by looking at the tcp
> flags field.
>
> So we enhance flow_classify to add an additional acl table for tcp 5-tuples.
> If
> the input-file-parser detects a filter for a tcp flow with a non-wildcard
> argument for tcp_flags, the IP4_TCP_5TUPLE table is used by flow_classify.
>
> Signed-off-by: Sowmini Varadhan <sovar...@linux.microsoft.com>
> ---
If you want comment on the patch updates (optional), it can be added here after
the --- line
> examples/flow_classify/flow_classify.c | 41 +++++++---
> lib/flow_classify/rte_flow_classify.c | 87 +++++++++++++++++++++
> lib/flow_classify/rte_flow_classify.h | 19 +++++
> lib/flow_classify/rte_flow_classify_parse.c | 8 +-
> 4 files changed, 142 insertions(+), 13 deletions(-)
This patch contains changes to the flow_classify example and the flow_classify
library.
It needs to be split in two, a patch for the flow_classify example and a patch
for the flow classify library.
>
> diff --git a/examples/flow_classify/flow_classify.c
> b/examples/flow_classify/flow_classify.c
> index ff4c8bc2f5..8f6aacae03 100644
> --- a/examples/flow_classify/flow_classify.c
> +++ b/examples/flow_classify/flow_classify.c
> @@ -723,11 +723,6 @@ add_classify_rule(struct rte_eth_ntuple_filter
> *ntuple_filter,
> return ret;
> }
>
> - /* XXX but this only adds table_type of
> - * RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE
> - * i.e., it only ever does allocate_acl_ipv4_5tuple_rule()
> - * so the tcp_flags is ignored!
> - */
> rule = rte_flow_classify_table_entry_add(
> cls_app->cls, &attr, pattern_ipv4_5tuple,
> actions, &key_found, &error);
> @@ -856,7 +851,8 @@ main(int argc, char *argv[])
> int ret;
> int socket_id;
> struct rte_table_acl_params table_acl_params;
> - struct rte_flow_classify_table_params cls_table_params;
> + struct rte_table_acl_params table_acl_tcp_params;
> + struct rte_flow_classify_table_params cls_table_params[2];
> struct flow_classifier *cls_app;
> struct rte_flow_classifier_params cls_params;
> uint32_t size;
> @@ -923,21 +919,42 @@ main(int argc, char *argv[])
> memcpy(table_acl_params.field_format, ipv4_defs,
> sizeof(ipv4_defs));
>
> /* initialise table create params */
> - cls_table_params.ops = &rte_table_acl_ops;
> - cls_table_params.arg_create = &table_acl_params;
> - cls_table_params.type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> + cls_table_params[0].ops = &rte_table_acl_ops;
> + cls_table_params[0].arg_create = &table_acl_params;
> + cls_table_params[0].type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +
> + /* initialise ACL table params */
> + table_acl_tcp_params.name = "table_acl_ipv4_tcp_5tuple";
> + table_acl_tcp_params.n_rules = FLOW_CLASSIFY_MAX_RULE_NUM;
> + table_acl_tcp_params.n_rule_fields = RTE_DIM(ipv4_defs);
> + memcpy(table_acl_tcp_params.field_format, ipv4_defs,
> +sizeof(ipv4_defs));
> +
> + /* initialise table create params */
> + cls_table_params[1].ops = &rte_table_acl_ops;
> + cls_table_params[1].arg_create = &table_acl_tcp_params;
> + cls_table_params[1].type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
>
> - ret = rte_flow_classify_table_create(cls_app->cls,
> &cls_table_params);
> + ret = rte_flow_classify_table_create(cls_app->cls,
> + &cls_table_params[0]);
> if (ret) {
> rte_flow_classifier_free(cls_app->cls);
> rte_free(cls_app);
> rte_exit(EXIT_FAILURE, "Failed to create classifier table\n");
> }
> + ret = rte_flow_classify_table_create(cls_app->cls,
> + &cls_table_params[1]);
> + if (ret) {
> + rte_flow_classifier_free(cls_app->cls);
> + rte_free(cls_app);
> + rte_exit(EXIT_FAILURE,
> + "Failed to create classifier table\n");
> + }
> +
> /* >8 End of initialization of table create params. */
>
> /* read file of IPv4 5 tuple rules and initialize parameters
> - * for rte_flow_classify_validate and
> rte_flow_classify_table_entry_add
> - * API's.
> + * for rte_flow_classify_validate and
> + * rte_flow_classify_table_entry_add API's.
> */
>
> /* Read file of IPv4 tuple rules. 8< */ diff --git
> a/lib/flow_classify/rte_flow_classify.c
> b/lib/flow_classify/rte_flow_classify.c
> index d3ba2ed227..e960c3b140 100644
> --- a/lib/flow_classify/rte_flow_classify.c
> +++ b/lib/flow_classify/rte_flow_classify.c
> @@ -60,6 +60,7 @@ enum {
> DST_FIELD_IPV4,
> SRCP_FIELD_IPV4,
> DSTP_FIELD_IPV4,
> + TCP_FLAGS_FIELD,
I think it would be better to rename TCP_FLAGS_FIELD to FLAGS_FIELD_TCP in line
with the other values.
> NUM_FIELDS_IPV4
> };
>
> @@ -72,6 +73,7 @@ struct classify_rules {
> enum rte_flow_classify_rule_type type;
> union {
> struct rte_flow_classify_ipv4_5tuple ipv4_5tuple;
> + struct rte_flow_classify_ipv4_tcp_5tuple ipv4_tcp_5tuple;
> } u;
> };
>
> @@ -477,6 +479,84 @@ allocate_acl_ipv4_5tuple_rule(struct
> rte_flow_classifier *cls)
> return rule;
> }
>
> +static struct rte_flow_classify_rule *
> +allocate_acl_ipv4_tcp_5tuple_rule(struct rte_flow_classifier *cls) {
> + struct rte_flow_classify_rule *rule;
> + int log_level;
> +
> + rule = malloc(sizeof(struct rte_flow_classify_rule));
> + if (!rule)
> + return rule;
> +
> + memset(rule, 0, sizeof(struct rte_flow_classify_rule));
> + rule->id = unique_id++;
> + rule->rules.type =
> RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE;
> +
> + /* key add values */
> + rule->u.key.key_add.priority = cls->ntuple_filter.priority;
> + rule-
> >u.key.key_add.field_value[PROTO_FIELD_IPV4].mask_range.u8 =
> + cls->ntuple_filter.proto_mask;
> + rule->u.key.key_add.field_value[PROTO_FIELD_IPV4].value.u8 =
> + cls->ntuple_filter.proto;
> + rule->rules.u.ipv4_tcp_5tuple.proto = cls->ntuple_filter.proto;
> + rule->rules.u.ipv4_tcp_5tuple.proto_mask =
> + cls->ntuple_filter.proto_mask;
> +
> + rule->u.key.key_add.field_value[SRC_FIELD_IPV4].mask_range.u32
> =
> + cls->ntuple_filter.src_ip_mask;
> + rule->u.key.key_add.field_value[SRC_FIELD_IPV4].value.u32 =
> + cls->ntuple_filter.src_ip;
> + rule->rules.u.ipv4_tcp_5tuple.src_ip_mask =
> + cls->ntuple_filter.src_ip_mask;
> + rule->rules.u.ipv4_tcp_5tuple.src_ip = cls->ntuple_filter.src_ip;
> +
> + rule->u.key.key_add.field_value[DST_FIELD_IPV4].mask_range.u32
> =
> + cls->ntuple_filter.dst_ip_mask;
> + rule->u.key.key_add.field_value[DST_FIELD_IPV4].value.u32 =
> + cls->ntuple_filter.dst_ip;
> + rule->rules.u.ipv4_tcp_5tuple.dst_ip_mask =
> + cls->ntuple_filter.dst_ip_mask;
> + rule->rules.u.ipv4_tcp_5tuple.dst_ip = cls->ntuple_filter.dst_ip;
> +
> + rule-
> >u.key.key_add.field_value[SRCP_FIELD_IPV4].mask_range.u16 =
> + cls->ntuple_filter.src_port_mask;
> + rule->u.key.key_add.field_value[SRCP_FIELD_IPV4].value.u16 =
> + cls->ntuple_filter.src_port;
> + rule->rules.u.ipv4_tcp_5tuple.src_port_mask =
> + cls->ntuple_filter.src_port_mask;
> + rule->rules.u.ipv4_tcp_5tuple.src_port = cls->ntuple_filter.src_port;
> +
> + rule-
> >u.key.key_add.field_value[DSTP_FIELD_IPV4].mask_range.u16 =
> + cls->ntuple_filter.dst_port_mask;
> + rule->u.key.key_add.field_value[DSTP_FIELD_IPV4].value.u16 =
> + cls->ntuple_filter.dst_port;
> + rule->rules.u.ipv4_tcp_5tuple.dst_port_mask =
> + cls->ntuple_filter.dst_port_mask;
> + rule->rules.u.ipv4_tcp_5tuple.dst_port = cls->ntuple_filter.dst_port;
> +
> + rule-
> >u.key.key_add.field_value[TCP_FLAGS_FIELD].mask_range.u32 =
> + rte_be_to_cpu_32(0xff);
> + rule->u.key.key_add.field_value[TCP_FLAGS_FIELD].value.u32 =
> + rte_be_to_cpu_32(cls->ntuple_filter.tcp_flags);
> + rule->rules.u.ipv4_tcp_5tuple.tcp_flags =
> +cls->ntuple_filter.tcp_flags;
> +
> + log_level = rte_log_get_level(librte_flow_classify_logtype);
> +
> + if (log_level == RTE_LOG_DEBUG)
> + print_acl_ipv4_key_add(&rule->u.key.key_add);
> +
> + /* key delete values */
> + memcpy(&rule->u.key.key_del.field_value[PROTO_FIELD_IPV4],
> + &rule->u.key.key_add.field_value[PROTO_FIELD_IPV4],
> + NUM_FIELDS_IPV4 * sizeof(struct rte_acl_field));
> +
> + if (log_level == RTE_LOG_DEBUG)
> + print_acl_ipv4_key_delete(&rule->u.key.key_del);
> +
> + return rule;
> +}
> +
> struct rte_flow_classify_rule *
> rte_flow_classify_table_entry_add(struct rte_flow_classifier *cls,
> const struct rte_flow_attr *attr,
> @@ -514,6 +594,13 @@ rte_flow_classify_table_entry_add(struct
> rte_flow_classifier *cls,
> rule->tbl_type = table_type;
> cls->table_mask |= table_type;
> break;
> + case RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE:
> + rule = allocate_acl_ipv4_tcp_5tuple_rule(cls);
> + if (!rule)
> + return NULL;
> + rule->tbl_type = table_type;
> + cls->table_mask |= table_type;
> + break;
> default:
> return NULL;
> }
> diff --git a/lib/flow_classify/rte_flow_classify.h
> b/lib/flow_classify/rte_flow_classify.h
> index 82ea92b6a6..65585d9f92 100644
> --- a/lib/flow_classify/rte_flow_classify.h
> +++ b/lib/flow_classify/rte_flow_classify.h
> @@ -80,6 +80,8 @@ enum rte_flow_classify_rule_type {
> RTE_FLOW_CLASSIFY_RULE_TYPE_NONE,
> /** IPv4 5tuple type */
> RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_5TUPLE,
> + /** IPv4 TCP 5tuple type */
> + RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE,
> };
>
> /** Flow classify table type */
> @@ -92,6 +94,8 @@ enum rte_flow_classify_table_type {
> RTE_FLOW_CLASSIFY_TABLE_ACL_VLAN_IP4_5TUPLE = 1 << 2,
> /** ACL QinQ IP4 5TUPLE */
> RTE_FLOW_CLASSIFY_TABLE_ACL_QINQ_IP4_5TUPLE = 1 << 3,
> + /** ACL IP4 5TUPLE with tcp_flags */
> + RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE = 1 << 4,
>
> };
>
> @@ -131,6 +135,21 @@ struct rte_flow_classify_ipv4_5tuple {
> uint8_t proto_mask; /**< Mask of L4 protocol. */
> };
>
> +/** IPv4 5-tuple data with tcp flags*/
> +struct rte_flow_classify_ipv4_tcp_5tuple {
> + uint32_t dst_ip; /**< Destination IP address in big endian. */
> + uint32_t dst_ip_mask; /**< Mask of destination IP address. */
> + uint32_t src_ip; /**< Source IP address in big endian. */
> + uint32_t src_ip_mask; /**< Mask of destination IP address. */
> + uint16_t dst_port; /**< Destination port in big endian. */
> + uint16_t dst_port_mask; /**< Mask of destination port. */
> + uint16_t src_port; /**< Source Port in big endian. */
> + uint16_t src_port_mask; /**< Mask of source port. */
> + uint8_t proto; /**< L4 protocol. */
> + uint8_t proto_mask; /**< Mask of L4 protocol. */
> + uint8_t tcp_flags; /**< Tcp only */
> +};
> +
> /**
> * Flow stats
> *
> diff --git a/lib/flow_classify/rte_flow_classify_parse.c
> b/lib/flow_classify/rte_flow_classify_parse.c
> index 465330291f..fe4ee05b6f 100644
> --- a/lib/flow_classify/rte_flow_classify_parse.c
> +++ b/lib/flow_classify/rte_flow_classify_parse.c
> @@ -216,6 +216,7 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
> const struct rte_flow_action_count *count;
> const struct rte_flow_action_mark *mark_spec;
> uint32_t index;
> + bool have_tcp = false;
>
> /* parse pattern */
> index = 0;
> @@ -375,6 +376,8 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
> filter->dst_port = tcp_spec->hdr.dst_port;
> filter->src_port = tcp_spec->hdr.src_port;
> filter->tcp_flags = tcp_spec->hdr.tcp_flags;
> + if (filter->tcp_flags != 0)
> + have_tcp = true;
> } else if (item->type == RTE_FLOW_ITEM_TYPE_UDP) {
> udp_mask = item->mask;
>
> @@ -434,7 +437,10 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
> return -EINVAL;
> }
>
> - table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> + if (have_tcp)
> + table_type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
> + else
> + table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
>
> /* parse attr */
> /* must be input direction */
> --
> 2.17.1
Regards,
Bernard.
