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

[Pavan Nikhilesh Bhagavatula]

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.

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