Hi Xiang,

-----Original Message-----
From: dev <redacted> On Behalf Of Wang Xiang
Sent: Wednesday, February 26, 2020 11:03 AM
To: Ori Kam <redacted>
Cc: Jerin Jacob <redacted>; Jerin Jacob <redacted>;
dpdk-dev [off-list ref]; Pavan Nikhilesh [off-list ref];
Shahaf Shuler [off-list ref]; Hemant Agrawal
[off-list ref]; Opher Reviv [off-list ref]; Alex
Rosenbaum [off-list ref]; dovrat@marvell.com; Prasun Kapoor
[off-list ref]; Nipun Gupta [off-list ref]; Richardson,
Bruce [off-list ref]; 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
[off-list ref]
Subject: Re: [dpdk-dev] [PATCH v3] regexdev: introduce regexdev subsystem

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.

I don't mind to keep the union as it was in V3, but I would like to remove the
configuration bit (RTE_REGEX_DEV_CFG_MATCH_AS_START). 
Meaning that if the device reports RTE_REGEX_DEV_SUPP_MATCH_AS_START
the result will always be with start_offset and len.

Best,
Ori

Thanks,
Xiang

quoted

quoted

quoted

quoted

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

quoted

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.

quoted

or We can keep it as len or end_offset based on which drivers upstream

first,

quoted

quoted

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:

quoted

quoted

-----Original Message-----
From: Jerin Jacob <redacted>
Sent: Tuesday, February 25, 2020 7:57 AM
To: Ori Kam <redacted>
Cc: Jerin Jacob <redacted>; xiang.w.wang@intel.com; dpdk-dev
[off-list ref]; Pavan Nikhilesh [off-list ref]; Shahaf
Shuler [off-list ref]; Hemant Agrawal
[off-list ref]; Opher Reviv [off-list ref]; Alex
Rosenbaum [off-list ref]; dovrat@marvell.com; Prasun Kapoor
[off-list ref]; Nipun Gupta [off-list ref];

Richardson,

quoted

quoted

Bruce [off-list ref]; yang.a.hong@intel.com;
harry.chang@intel.com; gu.jian1@zte.com.cn;

shanjiangh@chinatelecom.cn;

quoted

quoted

zhangy.yun@chinatelecom.cn; lixingfu@huachentel.com;

wushuai@inspur.com;

quoted

quoted

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
[off-list ref]
Subject: Re: [dpdk-dev] [PATCH v3] regexdev: introduce regexdev subsystem

quoted

quoted

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

quoted

quoted

will have HW

quoted

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.

quoted

quoted

quoted

5) Need a maintainer for maintaining the regex subsystem

We wish to maintain it if you agree.

Yes. Please.

Great.

quoted

quoted

quoted

quoted

One more thing, regarding the ops structure, I think it is better to split

it

quoted

quoted

to 2

quoted

quoted

different

quoted

structures one enque and one for dequeue, since there are no real

shared

quoted

quoted

quoted

quoted

data and we will

quoted

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

quoted

quoted

quoted

quoted

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

quoted

also this will enable us to replace groupX with group[] which will allow

even

quoted

quoted

more groups.

quoted

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

quoted

quoted

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.

quoted

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.

quoted

b) scan_size

Please see above.

quoted

c) user_id (This would be main one)

Agree

quoted

# 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.

quoted

quoted

quoted

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

quoted

coding the rest of the common code.