Re: [dpdk-dev] [PATCH v23 1/6] dmadev: introduce DMA device library

[Thomas Monjalon <thomas@monjalon.net>]

24/09/2021 12:53, Chengwen Feng:
> The 'dmadevice' is a generic type of DMA device.

Do you mean 'dmadev' ?

> This patch introduce the 'dmadevice' device allocation APIs.
> 
> The infrastructure is prepared to welcome drivers in drivers/dma/

Good

[...]
> +The DMA library provides a DMA device framework for management and provisioning
> +of hardware and software DMA poll mode drivers, defining generic APIs which

We could consider the whole as *one* API.

> +support a number of different DMA operations.
> +
> +
> +Design Principles
> +-----------------
> +
> +The DMA library follows the same basic principles as those used in DPDK's
> +Ethernet Device framework and the RegEx framework.

Not sure what this sentence means. Better to remove.

> The DMA framework provides
> +a generic DMA device framework which supports both physical (hardware)
> +and virtual (software) DMA devices as well as a generic DMA API which allows
> +DMA devices to be managed and configured and supports DMA operations to be
> +provisioned on DMA poll mode driver.

You could split this long sentence.

[...]
> +Physical DMA controllers are discovered during the PCI probe/enumeration of the
> +EAL function which is executed at DPDK initialization, this is based on their
> +PCI BDF (bus/bridge, device, function). Specific physical DMA controllers, like
> +other physical devices in DPDK can be listed using the EAL command line options.
> +
> +The dmadevs are dynamically allocated by using the API

not API, but function

> +``rte_dma_pmd_allocate`` based on the number of hardware DMA channels. After the
> +dmadev initialized successfully, the driver needs to switch the dmadev state to
> +``RTE_DMA_DEV_READY``.

Are we sure we need these details?

> +Device Identification
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +Each DMA device, whether physical or virtual is uniquely designated by two
> +identifiers:
> +
> +- A unique device index used to designate the DMA device in all functions
> +  exported by the DMA API.
> +
> +- A device name used to designate the DMA device in console messages, for
> +  administration or debugging purposes.

Good

[...]
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -106,6 +106,10 @@ New Features
>    Added command-line options to specify total number of processes and
>    current process ID. Each process owns subset of Rx and Tx queues.
>  
> +* **Introduced dmadev library with:**
> +
> +  * Device allocation APIs.

There is no API for that, it is internal.
From an user perspective, you need only to list outstanding features
in the release notes.

[...]
> +++ b/lib/dmadev/rte_dmadev.c
> +RTE_LOG_REGISTER_DEFAULT(rte_dma_logtype, INFO);
> +#define RTE_DMA_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, rte_dma_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)

I don't like having function name in all logs.
I recommend this form of macro:
#define RTE_DMA_LOG(level, ...) \
    rte_log(RTE_LOG_ ## level, rte_dma_logtype, RTE_FMT("dma: " \
        RTE_FMT_HEAD(__VA_ARGS__,) "\n", RTE_FMT_TAIL(__VA_ARGS__,)))

> +/* Macros to check for valid device id */
> +#define RTE_DMA_VALID_DEV_ID_OR_ERR_RET(dev_id, retval) do { \
> +	if (!rte_dma_is_valid(dev_id)) { \
> +		RTE_DMA_LOG(ERR, "Invalid dev_id=%d", dev_id); \
> +		return retval; \
> +	} \
> +} while (0)

I dislike this macro doing a return. It is hiding stuff.
I know we have it in other classes but I think it is a mistake,
we should avoid macro blocks.

> +static int16_t
> +dma_find_free_dev(void)

Actually it is looking for an ID,
so it should be dma_find_free_id.

> +{
> +	int16_t i;
> +
> +	if (rte_dma_devices == NULL)
> +		return -1;
> +
> +	for (i = 0; i < dma_devices_max; i++) {
> +		if (rte_dma_devices[i].dev_name[0] == '\0')

Instead of checking its name, it looks more logical to check the state.

> +			return i;
> +	}
> +
> +	return -1;
> +}
> +
> +static struct rte_dma_dev*
> +dma_find(const char *name)

dma_find_by_name?

[...]
> +++ b/lib/dmadev/rte_dmadev.h
> + * The dmadev are dynamically allocated by rte_dma_pmd_allocate() during the
> + * PCI/SoC device probing phase performed at EAL initialization time. And could
> + * be released by rte_dma_pmd_release() during the PCI/SoC device removing
> + * phase.

I don't think this text has value,
and we could imagine allocating a device ata later stage.

[...]
> + * Configure the maximum number of dmadevs.
> + * @note This function can be invoked before the primary process rte_eal_init()
> + * to change the maximum number of dmadevs.

You should mention what is the default.
Is the default exported to the app in this file?

> + *
> + * @param dev_max
> + *   maximum number of dmadevs.
> + *
> + * @return
> + *   0 on success. Otherwise negative value is returned.
> + */
> +__rte_experimental
> +int rte_dma_dev_max(size_t dev_max);

What about a function able to do more with the name rte_dma_init?
It should allocate the inter-process shared memory,
and do the lookup in case of secondary process.

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Get the device identifier for the named DMA device.
> + *
> + * @param name
> + *   DMA device name.
> + *
> + * @return
> + *   Returns DMA device identifier on success.
> + *   - <0: Failure to find named DMA device.
> + */
> +__rte_experimental
> +int rte_dma_get_dev_id(const char *name);

Should we add _by_name?
We could have a function to retrieve the ID by devargs as well.

> +++ b/lib/dmadev/rte_dmadev_core.h
> +/**
> + * @file
> + *
> + * DMA Device internal header.
> + *
> + * This header contains internal data types, that are used by the DMA devices
> + * in order to expose their ops to the class.
> + *
> + * Applications should not use these API directly.

If it is not part of the API, it should not be exposed at all.
Why not having all these stuff in a file dmadev_driver.h?
Is it used by some inline functions?

[...]
> +++ b/lib/dmadev/rte_dmadev_pmd.h
> +/**
> + * @file
> + *
> + * DMA Device PMD APIs
> + *
> + * Driver facing APIs for a DMA device. These are not to be called directly by

You cannot say API for drivers, because API means application interface.
What you mean is "driver interface".

> + * any application.
> + */
[...]
> + * Allocates a new dmadev slot for an DMA device and returns the pointer
> + * to that slot for the driver to use.

Please in all comments, use the infinitive form. Example:
	Allocates -> Allocate

> + *
> + * @param name
> + *   DMA device name.
> + * @param numa_node
> + *   Driver's private data's numa node.

s/numa/NUMA/

> + * @param private_data_size
> + *   Driver's private data size.
> + *
> + * @return
> + *   A pointer to the DMA device slot case of success,
> + *   NULL otherwise.
> + */
> +__rte_internal
> +struct rte_dma_dev *rte_dma_pmd_allocate(const char *name, int numa_node,
> +					 size_t private_data_size);


OK, sorry there are a lot of comments.
Overrall, that's a good work.
I know you are in holidays, I hope we can finish during next week.