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

[Ori Kam]

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.                         
*/

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