Re: [PATCH v4] net/kni: add KNI PMD

[Yong Wang <yongwang@vmware.com>]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Thursday, December 15, 2016 7:56 AM
> To: Yong Wang <yongwang@vmware.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4] net/kni: add KNI PMD
> 
> On 12/14/2016 7:25 PM, Yong Wang wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Wednesday, December 14, 2016 8:00 AM
> >> To: Yong Wang <yongwang@vmware.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v4] net/kni: add KNI PMD
> >>
> >> On 12/12/2016 9:59 PM, Yong Wang wrote:
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >>>> Sent: Wednesday, November 30, 2016 10:12 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Yong Wang
> >>>> <yongwang@vmware.com>
> >>>> Subject: [PATCH v4] net/kni: add KNI PMD
> >>>>
> >>>> Add KNI PMD which wraps librte_kni for ease of use.
> >>>>
> >>>> KNI PMD can be used as any regular PMD to send / receive packets to
> the
> >>>> Linux networking stack.
> >>>>
> >>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> ---
> >>>>
> >>>> v4:
> >>>> * allow only single queue
> >>>> * use driver.name as name
> >>>>
> >>>> v3:
> >>>> * rebase on top of latest master
> >>>>
> >>>> v2:
> >>>> * updated driver name eth_kni -> net_kni
> >>>> ---
> >>>>  config/common_base                      |   1 +
> >>>>  config/common_linuxapp                  |   1 +
> >>>>  drivers/net/Makefile                    |   1 +
> >>>>  drivers/net/kni/Makefile                |  63 +++++
> >>>>  drivers/net/kni/rte_eth_kni.c           | 462
> >>>> ++++++++++++++++++++++++++++++++
> >>>>  drivers/net/kni/rte_pmd_kni_version.map |   4 +
> >>>>  mk/rte.app.mk                           |  10 +-
> >>>>  7 files changed, 537 insertions(+), 5 deletions(-)
> >>>>  create mode 100644 drivers/net/kni/Makefile
> >>>>  create mode 100644 drivers/net/kni/rte_eth_kni.c
> >>>>  create mode 100644 drivers/net/kni/rte_pmd_kni_version.map
> >>>>
> >>>> diff --git a/config/common_base b/config/common_base
> >>>> index 4bff83a..3385879 100644
> >>>> --- a/config/common_base
> >>>> +++ b/config/common_base
> >>>> @@ -543,6 +543,7 @@ CONFIG_RTE_PIPELINE_STATS_COLLECT=n
> >>>>  # Compile librte_kni
> >>>>  #
> >>>>  CONFIG_RTE_LIBRTE_KNI=n
> >>>> +CONFIG_RTE_LIBRTE_PMD_KNI=n
> >>>>  CONFIG_RTE_KNI_KMOD=n
> >>>>  CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
> >>>>  CONFIG_RTE_KNI_VHOST=n
> >>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
> >>>> index 2483dfa..2ecd510 100644
> >>>> --- a/config/common_linuxapp
> >>>> +++ b/config/common_linuxapp
> >>>> @@ -39,6 +39,7 @@ CONFIG_RTE_EAL_IGB_UIO=y
> >>>>  CONFIG_RTE_EAL_VFIO=y
> >>>>  CONFIG_RTE_KNI_KMOD=y
> >>>>  CONFIG_RTE_LIBRTE_KNI=y
> >>>> +CONFIG_RTE_LIBRTE_PMD_KNI=y
> >>>>  CONFIG_RTE_LIBRTE_VHOST=y
> >>>>  CONFIG_RTE_LIBRTE_PMD_VHOST=y
> >>>>  CONFIG_RTE_LIBRTE_PMD_AF_PACKET=y
> >>>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> >>>> index bc93230..c4771cd 100644
> >>>> --- a/drivers/net/Makefile
> >>>> +++ b/drivers/net/Makefile
> >>>> @@ -41,6 +41,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic
> >>>>  DIRS-$(CONFIG_RTE_LIBRTE_FM10K_PMD) += fm10k
> >>>>  DIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e
> >>>>  DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe
> >>>> +DIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += kni
> >>>>  DIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4
> >>>>  DIRS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5
> >>>>  DIRS-$(CONFIG_RTE_LIBRTE_MPIPE_PMD) += mpipe
> >>>> diff --git a/drivers/net/kni/Makefile b/drivers/net/kni/Makefile
> >>>> new file mode 100644
> >>>> index 0000000..0b7cf91
> >>>> --- /dev/null
> >>>> +++ b/drivers/net/kni/Makefile
> >>>> @@ -0,0 +1,63 @@
> >>>> +#   BSD LICENSE
> >>>> +#
> >>>> +#   Copyright(c) 2016 Intel Corporation. All rights reserved.
> >>>> +#
> >>>> +#   Redistribution and use in source and binary forms, with or without
> >>>> +#   modification, are permitted provided that the following conditions
> >>>> +#   are met:
> >>>> +#
> >>>> +#     * Redistributions of source code must retain the above copyright
> >>>> +#       notice, this list of conditions and the following disclaimer.
> >>>> +#     * Redistributions in binary form must reproduce the above
> copyright
> >>>> +#       notice, this list of conditions and the following disclaimer in
> >>>> +#       the documentation and/or other materials provided with the
> >>>> +#       distribution.
> >>>> +#     * Neither the name of Intel Corporation nor the names of its
> >>>> +#       contributors may be used to endorse or promote products
> derived
> >>>> +#       from this software without specific prior written permission.
> >>>> +#
> >>>> +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> >>>> CONTRIBUTORS
> >>>> +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING,
> >> BUT
> >>>> NOT
> >>>> +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> >>>> FITNESS FOR
> >>>> +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> >>>> COPYRIGHT
> >>>> +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> >>>> INCIDENTAL,
> >>>> +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> >> BUT
> >>>> NOT
> >>>> +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> >> LOSS
> >>>> OF USE,
> >>>> +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> CAUSED
> >>>> AND ON ANY
> >>>> +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> OR
> >>>> TORT
> >>>> +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT
> >> OF
> >>>> THE USE
> >>>> +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> >>>> DAMAGE.
> >>>> +
> >>>> +include $(RTE_SDK)/mk/rte.vars.mk
> >>>> +
> >>>> +#
> >>>> +# library name
> >>>> +#
> >>>> +LIB = librte_pmd_kni.a
> >>>> +
> >>>> +CFLAGS += -O3
> >>>> +CFLAGS += $(WERROR_FLAGS)
> >>>> +LDLIBS += -lpthread
> >>>> +
> >>>> +EXPORT_MAP := rte_pmd_kni_version.map
> >>>> +
> >>>> +LIBABIVER := 1
> >>>> +
> >>>> +#
> >>>> +# all source are stored in SRCS-y
> >>>> +#
> >>>> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += rte_eth_kni.c
> >>>> +
> >>>> +#
> >>>> +# Export include files
> >>>> +#
> >>>> +SYMLINK-y-include +=
> >>>> +
> >>>> +# this lib depends upon:
> >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += lib/librte_eal
> >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += lib/librte_ether
> >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += lib/librte_kni
> >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += lib/librte_mbuf
> >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += lib/librte_mempool
> >>>> +
> >>>> +include $(RTE_SDK)/mk/rte.lib.mk
> >>>> diff --git a/drivers/net/kni/rte_eth_kni.c
> b/drivers/net/kni/rte_eth_kni.c
> >>>> new file mode 100644
> >>>> index 0000000..6c4df96
> >>>> --- /dev/null
> >>>> +++ b/drivers/net/kni/rte_eth_kni.c
> >>>> @@ -0,0 +1,462 @@
> >>>> +/*-
> >>>> + *   BSD LICENSE
> >>>> + *
> >>>> + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> >>>> + *   All rights reserved.
> >>>> + *
> >>>> + *   Redistribution and use in source and binary forms, with or without
> >>>> + *   modification, are permitted provided that the following conditions
> >>>> + *   are met:
> >>>> + *
> >>>> + *     * Redistributions of source code must retain the above copyright
> >>>> + *       notice, this list of conditions and the following disclaimer.
> >>>> + *     * Redistributions in binary form must reproduce the above
> copyright
> >>>> + *       notice, this list of conditions and the following disclaimer in
> >>>> + *       the documentation and/or other materials provided with the
> >>>> + *       distribution.
> >>>> + *     * Neither the name of Intel Corporation nor the names of its
> >>>> + *       contributors may be used to endorse or promote products
> derived
> >>>> + *       from this software without specific prior written permission.
> >>>> + *
> >>>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> >>>> CONTRIBUTORS
> >>>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING,
> >> BUT
> >>>> NOT
> >>>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
> AND
> >>>> FITNESS FOR
> >>>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> THE
> >>>> COPYRIGHT
> >>>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> >>>> INCIDENTAL,
> >>>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> (INCLUDING,
> >> BUT
> >>>> NOT
> >>>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> >> LOSS
> >>>> OF USE,
> >>>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> CAUSED
> >>>> AND ON ANY
> >>>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> OR
> >>>> TORT
> >>>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT
> >> OF
> >>>> THE USE
> >>>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> >>>> DAMAGE.
> >>>> + */
> >>>> +
> >>>> +#include <fcntl.h>
> >>>> +#include <pthread.h>
> >>>> +#include <unistd.h>
> >>>> +
> >>>> +#include <rte_ethdev.h>
> >>>> +#include <rte_kni.h>
> >>>> +#include <rte_malloc.h>
> >>>> +#include <rte_vdev.h>
> >>>> +
> >>>> +/* Only single queue supported */
> >>>> +#define KNI_MAX_QUEUE_PER_PORT 1
> >>>> +
> >>>> +#define MAX_PACKET_SZ 2048
> >>>> +#define MAX_KNI_PORTS 8
> >>>> +
> >>>> +struct pmd_queue_stats {
> >>>> +	uint64_t pkts;
> >>>> +	uint64_t bytes;
> >>>> +	uint64_t err_pkts;
> >>>> +};
> >>>> +
> >>>> +struct pmd_queue {
> >>>> +	struct pmd_internals *internals;
> >>>> +	struct rte_mempool *mb_pool;
> >>>> +
> >>>> +	struct pmd_queue_stats rx;
> >>>> +	struct pmd_queue_stats tx;
> >>>> +};
> >>>> +
> >>>> +struct pmd_internals {
> >>>> +	struct rte_kni *kni;
> >>>> +	int is_kni_started;
> >>>> +
> >>>> +	pthread_t thread;
> >>>> +	int stop_thread;
> >>>> +
> >>>> +	struct pmd_queue rx_queues[KNI_MAX_QUEUE_PER_PORT];
> >>>> +	struct pmd_queue tx_queues[KNI_MAX_QUEUE_PER_PORT];
> >>>> +};
> >>>> +
> >>>> +static struct ether_addr eth_addr;
> >>>> +static struct rte_eth_link pmd_link = {
> >>>> +		.link_speed = ETH_SPEED_NUM_10G,
> >>>> +		.link_duplex = ETH_LINK_FULL_DUPLEX,
> >>>> +		.link_status = 0
> >>>> +};
> >>>> +static int is_kni_initialized;
> >>>> +
> >>>> +static uint16_t
> >>>> +eth_kni_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
> >>>> +{
> >>>> +	struct pmd_queue *kni_q = q;
> >>>> +	struct rte_kni *kni = kni_q->internals->kni;
> >>>> +	uint16_t nb_pkts;
> >>>> +
> >>>> +	nb_pkts = rte_kni_rx_burst(kni, bufs, nb_bufs);
> >>>> +
> >>>> +	kni_q->rx.pkts += nb_pkts;
> >>>> +	kni_q->rx.err_pkts += nb_bufs - nb_pkts;
> >>>> +
> >>>> +	return nb_pkts;
> >>>> +}
> >>>> +
> >>>> +static uint16_t
> >>>> +eth_kni_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
> >>>> +{
> >>>> +	struct pmd_queue *kni_q = q;
> >>>> +	struct rte_kni *kni = kni_q->internals->kni;
> >>>> +	uint16_t nb_pkts;
> >>>> +
> >>>> +	nb_pkts =  rte_kni_tx_burst(kni, bufs, nb_bufs);
> >>>> +
> >>>> +	kni_q->tx.pkts += nb_pkts;
> >>>> +	kni_q->tx.err_pkts += nb_bufs - nb_pkts;
> >>>> +
> >>>> +	return nb_pkts;
> >>>> +}
> >>>> +
> >>>> +static void *
> >>>> +kni_handle_request(void *param)
> >>>> +{
> >>>> +	struct pmd_internals *internals = param;
> >>>> +#define MS 1000
> >>>> +
> >>>> +	while (!internals->stop_thread) {
> >>>> +		rte_kni_handle_request(internals->kni);
> >>>> +		usleep(500 * MS);
> >>>> +	}
> >>>> +
> >>>> +	return param;
> >>>> +}
> >>>> +
> >>>
> >>> Do we really need a thread to handle request by default? I know there
> are
> >> apps that handle request their own way and having a separate thread
> could
> >> add synchronization problems.  Can we at least add an option to disable
> this?
> >>
> >> I didn't think about there can be a use case that requires own request
> >> handling.
> >>
> >> But, kni requests should be handled to make kni interface run properly,
> >> and to handle interface "kni" handler (internals->kni) required, which
> >> this PMD doesn't expose.
> >>
> >> So, just disabling this thread won't work on its own.
> >
> > I understand that and what I am asking is a way to at least disable this
> without having to make code changes for applications that have their own
> way of handling KNI request and the callback mentioned below sounds good
> to me.  I am fine with adding this capability with this commit or in a separate
> commit after you have this commit checked in.
> 
> I don't mind adding in new version, only I am trying to understand it.
> 
> Normally what it does is calling KNI library rte_kni_handle_request()
> API periodically on KNI handler. What an app may be doing own its way,
> other than tweaking the period?

It's the context that calls into rte_kni_handle_request() that I am referring to.  For applications that already handle this in their own thread or in the pmd thread, they don't need the extra thread created here.  It's not a big deal as they can just change the behavior by modifying the source code but I think it's reasonable to opt out of this default thread without making any source code changes to kni pmd.