From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C0BD1A2EFC for ; Thu, 19 Sep 2019 08:40:33 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7B4E11E956; Thu, 19 Sep 2019 08:40:33 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 43DD01BEE0 for ; Thu, 19 Sep 2019 08:40:31 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Sep 2019 23:40:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,522,1559545200"; d="scan'208";a="271116174" Received: from hs1.sh.intel.com (HELO hs1) ([10.67.119.11]) by orsmga001.jf.intel.com with ESMTP; 18 Sep 2019 23:40:23 -0700 Date: Thu, 19 Sep 2019 09:58:57 -0400 From: Wang Xiang To: Jerin Jacob Kollanukkaran Cc: Thomas Monjalon , "dev@dpdk.org" , Pavan Nikhilesh Bhagavatula , Shahaf Shuler , Hemant Agrawal , Opher Reviv , Alex Rosenbaum , Dovrat Zifroni , Prasun Kapoor , Nipun Gupta , "Richardson, Bruce" , "Hong, Yang A" , "Chang, Harry" , "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" , "Ni, Hongjun" , "j.bromhead@titan-ic.com" , "deri@ntop.org" , "fc@napatech.com" , "arthur.su@lionic.com" , Guy Kaneti , Smadar Fuks , Liron Himi , edwin.verplanke@intel.com, keith.wiles@intel.com Message-ID: <20190919135857.GA82263@hs1> References: <20190627155036.56940-1-jerinj@marvell.com> <8285913.8xKIzI91KM@xps> <1922242.dABWq9CbNQ@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [RFC PATCH v1] regexdev: introduce regexdev subsystem X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Jerin, Thanks for your response. More comments below and inline. 1) I think the size of some varaibles (e.g. nb_matches, scan_size, matching offset, etc) should be increased based on what Hyperscan supports. a) struct rte_regex_ops: uint16_t scan_size => uint32_t scan_size uint8_t nb_actual_matches => uint64 nb_actual_matches uint8_t nb_matches => uint64 nb__matches b) struct rte_regex_match: uint16_t offset => uint32_t offset uint16_t len => uint32_t len c) uint16_t rte_regex_rule_db_update(uint8_t dev_id, const struct rte_regex_rule *rules, uint16_t nb_rules); => uint32_t rte_regex_rule_db_update(uint8_t dev_id, const struct rte_regex_rule *rules, uint32_t nb_rules); d) int rte_regex_queue_pair_setup(uint8_t dev_id, uint8_t queue_pair_id, const struct rte_regex_qp_conf *qp_conf); => int rte_regex_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id, const struct rte_regex_qp_conf *qp_conf); e) struct rte_regex_dev_config: uint8_t nb_max_matches => uint64_t nb_max_matches f) struct rte_regex_dev_info: uint8_t max_matches => uint64_t max_matches 2) There are rte_regex_dev_attr_get() and rte_regex_dev_attr_set() defined. Are all the attributes below could be set by users? Is any of them read-only? /** Enumerates RegEx device attribute identifier */ enum rte_regex_dev_attr_id { RTE_REGEX_DEV_ATTR_SOCKET_ID, /**< The NUMA socket id to which the device is connected or * a default of zero if the socket could not be determined. * datatype: *int* * operation: *get* */ RTE_REGEX_DEV_ATTR_MAX_MATCHES, /**< Maximum number of matches per scan. * datatype: *uint8_t* * operation: *get* and *set* * * @see RTE_REGEX_OPS_RSP_MAX_MATCH_F */ RTE_REGEX_DEV_ATTR_MAX_SCAN_TIMEOUT, /**< Upper bound scan time in ns. * datatype: *uint16_t* * operation: *get* and *set* * * @see RTE_REGEX_OPS_RSP_MAX_SCAN_TIMEOUT_F */ RTE_REGEX_DEV_ATTR_MAX_PREFIX, /**< Maximum number of prefix detected per scan. * This would be useful for denial of service detection. * datatype: *uint16_t* * operation: *get* and *set* * * @see RTE_REGEX_OPS_RSP_MAX_PREFIX_F */ }; 3) Both RTE_REGEX_PCRE_RULE_* and RTE_REGEX_DEV_PCRE_UNSUP_* can be viewed as device capabilities. Can we merge them with RTE_REGEX_DEV_CAPA_RUNTIME_COMPILATION_F and have a unified regex_dev_capa in struct rte_regex_dev_info. 4) It'll be good if we can also define synchronous matching API for users who want to have a one-off scan and wait for the results. On Tue, Sep 10, 2019 at 08:05:39AM +0000, Jerin Jacob Kollanukkaran wrote: > Hi Xiang, > > Sorry for delay in response(Was busy with 19.11 proposal deadline). Please see inline. > > > > > Reply to Xiang's queries in main thread: > > > > Hi all, > > > > Some questions regarding APIs. Could you please give more insights? > > > > 1) rte_regex_ops > > a) rsp_flags > > These two flags RTE_REGEX_OPS_RSP_PMI_SOJ_F and > > RTE_REGEX_OPS_RSP_PMI_EOJ_F are used for cross buffer scan. > > RTE_REGEX_OPS_RSP_PMI_EOJ_F tells whether we have a partial match > > at the end of current buffer after scan. > > What's the purpose of having RTE_REGEX_OPS_RSP_PMI_SOJ_F? > > > > [Jerin] Since we need three states to represent partial match buffer, > > RTE_REGEX_OPS_RSP_PMI_SOJ_F to > > represent start of the buffer, intermediate buffers with no flag, and end of > > the buffer with RTE_REGEX_OPS_RSP_PMI_EOJ > > > [Xiang] How could a user leverage these flags for matching? Suppose a large > > buffer is divided into multiple chunks. Will RTE_REGEX_OPS_RSP_PMI_SOJ_F > > cause an early quit once it isn't set after scan the first chunk. Similarly, > > RTE_REGEX_OPS_RSP_PMI_EOJ tells a user whether to stop matching future > > buffers after finish the last chunk? > > Let me describe with an example, > > Assume, > 1) struct rte_regex_dev_info:: max_payload_size set to 1024 > 2) rte_regex_dev_config:: dev_cfg_flags configured with RTE_REGEX_DEV_CFG_CROSS_BUFFER_SCAN_F > 3) Device programmed with matching "hello\s+world" pattern > 4) user enqueue struct rte_regex_ops:: buf_addr point following "data" and struct rte_regex_op:: scan_size = 1024 > > data[0..1021] = data don???t have hello world pattern > data[1022] = 'h' > data[1023] = 'e' > > 5) user enqueue struct rte_regex_ops:: buf_addr point following "data" and struct rte_regex_op:: scan_size = 9 > > data[0] = 'l' > data[1] = 'l' > data[2] = 'o' > data[3] = ' ' > data[4] = 'w' > data[5] = 'o' > data[6] = 'r' > data[7] = 'l' > data[8] = 'd' > > If so, > > Response to 4) will be RTE_REGEX_OPS_RSP_PMI_SOJ_F in rte_regex_ops:: rsp_flags on dequeue > Where rte_regex_match:: offset is 1022 and len 2 > > Response to 5) will be RTE_REGEX_OPS_RSP_PMI_EOJ_F in rte_regex_ops:: rsp_flags on dequeue > Where rte_regex_match:: offset is 0 and len 9 > If the defined pattern is "hello.*world" instead of "hello\s+world", and we enqueue following struct rte_regex_ops: 1) rte_regex_op:: scan_size = 1024 data[0..1021] = data don???t have hello world pattern data[1022] = 'h' data[1023] = 'e' 2) rte_regex_op:: scan_size = 9 data[0] = 'l' data[1] = 'l' data[2] = 'o' data[3] = ' ' data[4] = 'w' data[5] = 'o' data[6] = 'r' data[7] = 'l' data[8] = 'd' 3) rte_regex_op:: scan_size = 5 data[0] = 'w' data[1] = 'o' data[2] = 'r' data[3] = 'l' data[4] = 'd' Will response to 3) have RTE_REGEX_OPS_RSP_PMI_EOJ_F in rte_regex_ops:: rsp_flags on dequeue Where rte_regex_match:: offset is 0 and len 4? I am wondering what's your expected behavior for .* or similar syntax and if there are syntax compatability issues. We report all matches in Hyperscan, e.g. report end match offsets 11 and 16 for pattern "hello.*world" and corpus "hello worldworld". BTW, not sure how other hardware devices handle cross buffer scan. Hyperscan doesn't reports matches for start and intermediate buffers but only reports end offset if a full match is found. > > > > > RTE_REGEX_OPS_RSP_MAX_PREFIX_F: This looks like a definition for a > > specific hardware implementation. I am wondering what this PREFIX refers > > to:)? > > > > [Jerin] Yes. Looks like it is for hardware specific implementation. Introduced > > rte_regex_dev_attr_set/get functions to make it portable and > > To add new implementation specific fields. > > For example, if a rule is > > /ABCDEF.*XYZ/, ABCD is considered the prefix, and EF.*XYZ is considered the > > factor. The prefix is a literal > > string, while the factor can contain complex regular expression constructs. As > > a result, rule matching occurs in > > two stages: prefix matching and factor matching. > > > > b) user_id or user_ptr > > Under what kind of circumstances should an application pass value into > > these variables for enqueue and dequeuer operations? > > > > [Jerin] Just like rte_crypto_ops, struct rte_regex_ops also allocated using > > mempool normally, on enqueue, user can specify user_id > > If needed to in order identify the op on dequeue if required. The use case > > could be to store the sequence number from application > > POV or storing the mbuf ptr in which pattern is requested etc. > > > > > > 2) rte_regex_match > > a) offset; /**< Starting Byte Position for matched rule. */ and uint16_t > > len; /**< Length of match in bytes */ > > Looks like the matching offset is defined as *starting matching offset* > > instead of *end matching offset*, e.g. report the offset of "a" instead of "c" > > for pattern "abc". > > If so, this makes it hard to integrate software regex libraries such as > > Hyperscan and RE2 as they only report *end matching offset* without length > > of match. > > Although Hyperscan has API for *starting matching offset*, it only delivers > > partial syntax support. So I think we have to define *end of matching offset* > > for software solutions. > > > > [Jerin] I understand the hyperscan's HS_FLAG_SOM_LEFTMOST tradeoffs. I > > thought application would need always the length of the match. > > Probably we will see how other HW implementation (from Mellanox) etc. We > > will try to abstract it, probably we can make it as function of "user > > requested". > > [Xiang] Yes, it will be good to make it per user request. At least from > > Hyperscan user's point of view, start of match and match length are not > > mandatory. > > OK. I think, we can introduce RTE_REGEX_DEV_CFG_MATCH_AS_START > In device configure. > > Since offset+len == end, we can introduce following generic inline function. > > static inline > rte_regex_match_end(truct rte_regex_match *match) > { > match->offset + match->len; > } > > Example: pattern to match is "hello\s+world" and data is following > data[4] = 'h' > data[5] = 'e' > data[6] = 'l' > data[7] = 'l' > data[8] = 'o' > data[9] = ' ' > data[10] = 'w' > data[11] = 'o' > data[12] = 'r' > data[13] = 'l' > data[14] = 'd' > > if device is configured with RTE_REGEX_DEV_CFG_MATCH_AS_START > match->offset returns 4 > match->len returns 11 > > if device is NOT configured with RTE_REGEX_DEV_CFG_MATCH_AS_START > driver MAY return the following(in hyperscan case) > match->offset returns 0 > match->len returns 11 + 4 > > In both case(irrespective of flags, to make application life easy) rte_regex_match_end() would return 15. > If application demands for MATCH_AS_START then driver can return match->offset returns 4 and match->len returns 11 > Aka set HS_FLAG_SOM_LEFTMOST in hyperscan driver, But application should use rte_regex_match_end() > for finding the end of the match. To make, work in all cases. > > Is it OK? > Can we replace len with end offset? So we can change "offset" to "start_offset" and len to "end_ offset" in struct rte_regex_match. Users interested in len could take "end_offset - start_offset". We may also change RTE_REGEX_DEV_CFG_MATCH_AS_START to RTE_REGEX_DEV_CFG_MATCH_START In your example, if device is configured with RTE_REGEX_DEV_CFG_MATCH_START match->start_offset returns 4 match->end_offset returns 15 if device is NOT configured with RTE_REGEX_DEV_CFG_MATCH_START match->start_offset returns 0 match->end_offset returns 15 > > > > 3) rte_regex_rule_db_update() > > Does this mean we can dynamically add or delete rules for an already > > generated database without recompile from scratch for hardware Regex > > implementation? > > If so, this isn't possible for software solutions as they don't support > > dynamic database update and require recompile. > > > > [Jerin] rte_regex_rule_db_update() internally it would call recompile > > function for both HW and SW. > > See rte_regex_dev_config::rule_db in rte_regex_dev_configure() for > > precompiled rule database case. > > [Xiang] OK, sounds like we have to save the original rule-set for the device in > > order to do recompile. I see both ADD and REMOVE operators from > > rte_regex_rule. > > For rules with REMOVE operator, what's the expected behavior to handle > > them for the old rule-set? Do we need to go through the old rule-set and > > remove corresponding rules before doing recompile? > > Yes. > I think it'll be better to change rte_regex_rule_db_update() to rte_regex_rule_compile() and have users to provide a full rule-set. So we don't have to maintain old rule-set and decide which one to keep and remove. We can simply recompile new rule-set and get rid of rte_regex_rule_op in this case.