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

["Jakub Grajciar -X (jgrajcia - PANTHEON TECHNOLOGIES at Cisco)" <jgrajcia@cisco.com>]

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

Thanks

> Thanks,
> ferruh