[dpdk-dev] [PATCH v5] drivers/net:new PMD using tun/tap host interface

[Wiles, Keith]

Regards,
Keith

> On Oct 12, 2016, at 9:56 AM, Yigit, Ferruh <ferruh.yigit at intel.com> wrote:
> 
> On 10/11/2016 10:51 PM, Keith Wiles wrote:
>> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
>> on the local host. The PMD allows for DPDK and the host to
>> communicate using a raw device interface on the host and in
>> the DPDK application. The device created is a Tap device with
>> a L2 packet header.
>> 
>> v5 - merge in changes from list review see related emails.
>>     fixed checkpatch issues and many minor edits
>> v4 - merge with latest driver changes
>> v3 - fix includes by removing ifdef for other type besides Linux.
>>     Fix the copyright notice in the Makefile
>> v2 - merge all of the patches into one patch.
>>     Fix a typo on naming the tap device.
>>     Update the maintainers list
>> 
>> Signed-off-by: Keith Wiles <keith.wiles at intel.com>
>> ---
> 
> <..>
> 
>> diff --git a/config/common_base b/config/common_base
>> index f5d2eff..356c631 100644
>> --- a/config/common_base
>> +++ b/config/common_base
>> @@ -592,3 +592,8 @@ CONFIG_RTE_APP_TEST_RESOURCE_TAR=n
>> CONFIG_RTE_TEST_PMD=y
>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES=n
>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS=n
>> +
>> +#
>> +# Set TAP PMD to 'n' as it is only supported in Linux for now.
> 
> This comments moved to final .config and creates confusion, can we
> remove it if you don't have a strong option to keep it?

What do you mean, the statement is confusing or causes problems?

> 
> <..>
> 
>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>> new file mode 100644
>> index 0000000..eed81ec
>> --- /dev/null
>> +++ b/doc/guides/nics/tap.rst
> 
> <..>
> 
>> +.. code-block:: console
>> +
>> +   The interfaced name can be changed by adding the iface=foo0
>> +   e.g. --vdev=eth_tap,iface=foo0 --vdev=eth_tap,iface=foo1, ...
> 
> For all file:
> %s/eth_tap/net_tap/g, there are multiple lines with this usage

Missed that one.

> 
> 
> <..>
> 
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> new file mode 100644
>> index 0000000..c13aa1b
>> --- /dev/null
>> +++ b/drivers/net/tap/rte_eth_tap.c
> 
> <..>
> 
>> +
>> +struct tap_info {
>> +	char name[RTE_ETH_NAME_MAX_LEN]; /* Interface name supplied/given */
>> +	int speed;			 /* Speed of interface */
>> +};
> 
> This struct can go away, it is not used, since with the updated code
> rte_pmd_tap_probe() used tap_name and speed as seperate variables
> instead of struct.
> 

OK.

> 
> <..>
> 
>> +
>> +	/* If the name is different that new name as default */
>> +	if (name && strcmp(name, ifr.ifr_name))
>> +		snprintf(name, RTE_ETH_NAME_MAX_LEN-1, "%s", ifr.ifr_name);
> 
> syntax, space around "-"
> 
> <..>
> 
>> +
>> +static void
>> +tap_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *tap_stats)
>> +{
>> +	unsigned i, imax;
> 
> checkpatch complain about not using "unsigned int”

I ran checkpatch on the patch and saw no errors reported via the scripts/checkpatch.sh. Which checkpatch are you using here?
> 
> 
> <..>
> 
>> +static int
>> +tap_setup_queue(struct rte_eth_dev *dev,
>> +		struct pmd_internals *internals,
>> +		uint16_t qid)
>> +{
>> +	struct rx_queue *rx = &internals->rxq[qid];
>> +	struct tx_queue *tx = &internals->txq[qid];
>> +	int fd;
>> +
>> +	if ((fd = rx->fd) < 0)
>> +		if ((fd = tx->fd) < 0) {
>> +			RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n",
>> +				dev->data->name, qid);
>> +			if ((fd = tun_alloc(dev->data->name)) < 0) {
> 
> checkpatch complain about assignment in the if condition
> 
> 
> <..>
> 
>> +	/* Now get the space available for data in the mbuf */
>> +	buf_size = (uint16_t) (rte_pktmbuf_data_room_size(mp) -
> 
> syntax, no space after cast
> 
> 
> <..>
> 
>> +	/* Create the first Tap device */
>> +	if ((fd = tun_alloc(tap_name)) < 0) {
> 
> checkpatch complains about assignment in if condition
> 
>> +		RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", dev->data->name);
>> +		rte_free(pmd);
> 
> rte_free(data) or goto error_exit; ?
> 
>> +		rte_eth_dev_release_port(dev);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Presetup the fds to -1 as being not working */
>> +	for(i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>> +		pmd->fds[i] = -1;
>> +		pmd->rxq[i].fd = -1;
>> +		pmd->txq[i].fd = -1;
>> +	}
>> +
>> +	/* Take the TUN/TAP fd and place in the first location */
>> +	pmd->rxq[0].fd = fd;
>> +	pmd->txq[0].fd = fd;
>> +	pmd->fds[0] = fd;
>> +
>> +	if (pmd_mac_address(fd, dev, &pmd->eth_addr) < 0) {
>> +		rte_free(pmd);
> 
> rte_free(data) or goto error_exit; ?
> 
> 
> <..>
> 
>> +static int
>> +set_interface_name(const char *key __rte_unused,
>> +		   const char *value,
>> +		   void *extra_args)
>> +{
>> +	char *name = (char *)extra_args;
>> +
>> +	if (value)
>> +		snprintf(name, RTE_ETH_NAME_MAX_LEN-1, "%s", value);
> 
> syntax, space around "-"
> 
>> +	else
>> +		snprintf(name, RTE_ETH_NAME_MAX_LEN-1, "%s%d",
> 
> syntax, space around "-"
> 
>> +			 DEFAULT_TAP_NAME, (tap_unit-1));
> 
> syntax, space around "-"
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +set_interface_speed(const char *key __rte_unused,
>> +		    const char *value,
>> +		    void *extra_args)
>> +{
>> +	*(int *)extra_args = (value)? atoi(value) : ETH_SPEED_NUM_10G;
> 
> syntax, space around "?"
> 
> <..>
> 
>> +		kvlist = rte_kvargs_parse(params, valid_arguments);
>> +		if (kvlist) {
>> +			if (rte_kvargs_count(kvlist, ETH_TAP_SPEED_ARG) == 1) {
>> +				ret = rte_kvargs_process(kvlist,
>> +							 ETH_TAP_SPEED_ARG,
>> +							 &set_interface_speed,
>> +						         &speed);
> 
> whitespace, space and tab mixed
> 
>> +				if (ret == -1)
>> +					goto leave;
>> +			}
>> +
>> +			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
>> +				ret = rte_kvargs_process(kvlist,
>> +							 ETH_TAP_IFACE_ARG,
>> +							 &set_interface_name,
>> +						         tap_name);
> 
> whitespace, space and tab mixed
> 
> <..>
> 
>> +static int
>> +rte_pmd_tap_remove(const char *name)
>> +{
>> +	struct rte_eth_dev *eth_dev = NULL;
>> +	struct pmd_internals *internals;
>> +	int i;
>> +
>> +	RTE_LOG(INFO, PMD, "Closing TUN/TAP Ethernet device on numa %u\n",
>> +		rte_socket_id());
>> +
>> +	/* find the ethdev entry */
>> +	eth_dev = rte_eth_dev_allocated(name);
> 
> This may cause a problem. Device created by tap_name, but searching with
> name. I suspenct this will always return NULL.
> 
> <..>
> 
> Thanks,
> ferru