On Tue, Apr 7, 2020 at 12:16 PM Ori Kam <or...@mellanox.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjac...@gmail.com>
> > Sent: Tuesday, April 7, 2020 8:50 AM
> > To: Ori Kam <or...@mellanox.com>
> > Cc: Thomas Monjalon <tho...@monjalon.net>; Jerin Jacob Kollanukkaran
> > <jer...@marvell.com>; xiang.w.w...@intel.com; Pavan Nikhilesh Bhagavatula
> > <pbhagavat...@marvell.com>; dev@dpdk.org; Shahaf Shuler
> > <shah...@mellanox.com>; hemant.agra...@nxp.com; Opher Reviv
> > <op...@mellanox.com>; Alex Rosenbaum <al...@mellanox.com>; Dovrat
> > Zifroni <dov...@marvell.com>; Prasun Kapoor <pkap...@marvell.com>;
> > nipun.gu...@nxp.com; bruce.richard...@intel.com; yang.a.h...@intel.com;
> > harry.ch...@intel.com; gu.ji...@zte.com.cn; shanjia...@chinatelecom.cn;
> > zhangy....@chinatelecom.cn; lixin...@huachentel.com; wush...@inspur.com;
> > yuying...@yxlink.com; fanchengg...@sunyainfo.com;
> > davidf...@tencent.com; liuzho...@chinaunicom.cn;
> > zhaoyon...@huawei.com; o...@yunify.com; j...@netgate.com;
> > hongjun...@intel.com; j.bromh...@titan-ic.com; d...@ntop.org;
> > f...@napatech.com; arthur...@lionic.com; Parav Pandit <pa...@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.
OK.
>
> 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.
OK. Just to clarify, what I meant is, when we add the vdev, it should
simple as following[1]
ie. rte_event_pmd_allocate() and rte_event_pmd_release() etc reused
from PCI and VDEV.
i.e
a) lets have common routes like rte_event_pmd_allocate() for PMD
device allocation irrespective of VDEV for PCI
b) add PCI specific code
c) add VDEV specific code like [1]
[1]
static inline struct rte_eventdev *
rte_event_pmd_vdev_init(const char *name, size_t dev_private_size,
int socket_id)
{
struct rte_eventdev *eventdev;
/* Allocate device structure */
eventdev = rte_event_pmd_allocate(name, socket_id);
if (eventdev == NULL)
return NULL;
/* Allocate private device structure */
if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
eventdev->data->dev_private =
rte_zmalloc_socket("eventdev device private",
dev_private_size,
RTE_CACHE_LINE_SIZE,
socket_id);
if (eventdev->data->dev_private == NULL)
rte_panic("Cannot allocate memzone for private device"
" data");
}
return eventdev;
}
static inline int
rte_event_pmd_vdev_uninit(const char *name)
{
int ret;
struct rte_eventdev *eventdev;
if (name == NULL)
return -EINVAL;
eventdev = rte_event_pmd_get_named_dev(name);
if (eventdev == NULL)
return -ENODEV;
ret = rte_event_dev_close(eventdev->data->dev_id);
if (ret < 0)
return ret;
/* Free the event device */
rte_event_pmd_release(eventdev);
return 0;
}
>
