From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yong Wang Subject: Re: [PATCH v4] net/kni: add KNI PMD Date: Mon, 19 Dec 2016 17:52:49 +0000 Message-ID: References: <20161010131946.13303-1-ferruh.yigit@intel.com> <20161130181228.25380-1-ferruh.yigit@intel.com> <479e706e-4d7e-53d1-e140-5c26f001c2c5@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: Ferruh Yigit , "dev@dpdk.org" Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0073.outbound.protection.outlook.com [104.47.40.73]) by dpdk.org (Postfix) with ESMTP id 682DFF91B for ; Mon, 19 Dec 2016 18:52:51 +0100 (CET) In-Reply-To: <479e706e-4d7e-53d1-e140-5c26f001c2c5@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] > Sent: Thursday, December 15, 2016 7:56 AM > To: Yong Wang ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4] net/kni: add KNI PMD >=20 > 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 ; 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 ; Yong Wang > >>>> > >>>> 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 > >>>> --- > >>>> > >>>> 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=3Dn > >>>> # Compile librte_kni > >>>> # > >>>> CONFIG_RTE_LIBRTE_KNI=3Dn > >>>> +CONFIG_RTE_LIBRTE_PMD_KNI=3Dn > >>>> CONFIG_RTE_KNI_KMOD=3Dn > >>>> CONFIG_RTE_KNI_PREEMPT_DEFAULT=3Dy > >>>> CONFIG_RTE_KNI_VHOST=3Dn > >>>> 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=3Dy > >>>> CONFIG_RTE_EAL_VFIO=3Dy > >>>> CONFIG_RTE_KNI_KMOD=3Dy > >>>> CONFIG_RTE_LIBRTE_KNI=3Dy > >>>> +CONFIG_RTE_LIBRTE_PMD_KNI=3Dy > >>>> CONFIG_RTE_LIBRTE_VHOST=3Dy > >>>> CONFIG_RTE_LIBRTE_PMD_VHOST=3Dy > >>>> CONFIG_RTE_LIBRTE_PMD_AF_PACKET=3Dy > >>>> 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) +=3D enic > >>>> DIRS-$(CONFIG_RTE_LIBRTE_FM10K_PMD) +=3D fm10k > >>>> DIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) +=3D i40e > >>>> DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) +=3D ixgbe > >>>> +DIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D kni > >>>> DIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) +=3D mlx4 > >>>> DIRS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) +=3D mlx5 > >>>> DIRS-$(CONFIG_RTE_LIBRTE_MPIPE_PMD) +=3D 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 with= out > >>>> +# modification, are permitted provided that the following conditi= ons > >>>> +# are met: > >>>> +# > >>>> +# * Redistributions of source code must retain the above copyri= ght > >>>> +# notice, this list of conditions and the following disclaime= r. > >>>> +# * Redistributions in binary form must reproduce the above > copyright > >>>> +# notice, this list of conditions and the following disclaime= r 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 permissio= n. > >>>> +# > >>>> +# 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 =3D librte_pmd_kni.a > >>>> + > >>>> +CFLAGS +=3D -O3 > >>>> +CFLAGS +=3D $(WERROR_FLAGS) > >>>> +LDLIBS +=3D -lpthread > >>>> + > >>>> +EXPORT_MAP :=3D rte_pmd_kni_version.map > >>>> + > >>>> +LIBABIVER :=3D 1 > >>>> + > >>>> +# > >>>> +# all source are stored in SRCS-y > >>>> +# > >>>> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D rte_eth_kni.c > >>>> + > >>>> +# > >>>> +# Export include files > >>>> +# > >>>> +SYMLINK-y-include +=3D > >>>> + > >>>> +# this lib depends upon: > >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D lib/librte_eal > >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D lib/librte_ether > >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D lib/librte_kni > >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D lib/librte_mbuf > >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_KNI) +=3D 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 wit= hout > >>>> + * modification, are permitted provided that the following condit= ions > >>>> + * are met: > >>>> + * > >>>> + * * Redistributions of source code must retain the above copyr= ight > >>>> + * notice, this list of conditions and the following disclaim= er. > >>>> + * * Redistributions in binary form must reproduce the above > copyright > >>>> + * notice, this list of conditions and the following disclaim= er 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 permissi= on. > >>>> + * > >>>> + * 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 > >>>> +#include > >>>> +#include > >>>> + > >>>> +#include > >>>> +#include > >>>> +#include > >>>> +#include > >>>> + > >>>> +/* 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 =3D { > >>>> + .link_speed =3D ETH_SPEED_NUM_10G, > >>>> + .link_duplex =3D ETH_LINK_FULL_DUPLEX, > >>>> + .link_status =3D 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 =3D q; > >>>> + struct rte_kni *kni =3D kni_q->internals->kni; > >>>> + uint16_t nb_pkts; > >>>> + > >>>> + nb_pkts =3D rte_kni_rx_burst(kni, bufs, nb_bufs); > >>>> + > >>>> + kni_q->rx.pkts +=3D nb_pkts; > >>>> + kni_q->rx.err_pkts +=3D 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 =3D q; > >>>> + struct rte_kni *kni =3D kni_q->internals->kni; > >>>> + uint16_t nb_pkts; > >>>> + > >>>> + nb_pkts =3D rte_kni_tx_burst(kni, bufs, nb_bufs); > >>>> + > >>>> + kni_q->tx.pkts +=3D nb_pkts; > >>>> + kni_q->tx.err_pkts +=3D nb_bufs - nb_pkts; > >>>> + > >>>> + return nb_pkts; > >>>> +} > >>>> + > >>>> +static void * > >>>> +kni_handle_request(void *param) > >>>> +{ > >>>> + struct pmd_internals *internals =3D 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 disabl= e > 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 thi= s > 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 se= parate > commit after you have this commit checked in. >=20 > I don't mind adding in new version, only I am trying to understand it. >=20 > 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 referri= ng 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 maki= ng any source code changes to kni pmd.