[v1,3/4] regexdev: add regexdev core functions
Checks
Commit Message
This commit introduce the API that is needed by the RegEx devices in
order to work with the RegEX lib.
During the probe of a RegEx device, the device should configure itself,
and allocate the resources it requires.
On completion of the device init, it should call the
rte_regex_dev_register in order to register itself as a RegEx device.
Signed-off-by: Ori Kam <orika@mellanox.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
config/common_base | 3 +-
config/meson.build | 1 +
lib/librte_regexdev/Makefile | 1 +
lib/librte_regexdev/meson.build | 5 ++-
lib/librte_regexdev/rte_regexdev.c | 74 ++++++++++++++++++++++++++++++-
lib/librte_regexdev/rte_regexdev.h | 7 +++
lib/librte_regexdev/rte_regexdev_core.h | 2 +
lib/librte_regexdev/rte_regexdev_driver.h | 50 +++++++++++++++++++++
meson_options.txt | 2 +
9 files changed, 142 insertions(+), 3 deletions(-)
create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
Comments
Looks like this implementation is incomplete?
I don't see any pmd specific helper functions for @see rte_cryptodev_pmd.c, rte_eventdev_pmd*
>This commit introduce the API that is needed by the RegEx devices in
>order to work with the RegEX lib.
>
>During the probe of a RegEx device, the device should configure itself,
>and allocate the resources it requires.
>On completion of the device init, it should call the
>rte_regex_dev_register in order to register itself as a RegEx device.
>
>Signed-off-by: Ori Kam <orika@mellanox.com>
>Signed-off-by: Parav Pandit <parav@mellanox.com>
>---
> config/common_base | 3 +-
> config/meson.build | 1 +
> lib/librte_regexdev/Makefile | 1 +
> lib/librte_regexdev/meson.build | 5 ++-
> lib/librte_regexdev/rte_regexdev.c | 74
>++++++++++++++++++++++++++++++-
> lib/librte_regexdev/rte_regexdev.h | 7 +++
> lib/librte_regexdev/rte_regexdev_core.h | 2 +
> lib/librte_regexdev/rte_regexdev_driver.h | 50
>+++++++++++++++++++++
> meson_options.txt | 2 +
> 9 files changed, 142 insertions(+), 3 deletions(-)
> create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
>
<snip>
Hi Pavan,
Thanks for your review, PSB.
Best,
Ori
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh Bhagavatula
>
> Looks like this implementation is incomplete?
> I don't see any pmd specific helper functions for @see rte_cryptodev_pmd.c,
> rte_eventdev_pmd*
>
I think the current implementation includes all needed functions,
at least for the first stage.
You can find in rte_regexdev_driver.h the functions that should be called
by the PMD. We have the register / unregister which acts the same as create
and destroy. For parsing argument the PMD may call rte_kvargs_parse.
> >This commit introduce the API that is needed by the RegEx devices in
> >order to work with the RegEX lib.
> >
> >During the probe of a RegEx device, the device should configure itself,
> >and allocate the resources it requires.
> >On completion of the device init, it should call the
> >rte_regex_dev_register in order to register itself as a RegEx device.
> >
> >Signed-off-by: Ori Kam <orika@mellanox.com>
> >Signed-off-by: Parav Pandit <parav@mellanox.com>
> >---
> > config/common_base | 3 +-
> > config/meson.build | 1 +
> > lib/librte_regexdev/Makefile | 1 +
> > lib/librte_regexdev/meson.build | 5 ++-
> > lib/librte_regexdev/rte_regexdev.c | 74
> >++++++++++++++++++++++++++++++-
> > lib/librte_regexdev/rte_regexdev.h | 7 +++
> > lib/librte_regexdev/rte_regexdev_core.h | 2 +
> > lib/librte_regexdev/rte_regexdev_driver.h | 50
> >+++++++++++++++++++++
> > meson_options.txt | 2 +
> > 9 files changed, 142 insertions(+), 3 deletions(-)
> > create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
> >
>
> <snip>
Hi Ori,
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
>Bhagavatula
>>
>> Looks like this implementation is incomplete?
>> I don't see any pmd specific helper functions for @see
>rte_cryptodev_pmd.c,
>> rte_eventdev_pmd*
>>
>I think the current implementation includes all needed functions,
>at least for the first stage.
>You can find in rte_regexdev_driver.h the functions that should be
>called
>by the PMD. We have the register / unregister which acts the same as
>create
>and destroy. For parsing argument the PMD may call rte_kvargs_parse.
>
_driver.h should atleast include
rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
else there would be a lot of code repetition and possibly udefined behavior
at the driver layer.
And also why take a different path than the rest of the rte subsystems?
>
>> >This commit introduce the API that is needed by the RegEx devices in
>> >order to work with the RegEX lib.
>> >
>> >During the probe of a RegEx device, the device should configure
>itself,
>> >and allocate the resources it requires.
>> >On completion of the device init, it should call the
>> >rte_regex_dev_register in order to register itself as a RegEx device.
>> >
>> >Signed-off-by: Ori Kam <orika@mellanox.com>
>> >Signed-off-by: Parav Pandit <parav@mellanox.com>
>> >---
>> > config/common_base | 3 +-
>> > config/meson.build | 1 +
>> > lib/librte_regexdev/Makefile | 1 +
>> > lib/librte_regexdev/meson.build | 5 ++-
>> > lib/librte_regexdev/rte_regexdev.c | 74
>> >++++++++++++++++++++++++++++++-
>> > lib/librte_regexdev/rte_regexdev.h | 7 +++
>> > lib/librte_regexdev/rte_regexdev_core.h | 2 +
>> > lib/librte_regexdev/rte_regexdev_driver.h | 50
>> >+++++++++++++++++++++
>> > meson_options.txt | 2 +
>> > 9 files changed, 142 insertions(+), 3 deletions(-)
>> > create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
>> >
>>
>> <snip>
Hi Pavan,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh Bhagavatula
> Sent: Sunday, April 5, 2020 8:11 PM
> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; xiang.w.wang@intel.com
> Cc: 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; Thomas Monjalon
> <thomas@monjalon.net>; Parav Pandit <parav@mellanox.com>
> Subject: Re: [dpdk-dev] [EXT] [PATCH v1 3/4] regexdev: add regexdev core
> functions
>
> Hi Ori,
>
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
> >Bhagavatula
> >>
> >> Looks like this implementation is incomplete?
> >> I don't see any pmd specific helper functions for @see
> >rte_cryptodev_pmd.c,
> >> rte_eventdev_pmd*
> >>
> >I think the current implementation includes all needed functions,
> >at least for the first stage.
> >You can find in rte_regexdev_driver.h the functions that should be
> >called
> >by the PMD. We have the register / unregister which acts the same as
> >create
> >and destroy. For parsing argument the PMD may call rte_kvargs_parse.
> >
>
> _driver.h should atleast include
> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> else there would be a lot of code repetition and possibly udefined behavior
> at the driver layer.
>
Why should they be included? At least in this stage, there is no code to share
ethdev why should we add code for the vdev?
I agree that if we see that there is shared code, we should think about creating
those functions.
> And also why take a different path than the rest of the rte subsystems?
>
Even now if you look at the reference code you will see that there is really minimum shared code.
also this result in that the PMD is not free to allocate resource in the order he needs.
My thinking is that if there are only 2 lines of shared code I prefer to let the PMD handle it.
I know that sharing code should be the first option, but this also makes the PMD developer unaware what is going on.
and lose some control. For example if the PMD programmer wants to create hybrid PMD net +
regex for example, then either he will be forced to do some hacks or just by pass the function
so when this function will be updated his code will break. So I prefer if it is very short code
and this code can be developed in different ways to leave it to the PMD.
I suggest that if needed we will add such function. Is that O.K by you?
> >
> >> >This commit introduce the API that is needed by the RegEx devices in
> >> >order to work with the RegEX lib.
> >> >
> >> >During the probe of a RegEx device, the device should configure
> >itself,
> >> >and allocate the resources it requires.
> >> >On completion of the device init, it should call the
> >> >rte_regex_dev_register in order to register itself as a RegEx device.
> >> >
> >> >Signed-off-by: Ori Kam <orika@mellanox.com>
> >> >Signed-off-by: Parav Pandit <parav@mellanox.com>
> >> >---
> >> > config/common_base | 3 +-
> >> > config/meson.build | 1 +
> >> > lib/librte_regexdev/Makefile | 1 +
> >> > lib/librte_regexdev/meson.build | 5 ++-
> >> > lib/librte_regexdev/rte_regexdev.c | 74
> >> >++++++++++++++++++++++++++++++-
> >> > lib/librte_regexdev/rte_regexdev.h | 7 +++
> >> > lib/librte_regexdev/rte_regexdev_core.h | 2 +
> >> > lib/librte_regexdev/rte_regexdev_driver.h | 50
> >> >+++++++++++++++++++++
> >> > meson_options.txt | 2 +
> >> > 9 files changed, 142 insertions(+), 3 deletions(-)
> >> > create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
> >> >
> >>
> >> <snip>
Hi Ori,
>Hi Pavan,
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
>Bhagavatula
>> Sent: Sunday, April 5, 2020 8:11 PM
>> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>; xiang.w.wang@intel.com
>> Cc: 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; Thomas Monjalon
>> <thomas@monjalon.net>; Parav Pandit <parav@mellanox.com>
>> Subject: Re: [dpdk-dev] [EXT] [PATCH v1 3/4] regexdev: add regexdev
>core
>> functions
>>
>> Hi Ori,
>>
>> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan
>Nikhilesh
>> >Bhagavatula
>> >>
>> >> Looks like this implementation is incomplete?
>> >> I don't see any pmd specific helper functions for @see
>> >rte_cryptodev_pmd.c,
>> >> rte_eventdev_pmd*
>> >>
>> >I think the current implementation includes all needed functions,
>> >at least for the first stage.
>> >You can find in rte_regexdev_driver.h the functions that should be
>> >called
>> >by the PMD. We have the register / unregister which acts the same
>as
>> >create
>> >and destroy. For parsing argument the PMD may call
>rte_kvargs_parse.
>> >
>>
>> _driver.h should atleast include
>> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
>> else there would be a lot of code repetition and possibly udefined
>behavior
>> at the driver layer.
>>
>Why should they be included? At least in this stage, there is no code to
>share
>ethdev why should we add code for the vdev?
Ok I think I failed to communicate my concerns across.
Let me retry
1. SW based regex devices such as PCRE/Hyperscan rely on vdev framework
i.e. user needs to pass an EAL argument --vdev="regex_pcre" for the driver to
initialize all the other EAL subsystems (ethdev, eventdev, cryptodev, etc..)support this.
2. HW based independent regex devices that are exposed as PCI devices would need
pci probe helpers.
>I agree that if we see that there is shared code, we should think about
>creating
>those functions.
>
>> And also why take a different path than the rest of the rte
>subsystems?
>>
>
>Even now if you look at the reference code you will see that there is
>really minimum shared code.
>also this result in that the PMD is not free to allocate resource in the
>order he needs.
>My thinking is that if there are only 2 lines of shared code I prefer to let
>the PMD handle it.
>I know that sharing code should be the first option, but this also makes
>the PMD developer unaware what is going on.
>and lose some control. For example if the PMD programmer wants to
>create hybrid PMD net +
>regex for example, then either he will be forced to do some hacks or
>just by pass the function
>so when this function will be updated his code will break.
Shouldn't the application/end user make that decision rather than PMD programmer?.
If application wants to connect net to regex it should be made possible to do it via rte_flow.
As an example if we see event device spec. It has robust features to connect multiple
subsystems(ethernet/crypto/timer) to event device and it is controlled from RTE layer.
PMD layer should act on the inputs from RTE layer rather than action on it own.
Thoughts?
Thanks,
Pavan.
So I prefer if it
>is very short code
>and this code can be developed in different ways to leave it to the
>PMD.
>I suggest that if needed we will add such function. Is that O.K by you?
>
>> >
>> >> >This commit introduce the API that is needed by the RegEx
>devices in
>> >> >order to work with the RegEX lib.
>> >> >
>> >> >During the probe of a RegEx device, the device should configure
>> >itself,
>> >> >and allocate the resources it requires.
>> >> >On completion of the device init, it should call the
>> >> >rte_regex_dev_register in order to register itself as a RegEx
>device.
>> >> >
>> >> >Signed-off-by: Ori Kam <orika@mellanox.com>
>> >> >Signed-off-by: Parav Pandit <parav@mellanox.com>
>> >> >---
>> >> > config/common_base | 3 +-
>> >> > config/meson.build | 1 +
>> >> > lib/librte_regexdev/Makefile | 1 +
>> >> > lib/librte_regexdev/meson.build | 5 ++-
>> >> > lib/librte_regexdev/rte_regexdev.c | 74
>> >> >++++++++++++++++++++++++++++++-
>> >> > lib/librte_regexdev/rte_regexdev.h | 7 +++
>> >> > lib/librte_regexdev/rte_regexdev_core.h | 2 +
>> >> > lib/librte_regexdev/rte_regexdev_driver.h | 50
>> >> >+++++++++++++++++++++
>> >> > meson_options.txt | 2 +
>> >> > 9 files changed, 142 insertions(+), 3 deletions(-)
>> >> > create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
>> >> >
>> >>
>> >> <snip>
06/04/2020 14:48, Pavan Nikhilesh Bhagavatula:
> > From: Pavan Nikhilesh Bhagavatula
> >> >> From: Pavan Nikhilesh Bhagavatula
> >> >>
> >> >> Looks like this implementation is incomplete?
> >> >> I don't see any pmd specific helper functions for @see
> >> >rte_cryptodev_pmd.c,
> >> >> rte_eventdev_pmd*
> >> >>
> >> >I think the current implementation includes all needed functions,
> >> >at least for the first stage.
> >> >You can find in rte_regexdev_driver.h the functions that should be
> >> >called
> >> >by the PMD. We have the register / unregister which acts the same
> >as
> >> >create
> >> >and destroy. For parsing argument the PMD may call
> >rte_kvargs_parse.
> >> >
> >>
> >> _driver.h should atleast include
> >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> >> else there would be a lot of code repetition and possibly udefined
> >behavior
> >> at the driver layer.
> >>
> >Why should they be included? At least in this stage, there is no code to
> >share
> >ethdev why should we add code for the vdev?
>
> Ok I think I failed to communicate my concerns across.
> Let me retry
>
> 1. SW based regex devices such as PCRE/Hyperscan rely on vdev framework
> i.e. user needs to pass an EAL argument --vdev="regex_pcre" for the driver to
> initialize all the other EAL subsystems (ethdev, eventdev, cryptodev, etc..)support this.
vdev helpers do not have to be part of the first patches which define API.
It should be added when adding the first vdev driver.
> 2. HW based independent regex devices that are exposed as PCI devices would need
> pci probe helpers.
Same, PCI helpers can be added while adding the first PCI driver.
We can synchronize about how to split the work, avoiding two developers
doing the same thing. But let's not mandate this work to be done
as part of this first series.
On Mon, Apr 6, 2020 at 7:00 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 06/04/2020 14:48, Pavan Nikhilesh Bhagavatula:
> > > From: Pavan Nikhilesh Bhagavatula
> > >> >> From: Pavan Nikhilesh Bhagavatula
> > >> >>
> > >> >> Looks like this implementation is incomplete?
> > >> >> I don't see any pmd specific helper functions for @see
> > >> >rte_cryptodev_pmd.c,
> > >> >> rte_eventdev_pmd*
> > >> >>
> > >> >I think the current implementation includes all needed functions,
> > >> >at least for the first stage.
> > >> >You can find in rte_regexdev_driver.h the functions that should be
> > >> >called
> > >> >by the PMD. We have the register / unregister which acts the same
> > >as
> > >> >create
> > >> >and destroy. For parsing argument the PMD may call
> > >rte_kvargs_parse.
> > >> >
> > >>
> > >> _driver.h should atleast include
> > >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> > >> else there would be a lot of code repetition and possibly udefined
> > >behavior
> > >> at the driver layer.
> > >>
> > >Why should they be included? At least in this stage, there is no code to
> > >share
> > >ethdev why should we add code for the vdev?
> >
> > Ok I think I failed to communicate my concerns across.
> > Let me retry
> >
> > 1. SW based regex devices such as PCRE/Hyperscan rely on vdev framework
> > i.e. user needs to pass an EAL argument --vdev="regex_pcre" for the driver to
> > initialize all the other EAL subsystems (ethdev, eventdev, cryptodev, etc..)support this.
>
> vdev helpers do not have to be part of the first patches which define API.
> It should be added when adding the first vdev driver.
Yes. When we add vdev, we should not rewrite again to support vdev.
So care must taken for proper implementation in the first place avoid rework.
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,
>
> > 2. HW based independent regex devices that are exposed as PCI devices would need
> > pci probe helpers.
>
> Same, PCI helpers can be added while adding the first PCI driver.
>
> We can synchronize about how to split the work, avoiding two developers
> doing the same thing. But let's not mandate this work to be done
> as part of this first series.
>
>
>
Hi
Best,
Ori
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, April 6, 2020 4:39 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Ori Kam <orika@mellanox.com>; 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
>
> On Mon, Apr 6, 2020 at 7:00 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> >
> > 06/04/2020 14:48, Pavan Nikhilesh Bhagavatula:
> > > > From: Pavan Nikhilesh Bhagavatula
> > > >> >> From: Pavan Nikhilesh Bhagavatula
> > > >> >>
> > > >> >> Looks like this implementation is incomplete?
> > > >> >> I don't see any pmd specific helper functions for @see
> > > >> >rte_cryptodev_pmd.c,
> > > >> >> rte_eventdev_pmd*
> > > >> >>
> > > >> >I think the current implementation includes all needed functions,
> > > >> >at least for the first stage.
> > > >> >You can find in rte_regexdev_driver.h the functions that should be
> > > >> >called
> > > >> >by the PMD. We have the register / unregister which acts the same
> > > >as
> > > >> >create
> > > >> >and destroy. For parsing argument the PMD may call
> > > >rte_kvargs_parse.
> > > >> >
> > > >>
> > > >> _driver.h should atleast include
> > > >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> > > >> else there would be a lot of code repetition and possibly udefined
> > > >behavior
> > > >> at the driver layer.
> > > >>
> > > >Why should they be included? At least in this stage, there is no code to
> > > >share
> > > >ethdev why should we add code for the vdev?
> > >
> > > Ok I think I failed to communicate my concerns across.
> > > Let me retry
> > >
> > > 1. SW based regex devices such as PCRE/Hyperscan rely on vdev framework
> > > i.e. user needs to pass an EAL argument --vdev="regex_pcre" for the driver
> to
> > > initialize all the other EAL subsystems (ethdev, eventdev, cryptodev,
> etc..)support this.
> >
> > vdev helpers do not have to be part of the first patches which define API.
> > It should be added when adding the first vdev driver.
>
> Yes. When we add vdev, we should not rewrite again to support vdev.
> So care must taken for proper implementation in the first place avoid rework.
>
> 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.
>
> >
> > > 2. HW based independent regex devices that are exposed as PCI devices
> would need
> > > pci probe helpers.
> >
> > Same, PCI helpers can be added while adding the first PCI driver.
> >
> > We can synchronize about how to split the work, avoiding two developers
> > doing the same thing. But let's not mandate this work to be done
> > as part of this first series.
> >
> >
> >
> >
> > 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.
> -----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.
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;
}
>
07/04/2020 07:49, Jerin Jacob:
> > >
> > > 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,
No, ethdev has more features and is evolving as a better API.
> My request is to take cryptodev and
> eventdev as a base
> change to accordingly.
When writing a new API, we should learn from past lessons,
including eventdev and cryptodev, but also all the things evolving
in ethdev.
> 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.
Again, leveraging doesn't mean to implement all in one patch.
Instead of asking for adding more, please focus on what must be changed
in what is proposed in this series.
On Tue, Apr 7, 2020 at 5:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 07/04/2020 07:49, Jerin Jacob:
> > > >
> > > > 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,
>
> No, ethdev has more features and is evolving as a better API.
I meant the same, where ethdev filled with "ethdev specific" features,
better take another subsystem for devices
specific thing.
>
> > My request is to take cryptodev and
> > eventdev as a base
> > change to accordingly.
>
> When writing a new API, we should learn from past lessons,
> including eventdev and cryptodev, but also all the things evolving
> in ethdev.
It looks out of context.
>
> > 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.
>
> Again, leveraging doesn't mean to implement all in one patch.
> Instead of asking for adding more, please focus on what must be changed
> in what is proposed in this series.
Look at the configure function[1], Does it take care of reconfiguration?
Common code takes care of allocating memory for point for queue
objects. Where is that?
http://patches.dpdk.org/patch/67311/
I will put in other way out, We know how other devices works in DPDK,
if we are changing
the means then please mention in the cover letter for the proposed
design and why would
think, the new solution is better.
Regexdev specification(patch 1/4) is not different than other devices'
fundamental view.
In my understanding, we don't put any effort to understand, why other
subsystems were chosen the specific
implementation in the common code. For me, it worth spending my time
on the review once basic stuff is there.
int
rte_regexdev_configure(uint8_t dev_id, const struct rte_regexdev_config *cfg)
{
if (dev_id >= RTE_MAX_REGEXDEV_DEVS)
return -EINVAL;
if (regex_devices[dev_id] == NULL)
return -EINVAL;
if (cfg == NULL)
return -EINVAL;
if (regex_devices[dev_id]->dev_ops->dev_configure == NULL)
return -ENOTSUP;
return regex_devices[dev_id]->dev_ops->dev_configure
(regex_devices[dev_id], cfg);
}
>
>
Hi Ori,
>+int
>+rte_regexdev_register(struct rte_regexdev *dev)
>+{
>+ uint16_t dev_id;
>+ int res;
>+
>+ if (dev->dev_ops == NULL) {
>+ RTE_REGEXDEV_LOG(ERR, "RegEx device invalid device ops\n");
>+ return -EINVAL;
>+ }
>+ if (regexdev_allocated(dev->dev_name) != NULL) {
>+ RTE_REGEXDEV_LOG
>+ (ERR, "RegEx device with name %s already allocated\n",
>+ dev->dev_name);
>+ return -ENOMEM;
>+ }
>+ dev_id = regexdev_find_free_dev();
>+ if (dev_id == RTE_MAX_REGEXDEV_DEVS) {
>+ RTE_REGEXDEV_LOG
>+ (ERR, "Reached maximum number of regex devs\n");
>+ return -ENOMEM;
>+ }
>+ dev->dev_id = dev_id;
dev is of type struct rte_regexdev *, but I don't see in the definition of struct rte_regexdev a field dev_id
>+ regex_devices[dev_id] = dev;
>+ res = dev_id;
>+ return res;
>+}
Hi Guy,
Thanks for your review
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> Sent: Tuesday, April 7, 2020 5:22 PM
> To: Ori Kam <orika@mellanox.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Cc: 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
>
> Hi Ori,
>
> >+int
> >+rte_regexdev_register(struct rte_regexdev *dev)
> >+{
> >+ uint16_t dev_id;
> >+ int res;
> >+
> >+ if (dev->dev_ops == NULL) {
> >+ RTE_REGEXDEV_LOG(ERR, "RegEx device invalid device
> ops\n");
> >+ return -EINVAL;
> >+ }
> >+ if (regexdev_allocated(dev->dev_name) != NULL) {
> >+ RTE_REGEXDEV_LOG
> >+ (ERR, "RegEx device with name %s already
> allocated\n",
> >+ dev->dev_name);
> >+ return -ENOMEM;
> >+ }
> >+ dev_id = regexdev_find_free_dev();
> >+ if (dev_id == RTE_MAX_REGEXDEV_DEVS) {
> >+ RTE_REGEXDEV_LOG
> >+ (ERR, "Reached maximum number of regex devs\n");
> >+ return -ENOMEM;
> >+ }
> >+ dev->dev_id = dev_id;
>
> dev is of type struct rte_regexdev *, but I don't see in the definition of struct
> rte_regexdev a field dev_id
>
The definition of rte_regexdev appears in rte_regecdev_core.h
and the last member in this struct is the dev_id. (line146) it is added in this
commit.
> >+ regex_devices[dev_id] = dev;
> >+ res = dev_id;
> >+ return res;
> >+}
> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Tuesday, April 07, 2020 7:29 PM
> To: Guy Kaneti <guyk@marvell.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Cc: 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
>
> Hi Guy,
> Thanks for your review
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> > Sent: Tuesday, April 7, 2020 5:22 PM
> > To: Ori Kam <orika@mellanox.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> > Thomas Monjalon <thomas@monjalon.net>
> > Cc: 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
> >
> > Hi Ori,
> >
> > >+int
> > >+rte_regexdev_register(struct rte_regexdev *dev) {
> > >+ uint16_t dev_id;
> > >+ int res;
> > >+
> > >+ if (dev->dev_ops == NULL) {
> > >+ RTE_REGEXDEV_LOG(ERR, "RegEx device invalid device
> > ops\n");
> > >+ return -EINVAL;
> > >+ }
> > >+ if (regexdev_allocated(dev->dev_name) != NULL) {
> > >+ RTE_REGEXDEV_LOG
> > >+ (ERR, "RegEx device with name %s already
> > allocated\n",
> > >+ dev->dev_name);
> > >+ return -ENOMEM;
> > >+ }
> > >+ dev_id = regexdev_find_free_dev();
> > >+ if (dev_id == RTE_MAX_REGEXDEV_DEVS) {
> > >+ RTE_REGEXDEV_LOG
> > >+ (ERR, "Reached maximum number of regex devs\n");
> > >+ return -ENOMEM;
> > >+ }
> > >+ dev->dev_id = dev_id;
> >
> > dev is of type struct rte_regexdev *, but I don't see in the
> > definition of struct rte_regexdev a field dev_id
> >
>
> The definition of rte_regexdev appears in rte_regecdev_core.h and the last
> member in this struct is the dev_id. (line146) it is added in this commit.
>
I was looking for it in [v1,2/4] regexdev: add regex core h file
https://patches.dpdk.org/patch/67309/
> > >+ regex_devices[dev_id] = dev;
> > >+ res = dev_id;
> > >+ return res;
> > >+}
Hi Guy,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> Sent: Tuesday, April 7, 2020 7:37 PM
> To: Ori Kam <orika@mellanox.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Cc: 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
>
>
>
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Tuesday, April 07, 2020 7:29 PM
> > To: Guy Kaneti <guyk@marvell.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> > Thomas Monjalon <thomas@monjalon.net>
> > Cc: 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
> >
> > Hi Guy,
> > Thanks for your review
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> > > Sent: Tuesday, April 7, 2020 5:22 PM
> > > To: Ori Kam <orika@mellanox.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> > > Thomas Monjalon <thomas@monjalon.net>
> > > Cc: 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
> > >
> > > Hi Ori,
> > >
> > > >+int
> > > >+rte_regexdev_register(struct rte_regexdev *dev) {
> > > >+ uint16_t dev_id;
> > > >+ int res;
> > > >+
> > > >+ if (dev->dev_ops == NULL) {
> > > >+ RTE_REGEXDEV_LOG(ERR, "RegEx device invalid device
> > > ops\n");
> > > >+ return -EINVAL;
> > > >+ }
> > > >+ if (regexdev_allocated(dev->dev_name) != NULL) {
> > > >+ RTE_REGEXDEV_LOG
> > > >+ (ERR, "RegEx device with name %s already
> > > allocated\n",
> > > >+ dev->dev_name);
> > > >+ return -ENOMEM;
> > > >+ }
> > > >+ dev_id = regexdev_find_free_dev();
> > > >+ if (dev_id == RTE_MAX_REGEXDEV_DEVS) {
> > > >+ RTE_REGEXDEV_LOG
> > > >+ (ERR, "Reached maximum number of regex devs\n");
> > > >+ return -ENOMEM;
> > > >+ }
> > > >+ dev->dev_id = dev_id;
> > >
> > > dev is of type struct rte_regexdev *, but I don't see in the
> > > definition of struct rte_regexdev a field dev_id
> > >
> >
> > The definition of rte_regexdev appears in rte_regecdev_core.h and the last
> > member in this struct is the dev_id. (line146) it is added in this commit.
> >
>
> I was looking for it in [v1,2/4] regexdev: add regex core h file
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.d
> pdk.org%2Fpatch%2F67309%2F&data=02%7C01%7Corika%40mellanox.co
> m%7C5a297f12b59e49eb078e08d7db11fc2c%7Ca652971c7d2e4d9ba6a4d1492
> 56f461b%7C0%7C0%7C637218742604895700&sdata=6jJU9mMnmjf7fcMb
> rV00QMG8qcHt%2BfuPULHsUSbwTeY%3D&reserved=0
>
No problem, I added it when I had use for it.
It would be great if you can add your ack.
> > > >+ regex_devices[dev_id] = dev;
> > > >+ res = dev_id;
> > > >+ return res;
> > > >+}
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Monday, April 6, 2020 4:30 PM
> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; xiang.w.wang@intel.com; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>
> Cc: 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
>
> 06/04/2020 14:48, Pavan Nikhilesh Bhagavatula:
> > > From: Pavan Nikhilesh Bhagavatula
> > >> >> From: Pavan Nikhilesh Bhagavatula
> > >> >>
> > >> >> Looks like this implementation is incomplete?
> > >> >> I don't see any pmd specific helper functions for @see
> > >> >rte_cryptodev_pmd.c,
> > >> >> rte_eventdev_pmd*
> > >> >>
> > >> >I think the current implementation includes all needed functions,
> > >> >at least for the first stage.
> > >> >You can find in rte_regexdev_driver.h the functions that should be
> > >> >called
> > >> >by the PMD. We have the register / unregister which acts the same
> > >as
> > >> >create
> > >> >and destroy. For parsing argument the PMD may call
> > >rte_kvargs_parse.
> > >> >
> > >>
> > >> _driver.h should atleast include
> > >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> > >> else there would be a lot of code repetition and possibly udefined
> > >behavior
> > >> at the driver layer.
> > >>
> > >Why should they be included? At least in this stage, there is no code to
> > >share
> > >ethdev why should we add code for the vdev?
> >
> > Ok I think I failed to communicate my concerns across.
> > Let me retry
> >
> > 1. SW based regex devices such as PCRE/Hyperscan rely on vdev framework
> > i.e. user needs to pass an EAL argument --vdev="regex_pcre" for the driver to
> > initialize all the other EAL subsystems (ethdev, eventdev, cryptodev,
> etc..)support this.
>
> vdev helpers do not have to be part of the first patches which define API.
> It should be added when adding the first vdev driver.
>
> > 2. HW based independent regex devices that are exposed as PCI devices
> would need
> > pci probe helpers.
>
> Same, PCI helpers can be added while adding the first PCI driver.
>
> We can synchronize about how to split the work, avoiding two developers
> doing the same thing. But let's not mandate this work to be done
> as part of this first series.
>
>
I agree with Thomas, let's discuss this when adding the PMDs.
Hi Pavan
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh Bhagavatula
> Sent: Monday, April 6, 2020 3:48 PM
>
> Hi Ori,
>
> >Hi Pavan,
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan Nikhilesh
> >Bhagavatula
> >> Sent: Sunday, April 5, 2020 8:11 PM
> >> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> >> <jerinj@marvell.com>; xiang.w.wang@intel.com
> >> Cc: 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; Thomas Monjalon
> >> <thomas@monjalon.net>; Parav Pandit <parav@mellanox.com>
> >> Subject: Re: [dpdk-dev] [EXT] [PATCH v1 3/4] regexdev: add regexdev
> >core
> >> functions
> >>
> >> Hi Ori,
> >>
> >> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Pavan
> >Nikhilesh
> >> >Bhagavatula
> >> >>
> >> >> Looks like this implementation is incomplete?
> >> >> I don't see any pmd specific helper functions for @see
> >> >rte_cryptodev_pmd.c,
> >> >> rte_eventdev_pmd*
> >> >>
> >> >I think the current implementation includes all needed functions,
> >> >at least for the first stage.
> >> >You can find in rte_regexdev_driver.h the functions that should be
> >> >called
> >> >by the PMD. We have the register / unregister which acts the same
> >as
> >> >create
> >> >and destroy. For parsing argument the PMD may call
> >rte_kvargs_parse.
> >> >
> >>
> >> _driver.h should atleast include
> >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> >> else there would be a lot of code repetition and possibly udefined
> >behavior
> >> at the driver layer.
> >>
> >Why should they be included? At least in this stage, there is no code to
> >share
> >ethdev why should we add code for the vdev?
>
> Ok I think I failed to communicate my concerns across.
> Let me retry
>
> 1. SW based regex devices such as PCRE/Hyperscan rely on vdev framework
> i.e. user needs to pass an EAL argument --vdev="regex_pcre" for the driver to
> initialize all the other EAL subsystems (ethdev, eventdev, cryptodev,
> etc..)support this.
>
> 2. HW based independent regex devices that are exposed as PCI devices would
> need
> pci probe helpers.
>
>
Like mentions in other treads lets leave it as is for now,
And based on PMD implementation add the required functions.
> >I agree that if we see that there is shared code, we should think about
> >creating
> >those functions.
> >
> >> And also why take a different path than the rest of the rte
> >subsystems?
> >>
> >
> >Even now if you look at the reference code you will see that there is
> >really minimum shared code.
> >also this result in that the PMD is not free to allocate resource in the
> >order he needs.
> >My thinking is that if there are only 2 lines of shared code I prefer to let
> >the PMD handle it.
> >I know that sharing code should be the first option, but this also makes
> >the PMD developer unaware what is going on.
> >and lose some control. For example if the PMD programmer wants to
> >create hybrid PMD net +
> >regex for example, then either he will be forced to do some hacks or
> >just by pass the function
> >so when this function will be updated his code will break.
>
> Shouldn't the application/end user make that decision rather than PMD
> programmer?.
> If application wants to connect net to regex it should be made possible to do it
> via rte_flow.
>
In this case there will be I guess new rte_flow command, and there will be no
no use for the enqueue/deques or there will not be use for rx_burst / tx_burst
This is a story for another day.
In any case the PMD should be as flexiable as possible. This is why I think that
the most code should be in the PMD since it is PMD implemetion and if we add
common code in rte level this can make the PMD less flexable. For example how
to share resource between net / regex device. For example even in current
stage the application can receive packets from the net device and send those
packets to the regex device, if the application uses both devices from the same
company they can share information that can save time, for example memory
registration values.
> As an example if we see event device spec. It has robust features to connect
> multiple
> subsystems(ethernet/crypto/timer) to event device and it is controlled from
> RTE layer.
> PMD layer should act on the inputs from RTE layer rather than action on it own.
>
> Thoughts?
> Thanks,
> Pavan.
>
Like I said above until we support inline regex, the application
moves the packets just like it moves them between net devices,
so each PMD should have the ability to configure itself as best as it can.
Again I totally in favor that if we see common code we should
move it to rte level. But this should be done only after we have a code
and in any case this doesn't effect the API.
> So I prefer if it
> >is very short code
> >and this code can be developed in different ways to leave it to the
> >PMD.
> >I suggest that if needed we will add such function. Is that O.K by you?
> >
> >> >
> >> >> >This commit introduce the API that is needed by the RegEx
> >devices in
> >> >> >order to work with the RegEX lib.
> >> >> >
> >> >> >During the probe of a RegEx device, the device should configure
> >> >itself,
> >> >> >and allocate the resources it requires.
> >> >> >On completion of the device init, it should call the
> >> >> >rte_regex_dev_register in order to register itself as a RegEx
> >device.
> >> >> >
> >> >> >Signed-off-by: Ori Kam <orika@mellanox.com>
> >> >> >Signed-off-by: Parav Pandit <parav@mellanox.com>
> >> >> >---
> >> >> > config/common_base | 3 +-
> >> >> > config/meson.build | 1 +
> >> >> > lib/librte_regexdev/Makefile | 1 +
> >> >> > lib/librte_regexdev/meson.build | 5 ++-
> >> >> > lib/librte_regexdev/rte_regexdev.c | 74
> >> >> >++++++++++++++++++++++++++++++-
> >> >> > lib/librte_regexdev/rte_regexdev.h | 7 +++
> >> >> > lib/librte_regexdev/rte_regexdev_core.h | 2 +
> >> >> > lib/librte_regexdev/rte_regexdev_driver.h | 50
> >> >> >+++++++++++++++++++++
> >> >> > meson_options.txt | 2 +
> >> >> > 9 files changed, 142 insertions(+), 3 deletions(-)
> >> >> > create mode 100644 lib/librte_regexdev/rte_regexdev_driver.h
> >> >> >
> >> >>
> >> >> <snip>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ori Kam
> Sent: Wednesday, April 08, 2020 11:40 AM
> To: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; xiang.w.wang@intel.com; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>
> Cc: 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
>
>
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> > Sent: Monday, April 6, 2020 4:30 PM
> > To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; xiang.w.wang@intel.com; Pavan Nikhilesh
> > Bhagavatula <pbhagavatula@marvell.com>
> > Cc: 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
> >
> > 06/04/2020 14:48, Pavan Nikhilesh Bhagavatula:
> > > > From: Pavan Nikhilesh Bhagavatula
> > > >> >> From: Pavan Nikhilesh Bhagavatula
> > > >> >>
> > > >> >> Looks like this implementation is incomplete?
> > > >> >> I don't see any pmd specific helper functions for @see
> > > >> >rte_cryptodev_pmd.c,
> > > >> >> rte_eventdev_pmd*
> > > >> >>
> > > >> >I think the current implementation includes all needed
> > > >> >functions, at least for the first stage.
> > > >> >You can find in rte_regexdev_driver.h the functions that should
> > > >> >be called by the PMD. We have the register / unregister which
> > > >> >acts the same
> > > >as
> > > >> >create
> > > >> >and destroy. For parsing argument the PMD may call
> > > >rte_kvargs_parse.
> > > >> >
> > > >>
> > > >> _driver.h should atleast include
> > > >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> > > >> else there would be a lot of code repetition and possibly
> > > >> udefined
> > > >behavior
> > > >> at the driver layer.
> > > >>
> > > >Why should they be included? At least in this stage, there is no
> > > >code to share ethdev why should we add code for the vdev?
> > >
> > > Ok I think I failed to communicate my concerns across.
> > > Let me retry
> > >
> > > 1. SW based regex devices such as PCRE/Hyperscan rely on vdev
> > > framework i.e. user needs to pass an EAL argument
> > > --vdev="regex_pcre" for the driver to initialize all the other EAL
> > > subsystems (ethdev, eventdev, cryptodev,
> > etc..)support this.
> >
> > vdev helpers do not have to be part of the first patches which define API.
> > It should be added when adding the first vdev driver.
> >
> > > 2. HW based independent regex devices that are exposed as PCI
> > > devices
> > would need
> > > pci probe helpers.
> >
> > Same, PCI helpers can be added while adding the first PCI driver.
> >
> > We can synchronize about how to split the work, avoiding two
> > developers doing the same thing. But let's not mandate this work to be
> > done as part of this first series.
> >
> >
> I agree with Thomas, let's discuss this when adding the PMDs.
I am writing a now the Marvell OcteonTx2 PMD and I think that a PCI helper is missing.
When registering a PCI device, a .remove function is registered of type pci_remove_t *remove;
typedef int (pci_remove_t)(struct rte_pci_device *);
because this function receives struct rte_pci_device * as an argument
It is required to have a helper function to retrieve struct rte_regexdev *dev based on the device name (or another way).
similar to rte_cryptodev_pmd_get_named_dev(const char *name)
Hi Guy,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> Sent: Sunday, April 19, 2020 1:39 PM
> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> xiang.w.wang@intel.com; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>
> Cc: 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
>
>
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Ori Kam
> > Sent: Wednesday, April 08, 2020 11:40 AM
> > To: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; xiang.w.wang@intel.com; Pavan Nikhilesh
> > Bhagavatula <pbhagavatula@marvell.com>
> > Cc: 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
> >
> >
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> > > Sent: Monday, April 6, 2020 4:30 PM
> > > To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> > > <jerinj@marvell.com>; xiang.w.wang@intel.com; Pavan Nikhilesh
> > > Bhagavatula <pbhagavatula@marvell.com>
> > > Cc: 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
> > >
> > > 06/04/2020 14:48, Pavan Nikhilesh Bhagavatula:
> > > > > From: Pavan Nikhilesh Bhagavatula
> > > > >> >> From: Pavan Nikhilesh Bhagavatula
> > > > >> >>
> > > > >> >> Looks like this implementation is incomplete?
> > > > >> >> I don't see any pmd specific helper functions for @see
> > > > >> >rte_cryptodev_pmd.c,
> > > > >> >> rte_eventdev_pmd*
> > > > >> >>
> > > > >> >I think the current implementation includes all needed
> > > > >> >functions, at least for the first stage.
> > > > >> >You can find in rte_regexdev_driver.h the functions that should
> > > > >> >be called by the PMD. We have the register / unregister which
> > > > >> >acts the same
> > > > >as
> > > > >> >create
> > > > >> >and destroy. For parsing argument the PMD may call
> > > > >rte_kvargs_parse.
> > > > >> >
> > > > >>
> > > > >> _driver.h should atleast include
> > > > >> rte_regex_dev_pci_generic_probe/rte_regex_pmd_vdev_init
> > > > >> else there would be a lot of code repetition and possibly
> > > > >> udefined
> > > > >behavior
> > > > >> at the driver layer.
> > > > >>
> > > > >Why should they be included? At least in this stage, there is no
> > > > >code to share ethdev why should we add code for the vdev?
> > > >
> > > > Ok I think I failed to communicate my concerns across.
> > > > Let me retry
> > > >
> > > > 1. SW based regex devices such as PCRE/Hyperscan rely on vdev
> > > > framework i.e. user needs to pass an EAL argument
> > > > --vdev="regex_pcre" for the driver to initialize all the other EAL
> > > > subsystems (ethdev, eventdev, cryptodev,
> > > etc..)support this.
> > >
> > > vdev helpers do not have to be part of the first patches which define API.
> > > It should be added when adding the first vdev driver.
> > >
> > > > 2. HW based independent regex devices that are exposed as PCI
> > > > devices
> > > would need
> > > > pci probe helpers.
> > >
> > > Same, PCI helpers can be added while adding the first PCI driver.
> > >
> > > We can synchronize about how to split the work, avoiding two
> > > developers doing the same thing. But let's not mandate this work to be
> > > done as part of this first series.
> > >
> > >
> > I agree with Thomas, let's discuss this when adding the PMDs.
>
> I am writing a now the Marvell OcteonTx2 PMD and I think that a PCI helper is
> missing.
> When registering a PCI device, a .remove function is registered of type
> pci_remove_t *remove;
>
> typedef int (pci_remove_t)(struct rte_pci_device *);
> because this function receives struct rte_pci_device * as an argument
> It is required to have a helper function to retrieve struct rte_regexdev *dev
> based on the device name (or another way).
> similar to rte_cryptodev_pmd_get_named_dev(const char *name)i
Will add.
Thanks,
Ori
@@ -819,10 +819,11 @@ CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV=y
CONFIG_RTE_LIBRTE_PMD_NTB_RAWDEV=y
#
-# Compile regex device support
+# Compile RexEx device support
#
CONFIG_RTE_LIBRTE_REGEXDEV=y
CONFIG_RTE_LIBRTE_REGEXDEV_DEBUG=n
+CONFIG_RTE_MAX_REGEXDEV_DEVS=32
#
# Compile librte_ring
@@ -221,6 +221,7 @@ endforeach
dpdk_conf.set('RTE_MAX_LCORE', get_option('max_lcores'))
dpdk_conf.set('RTE_MAX_NUMA_NODES', get_option('max_numa_nodes'))
dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports'))
+dpdk_conf.set('RTE_MAX_REGEXDEV_DEVS', get_option('max_regexdev_devs'))
dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
# values which have defaults which may be overridden
dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
@@ -25,6 +25,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_REGEXDEV) := rte_regexdev.c
# export include files
SYMLINK-$(CONFIG_RTE_LIBRTE_REGEXDEV)-include += rte_regexdev.h
SYMLINK-$(CONFIG_RTE_LIBRTE_REGEXDEV)-include += rte_regexdev_core.h
+SYMLINK-$(CONFIG_RTE_LIBRTE_REGEXDEV)-include += rte_regexdev_driver.h
# versioning export map
EXPORT_MAP := rte_regexdev_version.map
@@ -1,7 +1,10 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright(c) 2020 Mellanox Corporation
+name = 'regexdev'
allow_experimental_apis = true
sources = files('rte_regexdev.c')
-headers = files('rte_regexdev.h', 'rte_regexdev_core.h')
+headers = files('rte_regexdev.h',
+ 'rte_regexdev_core.h',
+ 'rte_regexdev_driver.h')
deps += ['mbuf']
@@ -3,4 +3,76 @@
* Copyright(C) 2020 Mellanox International Ltd.
*/
-#include <rte_regexdev.h>
+#include <string.h>
+
+#include <rte_spinlock.h>
+#include <rte_memory.h>
+#include <rte_memcpy.h>
+#include <rte_memzone.h>
+#include <rte_string_fns.h>
+
+#include "rte_regexdev.h"
+#include "rte_regexdev_driver.h"
+
+static struct rte_regexdev *regex_devices[RTE_MAX_REGEXDEV_DEVS];
+
+int rte_regexdev_logtype;
+
+static uint16_t
+regexdev_find_free_dev(void)
+{
+ uint16_t i;
+
+ for (i = 0; i < RTE_MAX_REGEXDEV_DEVS; i++) {
+ if (regex_devices[i] == NULL)
+ return i;
+ }
+ return RTE_MAX_REGEXDEV_DEVS;
+}
+
+static const struct rte_regexdev*
+regexdev_allocated(const char *name)
+{
+ uint16_t i;
+
+ for (i = 0; i < RTE_MAX_REGEXDEV_DEVS; i++) {
+ if (regex_devices[i] != NULL)
+ if (!strcmp(name, regex_devices[i]->dev_name))
+ return regex_devices[i];
+ }
+ return NULL;
+}
+
+int
+rte_regexdev_register(struct rte_regexdev *dev)
+{
+ uint16_t dev_id;
+ int res;
+
+ if (dev->dev_ops == NULL) {
+ RTE_REGEXDEV_LOG(ERR, "RegEx device invalid device ops\n");
+ return -EINVAL;
+ }
+ if (regexdev_allocated(dev->dev_name) != NULL) {
+ RTE_REGEXDEV_LOG
+ (ERR, "RegEx device with name %s already allocated\n",
+ dev->dev_name);
+ return -ENOMEM;
+ }
+ dev_id = regexdev_find_free_dev();
+ if (dev_id == RTE_MAX_REGEXDEV_DEVS) {
+ RTE_REGEXDEV_LOG
+ (ERR, "Reached maximum number of regex devs\n");
+ return -ENOMEM;
+ }
+ dev->dev_id = dev_id;
+ regex_devices[dev_id] = dev;
+ res = dev_id;
+ return res;
+}
+
+void
+rte_regexdev_unregister(struct rte_regexdev *dev)
+{
+ regex_devices[dev->dev_id] = NULL;
+}
@@ -206,6 +206,13 @@
#include <rte_mbuf.h>
#include <rte_memory.h>
+#define RTE_REGEX_NAME_MAX_LEN RTE_DEV_NAME_MAX_LEN
+
+extern int rte_regexdev_logtype;
+
+#define RTE_REGEXDEV_LOG(level, ...) \
+ rte_log(RTE_LOG_ ## level, rte_regexdev_logtype, "" __VA_ARGS__)
+
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice.
@@ -142,6 +142,8 @@ struct rte_regexdev {
const struct rte_regexdev_ops *dev_ops;
/**< Functions exported by PMD */
struct rte_device *device; /**< Backing device */
+ char dev_name[RTE_REGEX_NAME_MAX_LEN]; /**< Unique identifier name */
+ uint16_t dev_id; /**< Device [external] identifier. */
} __rte_cache_aligned;
#endif /* _RTE_REGEX_CORE_H_ */
new file mode 100644
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Mellanox Corporation
+ */
+
+#ifndef _RTE_REGEXDEV_DRIVER_H_
+#define _RTE_REGEXDEV_DRIVER_H_
+
+/**
+ * @file
+ *
+ * RTE RegEx Device PMD API
+ *
+ * APIs that are used by the RegEx drivers, to comunicate with the
+ * RegEx lib.
+ */
+
+#include "rte_regexdev.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @internal
+ * Register a new regexdev slot for a RegEx device and returns the id
+ * to that slot for the driver to use.
+ *
+ * @param dev
+ * RegEx device structure..
+ *
+ * @return
+ * Slot in the rte_regex_devices array for a new device in case of success,
+ * negative errno otherwise.
+ */
+int rte_regexdev_register(struct rte_regexdev *dev);
+
+/**
+ * @internal
+ * Unregister the specified regexdev port.
+ *
+ * @param dev
+ * Device to be released.
+ */
+void rte_regexdev_unregister(struct rte_regexdev *dev);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_REGEXDEV_DRIVER_H_ */
@@ -30,6 +30,8 @@ option('max_lcores', type: 'integer', value: 128,
description: 'maximum number of cores/threads supported by EAL')
option('max_numa_nodes', type: 'integer', value: 4,
description: 'maximum number of NUMA nodes supported by EAL')
+option('max_regexdev_devs', type: 'integer', value: 32,
+ description: 'maximum number of RegEx devices')
option('tests', type: 'boolean', value: true,
description: 'build unit tests')
option('use_hpet', type: 'boolean', value: false,