Re: [dpdk-dev] [RFC v6] regexdev: introduce regexdev subsystem

[Ori Kam <orika@mellanox.com>]

Hi Pavan,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh Bhagavatula
> Sent: Tuesday, March 10, 2020 6:37 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] [RFC v6] regexdev: introduce regexdev subsystem
> 
> Hi Ori,
> >
> >Hi Pavan,
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
> >Bhagavatula
> >> Sent: Tuesday, March 10, 2020 3:42 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] [RFC v6] regexdev: introduce regexdev
> >subsystem
> >>
> >> Hi Ori,
> >>
> >> <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. */
> >>
> >> While implementing pcre2 SW driver I came across an oddity where
> >having
> >> mbuf alone
> >> wouldn’t suffice, we need to have scan start offset and scan length as
> >generally
> >> we would skip the
> >> L2/L3 header.
> >>
> >
> >Yes you are correct, in most cases the application will need
> >not the all mbuf or it will connect number of mbuf.
> >This can be acchived by modifying the mbuf to point to the correct data
> >start, and decrease the len.
> 
> Wouldn’t that complicate Txing the packet later on after dequeue from regex if
> the user decides to do so?.
> Instead we can have two fields in rte_regex_ops for storing scan_start_offset
> and
> scan_size
> 
The user will need to return the packet to the original state.  I agree that
that it is a bit harder for the application (but not by much). But in any case the user knows
the size he removed so when done he just need to return to the original value.
on the other end it save the user the working with iov structs.

Regarding your idea about start_offset and scan_size. It is a nice idea,
But I don't think it is worth it, since the start_offset is just what the user
needs to keep in order to return the mbuf to original state.
Also if the user wants to combine number of messages, he can't use this
approach  since he will need to remove the header also from the second
message and bind the two messages. So in any case the user must have some
logic.

> >In one of the previous version we used buffer address and iov to solve
> >this issue. But in order to keep the API the same as crypto we decided
> >to go
> >with mbuf.
> 
> The general idea was to save cycles converting mbuf and chain of mbuf to iov
> and back not
> just to stay in line with crypto.
> 

I agree and this was also my main thinking but Jerin and other community members raised 
this approach.
Each approach has advantages and disadvantages.
If the user wants he can just give the all mbuf. Also since at least in some
cases the regex will be done after crypto it make sense to use the same structs.
There is also the advantage of sharing code between all the drivers. (net/crypto/regex)
which can be done when using mbuf. (for example memory registration)

> >This API is experimental and based on the usage we might change it to
> >iov.
> >
> >> >+
> >> >+	/* W2 */
> >> >+	uint16_t group_id0;
> >> >+	/**< First group_id to match the rule against. At minimum one
> >> >group
> >> >+	 * should be valid. Behaviour is undefined non of the groups are
> >> >valid.
> >> >+	 *
> >> >+	 * @see RTE_REGEX_OPS_REQ_GROUP_ID0_VALID_F
> >> >+	 */
> >> >+	uint16_t group_id1;
> >> >+	/**< Second group_id to match the rule against.
> >> >+	 *
> >> >+	 * @see RTE_REGEX_OPS_REQ_GROUP_ID1_VALID_F
> >> >+	 */
> >> >+	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
> >> >+	 */
> >> >+};
> >> >+
> >>
> >> Thanks,
> >> Pavan.
> >
> >Thanks,
> >Ori