Re: [dpdk-dev] [PATCH v3] regexdev: introduce regexdev subsystem

[Wang Xiang <xiang.w.wang@intel.com>]

Hi Ori and Jerin,

One comment regarding my concern with len and end_offset problem.
From open source SW regex library(libpcre, re2 and Hyperscan) and 
Intel's perspective, the matching results returned are always start
offset and end offset. More importantly, Hyperscan only reports end offset
most of the time.

It'll be good to keep this union as an abstraction and enforce the default
behavior for each solution, i.e. HW solutions doesn't support MATCH_AS_START
flag at rule compile time. Applications will know the meaning of variable at
rule compile time with the flag so they don't have to do extra check at fast path
run-time matching.
Welcome for better abstraction ideas.

Thanks,
Xiang

> > > > +                       /**< 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.
>


On Tue, Feb 25, 2020 at 07:48:54AM +0000, Ori Kam wrote:
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday, February 25, 2020 7:57 AM
> > To: Ori Kam <orika@mellanox.com>
> > Cc: Jerin Jacob <jerinj@marvell.com>; xiang.w.wang@intel.com; dpdk-dev
> > <dev@dpdk.org>; Pavan Nikhilesh <pbhagavatula@marvell.com>; Shahaf
> > Shuler <shahafs@mellanox.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Opher Reviv <opher@mellanox.com>; Alex
> > Rosenbaum <alexr@mellanox.com>; dovrat@marvell.com; Prasun Kapoor
> > <pkapoor@marvell.com>; Nipun Gupta <nipun.gupta@nxp.com>; Richardson,
> > Bruce <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] [PATCH v3] regexdev: introduce regexdev subsystem
> > 
> > > > 4) app/test/test_regexdev.c like app/test/test_eventdev.c
> > >
> > > We started to create a super basic app, after the API will be finalized and we
> > will have HW
> > > we can push it. (if you need it faster than feel free)
> > 
> > A simple Unit test case needs to be present for the APIs. On the
> > course of developing common code,
> > it can be developed to test the common code with dummy/skeleton driver.
> > 
> 
> Agree this is what we are currently have.
> 
> > >
> > > > 5) Need a maintainer for maintaining the regex subsystem
> > > >
> > > We wish to maintain it if you agree.
> > 
> > Yes. Please.
> > 
> 
> Great.
> 
> > > > >
> > > > > 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.
> > >
> > > True that there will be overhead with 2 mempools (small one)
> > > but lets assume 255 results. This means that the buffer should be 255 *
> > sizeof(rte_regex_match) = 2K
> > > also this will enable us to replace groupX with group[] which will allow even
> > more groups.
> > > In addition don't think that crypto is a good example.
> > > The main difference is that in RegEx the output is different format then the
> > input.
> > 
> > # IMO, Some of the fields may be useful for a response as well. I
> > think application may be interested in following
> > req filed in the response.
> > a) buf_addr
> 
> I don't see how this can be used in the response. since if working in out of order result.
> you don’t know which result will be returned. 
> I also think it is error prone to use the same op for the enqueue and dequeue.
> 
> > b) scan_size
> 
> Please see above.
> 
> > c) user_id (This would be main one)
> 
> Agree
> 
> > 
> > # Having two mempools adds overhead per lcore L1 cache usage and extra
> > complexity to the application.
> > 
> > # IMO, From a performance perspective, one mempool is good due to less
> > stress on the cache and it is costly to
> > add new mempool for HW mempool implementations.
> > 
> > # I think, group[] use case we can add it when it required by
> > introducing "matches_start_offset" field, which will
> > tell the req, where is the end of group[] and where "matches" start
> > with single mempool scheme also.
> > 
> > # I think, one of the other use case for "matches_start_offset" that,
> > It may possible to put vendor-specific
> > opaque data. It will be filled by driver on response. The application
> > can reference the matches as
> > 
> > struct rte_regex_match *matches = RTE_PTR_ADD(ops, ops-
> > 
> >matches_start_offset);
> > 
> 
> O.K for now we will keep  it as is, and we will see what will be in the future.
> 
> > >
> > > > I assume you will send the v4 with these comments. I think, with v4 we
> > > > can start implementing common library code.
> > >
> > > Just need to agree on the split (one more iteration )
> > > and I will start working on the common code.
> > 
> > Ack.
> 
> Great,
> I'm starting to work on V4 with all comments so the RFC will be acked and then will start 
> coding the rest of the common code.
>