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 99600A0524; Sun, 23 Feb 2020 10:53:59 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 631681BE85; Sun, 23 Feb 2020 10:53:58 +0100 (CET) Received: from mail-il1-f194.google.com (mail-il1-f194.google.com [209.85.166.194]) by dpdk.org (Postfix) with ESMTP id 05E112C39 for ; Sun, 23 Feb 2020 10:53:56 +0100 (CET) Received: by mail-il1-f194.google.com with SMTP id p8so5290653iln.12 for ; Sun, 23 Feb 2020 01:53:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RDtCRzIFxh03sZhFlmOgNBdxSW+yolieoE0t/+f/TCg=; b=T9bHcIRNJZSJZ6rbuvhKApSe66tq0hGfDI6dh6x1AXC6NulBgbVMme22E+eD7t12qr ctlVVQY2M2R0giOvxMcAMntjLVLZsKNbGNInNaHyl7XTDnBi5UNCFxVvZQtIjWe3cbKi xNOkCvD/l66D0MlYEAmleO5Er6Kwl3Y3YbWfvd8/XLo3JyaTlQDwXELH8GXsa0P1nswE iX+aWCsz6wyI94f/4w7XUWBo4ZgMjRfuQNxwnUaTGkn6IfK87M5scWZnA+x6nMDPjs2Z 2Hh0SBAOUP1PBwg5QIPWslrPNUUVSKmm7igc91Z7B5UNICy6FxiSek3OCPDjQq3FMQur cIrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RDtCRzIFxh03sZhFlmOgNBdxSW+yolieoE0t/+f/TCg=; b=LFH9utUZb0ewVkSGIrIQ+BVWl3J5xxVj1qfLyOjA97FbOwEsyfkmkvjHLkGuR2xUH6 aHtvaqEzUqFMyoC2CG4RVQME2lBnEb3t+RKrVimKpwCxKent7SilQ0DM/J53KYUUXK/H zIhAy3ADLUDi3yZ2iIPDxhqz6ne9Ovdrw1wMTkijJ52jZTtasNS2uOLUGQkQ7EwUmFgl 9nvs9pVz/Z+jozzHFM4naaZQk7gwmQIOoaulGRNYbuYRX0jsV27LUEE4geFYaGN/YzZc aC7jXJuNmARVeAU8RWqklQI4tKyD7ZwjRcBbDRjlyumI2vmL75tlS32EXyUbB04HqllS UsIg== X-Gm-Message-State: APjAAAWAvT24LUDj2zXuB661S4oS8ngIaEIpVYzS9bM0lI6r22E3zyzn GN3Lhw3RGGcJ6f5wu/DS4N8RFhi8djh3GAl+bws= X-Google-Smtp-Source: APXvYqwflr94ctquEkLuGpniJ4VuDQ8Y5/UfgpCaW3leWEwt4XgA2BvnfbbEQ4ad2aCa0i7HR4jG1wiHAiB+F6FI9hE= X-Received: by 2002:a05:6e02:f47:: with SMTP id y7mr52669564ilj.162.1582451636063; Sun, 23 Feb 2020 01:53:56 -0800 (PST) MIME-Version: 1.0 References: <20190627155036.56940-1-jerinj@marvell.com> <1580202029-37096-1-git-send-email-orika@mellanox.com> In-Reply-To: From: Jerin Jacob Date: Sun, 23 Feb 2020 15:23:39 +0530 Message-ID: To: Ori Kam Cc: Jerin Jacob , "xiang.w.wang@intel.com" , dpdk-dev , Pavan Nikhilesh , Shahaf Shuler , Hemant Agrawal , Opher Reviv , Alex Rosenbaum , "dovrat@marvell.com" , Prasun Kapoor , Nipun Gupta , "Richardson, Bruce" , "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 Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3] 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" On Sun, Feb 23, 2020 at 2:12 PM Ori Kam wrote: > > Hi Jerin, > > Thanks, for the review. > PSB Hi Ori Since we are finalizing the specification part, I thought of enumerating the list of work needs to be completed for a new subsystem in DPDK. 0) Finalize the first version of the spec. Hope v4 will do that. 1) Introduce common library code for based on the specification 2) One HW based driver implementation 3) One SW reference driver: libpcre library provides complete PCRE functionality. 4) app/test/test_regexdev.c like app/test/test_eventdev.c 5) Need a maintainer for maintaining the regex subsystem 6) The first version programming guide documentation 7) Add app/test-regexdev like app/test-eventdev 8) Add an examples/xxxxxx program IMO The following items need to be completed to accept a subsystem in dpdk(Need at least on HW and SW driver). 0) Finalize the first version of the spec. Hope v4 will do that. 1) Introduce common library code for based on the specification 2) One HW based driver implementation 3) One SW reference driver: libpcre library provides complete PCRE functionality. 4) app/test/test_regexdev.c like app/test/test_eventdev.c 5) Need a maintainer for maintaining the regex subsystem We have item (3) so Marvell would like to work on item (3). Our HW driver may ready by v20.05 or the worst case by 20.08. Let us know what other items Mellanox or community would like to work on. This is to avoid duplication of work to get clarity on the next steps. PSB > > > Ori Kam > > > -----Original Message----- > > From: Jerin Jacob > > Sent: Saturday, February 22, 2020 6:52 PM > > To: Ori Kam > > Cc: Jerin Jacob ; xiang.w.wang@intel.com; dpdk-dev > > ; Pavan Nikhilesh ; Shahaf > > Shuler ; Hemant Agrawal > > ; Opher Reviv ; Alex > > Rosenbaum ; dovrat@marvell.com; Prasun Kapoor > > ; Nipun Gupta ; Richardson, > > Bruce ; 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 > > > > Subject: Re: [dpdk-dev] [PATCH v3] regexdev: introduce regexdev subsystem > > > > > diff --git a/lib/librte_regexdev/rte_regexdev.h > > b/lib/librte_regexdev/rte_regexdev.h > > > new file mode 100644 > > > index 0000000..c42128b > > > --- /dev/null > > > +++ b/lib/librte_regexdev/rte_regexdev.h > > > @@ -0,0 +1,1411 @@ > > > +/* SPDX-License-Identifier: BSD-3-Clause > > > + * Copyright(C) 2019 Marvell International Ltd. > > > + * Copyright(C) 2020 Mellanox International Ltd. > > > > There are a few comments from Xiang as well. So let's add Intel also > > to the list. > > > > Sure no problem. Thanks > > > > + */ > > > + > > > +#ifndef _RTE_REGEXDEV_H_ > > > +#define _RTE_REGEXDEV_H_ > > > > > + > > > +/** > > > + * RegEx device information > > > + */ > > > +struct rte_regex_dev_info { > > > + const char *driver_name; /**< RegEx driver name. */ > > > + struct rte_device *dev; /**< Device information. */ > > > + uint16_t max_matches; > > > + /**< Maximum matches per scan supported by this device. */ > > > + uint16_t max_queue_pairs; > > > + /**< Maximum queue pairs supported by this device. */ > > > + uint16_t max_payload_size; > > > + /**< Maximum payload size for a pattern match request or scan. > > > + * @see RTE_REGEX_DEV_CFG_CROSS_BUFFER_SCAN_F > > > + */ > > > + uint32_t max_rules_per_group; > > > + /**< Maximum rules supported per group by this device. > > > + * This number can't be larger then 20 bits. > > > > s/then/than > > > > I think, we don't need to say this " This number can't be larger than 20 bits." > > It may help SW drivers. > > > > Agree I will remove the 20 bits part. > > > > > > > > + */ > > > + uint16_t max_groups; > > > + /**< Maximum group supported by this device. > > > + * This number can't be larger then 12 bits. > > s/then/than > > I think, we don't need to say this " This number can't be larger than 12 bits." > > It may help SW drivers. > > > > Agree will remove the 12 bits part. > > > > + */ > > > + uint32_t regex_dev_capa; > > > + /**< RegEx device capabilities. @see RTE_REGEX_DEV_CAPA_* */ > > > + uint64_t rule_flags; > > > + /**< Supported compiler rule flags. > > > + * @see RTE_REGEX_PCRE_RULE_*, struct rte_regex_rule::rule_flags > > > + */ > > > + uint8_t max_scatter_gather; > > > + /**< The max supported number of buffers that can > > > + * be used in a single ops. The total size of all elements > > > + * must be less then max_payload_size. > > > + */ > > > +}; > > > > > > > +int > > > +rte_regex_rule_db_compile(uint8_t dev_id); > > > + > > > > I think your "rte_regex_rule_db_compile_activate() - compile and > > activate the new rule set" > > API name looks good. I am for rte_regex_rule_db_compile_activate(). > > > > I like your name, will change to compile_activate. Ack. > > > > > > +/* Fast path APIs */ > > > + > > > +/** > > > + * The generic *rte_regex_match* structure to hold the RegEx match > > attributes. > > > + * @see struct rte_regex_ops::matches > > > + */ > > > +struct rte_regex_match { > > > + RTE_STD_C11 > > > + union { > > > + uint64_t u64; > > > + struct { > > > + uint32_t rule_id:20; > > > + /**< Rule identifier to which the pattern matched. > > > + * @see struct rte_regex_rule::rule_id > > > + */ > > > + uint32_t group_id:12; > > > + /**< Group identifier of the rule which the pattern > > > + * matched. @see struct rte_regex_rule::group_id > > > + */ > > > + uint16_t offset; > > > > Since we have end_offset now, IMO, it is better to change this offset > > to "start_offset". > > > > Agree, will change. > > > > > > + /**< Starting Byte Position for matched rule. */ > > > + RTE_STD_C11 > > > + union { > > > + uint16_t len; > > > + /**< Length of match in bytes */ > > > + uint16_t end_offset; > > > + /**< The end offset of the match. In case > > > + * MATCH_AS_START configuration is disabled. > > > + * @see RTE_REGEX_DEV_CFG_MATCH_AS_START > > > + */ > > > > We have not concluded on this scheme. Have one field which has > > different meaning will be difficult > > for application. i.e fast path we need to have a check for this. > > > > This is the time to conclude . at least for the first version. > Why do we have one field with different meaning? > The result can be ether len or end_offset. > > > I think, Based on the majority of HW/SW implementation, we need to > > either go with len or > > end_offset. What Mellanox HW returns? len or end_offset? > > > > From Mellanox perspective we prefer the len approach. We also think > it is much more user oriented. > > > or We can keep it as len or end_offset based on which drivers upstream first, > > other drivers when it comes, we can see how to abstract it? > > > > I can except that assuming we choose the start and len approach I think, we can have first version with "start and len" by removing RTE_REGEX_DEV_CFG_MATCH_AS_START. When can think, how to abstract new drivers when it upstream based on the overhead. > > > > + }; > > > + }; > > > + }; > > > +}; > > > > > +/** > > > + * 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 */ > > > + uint16_t num_of_bufs; > > > + /**< The number of bufs that are part of this ops. The total size of > > > + * the length of all the buffer must be smaller then the max buffer > > > + * len. > > > + */ > > > + uint16_t resv1; > > > + uint32_t resv2; > > > > One of the point came up in our implementation is that. > > HW can return an error due to various reasons. > > > > One option could be to make nb_matches as zero? and update some flag? > > > > What are your thoughts? updating the flag may be overkill. > > > > I think we can return just zero matches for now. Ack. > > > > > > + > > > + /* W2 */ > > > + struct rte_regex_iov *(*bufs)[]; > > > + /**< Holds a pointer to the buffers list.*/ > > > > This memory gets submitted to HW so it can not be from the heap. > > Cryptodev had a similar dilemma to use the container format for > > multi-segment case, Finally they choose to with mbuf. > > > > The following elements are in mbuf. Considering to avoid duplication and > > avoid overhead most common usecase DPI(Assume if it is rte_regex_iov, > > one need to copy all the elements from mbuf on fastpath). > > I propose to have mbuf here instead of rte_regex_iov. > > > > The application only needs to set the data pointers. (no copy is required. ) > I agree that there are advantages to the mbuf approach. > The main limitation for the mbufs approach is that the user will need to play with the offset > pointers and pointers to the next mbuf, in order to support cross buffer. > For example we have a packet and we want to add to the scan also the last part of the previous packet, > this means that the application must modify the data offset in the previous packet mbuf including > changing the next pointer to point to the head of the new packet, and then return the values to the original position. > > What do you think? > We can start with mbufs and see how it works, or start with the buffer and see how it works. I think, we can start with mbuf to align with other subsystems. We will see later the use case for struct rte_regex_iov. > > > > > > struct rte_regex_iov { > > RTE_STD_C11 > > union { > > uint64_t u64; > > /**< Allow 8-byte reserved on 32-bit system */ > > void *buf_addr; > > /**< Virtual address of the pattern to be matched. */ > > }; > > rte_iova_t buf_iova; > > /**< IOVA address of the pattern to be matched. */ > > uint16_t buf_size; /**< The buf size. */ > > }; > > > > > > > + > > > + /* W5 */ > > > + RTE_STD_C11 > > > + union { > > > + uint64_t cross_buf_id; > > > + /**< ID used by the RegEx device in order to handle cross > > > + * buffer detection. > > > + * This ID is given by the RegEx device on dequeue, and > > > + * the application must send it on the following enque. > > > + */ > > > + void *cross_buf_ptr; > > > + /**< Pointer representation of *cross_buf_id* */ > > > > Could you have some example of how to use cross_buf_id? > > Marvell HW does not support cross_buf_id, so we need to add this > > feature as capability. > > > > The idea is that this buffer will be used to keep some internal data for the engine. > For example the current state and what was found until now, and then reuse this > for the next buffer. > We can remove it for now if we agree that we can add it later. I think, adding it later would be better so that we can see how to abstract it well. > > > > One more thing, regarding the ops structure, I think it is better to split it to 2 different > structures one enque and one for dequeue, since there are no real shared data and we will > be able to save memory, what do you think? Ops are allocated from mempool so it will be overhead to manage both. moreover, some of the fields added in req can be used for resp as info. cryptodev follows the similar concept, I think, we can have symmetry with cryptodev wherever is possible to avoid end-user to learn new API models. > > > > > > > Regarding the rule attributes, We think, following needs to be added to control > > the rule compilation behavior. If it not converging in first look, I > > think, we can make > > below as separate patch once we have basic things. > > > > Regarding the new code, we need also to add a function to get the capabilities for the compiler or > add a new field in the dev_info which will report the complier supported features. I agree. Lets remove this new code from the first version. We can add it later with capability as a new patch. I assume you will send the v4 with these comments. I think, with v4 we can start implementing common library code.