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

[Ferruh Yigit]

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 at 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.

<...>

> +BBDEV API - EXPERIMENTAL
> +M: Amr Mokhtar <amr.mokhtar at 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.

<...>

> +# 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?

> +
> +#
> +# Compile PMD for NULL bbdev device
> +#
> +CONFIG_RTE_LIBRTE_PMD_BBDEV_NULL=y
> +
> +#
> +# Compile PMD for turbo software bbdev device
> +#
> +CONFIG_RTE_LIBRTE_PMD_BBDEV_TURBO_SW=n

Same as above, please add these when code added.

<...>

> @@ -41,6 +41,16 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>  
> +* **Added Wireless Base Band Device (bbdev).**

Added Wireless Base Band Device Abstraction?

> +
> +  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.

<...>

> +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?

<...>

> @@ -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.

<...>

> +LDLIBS += -lrte_eal -lrte_mempool -lrte_ring -lrte_mbuf
> +LDLIBS += -lrte_kvargs

Same question, do you use all these libraries?

> +
> +# library source files
> +SRCS-y += rte_bbdev.c

SRCS-$(CONFIG_RTE_LIBRTE_BBDEV) += rte_bbdev.c

to be consistent with other libraries.

> +
> +# export include files
> +SYMLINK-y-include += rte_bbdev_op.h
> +SYMLINK-y-include += rte_bbdev.h
> +SYMLINK-y-include += rte_bbdev_pmd.h

Similarly,
SYMLINK-$(CONFIG_RTE_LIBRTE_BBDEV)-include += ...

<...>

> +/* 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?

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

<...>

> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		mz = rte_memzone_reserve(MZ_RTE_BBDEV_DATA,
> +				RTE_BBDEV_MAX_DEVS * sizeof(*rte_bbdev_data),
> +				rte_socket_id(), flags);
> +	} else
> +		mz = rte_memzone_lookup(MZ_RTE_BBDEV_DATA);
> +	if (mz == NULL)
> +		rte_panic("Cannot allocate memzone for bbdev port data\n");

We are trying to escape from rte_panic(), reason is this is the library
application using, exiting should be decision of the application itself, so
please return error back to app.

<...>

> +	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?

<...>

> +		if ((conf->op_type == RTE_BBDEV_OP_NONE) &&
> +				(dev_info.capabilities[0].type ==
> +				RTE_BBDEV_OP_NONE)) {
> +			ret = 1;
> +		} else
> +			for (p = dev_info.capabilities;
> +					p->type != RTE_BBDEV_OP_NONE; p++) {
> +				if (conf->op_type == p->type) {
> +					ret = 1;
> +					break;
> +				}
> +			}

can you please add "{" to else, although this is valid, it becomes confusiong
with multiline statements.

<...>
<...>

> +int
> +rte_bbdev_queue_info_get(uint16_t dev_id, uint16_t queue_id,
> +		struct rte_bbdev_queue_info *dev_info)

Does it make sense to use queue_info instead of dev_info to not confuse with
rte_bbdev_info?

<...>
<...>

> +const char *
> +rte_bbdev_op_type_str(enum rte_bbdev_op_type op_type)
> +{
> +	static const char * const op_types[] = {
> +		"RTE_BBDEV_OP_NONE",
> +		"RTE_BBDEV_OP_TURBO_DEC",
> +		"RTE_BBDEV_OP_TURBO_ENC",
> +	};
> +
> +	if (op_type < RTE_BBDEV_OP_TYPE_COUNT)
> +		return op_types[op_type];
> +
> +	rte_bbdev_log(ERR, "Invalid operation type");
> +	return "";

Even with a wrong op_type it will return a valid pointer, hard for application
to distiguish that wrong input provided. Does it make sense to return NULL for
this case of is this behavior intentional (maybe because this function only
called by loggin functions)?

<...>

> +/**
> + * @file rte_bbdev.h
> + *
> + * Wireless base band device application APIs.

Wireless base band device abstraction APIs ?

<...>

> +/** 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

<...>
<...>

> +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?

<...>
<...>

> +++ b/lib/librte_bbdev/rte_bbdev_version.map
> @@ -0,0 +1,37 @@
> +EXPERIMENTAL {
> +	global:
> +
> +	rte_bbdev_allocate;
> +	rte_bbdev_callback_register;
> +	rte_bbdev_callback_unregister;
> +	rte_bbdev_close;
> +	rte_bbdev_setup_queues;
> +	rte_bbdev_intr_enable;
> +	rte_bbdev_count;

Can you please keep the function list sorted.

<...>

> @@ -96,6 +96,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_ETHER)          += -lrte_ethdev
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_CRYPTODEV)      += -lrte_cryptodev
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_SECURITY)       += -lrte_security
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_EVENTDEV)       += -lrte_eventdev
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_BBDEV)          += -lrte_bbdev

Please keep alphatecically sorted.

<...>

> +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.

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

<...>