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

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

> 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