Hi Ori,
>
>Hi Pavan,
>
>> -----Original Message-----
>> From: dev <dev-boun...@dpdk.org> On Behalf Of Pavan Nikhilesh
>Bhagavatula
>> Sent: Sunday, March 1, 2020 3:23 PM
>> To: Ori Kam <or...@mellanox.com>; Jerin Jacob Kollanukkaran
>> <jer...@marvell.com>; xiang.w.w...@intel.com
>> Cc: dev@dpdk.org; Shahaf Shuler <shah...@mellanox.com>;
>> hemant.agra...@nxp.com; Opher Reviv <op...@mellanox.com>;
>Alex
>> Rosenbaum <al...@mellanox.com>; Dovrat Zifroni
><dov...@marvell.com>;
>> Prasun Kapoor <pkap...@marvell.com>; nipun.gu...@nxp.com;
>> bruce.richard...@intel.com; yang.a.h...@intel.com;
>harry.ch...@intel.com;
>> gu.ji...@zte.com.cn; shanjia...@chinatelecom.cn;
>> zhangy....@chinatelecom.cn; lixin...@huachentel.com;
>wush...@inspur.com;
>> yuying...@yxlink.com; fanchengg...@sunyainfo.com;
>> davidf...@tencent.com; liuzho...@chinaunicom.cn;
>> zhaoyon...@huawei.com; o...@yunify.com; j...@netgate.com;
>> hongjun...@intel.com; j.bromh...@titan-ic.com; d...@ntop.org;
>> f...@napatech.com; arthur...@lionic.com; Thomas Monjalon
>> <tho...@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-boun...@dpdk.org> On Behalf Of Pavan
>Nikhilesh
>> >Bhagavatula
>> >> Sent: Sunday, March 1, 2020 8:13 AM
>> >> To: Ori Kam <or...@mellanox.com>; Jerin Jacob Kollanukkaran
>> >> <jer...@marvell.com>; xiang.w.w...@intel.com
>> >> Cc: dev@dpdk.org; Shahaf Shuler <shah...@mellanox.com>;
>> >> hemant.agra...@nxp.com; Opher Reviv <op...@mellanox.com>;
>> >Alex
>> >> Rosenbaum <al...@mellanox.com>; Dovrat Zifroni
>> ><dov...@marvell.com>;
>> >> Prasun Kapoor <pkap...@marvell.com>; nipun.gu...@nxp.com;
>> >> bruce.richard...@intel.com; yang.a.h...@intel.com;
>> >harry.ch...@intel.com;
>> >> gu.ji...@zte.com.cn; shanjia...@chinatelecom.cn;
>> >> zhangy....@chinatelecom.cn; lixin...@huachentel.com;
>> >wush...@inspur.com;
>> >> yuying...@yxlink.com; fanchengg...@sunyainfo.com;
>> >> davidf...@tencent.com; liuzho...@chinaunicom.cn;
>> >> zhaoyon...@huawei.com; o...@yunify.com; j...@netgate.com;
>> >> hongjun...@intel.com; j.bromh...@titan-ic.com; d...@ntop.org;
>> >> f...@napatech.com; arthur...@lionic.com; Thomas Monjalon
>> >> <tho...@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
>> >> >+ */
>> >> >+};
