Re: [dpdk-dev] [PATCH v10] net/memif: introduce memory interface (memif) PMD

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 5/31/2019 7:22 AM, Jakub Grajciar wrote:
> Memory interface (memif), provides high performance
> packet transfer over shared memory.

Almost there, can you please check below comments? I am hoping to merge next
version of the patch.

Thanks,
ferruh

> 
> Signed-off-by: Jakub Grajciar <jgrajcia@cisco.com>

<...>

> +static const char *valid_arguments[] = {
> +	ETH_MEMIF_ID_ARG,
> +	ETH_MEMIF_ROLE_ARG,
> +	ETH_MEMIF_PKT_BUFFER_SIZE_ARG,
> +	ETH_MEMIF_RING_SIZE_ARG,
> +	ETH_MEMIF_SOCKET_ARG,
> +	ETH_MEMIF_MAC_ARG,
> +	ETH_MEMIF_ZC_ARG,
> +	ETH_MEMIF_SECRET_ARG,
> +	NULL
> +};

Checkpatch is giving following warning:

WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be
static const char * const
#1885: FILE: drivers/net/memif/rte_eth_memif.c:39:
+static const char *valid_arguments[] = {

<...>

> +static int
> +rte_pmd_memif_probe(struct rte_vdev_device *vdev)
> +{
> +	RTE_BUILD_BUG_ON(sizeof(memif_msg_t) != 128);
> +	RTE_BUILD_BUG_ON(sizeof(memif_desc_t) != 16);
> +	int ret = 0;
> +	struct rte_kvargs *kvlist;
> +	const char *name = rte_vdev_device_name(vdev);
> +	enum memif_role_t role = MEMIF_ROLE_SLAVE;
> +	memif_interface_id_t id = 0;
> +	uint16_t pkt_buffer_size = ETH_MEMIF_DEFAULT_PKT_BUFFER_SIZE;
> +	memif_log2_ring_size_t log2_ring_size = ETH_MEMIF_DEFAULT_RING_SIZE;
> +	const char *socket_filename = ETH_MEMIF_DEFAULT_SOCKET_FILENAME;
> +	uint32_t flags = 0;
> +	const char *secret = NULL;
> +	struct rte_ether_addr *ether_addr = rte_zmalloc("", sizeof(struct rte_ether_addr), 0);

This is a long line, and breaking it won't reduce the readability, can you
please break it. Checkpatch warning:

WARNING:LONG_LINE: line over 80 characters
#2939: FILE: drivers/net/memif/rte_eth_memif.c:1093:
+       struct rte_ether_addr *ether_addr = rte_zmalloc("", sizeof(struct
rte_ether_addr), 0);

<...>

> +static int
> +rte_pmd_memif_remove(struct rte_vdev_device *vdev)
> +{
> +	struct rte_eth_dev *eth_dev;
> +	int i;
> +
> +	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
> +	if (eth_dev == NULL)
> +		return 0;
> +
> +	for (i = 0; i < eth_dev->data->nb_rx_queues; i++)
> +		(*eth_dev->dev_ops->rx_queue_release)(eth_dev->data->rx_queues[i]);
> +	for (i = 0; i < eth_dev->data->nb_tx_queues; i++)
> +		(*eth_dev->dev_ops->rx_queue_release)(eth_dev->data->tx_queues[i]);

Although they point same function, better to use 'dev_ops->tx_queue_release' for
Tx queues.

> +
> +	rte_free(eth_dev->process_private);
> +	eth_dev->process_private = NULL;

"process_private" is not used in this PMD at all, no need to free it I think.

> +
> +	rte_eth_dev_close(eth_dev->data->port_id);

There are two exit path from a PMD:
1) rte_eth_dev_close() API
2) rte_vdev_driver->remove() called by hotplug remove APIs ('rte_dev_remove()'
or 'rte_eal_hotplug_remove()')

Both should clear all PMD allocated resources. Since you are calling
'rte_eth_dev_close() from this .remove() function, it makes sense to move all
resource cleanup to .dev_close (like queue cleanup calls above).