Re: [dpdk-dev] [PATCH v2] acl: allow zero verdict

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Michał Mirosław [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Wednesday, December 14, 2016 5:24 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: [PATCH v2] acl: allow zero verdict
> 
> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> ---
> v2: fixes prog_guide and ACL tests
> 
> ---
>  app/test/test_acl.c                                  | 13 -------------
>  doc/guides/prog_guide/packet_classif_access_ctrl.rst |  3 ++-
>  lib/librte_acl/rte_acl.c                             |  3 +--
>  lib/librte_acl/rte_acl.h                             |  2 --
>  lib/librte_table/rte_table_acl.c                     |  2 +-
>  5 files changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/app/test/test_acl.c b/app/test/test_acl.c
> index be744ec..c6b511f 100644
> --- a/app/test/test_acl.c
> +++ b/app/test/test_acl.c
> @@ -1357,19 +1357,6 @@ test_invalid_rules(void)
>  		goto err;
>  	}
> 
> -	rule.dst_mask_len = 0;
> -	rule.src_mask_len = 0;
> -	rule.data.userdata = 0;
> -
> -	/* try adding this rule (it should fail because userdata is invalid) */
> -	ret = rte_acl_ipv4vlan_add_rules(acx, &rule, 1);
> -	if (ret == 0) {
> -		printf("Line %i: Adding a rule with invalid user data "
> -				"should have failed!\n", __LINE__);
> -		rte_acl_free(acx);
> -		return -1;
> -	}
> -
>  	rte_acl_free(acx);
> 
>  	return 0;
> diff --git a/doc/guides/prog_guide/packet_classif_access_ctrl.rst b/doc/guides/prog_guide/packet_classif_access_ctrl.rst
> index 5fd3d34..a6bee9b 100644
> --- a/doc/guides/prog_guide/packet_classif_access_ctrl.rst
> +++ b/doc/guides/prog_guide/packet_classif_access_ctrl.rst
> @@ -329,8 +329,9 @@ When creating a set of rules, for each rule, additional information must be supp
>      Each set could be assigned its own category and by combining them into a single database,
>      one lookup returns a result for each of the four sets.
> 
> -*   **userdata**: A user-defined field that could be any value except zero.
> +*   **userdata**: A user-defined value.
>      For each category, a successful match returns the userdata field of the highest priority matched rule.
> +    When no rules match, returned value is zero.

Might be good to add some extra explanation here:
"When no rules match, returned value is zero.
So while rules with userdata equal zero are allowed,
such rules would always return 'no match' found when hit."
Or might be some better wording could be found.
Konstantin
 
> 
>  .. note::
> 
> diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_acl/rte_acl.c
> index 8b7e92c..d1f40be 100644
> --- a/lib/librte_acl/rte_acl.c
> +++ b/lib/librte_acl/rte_acl.c
> @@ -313,8 +313,7 @@ acl_check_rule(const struct rte_acl_rule_data *rd)
>  	if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd->category_mask)) &
>  			rd->category_mask) == 0 ||
>  			rd->priority > RTE_ACL_MAX_PRIORITY ||
> -			rd->priority < RTE_ACL_MIN_PRIORITY ||
> -			rd->userdata == RTE_ACL_INVALID_USERDATA)
> +			rd->priority < RTE_ACL_MIN_PRIORITY)
>  		return -EINVAL;
>  	return 0;
>  }
> diff --git a/lib/librte_acl/rte_acl.h b/lib/librte_acl/rte_acl.h
> index caa91f7..b53179a 100644
> --- a/lib/librte_acl/rte_acl.h
> +++ b/lib/librte_acl/rte_acl.h
> @@ -120,8 +120,6 @@ enum {
>  	RTE_ACL_MIN_PRIORITY = 0,
>  };
> 
> -#define	RTE_ACL_INVALID_USERDATA	0
> -
>  #define	RTE_ACL_MASKLEN_TO_BITMASK(v, s)	\
>  ((v) == 0 ? (v) : (typeof(v))((uint64_t)-1 << ((s) * CHAR_BIT - (v))))
> 
> diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
> index 8f1f8ce..94b69a9 100644
> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -792,7 +792,7 @@ rte_table_acl_lookup(
> 
>  		pkts_mask &= ~pkt_mask;
> 
> -		if (action_table_pos != RTE_ACL_INVALID_USERDATA) {
> +		if (action_table_pos != 0) {
>  			pkts_out_mask |= pkt_mask;
>  			entries[pkt_pos] = (void *)
>  				&acl->memory[action_table_pos *
> --
> 2.10.2