* KNI broken again with 4.9 kernel
From: Stephen Hemminger @ 2016-12-14 23:40 UTC (permalink / raw)
To: dev
/build/lib/librte_eal/linuxapp/kni/igb_main.c:2317:21: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.ndo_set_vf_vlan = igb_ndo_set_vf_vlan,
^~~~~~~~~~~~~~~~~~~
I am sure Ferruh Yigit will fix it.
Which raises a couple of questions:
1. Why is DPDK still keeping KNI support for Intel specific ethtool functionality.
This always breaks, is code bloat, and means a 3rd copy of base code (Linux, DPDK PMD, + KNI)
2. Why is KNI not upstream?
If not acceptable due to security or supportablity then why does it still exist?
3. If not upstream, then maintainer should track upstream kernel changes and fix DPDK before
kernel is released. The ABI is normally set early in the rc cycle weeks before release.
^ permalink raw reply
* Re: [PATCH v4] net/kni: add KNI PMD
From: Yong Wang @ 2016-12-14 19:25 UTC (permalink / raw)
To: Ferruh Yigit, dev@dpdk.org
In-Reply-To: <c7978d58-7254-c92d-7792-3cfe47e7e296@intel.com>
> -----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.
> A solution can be found, like callback registraion, or get_handler API,
> but if an application has custom request handling, perhaps it may prefer
> to use kni library directly instead of this wrapper, since wrapper
> already doesn't expose all kni features.
I think one of the motivation of having KNI pmd is that it's abstracted the same way as other physical or virtual devices. I think it makes sense to achieve feature parity with the KNI library as much as possible. What's currently supported in KNI library but missing in KNI PMD and any specific reason they are not supported?
> >
> >> +static int
> >> +eth_kni_start(struct rte_eth_dev *dev)
> >> +{
> >> + struct pmd_internals *internals = dev->data->dev_private;
> >> + uint16_t port_id = dev->data->port_id;
> >> + struct rte_mempool *mb_pool;
> >> + struct rte_kni_conf conf;
> >> + const char *name = dev->data->name + 4; /* remove net_ */
> >> +
> >> + snprintf(conf.name, RTE_KNI_NAMESIZE, "%s", name);
> >> + conf.force_bind = 0;
> >> + conf.group_id = port_id;
> >> + conf.mbuf_size = MAX_PACKET_SZ;
> >> + mb_pool = internals->rx_queues[0].mb_pool;
> >> +
> >> + internals->kni = rte_kni_alloc(mb_pool, &conf, NULL);
> >> + if (internals->kni == NULL) {
> >> + RTE_LOG(ERR, PMD,
> >> + "Fail to create kni for port: %d\n", port_id);
> >> + return -1;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +eth_kni_dev_start(struct rte_eth_dev *dev)
> >> +{
> >> + struct pmd_internals *internals = dev->data->dev_private;
> >> + int ret;
> >> +
> >> + if (internals->is_kni_started == 0) {
> >> + ret = eth_kni_start(dev);
> >> + if (ret)
> >> + return -1;
> >> + internals->is_kni_started = 1;
> >> + }
> >> +
> >
> > In case is_kni_started is 1 already, shouldn't we return directly instead of
> proceeding?
>
> "is_kni_started" is just to protect "eth_kni_start()", as you can see it
> doesn't have a counterpart in eth_kni_dev_stop(). This flag is to be
> sure "eth_kni_start()" called only once during PMD life cycle.
>
> The check you mentioned already done, start() / stop() functions already
> balanced by APIs calling these functions.
What about KNI request handing thread then? Is it safe to have multiple threads calling into rte_kni_handle_request()? My understanding is that this is not safe as kni_fifo is not multi-thread safe. It's also a bit wasteful to create multiple threads here.
> >
> >> + ret = pthread_create(&internals->thread, NULL, kni_handle_request,
> >> + internals);
> >> + if (ret) {
> >> + RTE_LOG(ERR, PMD, "Fail to create kni request thread\n");
> >> + return -1;
> >> + }
> >> +
> >> + dev->data->dev_link.link_status = 1;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void
> >> +eth_kni_dev_stop(struct rte_eth_dev *dev)
> >> +{
> >> + struct pmd_internals *internals = dev->data->dev_private;
> >> + int ret;
> >> +
> >> + internals->stop_thread = 1;
> >> +
> >> + ret = pthread_cancel(internals->thread);
> >> + if (ret)
> >> + RTE_LOG(ERR, PMD, "Can't cancel the thread\n");
> >> +
> >> + ret = pthread_join(internals->thread, NULL);
> >> + if (ret)
> >> + RTE_LOG(ERR, PMD, "Can't join the thread\n");
> >> +
> >> + internals->stop_thread = 0;
> >> +
> >> + dev->data->dev_link.link_status = 0;
> >> +}
> >> +
> >> +static int
> >> +eth_kni_dev_configure(struct rte_eth_dev *dev __rte_unused)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static void
> >> +eth_kni_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info
> >> *dev_info)
> >> +{
> >> + struct rte_eth_dev_data *data = dev->data;
> >> +
> >> + dev_info->driver_name = data->drv_name;
> >> + dev_info->max_mac_addrs = 1;
> >> + dev_info->max_rx_pktlen = (uint32_t)-1;
> >> + dev_info->max_rx_queues = KNI_MAX_QUEUE_PER_PORT;
> >> + dev_info->max_tx_queues = KNI_MAX_QUEUE_PER_PORT;
> >> + dev_info->min_rx_bufsize = 0;
> >> + dev_info->pci_dev = NULL;
> >> +}
> >> +
> >> +static int
> >> +eth_kni_rx_queue_setup(struct rte_eth_dev *dev,
> >> + uint16_t rx_queue_id,
> >> + uint16_t nb_rx_desc __rte_unused,
> >> + unsigned int socket_id __rte_unused,
> >> + const struct rte_eth_rxconf *rx_conf __rte_unused,
> >> + struct rte_mempool *mb_pool)
> >> +{
> >> + struct pmd_internals *internals = dev->data->dev_private;
> >> + struct pmd_queue *q;
> >> +
> >> + q = &internals->rx_queues[rx_queue_id];
> >> + q->internals = internals;
> >> + q->mb_pool = mb_pool;
> >> +
> >> + dev->data->rx_queues[rx_queue_id] = q;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +eth_kni_tx_queue_setup(struct rte_eth_dev *dev,
> >> + uint16_t tx_queue_id,
> >> + uint16_t nb_tx_desc __rte_unused,
> >> + unsigned int socket_id __rte_unused,
> >> + const struct rte_eth_txconf *tx_conf __rte_unused)
> >> +{
> >> + struct pmd_internals *internals = dev->data->dev_private;
> >> + struct pmd_queue *q;
> >> +
> >> + q = &internals->tx_queues[tx_queue_id];
> >> + q->internals = internals;
> >> +
> >> + dev->data->tx_queues[tx_queue_id] = q;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void
> >> +eth_kni_queue_release(void *q __rte_unused)
> >> +{
> >> +}
> >> +
> >> +static int
> >> +eth_kni_link_update(struct rte_eth_dev *dev __rte_unused,
> >> + int wait_to_complete __rte_unused)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static void
> >> +eth_kni_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> >> +{
> >> + unsigned long rx_packets_total = 0, rx_bytes_total = 0;
> >> + unsigned long tx_packets_total = 0, tx_bytes_total = 0;
> >> + struct rte_eth_dev_data *data = dev->data;
> >> + unsigned long tx_packets_err_total = 0;
> >> + unsigned int i, num_stats;
> >> + struct pmd_queue *q;
> >> +
> >> + num_stats = RTE_MIN((unsigned
> >> int)RTE_ETHDEV_QUEUE_STAT_CNTRS,
> >> + data->nb_rx_queues);
> >> + for (i = 0; i < num_stats; i++) {
> >> + q = data->rx_queues[i];
> >> + stats->q_ipackets[i] = q->rx.pkts;
> >> + stats->q_ibytes[i] = q->rx.bytes;
> >> + rx_packets_total += stats->q_ipackets[i];
> >> + rx_bytes_total += stats->q_ibytes[i];
> >> + }
> >> +
> >> + num_stats = RTE_MIN((unsigned
> >> int)RTE_ETHDEV_QUEUE_STAT_CNTRS,
> >> + data->nb_tx_queues);
> >> + for (i = 0; i < num_stats; i++) {
> >> + q = data->tx_queues[i];
> >> + stats->q_opackets[i] = q->tx.pkts;
> >> + stats->q_obytes[i] = q->tx.bytes;
> >> + stats->q_errors[i] = q->tx.err_pkts;
> >> + tx_packets_total += stats->q_opackets[i];
> >> + tx_bytes_total += stats->q_obytes[i];
> >> + tx_packets_err_total += stats->q_errors[i];
> >> + }
> >> +
> >> + stats->ipackets = rx_packets_total;
> >> + stats->ibytes = rx_bytes_total;
> >> + stats->opackets = tx_packets_total;
> >> + stats->obytes = tx_bytes_total;
> >> + stats->oerrors = tx_packets_err_total;
> >> +}
> >> +
> >> +static void
> >> +eth_kni_stats_reset(struct rte_eth_dev *dev)
> >> +{
> >> + struct rte_eth_dev_data *data = dev->data;
> >> + struct pmd_queue *q;
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < data->nb_rx_queues; i++) {
> >> + q = data->rx_queues[i];
> >> + q->rx.pkts = 0;
> >> + q->rx.bytes = 0;
> >> + }
> >> + for (i = 0; i < data->nb_tx_queues; i++) {
> >> + q = data->tx_queues[i];
> >> + q->tx.pkts = 0;
> >> + q->tx.bytes = 0;
> >> + q->tx.err_pkts = 0;
> >> + }
> >> +}
> >> +
> >> +static const struct eth_dev_ops eth_kni_ops = {
> >> + .dev_start = eth_kni_dev_start,
> >> + .dev_stop = eth_kni_dev_stop,
> >> + .dev_configure = eth_kni_dev_configure,
> >> + .dev_infos_get = eth_kni_dev_info,
> >> + .rx_queue_setup = eth_kni_rx_queue_setup,
> >> + .tx_queue_setup = eth_kni_tx_queue_setup,
> >> + .rx_queue_release = eth_kni_queue_release,
> >> + .tx_queue_release = eth_kni_queue_release,
> >> + .link_update = eth_kni_link_update,
> >> + .stats_get = eth_kni_stats_get,
> >> + .stats_reset = eth_kni_stats_reset,
> >> +};
> >> +
> >> +static struct rte_vdev_driver eth_kni_drv;
> >> +
> >> +static struct rte_eth_dev *
> >> +eth_kni_create(const char *name, unsigned int numa_node)
> >> +{
> >> + struct pmd_internals *internals = NULL;
> >> + struct rte_eth_dev_data *data;
> >> + struct rte_eth_dev *eth_dev;
> >> +
> >> + RTE_LOG(INFO, PMD, "Creating kni ethdev on numa socket %u\n",
> >> + numa_node);
> >> +
> >> + data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
> >> + if (data == NULL)
> >> + goto error;
> >> +
> >> + internals = rte_zmalloc_socket(name, sizeof(*internals), 0,
> >> numa_node);
> >> + if (internals == NULL)
> >> + goto error;
> >> +
> >> + /* reserve an ethdev entry */
> >> + eth_dev = rte_eth_dev_allocate(name);
> >> + if (eth_dev == NULL)
> >> + goto error;
> >> +
> >> + data->dev_private = internals;
> >> + data->port_id = eth_dev->data->port_id;
> >> + memmove(data->name, eth_dev->data->name, sizeof(data-
> >>> name));
> >> + data->nb_rx_queues = 1;
> >> + data->nb_tx_queues = 1;
> >> + data->dev_link = pmd_link;
> >> + data->mac_addrs = ð_addr;
> >> +
> >> + eth_dev->data = data;
> >> + eth_dev->dev_ops = ð_kni_ops;
> >> + eth_dev->driver = NULL;
> >> +
> >> + data->dev_flags = RTE_ETH_DEV_DETACHABLE;
> >> + data->kdrv = RTE_KDRV_NONE;
> >> + data->drv_name = eth_kni_drv.driver.name;
> >> + data->numa_node = numa_node;
> >> +
> >> + return eth_dev;
> >> +
> >> +error:
> >> + rte_free(data);
> >> + rte_free(internals);
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static int
> >> +kni_init(void)
> >> +{
> >> + if (is_kni_initialized == 0)
> >> + rte_kni_init(MAX_KNI_PORTS);
> >> +
> >> + is_kni_initialized += 1;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +eth_kni_probe(const char *name, const char *params __rte_unused)
> >> +{
> >> + struct rte_eth_dev *eth_dev;
> >> + int ret;
> >> +
> >> + RTE_LOG(INFO, PMD, "Initializing eth_kni for %s\n", name);
> >> +
> >> + ret = kni_init();
> >> + if (ret < 0)
> >> + /* Not return error to prevent panic in rte_eal_init() */
> >> + return 0;
> >
> > If we don't return error here, the application that needs to add KNI ports
> eventually will fail. If it's a fail-stop situation, isn't it better to return error
> where the it happened?
>
> I am not sure this is fail-stop situation, but instead this gives a
> chance to applicaton for a graceful exit.
>
> If an error value returned here, it will lead to a rte_panic() and
> application terminated abnormally!
>
> But if we return a success at this point, since no ethernet device
> created, there is no handler in application to use, which also means no
> KNI interface created.
> Application can check number of ports and recognize KNI port is missing,
> app may chose to terminate or not, also it prefers to terminate, can do
> it properly.
I might be wrong but as far as I know, other virtual or physical PMDS do not have this behavior. What you proposed makes sense but it also means that the application needs extra logic (checking if all ports are successfully initialized) to handle such failures (depending on the application, it might be able to proceed or it might need to fail-stop). Personally I would prefer consistency across all PMDs here no matter what behavior we choose here as that's the "contract" the application needs to know.
> >
> >> + eth_dev = eth_kni_create(name, rte_socket_id());
> >> + if (eth_dev == NULL)
> >> + return -1;
> >> +
> >> + eth_dev->rx_pkt_burst = eth_kni_rx;
> >> + eth_dev->tx_pkt_burst = eth_kni_tx;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +eth_kni_remove(const char *name)
> >> +{
> >> + struct rte_eth_dev *eth_dev;
> >> + struct pmd_internals *internals;
> >> +
> >> + RTE_LOG(INFO, PMD, "Un-Initializing eth_kni for %s\n", name);
> >> +
> >> + /* find the ethdev entry */
> >> + eth_dev = rte_eth_dev_allocated(name);
> >> + if (eth_dev == NULL)
> >> + return -1;
> >> +
> >> + eth_kni_dev_stop(eth_dev);
> >> +
> >> + if (eth_dev->data) {
> >> + internals = eth_dev->data->dev_private;
> >> + rte_kni_release(internals->kni);
> >> +
> >> + rte_free(internals);
> >> + }
> >> + rte_free(eth_dev->data);
> >> +
> >> + rte_eth_dev_release_port(eth_dev);
> >> +
> >> + is_kni_initialized -= 1;
> >> + if (is_kni_initialized == 0)
> >> + rte_kni_close();
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static struct rte_vdev_driver eth_kni_drv = {
> >> + .probe = eth_kni_probe,
> >> + .remove = eth_kni_remove,
> >> +};
> >> +
> >> +RTE_PMD_REGISTER_VDEV(net_kni, eth_kni_drv);
> >> diff --git a/drivers/net/kni/rte_pmd_kni_version.map
> >> b/drivers/net/kni/rte_pmd_kni_version.map
> >> new file mode 100644
> >> index 0000000..31eca32
> >> --- /dev/null
> >> +++ b/drivers/net/kni/rte_pmd_kni_version.map
> >> @@ -0,0 +1,4 @@
> >> +DPDK_17.02 {
> >> +
> >> + local: *;
> >> +};
> >> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> >> index f75f0e2..af02816 100644
> >> --- a/mk/rte.app.mk
> >> +++ b/mk/rte.app.mk
> >> @@ -59,11 +59,6 @@ _LDLIBS-y += -L$(RTE_SDK_BIN)/lib
> >> #
> >> # Order is important: from higher level to lower level
> >> #
> >> -
> >> -ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> >> -_LDLIBS-$(CONFIG_RTE_LIBRTE_KNI) += -lrte_kni
> >> -endif
> >> -
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_PIPELINE) += -lrte_pipeline
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_TABLE) += -lrte_table
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_PORT) += -lrte_port
> >> @@ -84,6 +79,10 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER) += -
> >> lrte_power
> >>
> >> _LDLIBS-y += --whole-archive
> >>
> >> +ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> >> +_LDLIBS-$(CONFIG_RTE_LIBRTE_KNI) += -lrte_kni
> >> +endif
> >> +
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER) += -lrte_timer
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH) += -lrte_hash
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST) += -lrte_vhost
> >> @@ -115,6 +114,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) +=
> -
> >> lrte_pmd_enic
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_FM10K_PMD) += -lrte_pmd_fm10k
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += -lrte_pmd_i40e
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> >> +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += -lrte_pmd_kni
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += -lrte_pmd_mlx4 -
> >> libverbs
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += -lrte_pmd_mlx5 -
> >> libverbs
> >> _LDLIBS-$(CONFIG_RTE_LIBRTE_MPIPE_PMD) += -lrte_pmd_mpipe -
> lgxio
> >> --
> >> 2.9.3
> >
^ permalink raw reply
* [PATCH v4] null: fake PMD capabilities
From: Michał Mirosław @ 2016-12-14 19:16 UTC (permalink / raw)
To: dev
In-Reply-To: <66806fbdd31de8610a0be5463d813eaf7b816e85.1481592848.git.mirq-linux@rere.qmqm.pl>
From: Paweł Małachowski <pawel.malachowski@atendesoftware.pl>
This allows for testing code which needs offloads to be supported.
Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
---
drivers/net/null/rte_eth_null.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 836d982..e60516f 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -284,6 +284,9 @@ eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
return 0;
}
+static void
+eth_dev_void_ok(struct rte_eth_dev *dev __rte_unused) { return; }
+
static void
eth_dev_info(struct rte_eth_dev *dev,
@@ -304,6 +307,19 @@ eth_dev_info(struct rte_eth_dev *dev,
dev_info->pci_dev = NULL;
dev_info->reta_size = internals->reta_size;
dev_info->flow_type_rss_offloads = internals->flow_type_rss_offloads;
+ /* We hereby declare we can RX offload VLAN-s out of thin air (they are not there)
+ */
+ dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP |
+ DEV_RX_OFFLOAD_QINQ_STRIP;
+ /* We promise we will do any TX offloads before passing packets to /dev/null
+ */
+ dev_info->tx_offload_capa = DEV_TX_OFFLOAD_VLAN_INSERT |
+ DEV_TX_OFFLOAD_IPV4_CKSUM | DEV_TX_OFFLOAD_UDP_CKSUM |
+ DEV_TX_OFFLOAD_TCP_CKSUM | DEV_TX_OFFLOAD_SCTP_CKSUM |
+ DEV_TX_OFFLOAD_TCP_TSO | DEV_TX_OFFLOAD_UDP_TSO |
+ DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM | DEV_TX_OFFLOAD_QINQ_INSERT |
+ DEV_TX_OFFLOAD_VXLAN_TNL_TSO | DEV_TX_OFFLOAD_GRE_TNL_TSO |
+ DEV_TX_OFFLOAD_IPIP_TNL_TSO | DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
}
static void
@@ -477,7 +493,12 @@ static const struct eth_dev_ops ops = {
.reta_update = eth_rss_reta_update,
.reta_query = eth_rss_reta_query,
.rss_hash_update = eth_rss_hash_update,
- .rss_hash_conf_get = eth_rss_hash_conf_get
+ .rss_hash_conf_get = eth_rss_hash_conf_get,
+ /* Fake our capabilities */
+ .promiscuous_enable = eth_dev_void_ok,
+ .promiscuous_disable = eth_dev_void_ok,
+ .allmulticast_enable = eth_dev_void_ok,
+ .allmulticast_disable = eth_dev_void_ok
};
int
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 11/13] KNI: guard against unterminated dev_info.name leading to BUG in alloc_netdev()
From: Ferruh Yigit @ 2016-12-14 17:48 UTC (permalink / raw)
To: Michał Mirosław; +Cc: dev
In-Reply-To: <20161214173755.lnfn3qlnqdmanqi2@rere.qmqm.pl>
On 12/14/2016 5:37 PM, Michał Mirosław wrote:
> On Wed, Dec 14, 2016 at 05:33:11PM +0000, Ferruh Yigit wrote:
>> On 12/13/2016 1:08 AM, Michał Mirosław wrote:
>>> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
>>> ---
>>
>> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> I guess no harm doing extra check on user input.
>
> This actually prevents an OOPS that happens when you try to create KNI with
> too long name.
Thanks for fixing then.
^ permalink raw reply
* Re: [PATCH 07/13] pcap: fix timestamps in output pcap file
From: Ferruh Yigit @ 2016-12-14 17:45 UTC (permalink / raw)
To: Michał Mirosław, dev
In-Reply-To: <2a15ee2b33cf58e8cfbfe8d51d3af32b97595424.1481590851.git.mirq-linux@rere.qmqm.pl>
On 12/13/2016 1:08 AM, Michał Mirosław wrote:
> From: Piotr Bartosiewicz <piotr.bartosiewicz@atendesoftware.pl>
>
> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> ---
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
^ permalink raw reply
* Re: [PATCH 11/13] KNI: guard against unterminated dev_info.name leading to BUG in alloc_netdev()
From: Michał Mirosław @ 2016-12-14 17:37 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev
In-Reply-To: <5b69bbf4-f570-26c4-79e4-c3342769d81d@intel.com>
On Wed, Dec 14, 2016 at 05:33:11PM +0000, Ferruh Yigit wrote:
> On 12/13/2016 1:08 AM, Michał Mirosław wrote:
> > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> > ---
>
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> I guess no harm doing extra check on user input.
This actually prevents an OOPS that happens when you try to create KNI with
too long name.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH 10/13] KNI: provided netif name's source is user-space
From: Ferruh Yigit @ 2016-12-14 17:35 UTC (permalink / raw)
To: Michał Mirosław; +Cc: dev
In-Reply-To: <ac5c7173-0250-de63-1383-122be21472c4@intel.com>
On 12/14/2016 5:35 PM, Ferruh Yigit wrote:
> On 12/14/2016 5:19 PM, Michał Mirosław wrote:
>> On Wed, Dec 14, 2016 at 05:06:23PM +0000, Ferruh Yigit wrote:
>>> Hi Michal,
>>>
>>> On 12/13/2016 1:08 AM, Michał Mirosław wrote:
>>>> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
>>>> ---
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
^ permalink raw reply
* Re: [PATCH 10/13] KNI: provided netif name's source is user-space
From: Ferruh Yigit @ 2016-12-14 17:35 UTC (permalink / raw)
To: Michał Mirosław; +Cc: dev
In-Reply-To: <20161214171911.nupex5gi5jxbbbhm@rere.qmqm.pl>
On 12/14/2016 5:19 PM, Michał Mirosław wrote:
> On Wed, Dec 14, 2016 at 05:06:23PM +0000, Ferruh Yigit wrote:
>> Hi Michal,
>>
>> On 12/13/2016 1:08 AM, Michał Mirosław wrote:
>>> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
>>> ---
>>> lib/librte_eal/linuxapp/kni/kni_misc.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
>>> index 497db9b..f0247aa 100644
>>> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
>>> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
>>> @@ -363,8 +363,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>>> up_read(&knet->kni_list_lock);
>>>
>>> net_dev = alloc_netdev(sizeof(struct kni_dev), dev_info.name,
>>> -#ifdef NET_NAME_UNKNOWN
>>> - NET_NAME_UNKNOWN,
>>> +#ifdef NET_NAME_USER
>>> + NET_NAME_USER,
>>
>> Probably NET_NAME_USER fits better to definition.
>> But functionally, I wonder if you have a use case to use
>> "name_assign_type" ?
>
> I just read what NET_NAME_UNKNOWN/NET_NAME_USER is supposed to do.
> UNKNOWN is for "old" drivers that don't know the source of the name.
> USER is for a name, that comes from a user and as such is not to be
> renamed by udev.
That is what I wonder if you are doing anything special in userspace
related iface name. But it seems the patch is to make it more proper, I
have no objection.
>
> Best Regards,
> Michał Mirosław
>
^ permalink raw reply
* Re: [PATCH 11/13] KNI: guard against unterminated dev_info.name leading to BUG in alloc_netdev()
From: Ferruh Yigit @ 2016-12-14 17:33 UTC (permalink / raw)
To: Michał Mirosław, dev
In-Reply-To: <d1c6dd8ca22b51b781b2ce9f305f6a39ba9a914b.1481590851.git.mirq-linux@rere.qmqm.pl>
On 12/13/2016 1:08 AM, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> ---
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
I guess no harm doing extra check on user input.
^ permalink raw reply
* [PATCH] acl: remove invalid test
From: Michał Mirosław @ 2016-12-14 17:23 UTC (permalink / raw)
To: dev; +Cc: Ananyev, Konstantin
In-Reply-To: <f6d14b640928f69ab747793cd8749b48a1b74afb.1481590851.git.mirq-linux@rere.qmqm.pl>
rte_acl_add_rules() has no way of checking rule size.
This was hidden because the test effectively checked that
adding a rule with userdata == 0 failed.
Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
---
app/test/test_acl.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/app/test/test_acl.c b/app/test/test_acl.c
index 28955f0..be744ec 100644
--- a/app/test/test_acl.c
+++ b/app/test/test_acl.c
@@ -1515,26 +1515,6 @@ test_invalid_parameters(void)
/* free ACL context */
rte_acl_free(acx);
- /* set wrong rule_size so that adding any rules would fail */
- param.rule_size = RTE_ACL_IPV4VLAN_RULE_SZ + 4;
- acx = rte_acl_create(¶m);
- if (acx == NULL) {
- printf("Line %i: ACL context creation failed!\n", __LINE__);
- return -1;
- }
-
- /* try adding a rule with size different from context rule_size */
- result = rte_acl_ipv4vlan_add_rules(acx, &rule, 1);
- if (result == 0) {
- printf("Line %i: Adding an invalid sized rule "
- "should have failed!\n", __LINE__);
- rte_acl_free(acx);
- return -1;
- }
-
- /* free ACL context */
- rte_acl_free(acx);
-
/**
* rte_acl_ipv4vlan_build
--
2.10.2
^ permalink raw reply related
* [PATCH v2] acl: allow zero verdict
From: Michał Mirosław @ 2016-12-14 17:23 UTC (permalink / raw)
To: dev; +Cc: Ananyev, Konstantin
In-Reply-To: <f6d14b640928f69ab747793cd8749b48a1b74afb.1481590851.git.mirq-linux@rere.qmqm.pl>
Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
---
v2: fixes prog_guide and ACL tests
---
app/test/test_acl.c | 13 -------------
doc/guides/prog_guide/packet_classif_access_ctrl.rst | 3 ++-
lib/librte_acl/rte_acl.c | 3 +--
lib/librte_acl/rte_acl.h | 2 --
lib/librte_table/rte_table_acl.c | 2 +-
5 files changed, 4 insertions(+), 19 deletions(-)
diff --git a/app/test/test_acl.c b/app/test/test_acl.c
index be744ec..c6b511f 100644
--- a/app/test/test_acl.c
+++ b/app/test/test_acl.c
@@ -1357,19 +1357,6 @@ test_invalid_rules(void)
goto err;
}
- rule.dst_mask_len = 0;
- rule.src_mask_len = 0;
- rule.data.userdata = 0;
-
- /* try adding this rule (it should fail because userdata is invalid) */
- ret = rte_acl_ipv4vlan_add_rules(acx, &rule, 1);
- if (ret == 0) {
- printf("Line %i: Adding a rule with invalid user data "
- "should have failed!\n", __LINE__);
- rte_acl_free(acx);
- return -1;
- }
-
rte_acl_free(acx);
return 0;
diff --git a/doc/guides/prog_guide/packet_classif_access_ctrl.rst b/doc/guides/prog_guide/packet_classif_access_ctrl.rst
index 5fd3d34..a6bee9b 100644
--- a/doc/guides/prog_guide/packet_classif_access_ctrl.rst
+++ b/doc/guides/prog_guide/packet_classif_access_ctrl.rst
@@ -329,8 +329,9 @@ When creating a set of rules, for each rule, additional information must be supp
Each set could be assigned its own category and by combining them into a single database,
one lookup returns a result for each of the four sets.
-* **userdata**: A user-defined field that could be any value except zero.
+* **userdata**: A user-defined value.
For each category, a successful match returns the userdata field of the highest priority matched rule.
+ When no rules match, returned value is zero.
.. note::
diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_acl/rte_acl.c
index 8b7e92c..d1f40be 100644
--- a/lib/librte_acl/rte_acl.c
+++ b/lib/librte_acl/rte_acl.c
@@ -313,8 +313,7 @@ acl_check_rule(const struct rte_acl_rule_data *rd)
if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd->category_mask)) &
rd->category_mask) == 0 ||
rd->priority > RTE_ACL_MAX_PRIORITY ||
- rd->priority < RTE_ACL_MIN_PRIORITY ||
- rd->userdata == RTE_ACL_INVALID_USERDATA)
+ rd->priority < RTE_ACL_MIN_PRIORITY)
return -EINVAL;
return 0;
}
diff --git a/lib/librte_acl/rte_acl.h b/lib/librte_acl/rte_acl.h
index caa91f7..b53179a 100644
--- a/lib/librte_acl/rte_acl.h
+++ b/lib/librte_acl/rte_acl.h
@@ -120,8 +120,6 @@ enum {
RTE_ACL_MIN_PRIORITY = 0,
};
-#define RTE_ACL_INVALID_USERDATA 0
-
#define RTE_ACL_MASKLEN_TO_BITMASK(v, s) \
((v) == 0 ? (v) : (typeof(v))((uint64_t)-1 << ((s) * CHAR_BIT - (v))))
diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
index 8f1f8ce..94b69a9 100644
--- a/lib/librte_table/rte_table_acl.c
+++ b/lib/librte_table/rte_table_acl.c
@@ -792,7 +792,7 @@ rte_table_acl_lookup(
pkts_mask &= ~pkt_mask;
- if (action_table_pos != RTE_ACL_INVALID_USERDATA) {
+ if (action_table_pos != 0) {
pkts_out_mask |= pkt_mask;
entries[pkt_pos] = (void *)
&acl->memory[action_table_pos *
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 10/13] KNI: provided netif name's source is user-space
From: Michał Mirosław @ 2016-12-14 17:19 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev
In-Reply-To: <817f957e-80c3-bfc0-e02b-55f397581708@intel.com>
On Wed, Dec 14, 2016 at 05:06:23PM +0000, Ferruh Yigit wrote:
> Hi Michal,
>
> On 12/13/2016 1:08 AM, Michał Mirosław wrote:
> > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> > ---
> > lib/librte_eal/linuxapp/kni/kni_misc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > index 497db9b..f0247aa 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > @@ -363,8 +363,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
> > up_read(&knet->kni_list_lock);
> >
> > net_dev = alloc_netdev(sizeof(struct kni_dev), dev_info.name,
> > -#ifdef NET_NAME_UNKNOWN
> > - NET_NAME_UNKNOWN,
> > +#ifdef NET_NAME_USER
> > + NET_NAME_USER,
>
> Probably NET_NAME_USER fits better to definition.
> But functionally, I wonder if you have a use case to use
> "name_assign_type" ?
I just read what NET_NAME_UNKNOWN/NET_NAME_USER is supposed to do.
UNKNOWN is for "old" drivers that don't know the source of the name.
USER is for a name, that comes from a user and as such is not to be
renamed by udev.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH 10/13] KNI: provided netif name's source is user-space
From: Ferruh Yigit @ 2016-12-14 17:06 UTC (permalink / raw)
To: Michał Mirosław, dev
In-Reply-To: <ad817bb663ce3b8641b8884f1e994b145d141246.1481590851.git.mirq-linux@rere.qmqm.pl>
Hi Michal,
On 12/13/2016 1:08 AM, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> ---
> lib/librte_eal/linuxapp/kni/kni_misc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 497db9b..f0247aa 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -363,8 +363,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
> up_read(&knet->kni_list_lock);
>
> net_dev = alloc_netdev(sizeof(struct kni_dev), dev_info.name,
> -#ifdef NET_NAME_UNKNOWN
> - NET_NAME_UNKNOWN,
> +#ifdef NET_NAME_USER
> + NET_NAME_USER,
Probably NET_NAME_USER fits better to definition.
But functionally, I wonder if you have a use case to use
"name_assign_type" ?
> #endif
> kni_net_init);
> if (net_dev == NULL) {
>
^ permalink raw reply
* Re: No packets received if burst is too small in rte_eth_rx_burst
From: Bruce Richardson @ 2016-12-14 16:52 UTC (permalink / raw)
To: tom.barbette; +Cc: dev
In-Reply-To: <597694261.17905196.1481728433903.JavaMail.zimbra@ulg.ac.be>
On Wed, Dec 14, 2016 at 04:13:53PM +0100, tom.barbette@ulg.ac.be wrote:
> Hi list,
>
> Between 2.2.0 and 16.04 (up to at least 16.07.2 if not current), with the XL710 controller I do not get any packet when calling rte_eth_rx_burst if nb_pkts is too small. I would say smaller than 32. The input rate is not big, if that helps. But It should definitely get at least one packet per second.
>
> Any ideas? Is that a bug or expected behaviour? Could be caused by other ABI changes?
>
Does this issue still occur even if you disable the vector driver in
your build-time configuration?
/Bruce
^ permalink raw reply
* Re: [PATCH 01/22] ethdev: introduce generic flow API
From: Kevin Traynor @ 2016-12-14 16:11 UTC (permalink / raw)
To: Adrien Mazarguil
Cc: dev, Thomas Monjalon, Pablo de Lara, Olivier Matz,
sugesh.chandran
In-Reply-To: <20161214135423.GZ10340@6wind.com>
On 12/14/2016 01:54 PM, Adrien Mazarguil wrote:
>>
>>>>>>> + * @param[out] error
>>>>>>> + * Perform verbose error reporting if not NULL.
>>>>>>> + *
>>>>>>> + * @return
>>>>>>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
>>>>>>> + */
>>>>>>> +int
>>>>>>> +rte_flow_query(uint8_t port_id,
>>>>>>> + struct rte_flow *flow,
>>>>>>> + enum rte_flow_action_type action,
>>>>>>> + void *data,
>>>>>>> + struct rte_flow_error *error);
>>>>>>> +
>>>>>>> +#ifdef __cplusplus
>>>>>>> +}
>>>>>>> +#endif
>>>>>>
>>>>>> I don't see a way to dump all the rules for a port out. I think this is
>>>>>> neccessary for degbugging. You could have a look through dpif.h in OVS
>>>>>> and see how dpif_flow_dump_next() is used, it might be a good reference.
>>>>>
>>>>> DPDK does not maintain flow rules and, depending on hardware capabilities
>>>>> and level of compliance, PMDs do not necessarily do it either, particularly
>>>>> since it requires space and application probably have a better method to
>>>>> store these pointers for their own needs.
>>>>
>>>> understood
>>>>
>>>>>
>>>>> What you see here is only a PMD interface. Depending on applications needs,
>>>>> generic helper functions built on top of these may be added to manage flow
>>>>> rules in the future.
>>>>
>>>> I'm thinking of the case where something goes wrong and I want to get a
>>>> dump of all the flow rules from hardware, not query the rules I think I
>>>> have. I don't see a way to do it or something to build a helper on top of?
>>>
>>> Generic helper functions would exist on top of this API and would likely
>>> maintain a list of flow rules themselves. The dump in that case would be
>>> entirely implemented in software. I think that recovering flow rules from HW
>>> may be complicated in many cases (even without taking storage allocation and
>>> rules conversion issues into account), therefore if there is really a need
>>> for it, we could perhaps add a dump() function that PMDs are free to
>>> implement later.
>>>
>>
>> ok. Maybe there are some more generic stats that can be got from the
>> hardware that would help debugging that would suffice, like total flow
>> rule hits/misses (i.e. not on a per flow rule basis).
>>
>> You can get this from the software flow caches and it's widely used for
>> debugging. e.g.
>>
>> pmd thread numa_id 0 core_id 3:
>> emc hits:0
>> megaflow hits:0
>> avg. subtable lookups per hit:0.00
>> miss:0
>>
>
> Perhaps a rule such as the following could do the trick:
>
> group: 42 (or priority 42)
> pattern: void
> actions: count / passthru
>
> Assuming useful flow rules are defined with higher priorities (using lower
> group ID or priority level) and provide a terminating action, this one would
> count all packets that were not caught by them.
>
> That is one example to illustrate how "global" counters can be requested by
> applications.
>
> Otherwise you could just make sure all rules contain mark / flag actions, in
> which case mbufs would tell directly if they went through them or need
> additional SW processing.
>
ok, sounds like there's some options at least to work with on this which
is good. thanks.
^ permalink raw reply
* Re: [PATCH v4] net/kni: add KNI PMD
From: Ferruh Yigit @ 2016-12-14 15:59 UTC (permalink / raw)
To: Yong Wang, dev@dpdk.org
In-Reply-To: <BY2PR05MB23595855DDDC3B10D01E651CAF980@BY2PR05MB2359.namprd05.prod.outlook.com>
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.
A solution can be found, like callback registraion, or get_handler API,
but if an application has custom request handling, perhaps it may prefer
to use kni library directly instead of this wrapper, since wrapper
already doesn't expose all kni features.
>
>> +static int
>> +eth_kni_start(struct rte_eth_dev *dev)
>> +{
>> + struct pmd_internals *internals = dev->data->dev_private;
>> + uint16_t port_id = dev->data->port_id;
>> + struct rte_mempool *mb_pool;
>> + struct rte_kni_conf conf;
>> + const char *name = dev->data->name + 4; /* remove net_ */
>> +
>> + snprintf(conf.name, RTE_KNI_NAMESIZE, "%s", name);
>> + conf.force_bind = 0;
>> + conf.group_id = port_id;
>> + conf.mbuf_size = MAX_PACKET_SZ;
>> + mb_pool = internals->rx_queues[0].mb_pool;
>> +
>> + internals->kni = rte_kni_alloc(mb_pool, &conf, NULL);
>> + if (internals->kni == NULL) {
>> + RTE_LOG(ERR, PMD,
>> + "Fail to create kni for port: %d\n", port_id);
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +eth_kni_dev_start(struct rte_eth_dev *dev)
>> +{
>> + struct pmd_internals *internals = dev->data->dev_private;
>> + int ret;
>> +
>> + if (internals->is_kni_started == 0) {
>> + ret = eth_kni_start(dev);
>> + if (ret)
>> + return -1;
>> + internals->is_kni_started = 1;
>> + }
>> +
>
> In case is_kni_started is 1 already, shouldn't we return directly instead of proceeding?
"is_kni_started" is just to protect "eth_kni_start()", as you can see it
doesn't have a counterpart in eth_kni_dev_stop(). This flag is to be
sure "eth_kni_start()" called only once during PMD life cycle.
The check you mentioned already done, start() / stop() functions already
balanced by APIs calling these functions.
>
>> + ret = pthread_create(&internals->thread, NULL, kni_handle_request,
>> + internals);
>> + if (ret) {
>> + RTE_LOG(ERR, PMD, "Fail to create kni request thread\n");
>> + return -1;
>> + }
>> +
>> + dev->data->dev_link.link_status = 1;
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +eth_kni_dev_stop(struct rte_eth_dev *dev)
>> +{
>> + struct pmd_internals *internals = dev->data->dev_private;
>> + int ret;
>> +
>> + internals->stop_thread = 1;
>> +
>> + ret = pthread_cancel(internals->thread);
>> + if (ret)
>> + RTE_LOG(ERR, PMD, "Can't cancel the thread\n");
>> +
>> + ret = pthread_join(internals->thread, NULL);
>> + if (ret)
>> + RTE_LOG(ERR, PMD, "Can't join the thread\n");
>> +
>> + internals->stop_thread = 0;
>> +
>> + dev->data->dev_link.link_status = 0;
>> +}
>> +
>> +static int
>> +eth_kni_dev_configure(struct rte_eth_dev *dev __rte_unused)
>> +{
>> + return 0;
>> +}
>> +
>> +static void
>> +eth_kni_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info
>> *dev_info)
>> +{
>> + struct rte_eth_dev_data *data = dev->data;
>> +
>> + dev_info->driver_name = data->drv_name;
>> + dev_info->max_mac_addrs = 1;
>> + dev_info->max_rx_pktlen = (uint32_t)-1;
>> + dev_info->max_rx_queues = KNI_MAX_QUEUE_PER_PORT;
>> + dev_info->max_tx_queues = KNI_MAX_QUEUE_PER_PORT;
>> + dev_info->min_rx_bufsize = 0;
>> + dev_info->pci_dev = NULL;
>> +}
>> +
>> +static int
>> +eth_kni_rx_queue_setup(struct rte_eth_dev *dev,
>> + uint16_t rx_queue_id,
>> + uint16_t nb_rx_desc __rte_unused,
>> + unsigned int socket_id __rte_unused,
>> + const struct rte_eth_rxconf *rx_conf __rte_unused,
>> + struct rte_mempool *mb_pool)
>> +{
>> + struct pmd_internals *internals = dev->data->dev_private;
>> + struct pmd_queue *q;
>> +
>> + q = &internals->rx_queues[rx_queue_id];
>> + q->internals = internals;
>> + q->mb_pool = mb_pool;
>> +
>> + dev->data->rx_queues[rx_queue_id] = q;
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +eth_kni_tx_queue_setup(struct rte_eth_dev *dev,
>> + uint16_t tx_queue_id,
>> + uint16_t nb_tx_desc __rte_unused,
>> + unsigned int socket_id __rte_unused,
>> + const struct rte_eth_txconf *tx_conf __rte_unused)
>> +{
>> + struct pmd_internals *internals = dev->data->dev_private;
>> + struct pmd_queue *q;
>> +
>> + q = &internals->tx_queues[tx_queue_id];
>> + q->internals = internals;
>> +
>> + dev->data->tx_queues[tx_queue_id] = q;
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +eth_kni_queue_release(void *q __rte_unused)
>> +{
>> +}
>> +
>> +static int
>> +eth_kni_link_update(struct rte_eth_dev *dev __rte_unused,
>> + int wait_to_complete __rte_unused)
>> +{
>> + return 0;
>> +}
>> +
>> +static void
>> +eth_kni_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>> +{
>> + unsigned long rx_packets_total = 0, rx_bytes_total = 0;
>> + unsigned long tx_packets_total = 0, tx_bytes_total = 0;
>> + struct rte_eth_dev_data *data = dev->data;
>> + unsigned long tx_packets_err_total = 0;
>> + unsigned int i, num_stats;
>> + struct pmd_queue *q;
>> +
>> + num_stats = RTE_MIN((unsigned
>> int)RTE_ETHDEV_QUEUE_STAT_CNTRS,
>> + data->nb_rx_queues);
>> + for (i = 0; i < num_stats; i++) {
>> + q = data->rx_queues[i];
>> + stats->q_ipackets[i] = q->rx.pkts;
>> + stats->q_ibytes[i] = q->rx.bytes;
>> + rx_packets_total += stats->q_ipackets[i];
>> + rx_bytes_total += stats->q_ibytes[i];
>> + }
>> +
>> + num_stats = RTE_MIN((unsigned
>> int)RTE_ETHDEV_QUEUE_STAT_CNTRS,
>> + data->nb_tx_queues);
>> + for (i = 0; i < num_stats; i++) {
>> + q = data->tx_queues[i];
>> + stats->q_opackets[i] = q->tx.pkts;
>> + stats->q_obytes[i] = q->tx.bytes;
>> + stats->q_errors[i] = q->tx.err_pkts;
>> + tx_packets_total += stats->q_opackets[i];
>> + tx_bytes_total += stats->q_obytes[i];
>> + tx_packets_err_total += stats->q_errors[i];
>> + }
>> +
>> + stats->ipackets = rx_packets_total;
>> + stats->ibytes = rx_bytes_total;
>> + stats->opackets = tx_packets_total;
>> + stats->obytes = tx_bytes_total;
>> + stats->oerrors = tx_packets_err_total;
>> +}
>> +
>> +static void
>> +eth_kni_stats_reset(struct rte_eth_dev *dev)
>> +{
>> + struct rte_eth_dev_data *data = dev->data;
>> + struct pmd_queue *q;
>> + unsigned int i;
>> +
>> + for (i = 0; i < data->nb_rx_queues; i++) {
>> + q = data->rx_queues[i];
>> + q->rx.pkts = 0;
>> + q->rx.bytes = 0;
>> + }
>> + for (i = 0; i < data->nb_tx_queues; i++) {
>> + q = data->tx_queues[i];
>> + q->tx.pkts = 0;
>> + q->tx.bytes = 0;
>> + q->tx.err_pkts = 0;
>> + }
>> +}
>> +
>> +static const struct eth_dev_ops eth_kni_ops = {
>> + .dev_start = eth_kni_dev_start,
>> + .dev_stop = eth_kni_dev_stop,
>> + .dev_configure = eth_kni_dev_configure,
>> + .dev_infos_get = eth_kni_dev_info,
>> + .rx_queue_setup = eth_kni_rx_queue_setup,
>> + .tx_queue_setup = eth_kni_tx_queue_setup,
>> + .rx_queue_release = eth_kni_queue_release,
>> + .tx_queue_release = eth_kni_queue_release,
>> + .link_update = eth_kni_link_update,
>> + .stats_get = eth_kni_stats_get,
>> + .stats_reset = eth_kni_stats_reset,
>> +};
>> +
>> +static struct rte_vdev_driver eth_kni_drv;
>> +
>> +static struct rte_eth_dev *
>> +eth_kni_create(const char *name, unsigned int numa_node)
>> +{
>> + struct pmd_internals *internals = NULL;
>> + struct rte_eth_dev_data *data;
>> + struct rte_eth_dev *eth_dev;
>> +
>> + RTE_LOG(INFO, PMD, "Creating kni ethdev on numa socket %u\n",
>> + numa_node);
>> +
>> + data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
>> + if (data == NULL)
>> + goto error;
>> +
>> + internals = rte_zmalloc_socket(name, sizeof(*internals), 0,
>> numa_node);
>> + if (internals == NULL)
>> + goto error;
>> +
>> + /* reserve an ethdev entry */
>> + eth_dev = rte_eth_dev_allocate(name);
>> + if (eth_dev == NULL)
>> + goto error;
>> +
>> + data->dev_private = internals;
>> + data->port_id = eth_dev->data->port_id;
>> + memmove(data->name, eth_dev->data->name, sizeof(data-
>>> name));
>> + data->nb_rx_queues = 1;
>> + data->nb_tx_queues = 1;
>> + data->dev_link = pmd_link;
>> + data->mac_addrs = ð_addr;
>> +
>> + eth_dev->data = data;
>> + eth_dev->dev_ops = ð_kni_ops;
>> + eth_dev->driver = NULL;
>> +
>> + data->dev_flags = RTE_ETH_DEV_DETACHABLE;
>> + data->kdrv = RTE_KDRV_NONE;
>> + data->drv_name = eth_kni_drv.driver.name;
>> + data->numa_node = numa_node;
>> +
>> + return eth_dev;
>> +
>> +error:
>> + rte_free(data);
>> + rte_free(internals);
>> +
>> + return NULL;
>> +}
>> +
>> +static int
>> +kni_init(void)
>> +{
>> + if (is_kni_initialized == 0)
>> + rte_kni_init(MAX_KNI_PORTS);
>> +
>> + is_kni_initialized += 1;
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +eth_kni_probe(const char *name, const char *params __rte_unused)
>> +{
>> + struct rte_eth_dev *eth_dev;
>> + int ret;
>> +
>> + RTE_LOG(INFO, PMD, "Initializing eth_kni for %s\n", name);
>> +
>> + ret = kni_init();
>> + if (ret < 0)
>> + /* Not return error to prevent panic in rte_eal_init() */
>> + return 0;
>
> If we don't return error here, the application that needs to add KNI ports eventually will fail. If it's a fail-stop situation, isn't it better to return error where the it happened?
I am not sure this is fail-stop situation, but instead this gives a
chance to applicaton for a graceful exit.
If an error value returned here, it will lead to a rte_panic() and
application terminated abnormally!
But if we return a success at this point, since no ethernet device
created, there is no handler in application to use, which also means no
KNI interface created.
Application can check number of ports and recognize KNI port is missing,
app may chose to terminate or not, also it prefers to terminate, can do
it properly.
>
>> + eth_dev = eth_kni_create(name, rte_socket_id());
>> + if (eth_dev == NULL)
>> + return -1;
>> +
>> + eth_dev->rx_pkt_burst = eth_kni_rx;
>> + eth_dev->tx_pkt_burst = eth_kni_tx;
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +eth_kni_remove(const char *name)
>> +{
>> + struct rte_eth_dev *eth_dev;
>> + struct pmd_internals *internals;
>> +
>> + RTE_LOG(INFO, PMD, "Un-Initializing eth_kni for %s\n", name);
>> +
>> + /* find the ethdev entry */
>> + eth_dev = rte_eth_dev_allocated(name);
>> + if (eth_dev == NULL)
>> + return -1;
>> +
>> + eth_kni_dev_stop(eth_dev);
>> +
>> + if (eth_dev->data) {
>> + internals = eth_dev->data->dev_private;
>> + rte_kni_release(internals->kni);
>> +
>> + rte_free(internals);
>> + }
>> + rte_free(eth_dev->data);
>> +
>> + rte_eth_dev_release_port(eth_dev);
>> +
>> + is_kni_initialized -= 1;
>> + if (is_kni_initialized == 0)
>> + rte_kni_close();
>> +
>> + return 0;
>> +}
>> +
>> +static struct rte_vdev_driver eth_kni_drv = {
>> + .probe = eth_kni_probe,
>> + .remove = eth_kni_remove,
>> +};
>> +
>> +RTE_PMD_REGISTER_VDEV(net_kni, eth_kni_drv);
>> diff --git a/drivers/net/kni/rte_pmd_kni_version.map
>> b/drivers/net/kni/rte_pmd_kni_version.map
>> new file mode 100644
>> index 0000000..31eca32
>> --- /dev/null
>> +++ b/drivers/net/kni/rte_pmd_kni_version.map
>> @@ -0,0 +1,4 @@
>> +DPDK_17.02 {
>> +
>> + local: *;
>> +};
>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
>> index f75f0e2..af02816 100644
>> --- a/mk/rte.app.mk
>> +++ b/mk/rte.app.mk
>> @@ -59,11 +59,6 @@ _LDLIBS-y += -L$(RTE_SDK_BIN)/lib
>> #
>> # Order is important: from higher level to lower level
>> #
>> -
>> -ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_KNI) += -lrte_kni
>> -endif
>> -
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_PIPELINE) += -lrte_pipeline
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_TABLE) += -lrte_table
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_PORT) += -lrte_port
>> @@ -84,6 +79,10 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER) += -
>> lrte_power
>>
>> _LDLIBS-y += --whole-archive
>>
>> +ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_KNI) += -lrte_kni
>> +endif
>> +
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER) += -lrte_timer
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH) += -lrte_hash
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST) += -lrte_vhost
>> @@ -115,6 +114,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += -
>> lrte_pmd_enic
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_FM10K_PMD) += -lrte_pmd_fm10k
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += -lrte_pmd_i40e
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += -lrte_pmd_kni
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += -lrte_pmd_mlx4 -
>> libverbs
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += -lrte_pmd_mlx5 -
>> libverbs
>> _LDLIBS-$(CONFIG_RTE_LIBRTE_MPIPE_PMD) += -lrte_pmd_mpipe -lgxio
>> --
>> 2.9.3
>
^ permalink raw reply
* [PATCH v5] vhost: allow for many vhost user ports
From: Jan Wickbom @ 2016-12-14 15:30 UTC (permalink / raw)
To: yuanhan.liu; +Cc: dev, patrik.r.andersson, Jan Wickbom
In-Reply-To: <1480606010-6132-1-git-send-email-jan.wickbom@ericsson.com>
Currently select() is used to monitor file descriptors for vhostuser
ports. This limits the number of ports possible to create since the
fd number is used as index in the fd_set and we have seen fds > 1023.
This patch changes select() to poll(). This way we can keep an
packed (pollfd) array for the fds, e.g. as many fds as the size of
the array.
Also see:
http://dpdk.org/ml/archives/dev/2016-April/037024.html
Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---
v5:
* pack of arrays moved after poll loop
v4:
* fdset_del can not shrink the array. Took care of that in sveral places
* Moved rwfds[] into struct fdset
v3:
* removed unnecessary include
* removed fdset_fill, made it functionally part of poll loop
v2:
* removed unnecessary casts
* static array replacing allocated memory
lib/librte_vhost/fd_man.c | 234 ++++++++++++++++++++++++++--------------------
lib/librte_vhost/fd_man.h | 2 +
2 files changed, 135 insertions(+), 101 deletions(-)
diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..c7c2b35 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,93 +35,111 @@
#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
-#include <sys/select.h>
#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>
+#include <string.h>
#include <rte_common.h>
#include <rte_log.h>
#include "fd_man.h"
+#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)
+
/**
- * Returns the index in the fdset for a given fd.
- * If fd is -1, it means to search for a free entry.
+ * Adjusts the highest index populated in the array of fds
* @return
- * index for the fd, or -1 if fd isn't in the fdset.
+ * The new size of fdset.
*/
static int
-fdset_find_fd(struct fdset *pfdset, int fd)
+fdset_shrink(struct fdset *pfdset)
{
- int i;
+ int idx;
- if (pfdset == NULL)
- return -1;
+ /* Remove all empty spots in the end */
- for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+ for (idx = pfdset->num - 1;
+ idx >= 0 && pfdset->fd[idx].fd == -1;
+ idx--)
;
- return i == MAX_FDS ? -1 : i;
+ pfdset->num = idx + 1;
+
+ return pfdset->num;
}
-static int
-fdset_find_free_slot(struct fdset *pfdset)
+/**
+ * Moves the fd from last slot to specified slot, including
+ * corresponding pollfd
+ */
+static void
+fdset_move_last(struct fdset *pfdset, int idx)
{
- return fdset_find_fd(pfdset, -1);
+ int last_idx = pfdset->num - 1;
+
+ if (idx < last_idx) {
+ pfdset->fd[idx] = pfdset->fd[last_idx];
+ pfdset->fd[last_idx].fd = -1;
+
+ pfdset->rwfds[idx] = pfdset->rwfds[last_idx];
+ pfdset->rwfds[last_idx].revents = 0;
+ }
}
-static int
-fdset_add_fd(struct fdset *pfdset, int idx, int fd,
- fd_cb rcb, fd_cb wcb, void *dat)
-{
- struct fdentry *pfdentry;
- if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
- return -1;
+/**
+ * Packs the two arrays in the fdset structure
+ */
+static void
+fdset_pack(struct fdset *pfdset)
+{
+ int idx;
+ int num = pfdset->num;
- pfdentry = &pfdset->fd[idx];
- pfdentry->fd = fd;
- pfdentry->rcb = rcb;
- pfdentry->wcb = wcb;
- pfdentry->dat = dat;
+ for (idx = 0; idx < num; idx++) {
+ if (pfdset->fd[idx].fd == -1) {
+ num = fdset_shrink(pfdset);
+ fdset_move_last(pfdset, idx);
+ }
+ }
- return 0;
+ fdset_shrink(pfdset);
}
/**
- * Fill the read/write fd_set with the fds in the fdset.
+ * Returns the index in the fdset for a given fd.
+ * If fd is -1, it means to search for a free entry.
* @return
- * the maximum fds filled in the read/write fd_set.
+ * index for the fd, or -1 if fd isn't in the fdset.
*/
static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
+fdset_find_fd(struct fdset *pfdset, int fd)
{
- struct fdentry *pfdentry;
- int i, maxfds = -1;
- int num = MAX_FDS;
+ int i;
- if (pfdset == NULL)
- return -1;
+ for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
+ ;
- for (i = 0; i < num; i++) {
- pfdentry = &pfdset->fd[i];
- if (pfdentry->fd != -1) {
- int added = 0;
- if (pfdentry->rcb && rfset) {
- FD_SET(pfdentry->fd, rfset);
- added = 1;
- }
- if (pfdentry->wcb && wfset) {
- FD_SET(pfdentry->fd, wfset);
- added = 1;
- }
- if (added)
- maxfds = pfdentry->fd < maxfds ?
- maxfds : pfdentry->fd;
- }
- }
- return maxfds;
+ return i == pfdset->num ? -1 : i;
+}
+
+static void
+fdset_add_fd(struct fdset *pfdset, int idx, int fd,
+ fd_cb rcb, fd_cb wcb, void *dat)
+{
+ struct fdentry *pfdentry = &pfdset->fd[idx];
+ struct pollfd *pfd = &pfdset->rwfds[idx];
+
+ pfdentry->fd = fd;
+ pfdentry->rcb = rcb;
+ pfdentry->wcb = wcb;
+ pfdentry->dat = dat;
+
+ pfd->fd = fd;
+ pfd->events = rcb ? POLLIN : 0;
+ pfd->events |= wcb ? POLLOUT : 0;
+ pfd->revents = 0;
}
void
@@ -132,11 +150,15 @@
if (pfdset == NULL)
return;
+ pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
for (i = 0; i < MAX_FDS; i++) {
pfdset->fd[i].fd = -1;
pfdset->fd[i].dat = NULL;
}
pfdset->num = 0;
+
+ memset(pfdset->rwfds, 0, sizeof(pfdset->rwfds));
}
/**
@@ -152,14 +174,14 @@
pthread_mutex_lock(&pfdset->fd_mutex);
- /* Find a free slot in the list. */
- i = fdset_find_free_slot(pfdset);
- if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+ i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
+
+ if (i == -1) {
pthread_mutex_unlock(&pfdset->fd_mutex);
return -2;
}
- pfdset->num++;
+ fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -189,7 +211,6 @@
pfdset->fd[i].fd = -1;
pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
pfdset->fd[i].dat = NULL;
- pfdset->num--;
i = -1;
}
pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -198,24 +219,6 @@
return dat;
}
-/**
- * Unregister the fd at the specified slot from the fdset.
- */
-static void
-fdset_del_slot(struct fdset *pfdset, int index)
-{
- if (pfdset == NULL || index < 0 || index >= MAX_FDS)
- return;
-
- pthread_mutex_lock(&pfdset->fd_mutex);
-
- pfdset->fd[index].fd = -1;
- pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
- pfdset->fd[index].dat = NULL;
- pfdset->num--;
-
- pthread_mutex_unlock(&pfdset->fd_mutex);
-}
/**
* This functions runs in infinite blocking loop until there is no fd in
@@ -229,55 +232,74 @@
void
fdset_event_dispatch(struct fdset *pfdset)
{
- fd_set rfds, wfds;
- int i, maxfds;
+ int i;
+ struct pollfd *pfd;
struct fdentry *pfdentry;
- int num = MAX_FDS;
fd_cb rcb, wcb;
void *dat;
- int fd;
+ int fd, numfds;
int remove1, remove2;
- int ret;
if (pfdset == NULL)
return;
while (1) {
- struct timeval tv;
- tv.tv_sec = 1;
- tv.tv_usec = 0;
- FD_ZERO(&rfds);
- FD_ZERO(&wfds);
- pthread_mutex_lock(&pfdset->fd_mutex);
-
- maxfds = fdset_fill(&rfds, &wfds, pfdset);
-
- pthread_mutex_unlock(&pfdset->fd_mutex);
+ int has_holes;
/*
- * When select is blocked, other threads might unregister
+ * When poll is blocked, other threads might unregister
* listenfds from and register new listenfds into fdset.
- * When select returns, the entries for listenfds in the fdset
+ * When poll returns, the entries for listenfds in the fdset
* might have been updated. It is ok if there is unwanted call
* for new listenfds.
*/
- ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
- if (ret <= 0)
- continue;
+ pthread_mutex_lock(&pfdset->fd_mutex);
- for (i = 0; i < num; i++) {
- remove1 = remove2 = 0;
+ numfds = pfdset->num;
+
+ pthread_mutex_unlock(&pfdset->fd_mutex);
+
+ poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
+
+ has_holes = 0;
+ for (i = 0; i < numfds; i++) {
pthread_mutex_lock(&pfdset->fd_mutex);
+
pfdentry = &pfdset->fd[i];
fd = pfdentry->fd;
+ pfd = &pfdset->rwfds[i];
+
+ if (fd < 0) {
+ /* Removed during poll, care later */
+
+ has_holes = 1;
+
+ pthread_mutex_unlock(&pfdset->fd_mutex);
+
+ continue;
+ }
+
+ if (!pfd->revents) {
+
+ pthread_mutex_unlock(&pfdset->fd_mutex);
+
+ continue;
+ }
+
+ /* Valid fd, and at least one revent ... */
+
+ remove1 = remove2 = 0;
+
rcb = pfdentry->rcb;
wcb = pfdentry->wcb;
dat = pfdentry->dat;
pfdentry->busy = 1;
+
pthread_mutex_unlock(&pfdset->fd_mutex);
- if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+ if (rcb && pfd->revents & (POLLIN | FDPOLLERR))
rcb(fd, dat, &remove1);
- if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+ if (wcb && pfd->revents & (POLLOUT | FDPOLLERR))
wcb(fd, dat, &remove2);
pfdentry->busy = 0;
/*
@@ -292,8 +314,18 @@
* listen fd in another thread, we couldn't call
* fd_set_del.
*/
- if (remove1 || remove2)
- fdset_del_slot(pfdset, i);
+ if (remove1 || remove2) {
+ pfdentry->fd = -1;
+ has_holes = 1;
+ }
+ }
+
+ if (has_holes) {
+ pthread_mutex_lock(&pfdset->fd_mutex);
+
+ fdset_pack(pfdset);
+
+ pthread_mutex_unlock(&pfdset->fd_mutex);
}
}
}
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..d319cac 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -35,6 +35,7 @@
#define _FD_MAN_H_
#include <stdint.h>
#include <pthread.h>
+#include <poll.h>
#define MAX_FDS 1024
@@ -49,6 +50,7 @@ struct fdentry {
};
struct fdset {
+ struct pollfd rwfds[MAX_FDS];
struct fdentry fd[MAX_FDS];
pthread_mutex_t fd_mutex;
int num; /* current fd number of this fdset */
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-12-14 15:19 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <1480996340-29871-2-git-send-email-jerin.jacob@caviumnetworks.com>
On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> In a polling model, lcores poll ethdev ports and associated
> rx queues directly to look for packet. In an event driven model,
> by contrast, lcores call the scheduler that selects packets for
> them based on programmer-specified criteria. Eventdev library
> adds support for event driven programming model, which offer
> applications automatic multicore scaling, dynamic load balancing,
> pipelining, packet ingress order maintenance and
> synchronization services to simplify application packet processing.
>
> By introducing event driven programming model, DPDK can support
> both polling and event driven programming models for packet processing,
> and applications are free to choose whatever model
> (or combination of the two) that best suits their needs.
>
> This patch adds the eventdev specification header file.
>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
<snip>
> + *
> + * The *nb_events* parameter is the number of event objects to enqueue which are
> + * supplied in the *ev* array of *rte_event* structure.
> + *
> + * The rte_event_enqueue_burst() function returns the number of
> + * events objects it actually enqueued. A return value equal to *nb_events*
> + * means that all event objects have been enqueued.
> + *
> + * @param dev_id
> + * The identifier of the device.
> + * @param port_id
> + * The identifier of the event port.
> + * @param ev
> + * Points to an array of *nb_events* objects of type *rte_event* structure
> + * which contain the event object enqueue operations to be processed.
> + * @param nb_events
> + * The number of event objects to enqueue, typically number of
> + * rte_event_port_enqueue_depth() available for this port.
> + *
> + * @return
> + * The number of event objects actually enqueued on the event device. The
> + * return value can be less than the value of the *nb_events* parameter when
> + * the event devices queue is full or if invalid parameters are specified in a
> + * *rte_event*. If return value is less than *nb_events*, the remaining events
> + * at the end of ev[] are not consumed,and the caller has to take care of them
> + *
> + * @see rte_event_port_enqueue_depth()
> + */
> +uint16_t
> +rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id, struct rte_event ev[],
> + uint16_t nb_events);
> +
One suggestion - do we want to make the ev[] array const, to disallow
drivers from modifying the events passed in? Since the event structure
is only 16B big, it should be small enough to be copied around in
scheduler instances, allow the original events to remain unmodified.
/Bruce
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-12-14 15:15 UTC (permalink / raw)
To: Jerin Jacob
Cc: Van Haaren, Harry, dev@dpdk.org, thomas.monjalon@6wind.com,
hemant.agrawal@nxp.com, Eads, Gage
In-Reply-To: <20161214131356.GA4224@localhost.localdomain>
On Wed, Dec 14, 2016 at 06:43:58PM +0530, Jerin Jacob wrote:
> On Thu, Dec 08, 2016 at 11:02:16AM +0000, Van Haaren, Harry wrote:
> > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > Sent: Thursday, December 8, 2016 1:24 AM
> > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> >
> > <snip>
> >
> > > > * Operation and sched_type *increased* to 4 bits each (from previous value of 2) to
> > > allow future expansion without ABI changes
> > >
> > > Anyway it will break ABI if we add new operation. I would propose to keep 4bit
> > > reserved and add it when required.
> >
> > Ok sounds good. I'll suggest to move it to the middle between operation or sched type, which would allow expanding operation without ABI breaks. On expanding the field would remain in the same place with the same bits available in that place (no ABI break), but new bits can be added into the currently reserved space.
>
> OK. We will move the rsvd field as you suggested.
>
> >
> >
> > > > * Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits)
> > > > * sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of
> > > situations where 16 values for application specified identifiers of each event-type is
> > > genuinely not enough?
> > > One packet will not go beyond 16 stages but an application may have more stages and
> > > each packet may go mutually exclusive stages. For example,
> > >
> > > packet 0: stagex_0 ->stagex_1
> > > packet 1: stagey_0 ->stagey_1
> > >
> > > In that sense, IMO, more than 16 is required.(AFIAK, VPP has any much larger limit on
> > > number of stages)
> >
> > My understanding was that stages are linked to event queues, so the application can determine the stage the packet comes from by reading queue_id?
>
> That is one way of doing it. But it is limited to number of queues
> therefore scalability issues.Another approach is through
> sub_event_type scheme without depended on the number of queues.
>
> >
> > I'm not opposed to having an 8 bit sub_event_type, but it seems unnecessarily large from my point of view. If you have a use for it, I'm ok with 8 bits.
>
> OK
>
> >
> >
> > > > In my opinion this structure layout is more balanced, and will perform better due to
> > > less loads that will need masking to access the required value.
> > > OK. Considering more balanced layout and above points. I propose following scheme(based on
> > > your input)
> > >
> > > union {
> > > uint64_t event;
> > > struct {
> > > uint32_t flow_id: 20;
> > > uint32_t sub_event_type : 8;
> > > uint32_t event_type : 4;
> > >
> > > uint8_t rsvd: 4; /* for future additions */
> > > uint8_t operation : 2; /* new fwd drop */
> > > uint8_t sched_type : 2;
> > >
> > > uint8_t queue_id;
> > > uint8_t priority;
> > > uint8_t impl_opaque;
> > > };
> > > };
> > >
> > > Feedback and improvements welcomed,
> >
> >
> > So incorporating my latest suggestions on moving fields around, excluding sub_event_type *size* changes:
> >
> > union {
> > uint64_t event;
> > struct {
> > uint32_t flow_id: 20;
> > uint32_t event_type : 4;
> > uint32_t sub_event_type : 8; /* 8 bits now naturally aligned */
>
> Just one suggestion here. I am not sure about the correct split between
> number of bits to represent flow_id and sub_event_type fields. And its
> connected in our implementation, so I propose to move sub_event_type up so
> that future flow_id/sub_event_type bit field size change request wont
> impact our implementation. Since it is represented as 32bit as whole, I
> don't think there is an alignment issue.
>
> So incorporating my latest suggestions on moving sub_event_type field around:
>
> union {
> uint64_t event;
> struct {
> uint32_t flow_id: 20;
> uint32_t sub_event_type : 8;
> uint32_t event_type : 4;
>
The issue with the above layout is that you have an 8-bit value which
can never be accessed as a byte. With the layout above proposed by
Harry, the sub_event_type can be accessed without any bit manipultaion
operations just by doing a byte read. With the layout you propose, all
fields require masking and/or shifting to access. It won't affect the
scheduler performance for us, but it means potentially more cycles in
the app to access those fields.
/Bruce
^ permalink raw reply
* No packets received if burst is too small in rte_eth_rx_burst
From: tom.barbette @ 2016-12-14 15:13 UTC (permalink / raw)
To: dev
In-Reply-To: <415214732.17903310.1481728244157.JavaMail.zimbra@ulg.ac.be>
Hi list,
Between 2.2.0 and 16.04 (up to at least 16.07.2 if not current), with the XL710 controller I do not get any packet when calling rte_eth_rx_burst if nb_pkts is too small. I would say smaller than 32. The input rate is not big, if that helps. But It should definitely get at least one packet per second.
Any ideas? Is that a bug or expected behaviour? Could be caused by other ABI changes?
Thanks,
Tom
^ permalink raw reply
* Re: [PATCH 19/28] net/ena: use eal I/O device memory read/write API
From: Jan Mędala @ 2016-12-14 14:36 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, Ananyev, Konstantin, Thomas Monjalon, Bruce Richardson,
jianbo.liu, viktorin, Santosh Shukla, Jakub Palider,
Alexander Matushevsky
In-Reply-To: <1481680558-4003-20-git-send-email-jerin.jacob@caviumnetworks.com>
Despite the issue with naming convention (either it will be writel or
write32), I'm fine with this change and new API.
Acked-by: Jan Medala <jan@semihalf.com>
Jan
2016-12-14 2:55 GMT+01:00 Jerin Jacob <jerin.jacob@caviumnetworks.com>:
> From: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>
> Replace the raw I/O device memory read/write access with eal
> abstraction for I/O device memory read/write access to fix
> portability issues across different architectures.
>
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Jan Medala <jan@semihalf.com>
> CC: Jakub Palider <jpa@semihalf.com>
> ---
> drivers/net/ena/base/ena_plat_dpdk.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h
> b/drivers/net/ena/base/ena_plat_dpdk.h
> index 87c3bf1..4db07c7 100644
> --- a/drivers/net/ena/base/ena_plat_dpdk.h
> +++ b/drivers/net/ena/base/ena_plat_dpdk.h
> @@ -50,6 +50,7 @@
> #include <rte_spinlock.h>
>
> #include <sys/time.h>
> +#include <rte_io.h>
>
> typedef uint64_t u64;
> typedef uint32_t u32;
> @@ -226,12 +227,12 @@ typedef uint64_t dma_addr_t;
>
> static inline void writel(u32 value, volatile void *addr)
> {
> - *(volatile u32 *)addr = value;
> + rte_writel(value, addr);
> }
>
> static inline u32 readl(const volatile void *addr)
> {
> - return *(const volatile u32 *)addr;
> + return rte_readl(addr);
> }
>
> #define ENA_REG_WRITE32(value, reg) writel((value), (reg))
> --
> 2.5.5
>
>
^ permalink raw reply
* Re: [PATCH 01/22] ethdev: introduce generic flow API
From: Adrien Mazarguil @ 2016-12-14 13:54 UTC (permalink / raw)
To: Kevin Traynor
Cc: dev, Thomas Monjalon, Pablo de Lara, Olivier Matz,
sugesh.chandran
In-Reply-To: <5c53f86b-ead7-b539-6250-40613c7a57db@redhat.com>
Hi Kevin,
On Wed, Dec 14, 2016 at 11:48:04AM +0000, Kevin Traynor wrote:
> hi Adrien, sorry for the delay
>
> <...>
>
> >>>>
> >>>> Is it expected that the application or pmd will provide locking between
> >>>> these functions if required? I think it's going to have to be the app.
> >>>
> >>> Locking is indeed expected to be performed by applications. This API only
> >>> documents places where locking would make sense if necessary and expected
> >>> behavior.
> >>>
> >>> Like all control path APIs, this one assumes a single control thread.
> >>> Applications must take the necessary precautions.
> >>
> >> If you look at OVS now it's quite possible that you have 2 rx queues
> >> serviced by different threads, that would also install the flow rules in
> >> the software flow caches - possibly that could extend to adding hardware
> >> flows. There could also be another thread that is querying for stats. So
> >> anything that can be done to minimise the locking would be helpful -
> >> maybe query() could be atomic and not require any locking?
> >
> > I think we need basic functions with as few constraints as possible on PMDs
> > first, this API being somewhat complex to implement on their side. That
> > covers the common use case where applications have a single control thread
> > or otherwise perform locking on their own.
> >
> > Once the basics are there for most PMDs, we may add new functions, items,
> > properties and actions that provide additional constraints (timing,
> > multi-threading and so on), which remain to be defined according to
> > feedback. It is designed to be extended without causing ABI breakage.
>
> I think Sugesh and I are trying to foresee some of the issues that may
> arise when integrating with something like OVS. OTOH it's
> hard/impossible to say what will be needed exactly in the API right now
> to make it suitable for OVS.
>
> So, I'm ok with the approach you are taking by exposing a basic API
> but I think there should be an expectation that it may not be sufficient
> for a project like OVS to integrate in and may take several
> iterations/extensions - don't go anywhere!
>
> >
> > As for query(), let's see how PMDs handle it first. A race between query()
> > and create() on a given device is almost unavoidable without locking, same
> > for queries that reset counters in a given flow rule. Basic parallel queries
> > should not cause any harm otherwise, although this cannot be guaranteed yet.
>
> You still have a race if there is locking, except it is for the lock,
> but it has the same effect. The downside of my suggestion is that all
> the PMDs would need to guarantee they could gets stats atomically - I'm
> not sure if they can or it's too restrictive.
>
> >
>
> <...>
>
> >>
> >>>
> >>>>> +
> >>>>> +/**
> >>>>> + * Destroy a flow rule on a given port.
> >>>>> + *
> >>>>> + * Failure to destroy a flow rule handle may occur when other flow rules
> >>>>> + * depend on it, and destroying it would result in an inconsistent state.
> >>>>> + *
> >>>>> + * This function is only guaranteed to succeed if handles are destroyed in
> >>>>> + * reverse order of their creation.
> >>>>
> >>>> How can the application find this information out on error?
> >>>
> >>> Without maintaining a list, they cannot. The specified case is the only
> >>> possible guarantee. That does not mean PMDs should not do their best to
> >>> destroy flow rules, only that ordering must remain consistent in case of
> >>> inability to destroy one.
> >>>
> >>> What do you suggest?
> >>
> >> I think if the app cannot remove a specific rule it may want to remove
> >> all rules and deal with flows in software for a time. So once the app
> >> knows it fails that should be enough.
> >
> > OK, then since destruction may return an error already, is it fine?
> > Applications may call rte_flow_flush() (not supposed to fail unless there is
> > a serious issue, abort() in that case) and switch to SW fallback.
>
> yes, it's fine.
>
> >
>
> <...>
>
> >>>>> + * @param[out] error
> >>>>> + * Perform verbose error reporting if not NULL.
> >>>>> + *
> >>>>> + * @return
> >>>>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>>>> + */
> >>>>> +int
> >>>>> +rte_flow_query(uint8_t port_id,
> >>>>> + struct rte_flow *flow,
> >>>>> + enum rte_flow_action_type action,
> >>>>> + void *data,
> >>>>> + struct rte_flow_error *error);
> >>>>> +
> >>>>> +#ifdef __cplusplus
> >>>>> +}
> >>>>> +#endif
> >>>>
> >>>> I don't see a way to dump all the rules for a port out. I think this is
> >>>> neccessary for degbugging. You could have a look through dpif.h in OVS
> >>>> and see how dpif_flow_dump_next() is used, it might be a good reference.
> >>>
> >>> DPDK does not maintain flow rules and, depending on hardware capabilities
> >>> and level of compliance, PMDs do not necessarily do it either, particularly
> >>> since it requires space and application probably have a better method to
> >>> store these pointers for their own needs.
> >>
> >> understood
> >>
> >>>
> >>> What you see here is only a PMD interface. Depending on applications needs,
> >>> generic helper functions built on top of these may be added to manage flow
> >>> rules in the future.
> >>
> >> I'm thinking of the case where something goes wrong and I want to get a
> >> dump of all the flow rules from hardware, not query the rules I think I
> >> have. I don't see a way to do it or something to build a helper on top of?
> >
> > Generic helper functions would exist on top of this API and would likely
> > maintain a list of flow rules themselves. The dump in that case would be
> > entirely implemented in software. I think that recovering flow rules from HW
> > may be complicated in many cases (even without taking storage allocation and
> > rules conversion issues into account), therefore if there is really a need
> > for it, we could perhaps add a dump() function that PMDs are free to
> > implement later.
> >
>
> ok. Maybe there are some more generic stats that can be got from the
> hardware that would help debugging that would suffice, like total flow
> rule hits/misses (i.e. not on a per flow rule basis).
>
> You can get this from the software flow caches and it's widely used for
> debugging. e.g.
>
> pmd thread numa_id 0 core_id 3:
> emc hits:0
> megaflow hits:0
> avg. subtable lookups per hit:0.00
> miss:0
>
Perhaps a rule such as the following could do the trick:
group: 42 (or priority 42)
pattern: void
actions: count / passthru
Assuming useful flow rules are defined with higher priorities (using lower
group ID or priority level) and provide a terminating action, this one would
count all packets that were not caught by them.
That is one example to illustrate how "global" counters can be requested by
applications.
Otherwise you could just make sure all rules contain mark / flag actions, in
which case mbufs would tell directly if they went through them or need
additional SW processing.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH 00/28] introduce I/O device memory read/write operations
From: Jerin Jacob @ 2016-12-14 13:18 UTC (permalink / raw)
To: Yuanhan Liu
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin
In-Reply-To: <20161214025357.GF18991@yliu-dev.sh.intel.com>
On Wed, Dec 14, 2016 at 10:53:57AM +0800, Yuanhan Liu wrote:
> On Wed, Dec 14, 2016 at 07:25:30AM +0530, Jerin Jacob wrote:
> > patchset 14-28: Replace the raw readl/writel in the drivers with
> > new rte_read[b/w/l/q], rte_write[b/w/l/q] eal abstraction
>
> Instead of rte_read[b/w/l/q], there is another typical naming style:
> rte_read[8/16/32/64]. Any preferences? If you ask me, I'd prefer the
> later.
No strong opinion here. The rte_read[b/w/l/q] naming style is from Linux
kernel. I will change to rte_read[8/16/32/64] in v2 if there is no
objection.
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Jerin Jacob @ 2016-12-14 13:13 UTC (permalink / raw)
To: Van Haaren, Harry
Cc: dev@dpdk.org, thomas.monjalon@6wind.com, Richardson, Bruce,
hemant.agrawal@nxp.com, Eads, Gage
In-Reply-To: <E923DB57A917B54B9182A2E928D00FA6129C9B9C@IRSMSX102.ger.corp.intel.com>
On Thu, Dec 08, 2016 at 11:02:16AM +0000, Van Haaren, Harry wrote:
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Thursday, December 8, 2016 1:24 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
>
> <snip>
>
> > > * Operation and sched_type *increased* to 4 bits each (from previous value of 2) to
> > allow future expansion without ABI changes
> >
> > Anyway it will break ABI if we add new operation. I would propose to keep 4bit
> > reserved and add it when required.
>
> Ok sounds good. I'll suggest to move it to the middle between operation or sched type, which would allow expanding operation without ABI breaks. On expanding the field would remain in the same place with the same bits available in that place (no ABI break), but new bits can be added into the currently reserved space.
OK. We will move the rsvd field as you suggested.
>
>
> > > * Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits)
> > > * sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of
> > situations where 16 values for application specified identifiers of each event-type is
> > genuinely not enough?
> > One packet will not go beyond 16 stages but an application may have more stages and
> > each packet may go mutually exclusive stages. For example,
> >
> > packet 0: stagex_0 ->stagex_1
> > packet 1: stagey_0 ->stagey_1
> >
> > In that sense, IMO, more than 16 is required.(AFIAK, VPP has any much larger limit on
> > number of stages)
>
> My understanding was that stages are linked to event queues, so the application can determine the stage the packet comes from by reading queue_id?
That is one way of doing it. But it is limited to number of queues
therefore scalability issues.Another approach is through
sub_event_type scheme without depended on the number of queues.
>
> I'm not opposed to having an 8 bit sub_event_type, but it seems unnecessarily large from my point of view. If you have a use for it, I'm ok with 8 bits.
OK
>
>
> > > In my opinion this structure layout is more balanced, and will perform better due to
> > less loads that will need masking to access the required value.
> > OK. Considering more balanced layout and above points. I propose following scheme(based on
> > your input)
> >
> > union {
> > uint64_t event;
> > struct {
> > uint32_t flow_id: 20;
> > uint32_t sub_event_type : 8;
> > uint32_t event_type : 4;
> >
> > uint8_t rsvd: 4; /* for future additions */
> > uint8_t operation : 2; /* new fwd drop */
> > uint8_t sched_type : 2;
> >
> > uint8_t queue_id;
> > uint8_t priority;
> > uint8_t impl_opaque;
> > };
> > };
> >
> > Feedback and improvements welcomed,
>
>
> So incorporating my latest suggestions on moving fields around, excluding sub_event_type *size* changes:
>
> union {
> uint64_t event;
> struct {
> uint32_t flow_id: 20;
> uint32_t event_type : 4;
> uint32_t sub_event_type : 8; /* 8 bits now naturally aligned */
Just one suggestion here. I am not sure about the correct split between
number of bits to represent flow_id and sub_event_type fields. And its
connected in our implementation, so I propose to move sub_event_type up so
that future flow_id/sub_event_type bit field size change request wont
impact our implementation. Since it is represented as 32bit as whole, I
don't think there is an alignment issue.
So incorporating my latest suggestions on moving sub_event_type field around:
union {
uint64_t event;
struct {
uint32_t flow_id: 20;
uint32_t sub_event_type : 8;
uint32_t event_type : 4;
uint8_t operation : 2; /* new fwd drop */
uint8_t rsvd: 4; /* for future additions, can be expanded into without ABI break */
uint8_t sched_type : 2;
uint8_t queue_id;
uint8_t priority;
uint8_t impl_opaque;
};
};
^ permalink raw reply
* Re: [PATCH 04/13] acl: allow zero verdict
From: Ananyev, Konstantin @ 2016-12-14 12:16 UTC (permalink / raw)
To: Michal Miroslaw; +Cc: dev@dpdk.org
In-Reply-To: <20161214021111.2l4h4gsbjxmbffca@rere.qmqm.pl>
Hi Michal,
> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Wednesday, December 14, 2016 2:11 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
>
> On Tue, Dec 13, 2016 at 09:55:12PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > Sent: Tuesday, December 13, 2016 6:03 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > >
> > > On Tue, Dec 13, 2016 at 05:27:31PM +0000, Ananyev, Konstantin wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > > > Sent: Tuesday, December 13, 2016 4:14 PM
> > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > > > >
> > > > > On Tue, Dec 13, 2016 at 03:13:42PM +0000, Ananyev, Konstantin wrote:
> > > > > [...]
> > > > > > > > > > > Subject: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> > > > > > > > > > > ---
> > > > > > > > > > > lib/librte_acl/rte_acl.c | 3 +--
> > > > > > > > > > > lib/librte_acl/rte_acl.h | 2 --
> > > > > > > > > > > lib/librte_table/rte_table_acl.c | 2 +-
> > > > > > > > > > > 3 files changed, 2 insertions(+), 5 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_acl/rte_acl.c
> > > > > > > > > > > index 8b7e92c..d1f40be 100644
> > > > > > > > > > > --- a/lib/librte_acl/rte_acl.c
> > > > > > > > > > > +++ b/lib/librte_acl/rte_acl.c
> > > > > > > > > > > @@ -313,8 +313,7 @@ acl_check_rule(const struct rte_acl_rule_data *rd)
> > > > > > > > > > > if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd->category_mask)) &
> > > > > > > > > > > rd->category_mask) == 0 ||
> > > > > > > > > > > rd->priority > RTE_ACL_MAX_PRIORITY ||
> > > > > > > > > > > - rd->priority < RTE_ACL_MIN_PRIORITY ||
> > > > > > > > > > > - rd->userdata == RTE_ACL_INVALID_USERDATA)
> > > > > > > > > > > + rd->priority < RTE_ACL_MIN_PRIORITY)
> > > > > > > > > > > return -EINVAL;
> > > > > > > > > > > return 0;
> > > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > I am not sure, how it supposed to work properly?
> > > > > > > > > > Zero value is reserved and ifnicates that no match were found for that input.
> > > > > > > > >
> > > > > > > > > This is actually in use by us. In our use we don't need to differentiate
> > > > > > > > > matching a rule with zero verdict vs not matching a rule at all. I also
> > > > > > > > > have a patch that changes the value returned in non-matching case, but
> > > > > > > > > it's in "dirty hack" state, as of yet.
> > > > > > > >
> > > > > > > > With that chane rte_acl_classify() might produce invalid results.
> > > > > > > > Even if you don't need it (I still don't understand how) , it doesn't mean other people
> > > > > > > > don't need it either and it is ok to change it.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > The ACL code does not treat zero userdata specially, so this is only
> > > > > > > > > a policy choice and as such would be better to be made by the user.
> > > > > > > >
> > > > > > > > I believe it does.
> > > > > > > > userdata==0 is a reserved value.
> > > > > > > > When rte_acl_clasify() returns 0 for that particular input, it means 'no matches were found'.
> > > > > > >
> > > > > > > Dear Konstantin,
> > > > > > >
> > > > > > > Can you describe how the ACL code treats zero specially? I could not find
> > > > > > > anything, really. The only thing I found is that iff I use zero userdata
> > > > > > > in a rule I won't be able to differentiate a case where it matched from
> > > > > > > a case where no rule matched.
> > > > > >
> > > > > > Yes, that's what I am talking about.
> > > > > >
> > > > > > > If I all my rules have non-zero userdata,
> > > > > > > then this patch changes nothing.
> > > > > >
> > > > > > Ok, then why do you remove a code that does checking for invalid userdata==0?
> > > > > > That supposed to prevent user to setup invalid value by mistake.
> > > > > >
> > > > > > But if I have a table where 0 means drop
> > > > > > > (default-drop policy) then being able to use zero userdata in DROP rules
> > > > > > > makes the ACLs just that more useful.
> > > > > >
> > > > > > Ok, and what prevents you from do +1 to your policy values before
> > > > > > you insert it into the ACL table and -1 after you retrieved it via rte_acl_classify()?
> > > > >
> > > > > The check is enforcing an assumption that all users want to distinguish
> > > > > the cases whether any rule matched and whether no rules matched. Not all
> > > > > users do, hence the assumption is invalid and this patch removes it.
> > > >
> > > > The check is based on the assumption that users might need to distinguish
> > > > the situation when no rules were matched.
> > > > To support that we need a reserved userdata value, which would mean
> > > > NO_MATCH.
> > > > From what I heard, most users do need this ability, those who don't
> > > > can easily overcome it.
> > >
> > > That's actually my point. Some users need the distinction, so they don't use
> > > zero userdata in their rules and have their work done. Some users don't need
> > > it and would prefer to just use the convenience of zero being no-match signal
> > > to insert "non-matching" rules (now they have to check two values for the
> > > same signal).
> > >
> > > > > Yes, people can work around it by loosing 1 of 2^32 useful values and
> > > > > convoluting their code.
> > > > Yes, one of 2^32 values is reserved.
> > > > Any reason why (2^32 - 1) values might not be enough?
> > >
> > > Sure. We're using userdata as a bitmask of actions to take on the packet,
> > > and because of this restriction we're loosing half of the userdata field.
> > > If we would add this "decrement if non-zero" workaround this would keep
> > > biting us on every occasion where we touch the ACL verdict code.
> > >
> > > > > You seem to argue that 0 is somehow an invalid value, but I can't find
> > > > > anything in the ACL that would require it to be so. Could you point me
> > > > > to the code in DPDK where this actually matters?
> > > >
> > > > It was a while, when I looked into ACL code in details, but as remember
> > > > that's the only reason: we need some value to be reserved as NO_MATCH.
> > > > Let say in build_trie() we set results to zero for rules with unused categories:
> > > > for (m = context->cfg.num_categories; 0 != m--; ) {
> > > > if (rule->f->data.category_mask & (1 << m)) {
> > > > end->mrt->results[m] = rule->f->data.userdata;
> > > > end->mrt->priority[m] = rule->f->data.priority;
> > > > } else {
> > > > end->mrt->results[m] = 0;
> > > > end->mrt->priority[m] = 0;
> > > > }
> > > > }
> > >
> > > So, if I understand correctly, 0 is a default value for category result.
> > > Any matching rule with priority >= 0 will override it (leaving last highest
> > > priority rule's userdata). This will just work the same for anyone needing
> > > the distinction (when he doesn't use userdata == 0) and also for those who
> > > don't -- when the restriction is removed.
> > >
> > > I think that it comes to documenting the behaviour and let users choose
> > > their way. At the beginning I haven't found any mention of the restriction
> > > in the docs, so I had to spend a fair amount of time to find out why the
> > > zero is so special (it wasn't).
> >
> > Ok, so you suggest the following:
> > 1. Zero value for both userdata and results still has a special meaning: NO_MATCH.
> > 2. Allow user to create a rule(s) that would on hit return NO_MATCH for it,
> > as if no rule was matched by that input (i.e. rule's userdata==0).
> > Is my understanding correct?
>
> That is exactly it.
Ok, the idea still seems a bit unusual to me, but I suppose might be useful for some cases.
Again, can't think up a scenario when it would break something.
So don't have any good reason to object :)
Before we proceed with it can I ask you for two things:
- Verify that autotest_acl works as expected.
- Update PG: http://dpdk.org/doc/guides/prog_guide/packet_classif_access_ctrl.html#rule-definition
I think these lines need to be repharsed because of that patch:
"userdata: A user-defined field that could be any value except zero."
Konstantin
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox