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

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

On 6/6/2019 11:25 AM, Jakub Grajciar -X (jgrajcia - PANTHEON TECHNOLOGIES at
Cisco) wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, June 6, 2019 11:24 AM
>> To: Jakub Grajciar -X (jgrajcia - PANTHEON TECHNOLOGIES at Cisco)
>> <jgrajcia@cisco.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v10] net/memif: introduce memory interface
>> (memif) PMD
>>
>> 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.
>>
> 
> 
> Sorry, I missed that mail. I'll get it fixed right away, but I don't understand what's wrong with 'static const char *valid_arguments[]...' other PMDs handle args the same way, can you please give me a hint?

It is not wrong, but good to have that second 'const', it prevents you update
the valid_arguments[] by mistake, like following will work without that 'const':

 char p = "test";
 valid_arguments[0] = p;

Since we don't support dynamically changing device arguments in runtime, good to
have the protection, it won't hurt.


> 
> Thanks
> 
>> Thanks,
>> ferruh