[dpdk-dev,v2,17/17] libte_acl: remove unused macros.

Message ID 1421090181-17150-18-git-send-email-konstantin.ananyev@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Ananyev, Konstantin Jan. 12, 2015, 7:16 p.m. UTC
  Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_acl/acl.h     | 39 ++++++++++++++++++++++++++++++++++++++-
 lib/librte_acl/acl_run.h |  1 -
 2 files changed, 38 insertions(+), 2 deletions(-)
  

Comments

Thomas Monjalon Jan. 19, 2015, 5:17 p.m. UTC | #1
2015-01-12 19:16, Konstantin Ananyev:
>  /*
> + * ACL RT structure is a set of multibit tries (with stride == 8)
> + * represented by an array of transitions. The next position is calculated
> + * based on the current position and the input byte.
> + * Each transition is 64 bit value with the following format:
> + * | node_type_specific : 32 | node_type : 3 | node_addr : 29 |
> + * For all node types except RTE_ACL_NODE_MATCH, node_addr is an index
> + * to the start of the node in the transtions array.
> + * Few different node types are used:
> + * RTE_ACL_NODE_MATCH:
> + * node_addr value is and index into an array that contains the return value
> + * and its priority for each category.
> + * Upper 32 bits of the transtion value are not used for that node type.
> + * RTE_ACL_NODE_QRANGE:
> + * that node consist of up to 5 transitions.
> + * Upper 32 bits are interpreted as 4 signed character values which
> + * are ordered from smallest(INT8_MIN) to largest (INT8_MAX).
> + * These values define 5 ranges:
> + * INT8_MIN <= range[0]  <= ((int8_t *)&transition)[4]
> + * ((int8_t *)&transition)[4] < range[1] <= ((int8_t *)&transition)[5]
> + * ((int8_t *)&transition)[5] < range[2] <= ((int8_t *)&transition)[6]
> + * ((int8_t *)&transition)[6] < range[3] <= ((int8_t *)&transition)[7]
> + * ((int8_t *)&transition)[7] < range[4] <= INT8_MAX
> + * So for input byte value within range[i] i-th transition within that node
> + * will be used.
> + * RTE_ACL_NODE_SINGLE:
> + * always transitions to the same node regardless of the input value.
> + * RTE_ACL_NODE_DFA:
> + * that node consits of up to 256 transitions.
> + * In attempt to conserve space all transitions are divided into 4 consecutive
> + * groups, by 64 transitions per group:
> + * group64[i] contains transitions[i * 64, .. i * 64 + 63].
> + * Upper 32 bits are interpreted as 4 unsigned character values one per group,
> + * which contain index to the start of the given group within the node.
> + * So to calculate transition index within the node for given input byte value:
> + * input_byte - ((uint8_t *)&transition)[4 + input_byte / 64].
> + */

It's maybe an error. You were only supposed to remove some macros in this patch.
  
Ananyev, Konstantin Jan. 20, 2015, 10:09 a.m. UTC | #2
Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, January 19, 2015 5:18 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 17/17] libte_acl: remove unused macros.
> 
> 2015-01-12 19:16, Konstantin Ananyev:
> >  /*
> > + * ACL RT structure is a set of multibit tries (with stride == 8)
> > + * represented by an array of transitions. The next position is calculated
> > + * based on the current position and the input byte.
> > + * Each transition is 64 bit value with the following format:
> > + * | node_type_specific : 32 | node_type : 3 | node_addr : 29 |
> > + * For all node types except RTE_ACL_NODE_MATCH, node_addr is an index
> > + * to the start of the node in the transtions array.
> > + * Few different node types are used:
> > + * RTE_ACL_NODE_MATCH:
> > + * node_addr value is and index into an array that contains the return value
> > + * and its priority for each category.
> > + * Upper 32 bits of the transtion value are not used for that node type.
> > + * RTE_ACL_NODE_QRANGE:
> > + * that node consist of up to 5 transitions.
> > + * Upper 32 bits are interpreted as 4 signed character values which
> > + * are ordered from smallest(INT8_MIN) to largest (INT8_MAX).
> > + * These values define 5 ranges:
> > + * INT8_MIN <= range[0]  <= ((int8_t *)&transition)[4]
> > + * ((int8_t *)&transition)[4] < range[1] <= ((int8_t *)&transition)[5]
> > + * ((int8_t *)&transition)[5] < range[2] <= ((int8_t *)&transition)[6]
> > + * ((int8_t *)&transition)[6] < range[3] <= ((int8_t *)&transition)[7]
> > + * ((int8_t *)&transition)[7] < range[4] <= INT8_MAX
> > + * So for input byte value within range[i] i-th transition within that node
> > + * will be used.
> > + * RTE_ACL_NODE_SINGLE:
> > + * always transitions to the same node regardless of the input value.
> > + * RTE_ACL_NODE_DFA:
> > + * that node consits of up to 256 transitions.
> > + * In attempt to conserve space all transitions are divided into 4 consecutive
> > + * groups, by 64 transitions per group:
> > + * group64[i] contains transitions[i * 64, .. i * 64 + 63].
> > + * Upper 32 bits are interpreted as 4 unsigned character values one per group,
> > + * which contain index to the start of the given group within the node.
> > + * So to calculate transition index within the node for given input byte value:
> > + * input_byte - ((uint8_t *)&transition)[4 + input_byte / 64].
> > + */
> 
> It's maybe an error. You were only supposed to remove some macros in this patch.

Ah yes, I added some comments about ACL internal layout.
Thought it might be useful.
Forgot to add it into patch description. 
Are you saying I need to split it into 2 patches, or it is ok like that?
Konstantin

> 
> --
> Thomas
  
Jim Thompson Jan. 20, 2015, 10:48 a.m. UTC | #3
> On Jan 20, 2015, at 4:09 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
> 
> Hi Thomas,
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> Sent: Monday, January 19, 2015 5:18 PM
>> To: Ananyev, Konstantin
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2 17/17] libte_acl: remove unused macros.
>> 
>> 2015-01-12 19:16, Konstantin Ananyev:
>>> /*
>>> + * ACL RT structure is a set of multibit tries (with stride == 8)
>>> + * represented by an array of transitions. The next position is calculated
>>> + * based on the current position and the input byte.
>>> + * Each transition is 64 bit value with the following format:
>>> + * | node_type_specific : 32 | node_type : 3 | node_addr : 29 |
>>> + * For all node types except RTE_ACL_NODE_MATCH, node_addr is an index
>>> + * to the start of the node in the transtions array.
>>> + * Few different node types are used:
>>> + * RTE_ACL_NODE_MATCH:
>>> + * node_addr value is and index into an array that contains the return value
>>> + * and its priority for each category.
>>> + * Upper 32 bits of the transtion value are not used for that node type.
>>> + * RTE_ACL_NODE_QRANGE:
>>> + * that node consist of up to 5 transitions.
>>> + * Upper 32 bits are interpreted as 4 signed character values which
>>> + * are ordered from smallest(INT8_MIN) to largest (INT8_MAX).
>>> + * These values define 5 ranges:
>>> + * INT8_MIN <= range[0]  <= ((int8_t *)&transition)[4]
>>> + * ((int8_t *)&transition)[4] < range[1] <= ((int8_t *)&transition)[5]
>>> + * ((int8_t *)&transition)[5] < range[2] <= ((int8_t *)&transition)[6]
>>> + * ((int8_t *)&transition)[6] < range[3] <= ((int8_t *)&transition)[7]
>>> + * ((int8_t *)&transition)[7] < range[4] <= INT8_MAX
>>> + * So for input byte value within range[i] i-th transition within that node
>>> + * will be used.
>>> + * RTE_ACL_NODE_SINGLE:
>>> + * always transitions to the same node regardless of the input value.
>>> + * RTE_ACL_NODE_DFA:
>>> + * that node consits of up to 256 transitions.
>>> + * In attempt to conserve space all transitions are divided into 4 consecutive
>>> + * groups, by 64 transitions per group:
>>> + * group64[i] contains transitions[i * 64, .. i * 64 + 63].
>>> + * Upper 32 bits are interpreted as 4 unsigned character values one per group,
>>> + * which contain index to the start of the given group within the node.
>>> + * So to calculate transition index within the node for given input byte value:
>>> + * input_byte - ((uint8_t *)&transition)[4 + input_byte / 64].
>>> + */
>> 
>> It's maybe an error. You were only supposed to remove some macros in this patch.
> 
> Ah yes, I added some comments about ACL internal layout.
> Thought it might be useful.
> Forgot to add it into patch description. 
> Are you saying I need to split it into 2 patches, or it is ok like that?

it’s great info, but it should probably go in doc/guides/prog_guide/packet_classif_access_ctrl.rst.

Jim
  
Ananyev, Konstantin Jan. 20, 2015, 11:11 a.m. UTC | #4
> From: Jim Thompson [mailto:jim@netgate.com]

> Sent: Tuesday, January 20, 2015 10:48 AM

> To: Ananyev, Konstantin

> Cc: Thomas Monjalon; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v2 17/17] libte_acl: remove unused macros.

> 

> 

> On Jan 20, 2015, at 4:09 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:

> 

> Hi Thomas,

> 

> 

> -----Original Message-----

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]

> Sent: Monday, January 19, 2015 5:18 PM

> To: Ananyev, Konstantin

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v2 17/17] libte_acl: remove unused macros.

> 

> 2015-01-12 19:16, Konstantin Ananyev:

> 

> /*

> + * ACL RT structure is a set of multibit tries (with stride == 8)

> + * represented by an array of transitions. The next position is calculated

> + * based on the current position and the input byte.

> + * Each transition is 64 bit value with the following format:

> + * | node_type_specific : 32 | node_type : 3 | node_addr : 29 |

> + * For all node types except RTE_ACL_NODE_MATCH, node_addr is an index

> + * to the start of the node in the transtions array.

> + * Few different node types are used:

> + * RTE_ACL_NODE_MATCH:

> + * node_addr value is and index into an array that contains the return value

> + * and its priority for each category.

> + * Upper 32 bits of the transtion value are not used for that node type.

> + * RTE_ACL_NODE_QRANGE:

> + * that node consist of up to 5 transitions.

> + * Upper 32 bits are interpreted as 4 signed character values which

> + * are ordered from smallest(INT8_MIN) to largest (INT8_MAX).

> + * These values define 5 ranges:

> + * INT8_MIN <= range[0]  <= ((int8_t *)&transition)[4]

> + * ((int8_t *)&transition)[4] < range[1] <= ((int8_t *)&transition)[5]

> + * ((int8_t *)&transition)[5] < range[2] <= ((int8_t *)&transition)[6]

> + * ((int8_t *)&transition)[6] < range[3] <= ((int8_t *)&transition)[7]

> + * ((int8_t *)&transition)[7] < range[4] <= INT8_MAX

> + * So for input byte value within range[i] i-th transition within that node

> + * will be used.

> + * RTE_ACL_NODE_SINGLE:

> + * always transitions to the same node regardless of the input value.

> + * RTE_ACL_NODE_DFA:

> + * that node consits of up to 256 transitions.

> + * In attempt to conserve space all transitions are divided into 4 consecutive

> + * groups, by 64 transitions per group:

> + * group64[i] contains transitions[i * 64, .. i * 64 + 63].

> + * Upper 32 bits are interpreted as 4 unsigned character values one per group,

> + * which contain index to the start of the given group within the node.

> + * So to calculate transition index within the node for given input byte value:

> + * input_byte - ((uint8_t *)&transition)[4 + input_byte / 64].

> + */

> 

> It's maybe an error. You were only supposed to remove some macros in this patch.

> 

> Ah yes, I added some comments about ACL internal layout.

> Thought it might be useful.

> Forgot to add it into patch description.

> Are you saying I need to split it into 2 patches, or it is ok like that?

> 

> it’s great info, but it should probably go in doc/guides/prog_guide/packet_classif_access_ctrl.rst.


Well, I think it is a good practise to have some brief description of the internal structure in the header file.
Same as rte_ring.h, rte_mempool.h, rte_timer.h.
When I'll start doc updating, I can put some sort of internal structure description into PG too, though not sure it is worth it.
From my perspective, PG should be more about features provided, API description, usage examples, etc.

Konstantin 

> 

> Jim
  
Thomas Monjalon Jan. 20, 2015, 12:26 p.m. UTC | #5
2015-01-20 10:09, Ananyev, Konstantin:
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Monday, January 19, 2015 5:18 PM
> > To: Ananyev, Konstantin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 17/17] libte_acl: remove unused macros.
> > 
> > 2015-01-12 19:16, Konstantin Ananyev:
> > >  /*
> > > + * ACL RT structure is a set of multibit tries (with stride == 8)
> > > + * represented by an array of transitions. The next position is calculated
> > > + * based on the current position and the input byte.
> > > + * Each transition is 64 bit value with the following format:
> > > + * | node_type_specific : 32 | node_type : 3 | node_addr : 29 |
> > > + * For all node types except RTE_ACL_NODE_MATCH, node_addr is an index
> > > + * to the start of the node in the transtions array.
> > > + * Few different node types are used:
> > > + * RTE_ACL_NODE_MATCH:
> > > + * node_addr value is and index into an array that contains the return value
> > > + * and its priority for each category.
> > > + * Upper 32 bits of the transtion value are not used for that node type.
> > > + * RTE_ACL_NODE_QRANGE:
> > > + * that node consist of up to 5 transitions.
> > > + * Upper 32 bits are interpreted as 4 signed character values which
> > > + * are ordered from smallest(INT8_MIN) to largest (INT8_MAX).
> > > + * These values define 5 ranges:
> > > + * INT8_MIN <= range[0]  <= ((int8_t *)&transition)[4]
> > > + * ((int8_t *)&transition)[4] < range[1] <= ((int8_t *)&transition)[5]
> > > + * ((int8_t *)&transition)[5] < range[2] <= ((int8_t *)&transition)[6]
> > > + * ((int8_t *)&transition)[6] < range[3] <= ((int8_t *)&transition)[7]
> > > + * ((int8_t *)&transition)[7] < range[4] <= INT8_MAX
> > > + * So for input byte value within range[i] i-th transition within that node
> > > + * will be used.
> > > + * RTE_ACL_NODE_SINGLE:
> > > + * always transitions to the same node regardless of the input value.
> > > + * RTE_ACL_NODE_DFA:
> > > + * that node consits of up to 256 transitions.
> > > + * In attempt to conserve space all transitions are divided into 4 consecutive
> > > + * groups, by 64 transitions per group:
> > > + * group64[i] contains transitions[i * 64, .. i * 64 + 63].
> > > + * Upper 32 bits are interpreted as 4 unsigned character values one per group,
> > > + * which contain index to the start of the given group within the node.
> > > + * So to calculate transition index within the node for given input byte value:
> > > + * input_byte - ((uint8_t *)&transition)[4 + input_byte / 64].
> > > + */
> > 
> > It's maybe an error. You were only supposed to remove some macros in this patch.
> 
> Ah yes, I added some comments about ACL internal layout.
> Thought it might be useful.
> Forgot to add it into patch description. 
> Are you saying I need to split it into 2 patches, or it is ok like that?

As it is not related to the other topic of the patch, yes please make
a separate patch.
  

Patch

diff --git a/lib/librte_acl/acl.h b/lib/librte_acl/acl.h
index 61b849a..e65e079 100644
--- a/lib/librte_acl/acl.h
+++ b/lib/librte_acl/acl.h
@@ -62,13 +62,50 @@  struct rte_acl_bitset {
 
 #define	RTE_ACL_NODE_DFA	(0 << RTE_ACL_TYPE_SHIFT)
 #define	RTE_ACL_NODE_SINGLE	(1U << RTE_ACL_TYPE_SHIFT)
-#define	RTE_ACL_NODE_QEXACT	(2U << RTE_ACL_TYPE_SHIFT)
 #define	RTE_ACL_NODE_QRANGE	(3U << RTE_ACL_TYPE_SHIFT)
 #define	RTE_ACL_NODE_MATCH	(4U << RTE_ACL_TYPE_SHIFT)
 #define	RTE_ACL_NODE_TYPE	(7U << RTE_ACL_TYPE_SHIFT)
 #define	RTE_ACL_NODE_UNDEFINED	UINT32_MAX
 
 /*
+ * ACL RT structure is a set of multibit tries (with stride == 8)
+ * represented by an array of transitions. The next position is calculated
+ * based on the current position and the input byte.
+ * Each transition is 64 bit value with the following format:
+ * | node_type_specific : 32 | node_type : 3 | node_addr : 29 |
+ * For all node types except RTE_ACL_NODE_MATCH, node_addr is an index
+ * to the start of the node in the transtions array.
+ * Few different node types are used:
+ * RTE_ACL_NODE_MATCH:
+ * node_addr value is and index into an array that contains the return value
+ * and its priority for each category.
+ * Upper 32 bits of the transtion value are not used for that node type.
+ * RTE_ACL_NODE_QRANGE:
+ * that node consist of up to 5 transitions.
+ * Upper 32 bits are interpreted as 4 signed character values which
+ * are ordered from smallest(INT8_MIN) to largest (INT8_MAX).
+ * These values define 5 ranges:
+ * INT8_MIN <= range[0]  <= ((int8_t *)&transition)[4]
+ * ((int8_t *)&transition)[4] < range[1] <= ((int8_t *)&transition)[5]
+ * ((int8_t *)&transition)[5] < range[2] <= ((int8_t *)&transition)[6]
+ * ((int8_t *)&transition)[6] < range[3] <= ((int8_t *)&transition)[7]
+ * ((int8_t *)&transition)[7] < range[4] <= INT8_MAX
+ * So for input byte value within range[i] i-th transition within that node
+ * will be used.
+ * RTE_ACL_NODE_SINGLE:
+ * always transitions to the same node regardless of the input value.
+ * RTE_ACL_NODE_DFA:
+ * that node consits of up to 256 transitions.
+ * In attempt to conserve space all transitions are divided into 4 consecutive
+ * groups, by 64 transitions per group:
+ * group64[i] contains transitions[i * 64, .. i * 64 + 63].
+ * Upper 32 bits are interpreted as 4 unsigned character values one per group,
+ * which contain index to the start of the given group within the node.
+ * So to calculate transition index within the node for given input byte value:
+ * input_byte - ((uint8_t *)&transition)[4 + input_byte / 64].
+ */
+
+/*
  * Structure of a node is a set of ptrs and each ptr has a bit map
  * of values associated with this transition.
  */
diff --git a/lib/librte_acl/acl_run.h b/lib/librte_acl/acl_run.h
index 850bc81..b2fc42c 100644
--- a/lib/librte_acl/acl_run.h
+++ b/lib/librte_acl/acl_run.h
@@ -40,7 +40,6 @@ 
 #define MAX_SEARCHES_AVX16	16
 #define MAX_SEARCHES_SSE8	8
 #define MAX_SEARCHES_SSE4	4
-#define MAX_SEARCHES_SSE2	2
 #define MAX_SEARCHES_SCALAR	2
 
 #define GET_NEXT_4BYTES(prm, idx)	\