Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Mar 6, 2020 at 8:25 PM Coyle, David <david.coyle@intel.com> wrote:
>
> > >
> > > /** Error Detection Algorithms */
> > > enum rte_rawdev_multi_fn_err_detect_algorithm {
> > >         RTE_RAWDEV_MULTI_FN_ERR_DETECT_CRC32_ETH,
> >
> > IMO, It does not make sense to add protocol specific stuff in rawdev
> > symbols.
> >
> > IMO, It is better to have a separate library for CRC and BIP32 acceleration like
> > the rte_security library and underneath still it can use rawdev or anydev if
> > required.
>
> [DC] This protocol stuff is only in the rawdev interface definition, which is known only to the application and the rawdev PMDs which will use this interface.
> So these defines/enums/structs etc for CRC and BIP are completely opaque to rte_rawdev itself.
>
> This is how all existing rawdev PMDs interfaces are defined, where the interface is very specific to the job(s) the PMD is implementing.

If you see .map file in driver/raw/. None of the drivers are exposing
any API with rte_rawdev_*.
This addition will be exposing new rte_rawdev_* APIs from
driver/rawdev/. That's is not correct.

 $ find drivers/raw/ -name *.map
drivers/raw/skeleton/rte_rawdev_skeleton_version.map
drivers/raw/octeontx2_ep/rte_rawdev_octeontx2_ep_version.map
drivers/raw/ntb/rte_rawdev_ntb_version.map
drivers/raw/dpaa2_qdma/rte_rawdev_dpaa2_qdma_version.map
drivers/raw/dpaa2_cmdif/rte_rawdev_dpaa2_cmdif_version.map
drivers/raw/ioat/rte_rawdev_ioat_version.map
drivers/raw/octeontx2_dma/rte_rawdev_octeontx2_dma_version.map
drivers/raw/ifpga/rte_rawdev_ifpga_version.map

IMO, Correct thing to do will be,

Either of

1) As mentioned below, If you would like to limit the scope only to a
new rawdev driver then
a) Create a new driver at driver/raw/<new driver>/
b) expose the drier specific customer API as
rte_<new-driver>_...(example:
drivers/raw/dpaa2_qdma/rte_rawdev_dpaa2_qdma_version.map

2) If we would like to have public API then create a subsystem like
libsecurity to have features. Let the API exposed from lib/...

>
> Also, these particular defines/enums/structs for CRC and BIP are only for defining xform and op chains containing these particular operations.
> The actual code to do the CRC and BIP is already in the AESNI-MB library or DPDK rte_net_crc library, which our aesni_mb and qat rawdev PMDs will call/use
>
> >
> > IMO, Exposing the public API in
> > drivers/raw/common/rte_rawdev_multi_fn.h is a shortcut.
> > IMO, public API should be in lib/..
>
> [DC] To be honest, I tend to agree. I don't like that public APIs are exposed from the drivers directory.
> But as I mentioned above, this is how all rawdev PMD interfaces are defined, where the interface definition is within the PMD directory (e.g. drivers/raw/dpaa2_cmdif/rte_pmd_dpaa2_cmdif.h)
> Our's is slightly different in that we have 2 PMDs which will use the same interface, which is why we have added it in drivers/raw/common
> So by keeping our interface under drivers, we are trying to be consistent with all existing rawdev PMDs
>
> As I mentioned in my previous post though, this could potentially be moved under lib in the future if other PMDs would find it useful

See above. Point (1).

>
> We could possibly rename our interface file to rte_pmd_multi_fn.h to be a bit more consistent with the majority of the existing PMDs and take away the idea for now that this is some kind of extension to the main rte_rawdev API.
> But unfortunately there is no full consistency in the rawdev PMD interface filenames (e.g. dpaa2_cmdif uses the "rte_pmd_" prefix - rte_pmd_dpaa2_cmdif.h, octeontx2_dma uses the "_rawdev" suffix - otx2_dpi_rawdev.h)
>
> >
> > Just my 2c.