From: "Wiles, Keith" <keith.wiles@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>
Cc: "<dev@dpdk.org>" <dev@dpdk.org>,
Panu Matilainen <pmatilai@redhat.com>,
"yuanhan.liu@linux.intel.com" <yuanhan.liu@linux.intel.com>
Subject: Re: [dpdk-dev] [PATCH v5] drivers/net:new PMD using tun/tap host interface
Date: Wed, 12 Oct 2016 18:19:48 +0000 [thread overview]
Message-ID: <CD8AE0A1-A99C-4AF5-BBCC-CB271EBC4D2F@intel.com> (raw)
In-Reply-To: <006f1d58-cf97-41b5-d573-810ecaf4ab31@intel.com>
Regards,
Keith
> On Oct 12, 2016, at 9:56 AM, Yigit, Ferruh <ferruh.yigit@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@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
next prev parent reply other threads:[~2016-10-12 18:19 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-15 14:10 [dpdk-dev] [PATCH 1/3] " Keith Wiles
2016-09-15 14:10 ` [dpdk-dev] [PATCH 2/3] docs:tun/tap PMD information Keith Wiles
2016-09-15 14:13 ` Wiles, Keith
2016-09-15 14:15 ` Wiles, Keith
2016-09-21 2:00 ` [dpdk-dev] [PATCH v3] drivers/net:new PMD using tun/tap host interface Keith Wiles
2016-10-04 14:45 ` [dpdk-dev] [PATCH v4] " Keith Wiles
2016-10-11 9:40 ` Ferruh Yigit
2016-10-11 11:30 ` Michał Mirosław
2016-10-11 20:56 ` Wiles, Keith
2016-10-12 8:14 ` Michał Mirosław
2016-10-11 11:49 ` Ferruh Yigit
2016-10-11 21:07 ` Wiles, Keith
2016-10-11 12:28 ` Ferruh Yigit
2016-10-11 20:57 ` Wiles, Keith
2016-10-11 21:51 ` [dpdk-dev] [PATCH v5] " Keith Wiles
2016-10-12 14:56 ` Ferruh Yigit
2016-10-12 18:19 ` Wiles, Keith [this message]
2016-10-12 19:57 ` Wiles, Keith
2016-10-12 20:54 ` [dpdk-dev] [PATCH v6] " Keith Wiles
2016-10-13 14:41 ` Ferruh Yigit
2016-10-13 15:36 ` [dpdk-dev] [PATCH v7] " Keith Wiles
2016-10-13 16:11 ` [dpdk-dev] [PATCH v8] " Keith Wiles
2016-10-13 16:33 ` Mcnamara, John
2016-10-13 22:03 ` [dpdk-dev] [PATCH v9] " Keith Wiles
2016-10-14 6:41 ` Mcnamara, John
2016-10-14 9:39 ` Ferruh Yigit
2016-11-21 12:56 ` Ferruh Yigit
2016-11-25 19:38 ` Aws Ismail
2016-11-29 21:36 ` Aws Ismail
2016-11-29 22:16 ` Wiles, Keith
2016-12-07 19:38 ` [dpdk-dev] [PATCH v10] drivers/net:new TUN/TAP device PMD Keith Wiles
2016-12-07 20:15 ` Aws Ismail
2016-12-09 18:16 ` Ferruh Yigit
2016-12-09 19:05 ` [dpdk-dev] [PATCH v11] " Keith Wiles
2016-12-12 12:39 ` Vasily Philipov
2016-12-12 14:24 ` [dpdk-dev] [PATCH v12] net/tap: new " Keith Wiles
2016-12-12 14:38 ` Keith Wiles
2016-12-12 19:13 ` Marc
2016-12-12 21:09 ` Wiles, Keith
2016-12-13 13:54 ` Ferruh Yigit
2017-01-20 12:14 ` Ferruh Yigit
2017-01-20 14:25 ` [dpdk-dev] [PATCH] net/tap: fix IFF_MULTI_QUEUE in older kernels not found Keith Wiles
2017-01-20 14:30 ` Keith Wiles
2017-01-20 16:11 ` Ferruh Yigit
2017-01-20 17:37 ` Thomas Monjalon
2016-09-15 14:10 ` [dpdk-dev] [PATCH 3/3] drivers/net:build support for new tap device driver Keith Wiles
2016-09-16 7:36 ` Panu Matilainen
2016-09-16 14:46 ` Wiles, Keith
2016-09-16 16:22 ` [dpdk-dev] [PATCH v2] drivers/net:new PMD using tun/tap host interface Keith Wiles
2016-09-18 13:25 ` Yuanhan Liu
2016-09-18 16:20 ` Wiles, Keith
2016-09-19 0:29 ` Yuanhan Liu
2016-09-19 15:56 ` Wiles, Keith
2016-09-20 3:54 ` Yuanhan Liu
2016-09-20 4:05 ` Yuanhan Liu
2016-09-21 1:32 ` Wiles, Keith
2016-09-21 2:13 ` Yuanhan Liu
2016-09-21 8:24 ` Thomas Monjalon
2016-09-21 23:55 ` Wiles, Keith
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CD8AE0A1-A99C-4AF5-BBCC-CB271EBC4D2F@intel.com \
--to=keith.wiles@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=pmatilai@redhat.com \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).