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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

Hi Ori,

>
>Hi Pavan,
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
>Bhagavatula
>> Sent: Sunday, March 1, 2020 3:23 PM
>> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>; xiang.w.wang@intel.com
>> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>;
>> hemant.agrawal@nxp.com; Opher Reviv <opher@mellanox.com>;
>Alex
>> Rosenbaum <alexr@mellanox.com>; Dovrat Zifroni
><dovrat@marvell.com>;
>> Prasun Kapoor <pkapoor@marvell.com>; nipun.gupta@nxp.com;
>> bruce.richardson@intel.com; yang.a.hong@intel.com;
>harry.chang@intel.com;
>> gu.jian1@zte.com.cn; shanjiangh@chinatelecom.cn;
>> zhangy.yun@chinatelecom.cn; lixingfu@huachentel.com;
>wushuai@inspur.com;
>> yuyingxia@yxlink.com; fanchenggang@sunyainfo.com;
>> davidfgao@tencent.com; liuzhong1@chinaunicom.cn;
>> zhaoyong11@huawei.com; oc@yunify.com; jim@netgate.com;
>> hongjun.ni@intel.com; j.bromhead@titan-ic.com; deri@ntop.org;
>> fc@napatech.com; arthur.su@lionic.com; Thomas Monjalon
>> <thomas@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@dpdk.org> On Behalf Of Pavan
>Nikhilesh
>> >Bhagavatula
>> >> Sent: Sunday, March 1, 2020 8:13 AM
>> >> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
>> >> <jerinj@marvell.com>; xiang.w.wang@intel.com
>> >> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>;
>> >> hemant.agrawal@nxp.com; Opher Reviv <opher@mellanox.com>;
>> >Alex
>> >> Rosenbaum <alexr@mellanox.com>; Dovrat Zifroni
>> ><dovrat@marvell.com>;
>> >> Prasun Kapoor <pkapoor@marvell.com>; nipun.gupta@nxp.com;
>> >> bruce.richardson@intel.com; yang.a.hong@intel.com;
>> >harry.chang@intel.com;
>> >> gu.jian1@zte.com.cn; shanjiangh@chinatelecom.cn;
>> >> zhangy.yun@chinatelecom.cn; lixingfu@huachentel.com;
>> >wushuai@inspur.com;
>> >> yuyingxia@yxlink.com; fanchenggang@sunyainfo.com;
>> >> davidfgao@tencent.com; liuzhong1@chinaunicom.cn;
>> >> zhaoyong11@huawei.com; oc@yunify.com; jim@netgate.com;
>> >> hongjun.ni@intel.com; j.bromhead@titan-ic.com; deri@ntop.org;
>> >> fc@napatech.com; arthur.su@lionic.com; Thomas Monjalon
>> >> <thomas@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) 

>
>> >
>> >> >+	/**< 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
>> >> >+	 */
>> >> >+};