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

[Ori Kam <orika@mellanox.com>]

Hi All,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, March 2, 2020 9:19 AM
> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Cc: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; xiang.w.wang@intel.com; 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
> 
> On Sun, Mar 1, 2020 at 9:28 PM Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com> wrote:
> >
> > Hi OrI,
> >
> > >
> > >Hi Pavan,
> > >
> > >
> > >> -----Original Message-----
> > >> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
> > >Bhagavatula
> > >> Sent: Sunday, March 1, 2020 4:38 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,
> > >> >
> > >> >> -----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)
> > >>
> > >
> > >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.
> > >*/
> >
> > Looks good with minor corrections as below
> >
> > /**< First group_id to match the rule against. This group must be valid.
> >   * In order to support more than one group per each op (up to 4 groups), any
> of the group_id<1-3> should
> >   * hold a valid group id along with RTE_REGEX_OPS_REQ_GROUP_ID<1-
> 3>_VALID_F flag set.
> >   * For example, to match against group 100 and 101, group_id0 should be set
> to 100 and group_id1 should
> >   * be set to 101 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.
> > */
> 
> I think, we can remove the limitation of group0 is always valid.
> There are use cases like each group belongs certain functionality and
> based on the packet type or
> so application decides the group. In that case, group 0 may or may not valid.
> 
> IMO, By spec, we can dictate,
> 
> At minimum of one of the group should be valid and selected, Behaviour
> is undefined if any of the group is not selected(This is to avoid fast
> path check).
> 
> Thoughts?
> 

I like your approach, lets go with this approach.

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