Hi All,
> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Monday, March 2, 2020 9:19 AM
> To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>
> Cc: Ori Kam <or...@mellanox.com>; Jerin Jacob Kollanukkaran
> <jer...@marvell.com>; xiang.w.w...@intel.com; 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
>
> On Sun, Mar 1, 2020 at 9:28 PM Pavan Nikhilesh Bhagavatula
> <pbhagavat...@marvell.com> wrote:
> >
> > Hi OrI,
> >
> > >
> > >Hi Pavan,
> > >
> > >
> > >> -----Original Message-----
> > >> From: dev <dev-boun...@dpdk.org> On Behalf Of Pavan Nikhilesh
> > >Bhagavatula
> > >> Sent: Sunday, March 1, 2020 4:38 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,
> > >> >
> > >> >> -----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)
> > >>
> > >
> > >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
> > >> >> >> >+ */
> > >> >> >> >+};
