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

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

On 6/5/2019 12:55 PM, Ferruh Yigit wrote:
> 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_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).
> 

Hi Jakup,

Above comments seems not implemented in v11, can you please check them?
If anything is not clear feel free to reach me on the irc.

Thanks,
ferruh