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

[Jerin Jacob <jerinjacobk@gmail.com>]

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?






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