Re: [dpdk-dev] [EXT] [PATCH v1 3/4] regexdev: add regexdev core functions

[Ori Kam <orika@mellanox.com>]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, April 7, 2020 8:50 AM
> To: Ori Kam <orika@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; xiang.w.wang@intel.com; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; 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; Parav Pandit <parav@mellanox.com>
> Subject: Re: [dpdk-dev] [EXT] [PATCH v1 3/4] regexdev: add regexdev core
> functions
> 
> > >
> > > If it abstracts it properly adding vdev and PCI is a simple change.
> > > See
> > >
> > > lib/librte_eventdev/rte_eventdev_pmd_vdev.h
> > > lib/librte_eventdev/rte_eventdev_pmd_pci.h
> > >
> > > I think, the common code should take from other matured subsystem
> instead if
> > > writing from scratch,
> > >
> > I agree with you about the rewrite, but this is why I don't want to add this
> code
> > until I know what this code should do and how it should be used.
> > I  don't agree, that one subsystem is like other one by default, and that if
> something
> > is done for one subsystem it should be done for other.
> > Not always what was done before is the best.
> >
> > Some time back there was a long thread about ethdev and the rte device
> > where should it be released and by whom.
> > My basic thinking is that unless proven otherwise the code should be in the
> PMD
> > this is also why it is important for me to get this rte level API acked.
> > when starting to implement the code for the PMD it will be cleared what
> > is the shared code and how it is best to configure the system.
> > Also this is not external API so it can be changed at any time.
> > Saying that I don't think we should wait long before adding such code.
> > I think that when we will have our first PMD we know better if such
> > function is needed.
> > Also think about that maybe this PMD will be shared with the
> > net PMD so such function will also introduce more complexity.
> 
> 
> My thought process was I like this when I added the common code for
> eventdev.
> I have checked ethdev, cryptodev and followed a similar scheme
> wherever it applicable for eventdev.
> If we see the regexdev API, it is similar to ethdev. cryptodev and
> eventdev API. From the device
> API PoV, the framework needs to follow the same behavior to avoid
> having new behavior for regexdev,
> Especially in configure->queue_setup->start->rx_burst->tx_burst->stop-
> >reconfigure->start
> sequence.
> 
> 
> Ethdev may be bloated by features, My request is to take cryptodev and
> eventdev as a base
> change to accordingly. That makes review process easy, As reviewer
> needs only think, The rationale  behind,
> Why it regexdev common code chosen a different approach. Writing from
> scratch makes the reviewer's job
> difficult.
> 
> We spend a lot of time reviewing and make mature cryptodev and
> eventdev common code, Please leverage that.

I fully agree with you that we should leverage known code as much as we
can. But just like you said the idea is to know what the RegEx needs 
and then see how it is done in other modules.
I don't know how the vdev pmd will look like (I can guess)
So I prefer to add this code in later stage where we will get better
Understanding on how best to implement it. 

We have the configure, queue_setup, start, enqueue, dequeue and stop.
I will add support for reconfigure like I said in previous code.
So if I understand correctly from your point of view, this patch is just missing
the vdev functions right? If so lets keep them out for now, I promise you that if needed
I will add this function.