[dpdk-dev] [EXT] [RFC v5] regexdev: introduce regexdev subsystem

[Pavan Nikhilesh Bhagavatula]

Hi OrI,

>
>Hi Pavan,
>
>
>> -----Original Message-----
>> From: dev <dev-bounces at dpdk.org> On Behalf Of Pavan Nikhilesh
>Bhagavatula
>> Sent: Sunday, March 1, 2020 4:38 PM
>> To: Ori Kam <orika at mellanox.com>; Jerin Jacob Kollanukkaran
>> <jerinj at marvell.com>; xiang.w.wang at intel.com
>> Cc: dev at dpdk.org; Shahaf Shuler <shahafs at mellanox.com>;
>> hemant.agrawal at nxp.com; Opher Reviv <opher at mellanox.com>;
>Alex
>> Rosenbaum <alexr at mellanox.com>; Dovrat Zifroni
><dovrat at marvell.com>;
>> Prasun Kapoor <pkapoor at marvell.com>; nipun.gupta at nxp.com;
>> bruce.richardson at intel.com; yang.a.hong at intel.com;
>harry.chang at intel.com;
>> gu.jian1 at zte.com.cn; shanjiangh at chinatelecom.cn;
>> zhangy.yun at chinatelecom.cn; lixingfu at huachentel.com;
>wushuai at inspur.com;
>> yuyingxia at yxlink.com; fanchenggang at sunyainfo.com;
>> davidfgao at tencent.com; liuzhong1 at chinaunicom.cn;
>> zhaoyong11 at huawei.com; oc at yunify.com; jim at netgate.com;
>> hongjun.ni at intel.com; j.bromhead at titan-ic.com; deri at ntop.org;
>> fc at napatech.com; arthur.su at lionic.com; Thomas Monjalon
>> <thomas at monjalon.net>
>> Subject: Re: [dpdk-dev] [EXT] [RFC v5] regexdev: introduce regexdev
>subsystem
>>
>> Hi Ori,
>>
>> >
>> >Hi Pavan,
>> >
>> >> -----Original Message-----
>> >> From: dev <dev-bounces at dpdk.org> On Behalf Of Pavan
>Nikhilesh
>> >Bhagavatula
>> >> Sent: Sunday, March 1, 2020 3:23 PM
>> >> To: Ori Kam <orika at mellanox.com>; Jerin Jacob Kollanukkaran
>> >> <jerinj at marvell.com>; xiang.w.wang at intel.com
>> >> Cc: dev at dpdk.org; Shahaf Shuler <shahafs at mellanox.com>;
>> >> hemant.agrawal at nxp.com; Opher Reviv <opher at mellanox.com>;
>> >Alex
>> >> Rosenbaum <alexr at mellanox.com>; Dovrat Zifroni
>> ><dovrat at marvell.com>;
>> >> Prasun Kapoor <pkapoor at marvell.com>; nipun.gupta at nxp.com;
>> >> bruce.richardson at intel.com; yang.a.hong at intel.com;
>> >harry.chang at intel.com;
>> >> gu.jian1 at zte.com.cn; shanjiangh at chinatelecom.cn;
>> >> zhangy.yun at chinatelecom.cn; lixingfu at huachentel.com;
>> >wushuai at inspur.com;
>> >> yuyingxia at yxlink.com; fanchenggang at sunyainfo.com;
>> >> davidfgao at tencent.com; liuzhong1 at chinaunicom.cn;
>> >> zhaoyong11 at huawei.com; oc at yunify.com; jim at netgate.com;
>> >> hongjun.ni at intel.com; j.bromhead at titan-ic.com; deri at ntop.org;
>> >> fc at napatech.com; arthur.su at lionic.com; Thomas Monjalon
>> >> <thomas at monjalon.net>
>> >> Subject: Re: [dpdk-dev] [EXT] [RFC v5] regexdev: introduce
>regexdev
>> >subsystem
>> >>
>> >> Hi Ori,
>> >>
>> >> >
>> >> >Hi Pavan,
>> >> >Thanks for the comments please see below.
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: dev <dev-bounces at dpdk.org> On Behalf Of Pavan
>> >Nikhilesh
>> >> >Bhagavatula
>> >> >> Sent: Sunday, March 1, 2020 8:13 AM
>> >> >> To: Ori Kam <orika at mellanox.com>; Jerin Jacob Kollanukkaran
>> >> >> <jerinj at marvell.com>; xiang.w.wang at intel.com
>> >> >> Cc: dev at dpdk.org; Shahaf Shuler <shahafs at mellanox.com>;
>> >> >> hemant.agrawal at nxp.com; Opher Reviv
><opher at mellanox.com>;
>> >> >Alex
>> >> >> Rosenbaum <alexr at mellanox.com>; Dovrat Zifroni
>> >> ><dovrat at marvell.com>;
>> >> >> Prasun Kapoor <pkapoor at marvell.com>;
>nipun.gupta at nxp.com;
>> >> >> bruce.richardson at intel.com; yang.a.hong at intel.com;
>> >> >harry.chang at intel.com;
>> >> >> gu.jian1 at zte.com.cn; shanjiangh at chinatelecom.cn;
>> >> >> zhangy.yun at chinatelecom.cn; lixingfu at huachentel.com;
>> >> >wushuai at inspur.com;
>> >> >> yuyingxia at yxlink.com; fanchenggang at sunyainfo.com;
>> >> >> davidfgao at tencent.com; liuzhong1 at chinaunicom.cn;
>> >> >> zhaoyong11 at huawei.com; oc at yunify.com; jim at netgate.com;
>> >> >> hongjun.ni at intel.com; j.bromhead at titan-ic.com;
>deri at ntop.org;
>> >> >> fc at napatech.com; arthur.su at lionic.com; Thomas Monjalon
>> >> >> <thomas at monjalon.net>
>> >> >> Subject: Re: [dpdk-dev] [EXT] [RFC v5] regexdev: introduce
>> >regexdev
>> >> >subsystem
>> >> >>
>> >> >> Hi Ori,
>> >> >>
>> >> >> Minor comments below.
>> >> >>
>> >> >> <snip>
>> >> >>
>> >> >> >+/**
>> >> >> >+ * The generic *rte_regex_ops* structure to hold the RegEx
>> >> >attributes
>> >> >> >+ * for enqueue and dequeue operation.
>> >> >> >+ */
>> >> >> >+struct rte_regex_ops {
>> >> >> >+	/* W0 */
>> >> >> >+	uint16_t req_flags;
>> >> >> >+	/**< Request flags for the RegEx ops.
>> >> >> >+	 * @see RTE_REGEX_OPS_REQ_*
>> >> >> >+	 */
>> >> >> >+	uint16_t rsp_flags;
>> >> >> >+	/**< Response flags for the RegEx ops.
>> >> >> >+	 * @see RTE_REGEX_OPS_RSP_*
>> >> >> >+	 */
>> >> >> >+	uint16_t nb_actual_matches;
>> >> >> >+	/**< The total number of actual matches detected by
>the
>> >> >> >Regex device.*/
>> >> >> >+	uint16_t nb_matches;
>> >> >> >+	/**< The total number of matches returned by the
>RegEx
>> >> >> >device for this
>> >> >> >+	 * scan. The size of *rte_regex_ops::matches* zero
>length
>> array
>> >> >> >will be
>> >> >> >+	 * this value.
>> >> >> >+	 *
>> >> >> >+	 * @see struct rte_regex_ops::matches, struct
>> >> >> >rte_regex_match
>> >> >> >+	 */
>> >> >> >+
>> >> >> >+	/* W1 */
>> >> >> >+	struct rte_mbuf mbuf; /**< source mbuf, to search in.
>*/
>> >> >>
>> >> >> This should be *mbuf.
>> >> >
>> >> >Yes you are correct will fix.
>> >> >
>> >> >>
>> >> >> >+
>> >> >> >+	/* W2 */
>> >> >> >+	uint16_t group_id0;
>> >> >>
>> >> >> This should be group_id1.
>> >> >>
>> >> >No this is correct is should be id0. We are starting from group 0.
>> >> >The comment below states that the first group, meaning group 0
>> >must
>> >> >be
>> >> >valid group while group 1 doesn’t have to be vaild.
>> >>
>> >> Would that mean that group_id0 is always valid?
>> >> Since there is no `RTE_REGEX_OPS_REQ_GROUP_ID0_VALID_F`
>flag.
>> >>
>> >Yes, you must have at least one group.
>>
>> Makes sense, I think we need to update the comment a bit as it only
>mentions
>> that
>> at least one group but it should be group_id0 has to be always valid.
>>
>> (An application can erroneously set valid group_id1 instead of
>group_id0)
>>
>
>What about the next comment?
>/**< First group_id to match the rule against. This group must be valid.
>In
>  * order to support more group (up to 4 groups). The group number
>should
>  * be set. For example to enable group 1 group_id1 should be set
>  * with the group value and  and the
>RTE_REGEX_OPS_REQ_GROUP_ID1_VALID_F flag should be set.
>  * Respectively similar flags for group_id2 and group_id3.
>  * Upon the match, struct rte_regex_match::group_id shall be updated
>  * with matching group ID by the device. Group ID scheme provides
>  * rule isolation and effective pattern matching.
>*/

Looks good with minor corrections as below

/**< First group_id to match the rule against. This group must be valid. 
  * In order to support more than one group per each op (up to 4 groups), any of the group_id<1-3> should 
  * hold a valid group id along with RTE_REGEX_OPS_REQ_GROUP_ID<1-3>_VALID_F flag set.
  * For example, to match against group 100 and 101, group_id0 should be set to 100 and group_id1 should 
  * be set to 101 and the RTE_REGEX_OPS_REQ_GROUP_ID1_VALID_F flag should be set.    
  * Respectively similar flags for group_id2 and group_id3.      
  * Upon the match, struct rte_regex_match::group_id shall be updated      
  * with matching group ID by the device. Group ID scheme provides         
  * rule isolation and effective pattern matching.                         
*/

Thanks,
Pavan.

>
>/**< First group_id to match the rule against. Minimum one group id
>  * must be provided by application.
>  * When RTE_REGEX_OPS_REQ_GROUP_ID1_VALID_F set then
>group_id1
>  * is valid, respectively similar flags for group_id2 and group_id3.
>  * Upon the match, struct rte_regex_match::group_id shall be updated
>  * with matching group ID by the device. Group ID scheme provides
>  * rule isolation and effective pattern matching.
>
>> >
>> >> >
>> >> >> >+	/**< First group_id to match the rule against. Minimum
>one
>> >> >> >group id
>> >> >> >+	 * must be provided by application.
>> >> >> >+	 * When RTE_REGEX_OPS_REQ_GROUP_ID1_VALID_F
>set then
>> >> >> >group_id1
>> >> >> >+	 * is valid, respectively similar flags for group_id2 and
>> group_id3.
>> >> >> >+	 * Upon the match, struct rte_regex_match::group_id
>shall be
>> >> >> >updated
>> >> >> >+	 * with matching group ID by the device. Group ID
>scheme
>> >> >> >provides
>> >> >> >+	 * rule isolation and effective pattern matching.
>> >> >> >+	 */
>> >> >> >+	uint16_t group_id1;
>> >> >> >+	/**< Second group_id to match the rule against.
>> >> >> >+	 *
>> >> >> >+	 * @see RTE_REGEX_OPS_REQ_GROUP_ID1_VALID_F
>> >> >> >+	 */
>> >> >>
>> >> >> The above `group_id1` should be removed as its duplicate.
>> >> >>
>> >> >
>> >> >This is not duplicate, see above comment.
>> >> >
>> >> >> >+	uint16_t group_id2;
>> >> >> >+	/**< Third group_id to match the rule against.
>> >> >> >+	 *
>> >> >> >+	 * @see RTE_REGEX_OPS_REQ_GROUP_ID2_VALID_F
>> >> >> >+	 */
>> >> >> >+	uint16_t group_id3;
>> >> >> >+	/**< Forth group_id to match the rule against.
>> >> >> >+	 *
>> >> >> >+	 * @see RTE_REGEX_OPS_REQ_GROUP_ID3_VALID_F
>> >> >> >+	 */
>> >> >> >+
>> >> >> >+	/* W3 */
>> >> >> >+	RTE_STD_C11
>> >> >> >+	union {
>> >> >> >+		uint64_t user_id;
>> >> >> >+		/**< Application specific opaque value. An
>application
>> >> >> >may use
>> >> >> >+		 * this field to hold application specific value to
>share
>> >> >> >+		 * between dequeue and enqueue operation.
>> >> >> >+		 * Implementation should not modify this field.
>> >> >> >+		 */
>> >> >> >+		void *user_ptr;
>> >> >> >+		/**< Pointer representation of *user_id* */
>> >> >> >+	};
>> >> >> >+
>> >> >> >+	/* W4 */
>> >> >> >+	struct rte_regex_match matches[];
>> >> >> >+	/**< Zero length array to hold the match tuples.
>> >> >> >+	 * The struct rte_regex_ops::nb_matches value holds
>the
>> >> >> >number of
>> >> >> >+	 * elements in this array.
>> >> >> >+	 *
>> >> >> >+	 * @see struct rte_regex_ops::nb_matches
>> >> >> >+	 */
>> >> >> >+};