Hi Pavan,
> -----Original Message----- > From: dev <[email protected]> On Behalf Of Pavan Nikhilesh Bhagavatula > Sent: Sunday, March 1, 2020 4:38 PM > To: Ori Kam <[email protected]>; Jerin Jacob Kollanukkaran > <[email protected]>; [email protected] > Cc: [email protected]; Shahaf Shuler <[email protected]>; > [email protected]; Opher Reviv <[email protected]>; Alex > Rosenbaum <[email protected]>; Dovrat Zifroni <[email protected]>; > Prasun Kapoor <[email protected]>; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; Thomas Monjalon > <[email protected]> > Subject: Re: [dpdk-dev] [EXT] [RFC v5] regexdev: introduce regexdev subsystem > > Hi Ori, > > > > >Hi Pavan, > > > >> -----Original Message----- > >> From: dev <[email protected]> On Behalf Of Pavan Nikhilesh > >Bhagavatula > >> Sent: Sunday, March 1, 2020 3:23 PM > >> To: Ori Kam <[email protected]>; Jerin Jacob Kollanukkaran > >> <[email protected]>; [email protected] > >> Cc: [email protected]; Shahaf Shuler <[email protected]>; > >> [email protected]; Opher Reviv <[email protected]>; > >Alex > >> Rosenbaum <[email protected]>; Dovrat Zifroni > ><[email protected]>; > >> Prasun Kapoor <[email protected]>; [email protected]; > >> [email protected]; [email protected]; > >[email protected]; > >> [email protected]; [email protected]; > >> [email protected]; [email protected]; > >[email protected]; > >> [email protected]; [email protected]; > >> [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected]; [email protected]; Thomas Monjalon > >> <[email protected]> > >> 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 <[email protected]> On Behalf Of Pavan > >Nikhilesh > >> >Bhagavatula > >> >> Sent: Sunday, March 1, 2020 8:13 AM > >> >> To: Ori Kam <[email protected]>; Jerin Jacob Kollanukkaran > >> >> <[email protected]>; [email protected] > >> >> Cc: [email protected]; Shahaf Shuler <[email protected]>; > >> >> [email protected]; Opher Reviv <[email protected]>; > >> >Alex > >> >> Rosenbaum <[email protected]>; Dovrat Zifroni > >> ><[email protected]>; > >> >> Prasun Kapoor <[email protected]>; [email protected]; > >> >> [email protected]; [email protected]; > >> >[email protected]; > >> >> [email protected]; [email protected]; > >> >> [email protected]; [email protected]; > >> >[email protected]; > >> >> [email protected]; [email protected]; > >> >> [email protected]; [email protected]; > >> >> [email protected]; [email protected]; [email protected]; > >> >> [email protected]; [email protected]; [email protected]; > >> >> [email protected]; [email protected]; Thomas Monjalon > >> >> <[email protected]> > >> >> 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 > >> >> >+ */ > >> >> >+};
