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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, Apr 7, 2020 at 12:16 PM Ori Kam <orika@mellanox.com> wrote:
>
>
>
> > -----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.

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;
}
>