Re: [dpdk-dev] [PATCH v3 1/5] bbdev: librte_bbdev library

["Mokhtar, Amr" <amr.mokhtar@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Saturday 9 December 2017 02:44
> To: Mokhtar, Amr <amr.mokhtar@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>; De
> Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Power, Niall
> <niall.power@intel.com>; Macnamara, Chris <chris.macnamara@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/5] bbdev: librte_bbdev library
> 
> On 12/7/2017 1:40 PM, Amr Mokhtar wrote:
> 
> Patch title has duplication, it can be something like:
> bbdev: introduce base band device abstraction library
> 
> > - BBDEV library files
> > - BBDEV is tagged as EXPERIMENTAL
> > - Makefiles and configuration macros definition
> > - The bbdev framework and the 'null' driver are enabled by default
> > - The bbdev test framework is enabled by default
> > - Release Notes of the initial version and MAINTAINERS
> >
> > Signed-off-by: Amr Mokhtar <amr.mokhtar@intel.com>
> > ---
> >  MAINTAINERS                            |   11 +
> >  config/common_base                     |   18 +
> >  doc/guides/rel_notes/release_18_02.rst |   10 +
> >  lib/Makefile                           |    3 +
> >  lib/librte_bbdev/Makefile              |   56 ++
> >  lib/librte_bbdev/rte_bbdev.c           | 1095
> ++++++++++++++++++++++++++++++++
> >  lib/librte_bbdev/rte_bbdev.h           |  741 +++++++++++++++++++++
> >  lib/librte_bbdev/rte_bbdev_op.h        |  532 ++++++++++++++++
> >  lib/librte_bbdev/rte_bbdev_op_ldpc.h   |  545 ++++++++++++++++
> 
> What is this file, a git merge artifact? Look like a slightly modified copy of
> rte_bbdev_op.h and not used.

It should be removed.

> 
> <...>
> 
> > +BBDEV API - EXPERIMENTAL
> > +M: Amr Mokhtar <amr.mokhtar@intel.com>
> > +F: lib/librte_bbdev/
> > +F: drivers/bbdev/
> 
> Please add only added files, as far as I can see this patch adds only library part.
> You can update same file in further patches to add these.

Ok. It makes more sense to have patches incrementally updated with relevant
added files.

> <...>
> 
> > +# Compile generic wireless base band device library # EXPERIMENTAL:
> > +API may change without prior notice # CONFIG_RTE_LIBRTE_BBDEV=y
> > +CONFIG_RTE_LIBRTE_BBDEV_DEBUG=n
> > +CONFIG_RTE_BBDEV_MAX_DEVS=128
> > +CONFIG_RTE_BBDEV_NAME_MAX_LEN=64
> 
> Overall we are trying to reduce configuration parameters, is NAME_MAX_LEN
> really an option end user may want to use with different values? Why not define
> this in source files?

Agree. It doesn't have to be a config option being exposed to the user.

> <...>
> > +
> > +  The Wireless Baseband Device library is an acceleration abstraction
> > + framework for 3gpp Layer 1 processing functions that provides a
> > + common  programming interface for seamless opeartion on integrated
> > + or discrete  hardware accelerators or using optimized software
> > + libraries for signal  processing.
> > +  The current release only supports 4G Turbo Code FEC function.
> > +
> > +  See the :doc:`../prog_guide/bbdev` programmer's guide for more details.
> >
> 
> Release notes "Shared Library Versions" section also needs to be updated, and
> new library added in that list, with a "+" sign at the beginning showing this is
> new.
> 

Right, I missed that.

> <...>
> 
> > +DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += librte_bbdev DEPDIRS-librte_bbdev
> > +:= librte_eal librte_mempool librte_ring librte_mbuf
> > +DEPDIRS-librte_bbdev += librte_kvargs
> 
> Do you use kvargs in library? Or rte_ring?

Good point. No, not really.
They are not used by the library itself. They are only needed by the drivers.

> 
> <...>
> 
> > @@ -0,0 +1,56 @@
> > +#   BSD LICENSE
> > +#
> > +#   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > +#
> > +#   Redistribution and use in source and binary forms, with or without
> > +#   modification, are permitted provided that the following conditions
> > +#   are met:
> > +#
> > +#     * Redistributions of source code must retain the above copyright
> > +#       notice, this list of conditions and the following disclaimer.
> > +#     * Redistributions in binary form must reproduce the above copyright
> > +#       notice, this list of conditions and the following disclaimer in
> > +#       the documentation and/or other materials provided with the
> > +#       distribution.
> > +#     * Neither the name of Intel Corporation nor the names of its
> > +#       contributors may be used to endorse or promote products derived
> > +#       from this software without specific prior written permission.
> > +#
> > +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> > +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> > +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS FOR
> > +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> NOT
> > +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
> OF USE,
> > +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> AND ON ANY
> > +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> > +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> THE USE
> > +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> 
> DPDK is switching to a new SPDX licensing method, this is new, can you please
> switch that as well? For all files.

Sure. Will get this updated in v4 patch.

> <...>
> 
> > +
> > +# library source files
> > +SRCS-y += rte_bbdev.c
> 
> SRCS-$(CONFIG_RTE_LIBRTE_BBDEV) += rte_bbdev.c
> 
> to be consistent with other libraries.

Other libraries are using same approach SRCS-y += ...
Because this module won't get included in the build in first place if
CONFIG_RTE_LIBRTE_BBDEV != y
So, it is conditional at the folder level, according to this condition ->
DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += librte_bbdev


> <...>
> 
> > +/* Global array of all devices. This is not static because it's used
> > +by the
> > + * inline enqueue and dequeue functions  */ struct rte_bbdev
> > +rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
> 
> Where these inline enqueue and dequeue functions implemented?

It's the data plane functions -> rte_bbdev_enqueue_* and rte_bbdev_dequeue_*
They are implemented in rte_bbdev.h file.

> 
> Does it make sense to keep struct static, but implement a getter API to prevent
> direct access to device list?

We have made it that way for quick access by data plane APIs.

Even if we use inline getter API, it will need to be defined in the header file and
rte_bbdev_devices[] will stay externed in the .h file.
It will be a nicer way of coding but with no real value. If still not agreed, can be
changed to use static inline getter API.


> <...>
> 
> > +	bbdev = &rte_bbdev_devices[dev_id];
> > +
> > +	if (rte_bbdev_data == NULL)
> > +		rte_bbdev_data_alloc();
> > +
> > +	bbdev->data = &rte_bbdev_data[dev_id];
> 
> The assumption that rte_bbdev_devices and rte_bbdev_data share same index,
> and rte_bbdev_devices being per process and rte_bbdev_data being shared is
> causing problem in multi process for ethdev. Since you are developing this from
> scratch, perhaps can design something better.
> 
> Think about a case there are two physical devices, A and B. In primary process
> you whitelist only "A" and it get port_id == 0, and using rte_bbdev_data[0].
> In secondary process you whitelist only "B", it gets again port_id == 0 in
> secondary process and uses same "rte_bbdev_data[0]" with "A". So they corrupt
> each others data.
> 
> Does is make sense to assign "data" by searching the "rte_bbdev_data", and get
> first free one in primary, search for name or something similar for secondary?
> 

Ok. Will think of a better mechanism.
But, can you explain more how primary and secondary processes are getting
different whitelists? Are they two processes in the same application?

> <...>
> 
> > +/** Macro used at end of bbdev PMD list */ #define
> > +RTE_BBDEV_END_OF_CAPABILITIES_LIST() \
> > +	{ RTE_BBDEV_OP_NONE }
> 
> Who is the user of this macro? In this patch it is not used.
> If this is used by PMDs, should go to pmd header file

I think this is the best place to have this macro defined.
This macro provides a standard way of marking the end of capabilities
list when being reported by the driver. It is true that it is usable by the drivers
but the bbdev library is responsible for providing a consistent reporting
mechanism for all drivers to come. I can explain more if still not clear.

> <...>
> <...>
> 
> > +int
> > +rte_bbdev_queue_info_get(uint16_t dev_id, uint16_t queue_id,
> > +		struct rte_bbdev_queue_info *dev_info);
> > +
> > +/** @internal The data structure associated with each queue of a
> > +device. */
> 
> Why internal data structures are in public header?
> For ethdev they are in public header because used by inline fuctions, and inline
> fuctions are preferred in data path for performance. Is there same concern
> here?

Yes, for the same purpose with bbdev.

> <...>
> 
> > +ifeq ($(CONFIG_RTE_LIBRTE_BBDEV),y)
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_NULL)     += -
> lrte_pmd_bbdev_null
> > +
> > +# TURBO SOFTWARE PMD is dependent on the FLEXRAN library
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW) +=
> > +-lrte_pmd_bbdev_turbo_sw
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW) +=
> > +-L$(FLEXRAN_SDK)/lib_common -lcommon
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW) +=
> > +-L$(FLEXRAN_SDK)/lib_crc -lcrc
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW) +=
> > +-L$(FLEXRAN_SDK)/lib_turbo -lturbo
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW) +=
> > +-L$(FLEXRAN_SDK)/lib_rate_matching -lrate_matching
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW) += -lirc -limf
> > +-lstdc++ -lipps endif # CONFIG_RTE_LIBRTE_BBDEV
> 
> You shouldn't add these libraries in this patch, patch by patch build also should
> be successfull and when you add them in this patch, because of missing libraries
> build will break.

Right. Should be added in relevant patch when dependent driver is added.

> Also can we link dependent libraries to the library itself instead of final
> application?

Unfortunately, with the current DPDK build layout, these libraries need to
be exposed to the application or the linker will fail.
Similar situation can be noticed with cryptodev.

> <...>

For all other comments that were skipped in this reply, are implicitly agreed upon
and will be fixed in v4 patchset.

Thanks,
Amr