* Re: [RFC] [PATCH v2] libeventdev: event driven programming model framework for DPDK
From: Jerin Jacob @ 2016-10-25 17:49 UTC (permalink / raw)
To: dev
Cc: thomas.monjalon, bruce.richardson, narender.vangati,
hemant.agrawal, gage.eads
In-Reply-To: <1476214216-31982-1-git-send-email-jerin.jacob@caviumnetworks.com>
On Wed, Oct 12, 2016 at 01:00:16AM +0530, Jerin Jacob wrote:
> Thanks to Intel and NXP folks for the positive and constructive feedback
> I've received so far. Here is the updated RFC(v2).
>
> I've attempted to address as many comments as possible.
>
> This series adds rte_eventdev.h to the DPDK tree with
> adequate documentation in doxygen format.
>
> Updates are also available online:
>
> Related draft header file (this patch):
> https://rawgit.com/jerinjacobk/libeventdev/master/rte_eventdev.h
>
> PDF version(doxgen output):
> https://rawgit.com/jerinjacobk/libeventdev/master/librte_eventdev_v2.pdf
>
> Repo:
> https://github.com/jerinjacobk/libeventdev
>
Hi Community,
So far, I have received constructive feedback from Intel, NXP and Linaro folks.
Let me know, if anyone else interested in contributing to the definition of eventdev?
If there are no major issues in proposed spec, then Cavium would like work on
implementing and up-streaming the common code(lib/librte_eventdev/) and
an associated HW driver.(Requested minor changes of v2 will be addressed
in next version).
We are planning to submit the work for 17.02 or 17.05 release(based on
how implementation goes).
/Jerin
Cavium
^ permalink raw reply
* Re: [PATCH v10 1/6] ethdev: add Tx preparation
From: Kulasek, TomaszX @ 2016-10-25 17:28 UTC (permalink / raw)
To: Olivier Matz, dev@dpdk.org; +Cc: Ananyev, Konstantin
In-Reply-To: <e86291cd-f970-c0c5-4a3b-6da62516ce6a@6wind.com>
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, October 25, 2016 16:42
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: Re: [PATCH v10 1/6] ethdev: add Tx preparation
>
> Hi Tomasz,
>
> On 10/24/2016 06:51 PM, Tomasz Kulasek wrote:
> > Added API for `rte_eth_tx_prep`
> >
> > [...]
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -182,6 +182,7 @@ extern "C" {
> > #include <rte_pci.h>
> > #include <rte_dev.h>
> > #include <rte_devargs.h>
> > +#include <rte_errno.h>
> > #include "rte_ether.h"
> > #include "rte_eth_ctrl.h"
> > #include "rte_dev_info.h"
> > @@ -699,6 +700,8 @@ struct rte_eth_desc_lim {
> > uint16_t nb_max; /**< Max allowed number of descriptors. */
> > uint16_t nb_min; /**< Min allowed number of descriptors. */
> > uint16_t nb_align; /**< Number of descriptors should be aligned to.
> > */
> > + uint16_t nb_seg_max; /**< Max number of segments per whole
> packet. */
> > + uint16_t nb_mtu_seg_max; /**< Max number of segments per one MTU */
>
> Sorry if it was not clear in my previous review, but I think this should
> be better explained here. You said that the "limitation of number of
> segments may differ depend of TSO/non TSO".
>
> As an application developer, I still have some difficulties to clearly
> understand what does that mean. Is it the maximum number of mbuf-segments
> that contain payload for one tcp-segment sent by the device?
>
> In that case, it looks quite difficult to verify that in an application.
> It looks that this field is not used by validate_offload(), so how should
> it be used by an application?
>
E.g. for i40e (from xl710 datasheet):
8.4.1 Transmit Packet in System Memory
...
A few rules related to the transmit packet in host memory are:
...
- A single transmit packet may span up to 8 buffers (up to 8 data descriptors
per packet including both the header and payload buffers).
- The total number of data descriptors for the whole TSO (explained later on in
this chapter) is unlimited as long as each segment within the TSO obeys
the previous rule (up to 8 data descriptors per segment for both the TSO
header and the segment payload buffers).
...
+#define I40E_TX_MAX_SEG UINT8_MAX
+#define I40E_TX_MAX_MTU_SEG 8
For ixgbe driver there's one limitation, both for TSO and non-TSO and it
is always 40-WTHRESH.
Such a differences causes that these values should checked in the tx_prep
callback (when it is required) for a specific device (e.g. i40e, ixgbe).
If other device have not such a limitations, or are higher than DPDK can
handle (nb_segs is uint8_t), then for performance reason it doesn't need
to be checked.
Validate_offload() is for general check, the drivers specific implementation
Is the part in the callback function e.g. in
uint16_t ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts)
for ixgbe.
Values in the "struct rte_eth_desc_lim" provides an information to the
application, which should to let you to not create malicious packets or
at least to limit their number.
Using Tx preparation API, these checks are made transparently for the
application and when nb_segs are out of limits, tx_prep function fails.
>
> > };
> >
> > /**
> > @@ -1188,6 +1191,11 @@ typedef uint16_t (*eth_tx_burst_t)(void *txq,
> > uint16_t nb_pkts);
> > /**< @internal Send output packets on a transmit queue of an Ethernet
> > device. */
> >
> > +typedef uint16_t (*eth_tx_prep_t)(void *txq,
> > + struct rte_mbuf **tx_pkts,
> > + uint16_t nb_pkts);
> > +/**< @internal Prepare output packets on a transmit queue of an
> > +Ethernet device. */
> > +
> > typedef int (*flow_ctrl_get_t)(struct rte_eth_dev *dev,
> > struct rte_eth_fc_conf *fc_conf); /**< @internal
> Get
> > current flow control parameter on an Ethernet device */ @@ -1622,6
> > +1630,7 @@ struct rte_eth_rxtx_callback { struct rte_eth_dev {
> > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function.
> */
> > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function.
> > */
> > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare
> > +function. */
> > struct rte_eth_dev_data *data; /**< Pointer to device data */
> > const struct eth_driver *driver;/**< Driver for this device */
> > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> > @@ -2816,6 +2825,93 @@ rte_eth_tx_burst(uint8_t port_id, uint16_t
> queue_id,
> > return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts,
> > nb_pkts); }
> >
> > +/**
> > + * Process a burst of output packets on a transmit queue of an Ethernet
> device.
> > + *
> > + * The rte_eth_tx_prep() function is invoked to prepare output
> > +packets to be
> > + * transmitted on the output queue *queue_id* of the Ethernet device
> > +designated
> > + * by its *port_id*.
> > + * The *nb_pkts* parameter is the number of packets to be prepared
> > +which are
> > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of
> > +them
> > + * allocated from a pool created with rte_pktmbuf_pool_create().
> > + * For each packet to send, the rte_eth_tx_prep() function performs
> > + * the following operations:
> > + *
> > + * - Check if packet meets devices requirements for tx offloads.
> > + *
> > + * - Check limitations about number of segments.
> > + *
> > + * - Check additional requirements when debug is enabled.
> > + *
> > + * - Update and/or reset required checksums when tx offload is set for
> packet.
> > + *
> > + * The rte_eth_tx_prep() function returns the number of packets ready
> > +to be
> > + * sent. A return value equal to *nb_pkts* means that all packets are
> > +valid and
> > + * ready to be sent.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * The value must be a valid port id.
> > + * @param queue_id
> > + * The index of the transmit queue through which output packets must
> be
> > + * sent.
> > + * The value must be in the range [0, nb_tx_queue - 1] previously
> supplied
> > + * to rte_eth_dev_configure().
> > + * @param tx_pkts
> > + * The address of an array of *nb_pkts* pointers to *rte_mbuf*
> structures
> > + * which contain the output packets.
> > + * @param nb_pkts
> > + * The maximum number of packets to process.
> > + * @return
> > + * The number of packets correct and ready to be sent. The return
> value can be
> > + * less than the value of the *tx_pkts* parameter when some packet
> doesn't
> > + * meet devices requirements with rte_errno set appropriately.
> > + */
>
> Inserting here the previous comment:
>
> >> Can we add the constraint that invalid packets are left untouched?
> >>
> >> I think most of the time there will be a software fallback in that
> >> case, so it would be good to ensure that this function does not
> >> change the flags or the packet data.
> >
> > In current implementation, if packet is invalid, its data is never
> modified. Only checks are done. The only exception is when checksum needs
> to be updated or initialized, but it's done after packet validation.
> > If we want to use/restore packet in application it didn't should be
> changed in any way for invalid packets.
> >
>
> I think this should be explicitly said in the API comment that valid
> packets may be modified (*), but invalid packets (whose index is >= than
> the return value) are left untouched.
>
> (*) we still need to discuss that point, see below.
>
> Another comment was made:
>
> >> Another thing that could be interesting for the caller is to know the
> >> reason of the failure. Maybe the different errno types could be
> >> detailed here. For instance:
> >> - EINVAL: offload flags are not correctly set (i.e. would fail whatever
> >> the hardware)
> >> - ENOTSUP: the offload feature is not supported by the hardware
> >> - ...
> >>
> >
> > Ok.
>
> Don't you feel it could go in the API comment too?
>
Ok, I will modify comments.
>
> > [...]
> >
> > +/**
> > + * Fix pseudo header checksum
> > + *
> > + * This function fixes pseudo header checksum for TSO and non-TSO
> > +tcp/udp in
> > + * provided mbufs packet data.
> > + *
> > + * - for non-TSO tcp/udp packets full pseudo-header checksum is counted
> and set
> > + * in packet data,
> > + * - for TSO the IP payload length is not included in pseudo header.
> > + *
> > + * This function expects that used headers are in the first data
> > +segment of
> > + * mbuf, and are not fragmented.
>
> There is another requirement about the cloning and reference count.
> Sorry I did not answer to Konstantin, but I still think this could be an
> issue.
>
> For instance, I think that the zero-copy mode of vhost application
> references the packet sent by the guest and send the data. The payload
> should not be modified because it is in guest memory (we don't know, maybe
> the guest also cloned it for its own purpose).
>
> It means that the tx_prep() API must not be used with clones, i.e the
> headers must not reside in a segment whose RTE_MBUF_INDIRECT(seg) or
> rte_mbuf_refcnt_read(seg) > 1.
>
> - if we really want this API in 16.11, it should be clearly explained
> in the API comment that it does not work with shared segments
>
Ok, I will.
> - for next versions, we have to take a decision whether it should be
> supported or not. In my opinion, cloned packets are useful and should
> be supported properly by all dpdk APIs.
>
>
> Thanks,
> Olivier
Thanks,
Tomasz
^ permalink raw reply
* Re: DPDK-QoS- Using un-used bandwidth within a class
From: Dumitrescu, Cristian @ 2016-10-25 17:10 UTC (permalink / raw)
To: sreenaath vasudevan, dev@dpdk.org
In-Reply-To: <CAJXCTanJHOFp3cJ=B=QdPYB7tc=txhwSfL7pBVMcb61aMnckjw@mail.gmail.com>
Hi Sreenaath,
I think you are simply hitting the known and documented performance issue with using a single pipe. The hierarchical scheduler performance is optimized for many pipes (hundreds, thousands , …), not for single pipe. The rationale is that if you only needs handful of queues, you can simply use a single level class based queuing device as opposed to the hierarchical scheduler.
Are you getting any issues when using hundreds/thousands of active (i.e. with traffic going through them) pipes?
Thanks,
Cristian
From: sreenaath vasudevan [mailto:sreenaathkv@gmail.com]
Sent: Monday, October 24, 2016 8:05 AM
To: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
Subject: DPDK-QoS- Using un-used bandwidth within a class
Hi
I am using DPDK QoS and I find something strange. I am not sure if something is wrong with my config or my understanding of queue weights is wrong.
In my config, I am using only 1 port and 1 subport and 1 pipe. Within that pipe, I am using only the last class (C3). Port, subport and pipe are configured with 100Mbps speed.
C3 is given the entire pipe's TB rate i.e entire bandwidth in essence.
In C3, I am giving relative weights of 1:4:2:2 for the four queues q0,q1,q2,q3
When no other traffic is coming in to q0,q2,q3, I am pumping ~100Mbps in to q1. However, I am seeing only 40% of the traffic going through q1. In other words the max throughput allowed through the queue is based on its weight and the unused bandwidth is not used.
Cannot the unused bandwidth from q0,q2 and q3 be used for q1?
Note-
Following is the QoS config output spit out by DPDK in syslog
SCHED: Low level config for pipe profile 0:
token bucket: period = 10, credits per period = 1, size = 25000
Traffic classes: period = 1250000, credits per period = [0, 0, 0, 125000]
Traffic class 3 oversubscription: weight = 0
WRR cost: [1, 1, 1, 1], [1, 1, 1, 1], [1, 1, 1, 1], [4, 1, 2, 2]
SCHED: Low level config for subport 0:
Token bucket: period = 10, credits per period = 1, size = 25000
Traffic classes: period = 1250000, credits per period = [0, 0, 0, 125000]
Traffic class 3 oversubscription: wm min = 0, wm max = 0
--
regards
sreenaath
^ permalink raw reply
* [PATCH v6 2/2] app/test_pmd: fix DCB configuration
From: Bernard Iremonger @ 2016-10-25 16:51 UTC (permalink / raw)
To: dev, rahul.r.shah, wenzhuo.lu, cristian.dumitrescu, jingjing.wu
Cc: Bernard Iremonger
In-Reply-To: <1476872471-23362-1-git-send-email-bernard.iremonger@intel.com>
Data Centre Bridge (DCB) configuration fails when SRIOV is
enabled if nb_rxq and nb_txq are not set to 1.
When dcb_mode is DCB_VT_ENABLED set nb_rxq and nb_txq to 1.
The failure occurs during configuration of the ixgbe PMD when
it is started, in the ixgbe_check_mq_mode function, if nb_rxq
and nb_txq are not set to 1.
Fixes: 2a977b891f99 ("app/testpmd: fix DCB configuration")
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
Acked-by: Wenzhuo Lu <Wenzhuo.lu@intel.com>
---
app/test-pmd/testpmd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 6185be6..ee567c3 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2002,8 +2002,8 @@ init_port_dcb_config(portid_t pid,
* and has the same number of rxq and txq in dcb mode
*/
if (dcb_mode == DCB_VT_ENABLED) {
- nb_rxq = rte_port->dev_info.max_rx_queues;
- nb_txq = rte_port->dev_info.max_tx_queues;
+ nb_rxq = 1;
+ nb_txq = 1;
} else {
/*if vt is disabled, use all pf queues */
if (rte_port->dev_info.vmdq_pool_base == 0) {
--
2.10.1
^ permalink raw reply related
* [PATCH v6 1/2] net/ixgbe: support multiqueue mode VMDq DCB with SRIOV
From: Bernard Iremonger @ 2016-10-25 16:51 UTC (permalink / raw)
To: dev, rahul.r.shah, wenzhuo.lu, cristian.dumitrescu, jingjing.wu
Cc: Bernard Iremonger
In-Reply-To: <1476872471-23362-1-git-send-email-bernard.iremonger@intel.com>
The folowing changes have been made to allow Data Centre Bridge
(DCB) configuration when SRIOV is enabled.
Modify ixgbe_check_mq_mode function,
when SRIOV is enabled, enable mq_mode
ETH_MQ_RX_VMDQ_DCB and ETH_MQ_TX_VMDQ_DCB.
Modify ixgbe_dcb_tx_hw_config function,
replace the struct ixgbe_hw parameter with a
struct rte_eth_dev parameter and handle SRIOV enabled.
Modify ixgbe_dev_mq_rx_configure function,
when SRIOV is enabled, enable mq_mode ETH_MQ_RX_VMDQ_DCB.
Modify ixgbe_configure_dcb function,
revise check on dev->data->nb_rx_queues.
Signed-off-by: Rahul R Shah <rahul.r.shah@intel.com>
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
Acked-by: Wenzhuo Lu <Wenzhuo.lu@intel.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 11 ++++++-----
drivers/net/ixgbe/ixgbe_rxtx.c | 35 ++++++++++++++++++++++-------------
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 4ca5747..4d5ce83 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1977,6 +1977,9 @@ ixgbe_check_mq_mode(struct rte_eth_dev *dev)
/* check multi-queue mode */
switch (dev_conf->rxmode.mq_mode) {
case ETH_MQ_RX_VMDQ_DCB:
+ PMD_INIT_LOG(INFO, "ETH_MQ_RX_VMDQ_DCB mode supported in SRIOV");
+ dev->data->dev_conf.rxmode.mq_mode = ETH_MQ_RX_VMDQ_DCB;
+ break;
case ETH_MQ_RX_VMDQ_DCB_RSS:
/* DCB/RSS VMDQ in SRIOV mode, not implement yet */
PMD_INIT_LOG(ERR, "SRIOV active,"
@@ -2012,11 +2015,9 @@ ixgbe_check_mq_mode(struct rte_eth_dev *dev)
switch (dev_conf->txmode.mq_mode) {
case ETH_MQ_TX_VMDQ_DCB:
- /* DCB VMDQ in SRIOV mode, not implement yet */
- PMD_INIT_LOG(ERR, "SRIOV is active,"
- " unsupported VMDQ mq_mode tx %d.",
- dev_conf->txmode.mq_mode);
- return -EINVAL;
+ PMD_INIT_LOG(INFO, "ETH_MQ_TX_VMDQ_DCB mode supported in SRIOV");
+ dev->data->dev_conf.txmode.mq_mode = ETH_MQ_TX_VMDQ_DCB;
+ break;
default: /* ETH_MQ_TX_VMDQ_ONLY or ETH_MQ_TX_NONE */
dev->data->dev_conf.txmode.mq_mode = ETH_MQ_TX_VMDQ_ONLY;
break;
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 2ce8234..b2d9f45 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* Copyright 2014 6WIND S.A.
* All rights reserved.
*
@@ -3313,15 +3313,16 @@ ixgbe_vmdq_dcb_configure(struct rte_eth_dev *dev)
/**
* ixgbe_dcb_config_tx_hw_config - Configure general DCB TX parameters
- * @hw: pointer to hardware structure
+ * @dev: pointer to eth_dev structure
* @dcb_config: pointer to ixgbe_dcb_config structure
*/
static void
-ixgbe_dcb_tx_hw_config(struct ixgbe_hw *hw,
+ixgbe_dcb_tx_hw_config(struct rte_eth_dev *dev,
struct ixgbe_dcb_config *dcb_config)
{
uint32_t reg;
uint32_t q;
+ struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
PMD_INIT_FUNC_TRACE();
if (hw->mac.type != ixgbe_mac_82598EB) {
@@ -3340,10 +3341,17 @@ ixgbe_dcb_tx_hw_config(struct ixgbe_hw *hw,
reg |= IXGBE_MTQC_VT_ENA;
IXGBE_WRITE_REG(hw, IXGBE_MTQC, reg);
- /* Disable drop for all queues */
- for (q = 0; q < 128; q++)
- IXGBE_WRITE_REG(hw, IXGBE_QDE,
- (IXGBE_QDE_WRITE | (q << IXGBE_QDE_IDX_SHIFT)));
+ if (RTE_ETH_DEV_SRIOV(dev).active == 0) {
+ /* Disable drop for all queues in VMDQ mode*/
+ for (q = 0; q < 128; q++)
+ IXGBE_WRITE_REG(hw, IXGBE_QDE,
+ (IXGBE_QDE_WRITE | (q << IXGBE_QDE_IDX_SHIFT)));
+ } else {
+ /* Enable drop for all queues in SRIOV mode */
+ for (q = 0; q < 128; q++)
+ IXGBE_WRITE_REG(hw, IXGBE_QDE,
+ (IXGBE_QDE_WRITE | (q << IXGBE_QDE_IDX_SHIFT) | IXGBE_QDE_ENABLE));
+ }
/* Enable the Tx desc arbiter */
reg = IXGBE_READ_REG(hw, IXGBE_RTTDCS);
@@ -3378,7 +3386,7 @@ ixgbe_vmdq_dcb_hw_tx_config(struct rte_eth_dev *dev,
vmdq_tx_conf->nb_queue_pools == ETH_16_POOLS ? 0xFFFF : 0xFFFFFFFF);
/*Configure general DCB TX parameters*/
- ixgbe_dcb_tx_hw_config(hw, dcb_config);
+ ixgbe_dcb_tx_hw_config(dev, dcb_config);
}
static void
@@ -3661,7 +3669,7 @@ ixgbe_dcb_hw_configure(struct rte_eth_dev *dev,
/*get DCB TX configuration parameters from rte_eth_conf*/
ixgbe_dcb_tx_config(dev, dcb_config);
/*Configure general DCB TX parameters*/
- ixgbe_dcb_tx_hw_config(hw, dcb_config);
+ ixgbe_dcb_tx_hw_config(dev, dcb_config);
break;
default:
PMD_INIT_LOG(ERR, "Incorrect DCB TX mode configuration");
@@ -3810,7 +3818,7 @@ void ixgbe_configure_dcb(struct rte_eth_dev *dev)
(dev_conf->rxmode.mq_mode != ETH_MQ_RX_DCB_RSS))
return;
- if (dev->data->nb_rx_queues != ETH_DCB_NUM_QUEUES)
+ if (dev->data->nb_rx_queues > ETH_DCB_NUM_QUEUES)
return;
/** Configure DCB hardware **/
@@ -4082,12 +4090,13 @@ ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev)
case ETH_MQ_RX_VMDQ_RSS:
ixgbe_config_vf_rss(dev);
break;
-
- /* FIXME if support DCB/RSS together with VMDq & SRIOV */
case ETH_MQ_RX_VMDQ_DCB:
+ ixgbe_vmdq_dcb_configure(dev);
+ break;
+ /* FIXME if support DCB/RSS together with VMDq & SRIOV */
case ETH_MQ_RX_VMDQ_DCB_RSS:
PMD_INIT_LOG(ERR,
- "Could not support DCB with VMDq & SRIOV");
+ "Could not support DCB/RSS with VMDq & SRIOV");
return -1;
default:
ixgbe_config_vf_default(dev);
--
2.10.1
^ permalink raw reply related
* [PATCH v6 0/2] net/ixgbe: VMDq DCB with SRIOV
From: Bernard Iremonger @ 2016-10-25 16:51 UTC (permalink / raw)
To: dev, rahul.r.shah, wenzhuo.lu, cristian.dumitrescu, jingjing.wu
Cc: Bernard Iremonger
In-Reply-To: <1476872471-23362-1-git-send-email-bernard.iremonger@intel.com>
Changes in v6:
rebase to latest master.
revise commit messages.
Changes in v5:
fix enable/disable of the QDE bit in the PFQDE register.
Changes in v4:
changes to ixgbe patch following comments.
Changes in v3:
rebase to latest master.
update commit message for ixgbe patch
add testpmd patch.
Changes in v2:
rebase to latest master.
Bernard Iremonger (2):
net/ixgbe: support multiqueue mode VMDq DCB with SRIOV
app/test_pmd: fix DCB configuration
app/test-pmd/testpmd.c | 4 ++--
drivers/net/ixgbe/ixgbe_ethdev.c | 11 ++++++-----
drivers/net/ixgbe/ixgbe_rxtx.c | 35 ++++++++++++++++++++++-------------
3 files changed, 30 insertions(+), 20 deletions(-)
--
2.10.1
^ permalink raw reply
* Re: [PATCHv3] examples/l3fwd: em: use hw accelerated crc hash function for arm64
From: Thomas Monjalon @ 2016-10-25 16:49 UTC (permalink / raw)
To: Hemant Agrawal; +Cc: dev, jerin.jacob
In-Reply-To: <1476443456-24382-1-git-send-email-hemant.agrawal@nxp.com>
2016-10-14 16:40, Hemant Agrawal:
> if machine level CRC extension are available, offload the
> hash to machine provide functions e.g. armv8-a CRC extensions
> support it
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> Reviewed-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Applied, thanks
^ permalink raw reply
* [PATCH] pdump: revert PCI device name conversion
From: Reshma Pattan @ 2016-10-25 16:31 UTC (permalink / raw)
To: dev; +Cc: Reshma Pattan
Earlier ethdev library created the device names in the
"bus:device.func" format hence pdump library implemented
its own conversion method for changing the user passed
device name format "domain:bus:device.func" to "bus:device.func"
for finding the port id using device name using ethdev library
calls. Now after ethdev and eal rework
http://dpdk.org/dev/patchwork/patch/15855/,
the device names are created in the format "domain:bus:device.func",
so pdump library conversion is not needed any more, hence removed
the corresponding code.
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
lib/librte_pdump/rte_pdump.c | 37 ++-----------------------------------
1 file changed, 2 insertions(+), 35 deletions(-)
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ea5ccd9..504a1ce 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -226,29 +226,6 @@ pdump_tx(uint8_t port __rte_unused, uint16_t qidx __rte_unused,
}
static int
-pdump_get_dombdf(char *device_id, char *domBDF, size_t len)
-{
- int ret;
- struct rte_pci_addr dev_addr = {0};
-
- /* identify if device_id is pci address or name */
- ret = eal_parse_pci_DomBDF(device_id, &dev_addr);
- if (ret < 0)
- return -1;
-
- if (dev_addr.domain)
- ret = snprintf(domBDF, len, "%u:%u:%u.%u", dev_addr.domain,
- dev_addr.bus, dev_addr.devid,
- dev_addr.function);
- else
- ret = snprintf(domBDF, len, "%u:%u.%u", dev_addr.bus,
- dev_addr.devid,
- dev_addr.function);
-
- return ret;
-}
-
-static int
pdump_regitser_rx_callbacks(uint16_t end_q, uint8_t port, uint16_t queue,
struct rte_ring *ring, struct rte_mempool *mp,
uint16_t operation)
@@ -885,7 +862,6 @@ rte_pdump_enable_by_deviceid(char *device_id, uint16_t queue,
void *filter)
{
int ret = 0;
- char domBDF[DEVICE_ID_SIZE];
ret = pdump_validate_ring_mp(ring, mp);
if (ret < 0)
@@ -894,11 +870,7 @@ rte_pdump_enable_by_deviceid(char *device_id, uint16_t queue,
if (ret < 0)
return ret;
- if (pdump_get_dombdf(device_id, domBDF, sizeof(domBDF)) > 0)
- ret = pdump_prepare_client_request(domBDF, queue, flags,
- ENABLE, ring, mp, filter);
- else
- ret = pdump_prepare_client_request(device_id, queue, flags,
+ ret = pdump_prepare_client_request(device_id, queue, flags,
ENABLE, ring, mp, filter);
return ret;
@@ -928,17 +900,12 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
uint32_t flags)
{
int ret = 0;
- char domBDF[DEVICE_ID_SIZE];
ret = pdump_validate_flags(flags);
if (ret < 0)
return ret;
- if (pdump_get_dombdf(device_id, domBDF, sizeof(domBDF)) > 0)
- ret = pdump_prepare_client_request(domBDF, queue, flags,
- DISABLE, NULL, NULL, NULL);
- else
- ret = pdump_prepare_client_request(device_id, queue, flags,
+ ret = pdump_prepare_client_request(device_id, queue, flags,
DISABLE, NULL, NULL, NULL);
return ret;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 1/3] drivers: add name alias registration for rte_driver
From: Thomas Monjalon @ 2016-10-25 16:17 UTC (permalink / raw)
To: Jan Blunck
Cc: Yuanhan Liu, dev, pablo.de.lara.guarch, john.mcnamara,
maxime.coquelin
In-Reply-To: <20161024174126.GX16751@yliu-dev.sh.intel.com>
2016-10-25 01:41, Yuanhan Liu:
> On Mon, Oct 24, 2016 at 12:22:21PM -0400, Jan Blunck wrote:
> > This adds infrastructure for drivers to allow being requested by an alias
> > so that a renamed driver can still get loaded by its legacy name.
> >
> > Signed-off-by: Jan Blunck <jblunck@infradead.org>
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Tested-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>
> Series Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH 2/2] examples/tep_term: Fix packet len for multi-seg mbuf
From: Thomas Monjalon @ 2016-10-25 15:53 UTC (permalink / raw)
To: Michael Qiu; +Cc: dev, Tan, Jianfeng, Michael Qiu
In-Reply-To: <ED26CBA2FAD1BF48A8719AEF02201E364E6C358A@SHSMSX103.ccr.corp.intel.com>
> > For multi-seg mbuf, ip->total_length should be pkt_len subtract
> > ether len.
> >
> > Fixes: 4abe471ed6fc("examples/tep_term: implement VXLAN processing")
> >
> > Signed-off-by: Michael Qiu <qiudayu@chinac.com>
>
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH v10 11/25] eal/pci: helpers for device name parsing/update
From: Pattan, Reshma @ 2016-10-25 15:49 UTC (permalink / raw)
To: Shreyansh Jain, dev@dpdk.org
Cc: viktorin@rehivetech.com, David Marchand, hemant.agrawal@nxp.com,
Thomas Monjalon
In-Reply-To: <1474000200-16705-12-git-send-email-shreyansh.jain@nxp.com>
Hi Shreyansh,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shreyansh Jain
> Sent: Friday, September 16, 2016 5:30 AM
> To: dev@dpdk.org
> Cc: viktorin@rehivetech.com; David Marchand <david.marchand@6wind.com>;
> hemant.agrawal@nxp.com; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Shreyansh Jain <shreyansh.jain@nxp.com>
> Subject: [dpdk-dev] [PATCH v10 11/25] eal/pci: helpers for device name
> parsing/update
>
> From: David Marchand <david.marchand@6wind.com>
>
> - Move rte_eth_dev_create_unique_device_name() from ether/rte_ethdev.c to
> common/include/rte_pci.h as rte_eal_pci_device_name(). Being a common
> method, can be used across crypto/net PCI PMDs.
> - Remove crypto specific routine and fallback to common name function.
> - Introduce a eal private Update function for PCI device naming.
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> [Shreyansh: Merge crypto/pci helper patches]
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
> lib/librte_cryptodev/rte_cryptodev.c | 27 +++---------------
> lib/librte_eal/bsdapp/eal/eal_pci.c | 49
> +++++++++++++++++++++++++++++++++
> lib/librte_eal/common/eal_private.h | 13 +++++++++
> lib/librte_eal/common/include/rte_pci.h | 24 ++++++++++++++++
> lib/librte_eal/linuxapp/eal/eal_pci.c | 13 +++++++++
> lib/librte_ether/rte_ethdev.c | 24 +++-------------
> 6 files changed, 107 insertions(+), 43 deletions(-)
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> b/lib/librte_cryptodev/rte_cryptodev.c
> index 2a3b649..c81e366 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -365,23 +365,6 @@ rte_cryptodev_pmd_allocate(const char *name, int
> socket_id)
> return cryptodev;
> }
>
> *
> * This function is private to EAL.
> diff --git a/lib/librte_eal/common/include/rte_pci.h
> b/lib/librte_eal/common/include/rte_pci.h
> index cf81898..e1f695f 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -82,6 +82,7 @@ extern "C" {
> /** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */ #define
> PCI_PRI_FMT "%.4" PRIx16 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> +#define PCI_PRI_STR_SIZE sizeof("XXXX:XX:XX.X")
>
> /** Short formatting string, without domain, for PCI device: Ex: 00:01.0 */
> #define PCI_SHORT_PRI_FMT "%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8 @@ -308,6
>
> +static inline void
> +rte_eal_pci_device_name(const struct rte_pci_addr *addr,
> + char *output, size_t size)
> +{
> + RTE_VERIFY(size >= PCI_PRI_STR_SIZE);
> + RTE_VERIFY(snprintf(output, size, PCI_PRI_FMT,
> + addr->domain, addr->bus,
> + addr->devid, addr->function) >= 0); }
> +
>
> +int
> +pci_update_device(const struct rte_pci_addr *addr) {
> + char filename[PATH_MAX];
> +
> + snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT,
> + pci_get_sysfs_path(), addr->domain, addr->bus, addr->devid,
> + addr->function);
> +
> + return pci_scan_one(filename, addr->domain, addr->bus, addr->devid,
> + addr->function);
> +}
> +
Earlier device names were created in the format "bus:deviceid.function" as per the below ethdev API.
Now after above new eal API the name format is "domain:bus:deviceid.func" was that intentional and why is that so.
> -static int
> -rte_eth_dev_create_unique_device_name(char *name, size_t size,
> - struct rte_pci_device *pci_dev)
> -{
> - int ret;
> -
> - ret = snprintf(name, size, "%d:%d.%d",
> - pci_dev->addr.bus, pci_dev->addr.devid,
> - pci_dev->addr.function);
> - if (ret < 0)
> - return ret;
> - return 0;
> -}
> -
^ permalink raw reply
* [PATCH v2] mempool: fix search of maximum contiguous pages
From: Olivier Matz @ 2016-10-25 15:01 UTC (permalink / raw)
To: dev, wei.dai
Cc: sergio.gonzalez.monroy, jianfeng.tan, thomas.monjalon, stable
In-Reply-To: <1476351445-18102-1-git-send-email-wei.dai@intel.com>
From: Wei Dai <wei.dai@intel.com>
paddr[i] + pg_sz always points to the start physical address of the
2nd page after pddr[i], so only up to 2 pages can be combinded to
be used. With this revision, more than 2 pages can be used.
Fixes: 84121f197187 ("mempool: store memory chunks in a list")
Signed-off-by: Wei Dai <wei.dai@intel.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
lib/librte_mempool/rte_mempool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 71017e1..e94e56f 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -428,7 +428,7 @@ rte_mempool_populate_phys_tab(struct rte_mempool *mp, char *vaddr,
/* populate with the largest group of contiguous pages */
for (n = 1; (i + n) < pg_num &&
- paddr[i] + pg_sz == paddr[i+n]; n++)
+ paddr[i + n - 1] + pg_sz == paddr[i + n]; n++)
;
ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz,
--
2.8.1
^ permalink raw reply related
* Re: [PATCH] mempool: fix search of maximum contiguous pages
From: Olivier Matz @ 2016-10-25 14:56 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Ananyev, Konstantin, Dai, Wei, Gonzalez Monroy, Sergio,
Tan, Jianfeng
In-Reply-To: <2906356.2N5hSckZQm@xps13>
Hi Thomas,
On 10/25/2016 04:37 PM, Thomas Monjalon wrote:
> 2016-10-13 17:05, Olivier MATZ:
>> Hi Wei,
>>
>> On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote:
>>>
>>>>
>>>>>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>>>>>> b/lib/librte_mempool/rte_mempool.c
>>>>>>> index 71017e1..e3e254a 100644
>>>>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct
>>>>>>> rte_mempool *mp, char *vaddr,
>>>>>>>
>>>>>>> for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) {
>>>>>>>
>>>>>>> + phys_addr_t paddr_next;
>>>>>>> + paddr_next = paddr[i] + pg_sz;
>>>>>>> +
>>>>>>> /* populate with the largest group of contiguous pages */
>>>>>>> for (n = 1; (i + n) < pg_num &&
>>>>>>> - paddr[i] + pg_sz == paddr[i+n]; n++)
>>>>>>> + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz)
>>>>>>> ;
>>>>>>
>>>>>> Good catch.
>>>>>> Why not just paddr[i + n - 1] != paddr[i + n]?
>>>>>
>>>>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course.
>>>>>
>>>>>> Then you don't need extra variable (paddr_next) here.
>>>>>> Konstantin
>>>>
>>>> Thank you, Konstantin
>>>> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning.
>>>> But I assume that my revision with paddr_next += pg_sz may have a bit better performance.
>>>
>>> I don't think there would be any real difference, again it is not performance critical code-path.
>>>
>>>> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it.
>>>
>>> Yes, that's one seems even better for me - make things more clear.
>>
>> Thank you for fixing this.
>>
>> My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]"
>>
>> If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no
>> problem with it either.
>
> No answer from Wei Dai.
> Please Olivier advise what to do with this patch.
> Thanks
>
I think it's good to have this fix in 16.11.
I'm sending a v2 based on Wei's patch.
Olivier
^ permalink raw reply
* Re: mbuf changes
From: Thomas Monjalon @ 2016-10-25 14:54 UTC (permalink / raw)
To: Ramia, Kannan Babu
Cc: dev, Olivier Matz, Morten Brørup, Ananyev, Konstantin,
Richardson, Bruce, Wiles, Keith
In-Reply-To: <682698A055A0F44AA47192B2014114976258DC35@BGSMSX102.gar.corp.intel.com>
2016-10-25 14:32, Ramia, Kannan Babu:
> I didn't get your question. The only information exchanged between the stages is mbuf pointer. So the information has to be in mbuf, whether it's part of Meta or in private area in the packet buffer headroom is that the question you are asking. The private area is application specific, while I am looking for the port information getting updated from driver to application and it's generic to multiple applications.
Thanks, your answer is perfect (except it is top-posted ;)
The discussion is more about performance. So you agree that the port
information is not performance sensitive.
It appears that it could be an information filled in a private area
at app-level, because the application knows which port it is polling.
However we may need a way to put some generic informations somewhere,
not in the first cache lines.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 25, 2016 6:54 PM
> To: Ramia, Kannan Babu <kannan.babu.ramia@intel.com>
> Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Morten Brørup <mb@smartsharesystems.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; Wiles, Keith <keith.wiles@intel.com>
> Subject: Re: [dpdk-dev] mbuf changes
>
> 2016-10-25 13:04, Ramia, Kannan Babu:
> > Port filed is important meta information for the application use like
> > CGNAT vEPC functions etc.
> > I strongly recommend to keep the field in mind meta.
>
> Have you tried to move this field outside of the mbuf?
> What is the performance degradation?
> We need more information than some assumptions.
>
^ permalink raw reply
* Re: [PATCH 0/2] crypto/qat: performance optimisation
From: De Lara Guarch, Pablo @ 2016-10-25 14:45 UTC (permalink / raw)
To: Trahe, Fiona, dev@dpdk.org
Cc: Griffin, John, Jain, Deepak K, Kusztal, ArkadiuszX
In-Reply-To: <1477310400-4934-1-git-send-email-fiona.trahe@intel.com>
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, October 24, 2016 5:00 AM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo; Trahe, Fiona; Griffin, John; Jain, Deepak K; Kusztal,
> ArkadiuszX
> Subject: [PATCH 0/2] crypto/qat: performance optimisation
>
> QAT PMD adjusts the buffer start address and offsets passed
> to the device so that the DMAs in and out of the device are
> 64-byte aligned.
> This gives more consistent throughput, which had been
> variable depending on how the application set up the mbuf.
> The message builder code had to be considerably re-factored
> to do this efficiently.
> Also performance test not taking IV prepend offsets
> into account were corrected.
>
> Fiona Trahe (2):
> crypto/qat: rework request builder for performance
> app/test: use correct offsets in AES perf test
>
> app/test/test_cryptodev_perf.c | 15 +-
> drivers/crypto/qat/qat_adf/icp_qat_hw.h | 5 +
> drivers/crypto/qat/qat_adf/qat_algs.h | 1 +
> drivers/crypto/qat/qat_adf/qat_algs_build_desc.c | 2 +
> drivers/crypto/qat/qat_crypto.c | 242 ++++++++++++++++-------
> 5 files changed, 185 insertions(+), 80 deletions(-)
>
> --
> 2.5.0
Applied to dpdk-next-crypto.
Thanks,
Pablo
^ permalink raw reply
* Re: mbuf changes
From: Olivier Matz @ 2016-10-25 14:45 UTC (permalink / raw)
To: Morten Brørup, Ananyev, Konstantin, Richardson, Bruce
Cc: Adrien Mazarguil, Wiles, Keith, dev, Oleg Kuporosov
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8C1@smartserver.smartshare.dk>
On 10/25/2016 04:25 PM, Morten Brørup wrote:
> It might also make sense documenting the mbuf fields in more detail somewhere. E.g. the need for nb_segs in the NIC's TX handler.
Good point, I'll do it at the same time than the first rework
proposition.
^ permalink raw reply
* Re: [PATCH v10 1/6] ethdev: add Tx preparation
From: Olivier Matz @ 2016-10-25 14:41 UTC (permalink / raw)
To: Tomasz Kulasek, dev; +Cc: konstantin.ananyev
In-Reply-To: <1477327917-18564-2-git-send-email-tomaszx.kulasek@intel.com>
Hi Tomasz,
On 10/24/2016 06:51 PM, Tomasz Kulasek wrote:
> Added API for `rte_eth_tx_prep`
>
> [...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -182,6 +182,7 @@ extern "C" {
> #include <rte_pci.h>
> #include <rte_dev.h>
> #include <rte_devargs.h>
> +#include <rte_errno.h>
> #include "rte_ether.h"
> #include "rte_eth_ctrl.h"
> #include "rte_dev_info.h"
> @@ -699,6 +700,8 @@ struct rte_eth_desc_lim {
> uint16_t nb_max; /**< Max allowed number of descriptors. */
> uint16_t nb_min; /**< Min allowed number of descriptors. */
> uint16_t nb_align; /**< Number of descriptors should be aligned to. */
> + uint16_t nb_seg_max; /**< Max number of segments per whole packet. */
> + uint16_t nb_mtu_seg_max; /**< Max number of segments per one MTU */
Sorry if it was not clear in my previous review, but I think this should
be better explained here. You said that the "limitation of number
of segments may differ depend of TSO/non TSO".
As an application developer, I still have some difficulties to
clearly understand what does that mean. Is it the maximum number
of mbuf-segments that contain payload for one tcp-segment sent by
the device?
In that case, it looks quite difficult to verify that in an application.
It looks that this field is not used by validate_offload(), so how
should it be used by an application?
> };
>
> /**
> @@ -1188,6 +1191,11 @@ typedef uint16_t (*eth_tx_burst_t)(void *txq,
> uint16_t nb_pkts);
> /**< @internal Send output packets on a transmit queue of an Ethernet device. */
>
> +typedef uint16_t (*eth_tx_prep_t)(void *txq,
> + struct rte_mbuf **tx_pkts,
> + uint16_t nb_pkts);
> +/**< @internal Prepare output packets on a transmit queue of an Ethernet device. */
> +
> typedef int (*flow_ctrl_get_t)(struct rte_eth_dev *dev,
> struct rte_eth_fc_conf *fc_conf);
> /**< @internal Get current flow control parameter on an Ethernet device */
> @@ -1622,6 +1630,7 @@ struct rte_eth_rxtx_callback {
> struct rte_eth_dev {
> eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
> eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
> + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare function. */
> struct rte_eth_dev_data *data; /**< Pointer to device data */
> const struct eth_driver *driver;/**< Driver for this device */
> const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> @@ -2816,6 +2825,93 @@ rte_eth_tx_burst(uint8_t port_id, uint16_t queue_id,
> return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts, nb_pkts);
> }
>
> +/**
> + * Process a burst of output packets on a transmit queue of an Ethernet device.
> + *
> + * The rte_eth_tx_prep() function is invoked to prepare output packets to be
> + * transmitted on the output queue *queue_id* of the Ethernet device designated
> + * by its *port_id*.
> + * The *nb_pkts* parameter is the number of packets to be prepared which are
> + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of them
> + * allocated from a pool created with rte_pktmbuf_pool_create().
> + * For each packet to send, the rte_eth_tx_prep() function performs
> + * the following operations:
> + *
> + * - Check if packet meets devices requirements for tx offloads.
> + *
> + * - Check limitations about number of segments.
> + *
> + * - Check additional requirements when debug is enabled.
> + *
> + * - Update and/or reset required checksums when tx offload is set for packet.
> + *
> + * The rte_eth_tx_prep() function returns the number of packets ready to be
> + * sent. A return value equal to *nb_pkts* means that all packets are valid and
> + * ready to be sent.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * The value must be a valid port id.
> + * @param queue_id
> + * The index of the transmit queue through which output packets must be
> + * sent.
> + * The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + * to rte_eth_dev_configure().
> + * @param tx_pkts
> + * The address of an array of *nb_pkts* pointers to *rte_mbuf* structures
> + * which contain the output packets.
> + * @param nb_pkts
> + * The maximum number of packets to process.
> + * @return
> + * The number of packets correct and ready to be sent. The return value can be
> + * less than the value of the *tx_pkts* parameter when some packet doesn't
> + * meet devices requirements with rte_errno set appropriately.
> + */
Inserting here the previous comment:
>> Can we add the constraint that invalid packets are left untouched?
>>
>> I think most of the time there will be a software fallback in that case,
>> so it would be good to ensure that this function does not change the flags
>> or the packet data.
>
> In current implementation, if packet is invalid, its data is never modified. Only checks are done. The only exception is when checksum needs to be updated or initialized, but it's done after packet validation.
> If we want to use/restore packet in application it didn't should be changed in any way for invalid packets.
>
I think this should be explicitly said in the API comment that
valid packets may be modified (*), but invalid packets (whose index
is >= than the return value) are left untouched.
(*) we still need to discuss that point, see below.
Another comment was made:
>> Another thing that could be interesting for the caller is to know the
>> reason of the failure. Maybe the different errno types could be detailed
>> here. For instance:
>> - EINVAL: offload flags are not correctly set (i.e. would fail whatever
>> the hardware)
>> - ENOTSUP: the offload feature is not supported by the hardware
>> - ...
>>
>
> Ok.
Don't you feel it could go in the API comment too?
> [...]
>
> +/**
> + * Fix pseudo header checksum
> + *
> + * This function fixes pseudo header checksum for TSO and non-TSO tcp/udp in
> + * provided mbufs packet data.
> + *
> + * - for non-TSO tcp/udp packets full pseudo-header checksum is counted and set
> + * in packet data,
> + * - for TSO the IP payload length is not included in pseudo header.
> + *
> + * This function expects that used headers are in the first data segment of
> + * mbuf, and are not fragmented.
There is another requirement about the cloning and reference count.
Sorry I did not answer to Konstantin, but I still think this could be
an issue.
For instance, I think that the zero-copy mode of vhost application
references the packet sent by the guest and send the data. The payload
should not be modified because it is in guest memory (we don't know,
maybe the guest also cloned it for its own purpose).
It means that the tx_prep() API must not be used with clones, i.e
the headers must not reside in a segment whose RTE_MBUF_INDIRECT(seg)
or rte_mbuf_refcnt_read(seg) > 1.
- if we really want this API in 16.11, it should be clearly explained
in the API comment that it does not work with shared segments
- for next versions, we have to take a decision whether it should be
supported or not. In my opinion, cloned packets are useful and should
be supported properly by all dpdk APIs.
Thanks,
Olivier
^ permalink raw reply
* Re: [PATCH] mempool: fix search of maximum contiguous pages
From: Thomas Monjalon @ 2016-10-25 14:37 UTC (permalink / raw)
To: Olivier MATZ
Cc: dev, Ananyev, Konstantin, Dai, Wei, Gonzalez Monroy, Sergio,
Tan, Jianfeng
In-Reply-To: <57FFA2C9.9000603@6wind.com>
2016-10-13 17:05, Olivier MATZ:
> Hi Wei,
>
> On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote:
> >
> >>
> >>>>> diff --git a/lib/librte_mempool/rte_mempool.c
> >>>>> b/lib/librte_mempool/rte_mempool.c
> >>>>> index 71017e1..e3e254a 100644
> >>>>> --- a/lib/librte_mempool/rte_mempool.c
> >>>>> +++ b/lib/librte_mempool/rte_mempool.c
> >>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct
> >>>>> rte_mempool *mp, char *vaddr,
> >>>>>
> >>>>> for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) {
> >>>>>
> >>>>> + phys_addr_t paddr_next;
> >>>>> + paddr_next = paddr[i] + pg_sz;
> >>>>> +
> >>>>> /* populate with the largest group of contiguous pages */
> >>>>> for (n = 1; (i + n) < pg_num &&
> >>>>> - paddr[i] + pg_sz == paddr[i+n]; n++)
> >>>>> + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz)
> >>>>> ;
> >>>>
> >>>> Good catch.
> >>>> Why not just paddr[i + n - 1] != paddr[i + n]?
> >>>
> >>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course.
> >>>
> >>>> Then you don't need extra variable (paddr_next) here.
> >>>> Konstantin
> >>
> >> Thank you, Konstantin
> >> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning.
> >> But I assume that my revision with paddr_next += pg_sz may have a bit better performance.
> >
> > I don't think there would be any real difference, again it is not performance critical code-path.
> >
> >> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it.
> >
> > Yes, that's one seems even better for me - make things more clear.
>
> Thank you for fixing this.
>
> My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]"
>
> If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no
> problem with it either.
No answer from Wei Dai.
Please Olivier advise what to do with this patch.
Thanks
^ permalink raw reply
* Re: mbuf changes
From: Ramia, Kannan Babu @ 2016-10-25 14:32 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev@dpdk.org, Olivier Matz, Morten Brørup,
Ananyev, Konstantin, Richardson, Bruce, Wiles, Keith
In-Reply-To: <2277634.HujMj1Cctl@xps13>
I didn't get your question. The only information exchanged between the stages is mbuf pointer. So the information has to be in mbuf, whether it's part of Meta or in private area in the packet buffer headroom is that the question you are asking. The private area is application specific, while I am looking for the port information getting updated from driver to application and it's generic to multiple applications.
Regards
Kannan Babu
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Tuesday, October 25, 2016 6:54 PM
To: Ramia, Kannan Babu <kannan.babu.ramia@intel.com>
Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Morten Brørup <mb@smartsharesystems.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; Wiles, Keith <keith.wiles@intel.com>
Subject: Re: [dpdk-dev] mbuf changes
2016-10-25 13:04, Ramia, Kannan Babu:
> Port filed is important meta information for the application use like
> CGNAT vEPC functions etc.
> I strongly recommend to keep the field in mind meta.
Have you tried to move this field outside of the mbuf?
What is the performance degradation?
We need more information than some assumptions.
^ permalink raw reply
* Re: [PATCH] examples/ip_pipeline: fix gcc v6.2.1 build error
From: Thomas Monjalon @ 2016-10-25 14:32 UTC (permalink / raw)
To: Singh, Jasvinder; +Cc: dev, Dumitrescu, Cristian
In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D8912647A92FFE@IRSMSX108.ger.corp.intel.com>
> > This patch fixes the misleading indentation error on compiling ip_pipeline
> > app with gcc v6.2.1.
> >
> > Fixes: 3f2c9f3bb6c6 ("examples/ip_pipeline: add TAP port")
> >
> > Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> > ---
> > examples/ip_pipeline/app.h | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] examples/ip_pipeline: fix freeBSD build error
From: Thomas Monjalon @ 2016-10-25 14:32 UTC (permalink / raw)
To: Singh, Jasvinder; +Cc: dev, Dumitrescu, Cristian
In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D8912647A936AB@IRSMSX108.ger.corp.intel.com>
> > Error log:
> > CC init.o
> > examples/ip_pipeline/init.c:38:22: fatal error: linux/if.h: No such file or
> > directory
> > #include <linux/if.h>
> > ^
> > Fixes: 3f2c9f3bb6c6 ("examples/ip_pipeline: add TAP port")
> >
> > Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Applied, thanks
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 14:25 UTC (permalink / raw)
To: Ananyev, Konstantin, Olivier Matz, Richardson, Bruce
Cc: Adrien Mazarguil, Wiles, Keith, dev, Oleg Kuporosov
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CC805@irsmsx105.ger.corp.intel.com>
Lots of good arguments from Konstantin below!
Thomas also wrote something worth repeating:
"The mbuf space must be kept jealously like a DPDK treasure :)"
Maybe we should take a step away from the keyboard and consider the decision process and guidance criteria for mbuf members.
Here is an example of what I am thinking:
1. It is the intention to optimize mbuf handling for the most likely fast path through the entire system on the most common applications.
2. It is the intention that the most likely fast path through the RX handler and RX processing of the most common applications only needs to touch the first cache line.
3. The first cache line should be preferred for metadata set by most common NIC hardware in the RX handler.
4. The second cache line should be preferred for metadata required by most common NIC hardware in the TX handler.
5. The first cache line could be used for metadata used by the RX processing in the most likely fast path of most common applications.
6. The second cache line could be used for metadata used by the TX processing in the most likely fast path of most common applications.
7. The RX and TX handlers are equally important, and the RX handler should not be optimized at the cost of the TX handler.
Please note the use of "most common" and "most likely". It should be clear which cases we are optimizing for.
We should probably also define "the most common applications" (e.g. a Layer 3 router with or without VPN) and "the most likely fast path" (e.g. a single packet matching a series of lookups and ultimately forwarded to a single port).
It might also make sense documenting the mbuf fields in more detail somewhere. E.g. the need for nb_segs in the NIC's TX handler.
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Tuesday, October 25, 2016 3:39 PM
> To: Olivier Matz; Richardson, Bruce; Morten Brørup
> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Oleg Kuporosov
> Subject: RE: [dpdk-dev] mbuf changes
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Sent: Tuesday, October 25, 2016 1:49 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>; Morten Brørup
> > <mb@smartsharesystems.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Wiles, Keith
> > <keith.wiles@intel.com>; dev@dpdk.org; Oleg Kuporosov
> > <olegk@mellanox.com>
> > Subject: Re: [dpdk-dev] mbuf changes
> >
> >
> >
> > On 10/25/2016 02:45 PM, Bruce Richardson wrote:
> > > On Tue, Oct 25, 2016 at 02:33:55PM +0200, Morten Brørup wrote:
> > >> Comments at the end.
> > >>
> > >> Med venlig hilsen / kind regards
> > >> - Morten Brørup
> > >>
> > >>> -----Original Message-----
> > >>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > >>> Sent: Tuesday, October 25, 2016 2:20 PM
> > >>> To: Morten Brørup
> > >>> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Olivier Matz;
> > >>> Oleg Kuporosov
> > >>> Subject: Re: [dpdk-dev] mbuf changes
> > >>>
> > >>> On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> > >>>> Comments inline.
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> > >>>>> Richardson
> > >>>>> Sent: Tuesday, October 25, 2016 1:14 PM
> > >>>>> To: Adrien Mazarguil
> > >>>>> Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz;
> > >>>>> Oleg Kuporosov
> > >>>>> Subject: Re: [dpdk-dev] mbuf changes
> > >>>>>
> > >>>>> On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil
> wrote:
> > >>>>>> On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> > >>>>>>> Comments inline.
> > >>>>>>>
> > >>>>>>> Med venlig hilsen / kind regards
> > >>>>>>> - Morten Brørup
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> -----Original Message-----
> > >>>>>>>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > >>>>>>>> Sent: Tuesday, October 25, 2016 11:39 AM
> > >>>>>>>> To: Bruce Richardson
> > >>>>>>>> Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz;
> > >>>>>>>> Oleg Kuporosov
> > >>>>>>>> Subject: Re: [dpdk-dev] mbuf changes
> > >>>>>>>>
> > >>>>>>>> On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson
> > >>> wrote:
> > >>>>>>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith
> > >>> wrote:
> > >>>>>>>> [...]
> > >>>>>>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
> > >>>>>>>> <mb@smartsharesystems.com> wrote:
> > >>>>>>>> [...]
> > >>>>
> > >>>>>>>>> One other point I'll mention is that we need to have a
> > >>>>>>>>> discussion on how/where to add in a timestamp value into
> > >>> the
> > >>>>>>>>> mbuf. Personally, I think it can be in a union with the
> > >>>>> sequence
> > >>>>>>>>> number value, but I also suspect that 32-bits of a
> > >>> timestamp
> > >>>>>>>>> is not going to be enough for
> > >>>>>>>> many.
> > >>>>>>>>>
> > >>>>>>>>> Thoughts?
> > >>>>>>>>
> > >>>>>>>> If we consider that timestamp representation should use
> > >>>>> nanosecond
> > >>>>>>>> granularity, a 32-bit value may likely wrap around too
> > >>> quickly
> > >>>>>>>> to be useful. We can also assume that applications
> requesting
> > >>>>>>>> timestamps may care more about latency than throughput, Oleg
> > >>>>> found
> > >>>>>>>> that using the second cache line for this purpose had a
> > >>>>> noticeable impact [1].
> > >>>>>>>>
> > >>>>>>>> [1] http://dpdk.org/ml/archives/dev/2016-
> October/049237.html
> > >>>>>>>
> > >>>>>>> I agree with Oleg about the latency vs. throughput importance
> > >>>>>>> for
> > >>>>> such applications.
> > >>>>>>>
> > >>>>>>> If you need high resolution timestamps, consider them to be
> > >>>>> generated by the NIC RX driver, possibly by the hardware itself
> > >>>>> (http://w3new.napatech.com/features/time-precision/hardware-
> time
> > >>>>> - stamp), so the timestamp belongs in the first cache line. And
> > >>>>> I am proposing that it should have the highest possible
> > >>>>> accuracy, which makes the value hardware dependent.
> > >>>>>>>
> > >>>>>>> Furthermore, I am arguing that we leave it up to the
> > >>> application
> > >>>>>>> to
> > >>>>> keep track of the slowly moving bits (i.e. counting whole
> > >>>>> seconds, hours and calendar date) out of band, so we don't use
> > >>>>> precious
> > >>> space
> > >>>>> in the mbuf. The application doesn't need the NIC RX driver's
> > >>>>> fast path to capture which date (or even which second) a packet
> > >>>>> was received. Yes, it adds complexity to the application, but
> we
> > >>>>> can't set aside 64 bit for a generic timestamp. Or as a weird
> tradeoff:
> > >>>>> Put the fast moving 32 bit in the first cache line and the slow
> > >>>>> moving 32 bit in the second cache line, as a placeholder for
> the
> > >>> application to fill out if needed.
> > >>>>> Yes, it means that the application needs to check the time and
> > >>>>> update its variable holding the slow moving time once every
> > >>>>> second or so; but that should be doable without significant
> effort.
> > >>>>>>
> > >>>>>> That's a good point, however without a 64 bit value, elapsed
> > >>>>>> time between two arbitrary mbufs cannot be measured reliably
> > >>>>>> due to
> > >>> not
> > >>>>>> enough context, one way or another the low resolution value is
> > >>>>>> also
> > >>>>> needed.
> > >>>>>>
> > >>>>>> Obviously latency-sensitive applications are unlikely to
> > >>>>>> perform lengthy buffering and require this but I'm not sure
> > >>>>>> about all the possible use-cases. Considering many NICs expose
> > >>>>>> 64 bit
> > >>> timestaps,
> > >>>>>> I suggest we do not truncate them.
> > >>>>>>
> > >>>>>> I'm not a fan of the weird tradeoff either, PMDs will be
> > >>>>>> tempted to fill the extra 32 bits whenever they can and negate
> > >>>>>> the performance improvement of the first cache line.
> > >>>>>
> > >>>>> I would tend to agree, and I don't really see any convenient
> way
> > >>>>> to avoid putting in a 64-bit field for the timestamp in cache-
> line 0.
> > >>>>> If we are ok with having this overlap/partially overlap with
> > >>>>> sequence number, it will use up an extra 4B of storage in that
> > >>> cacheline.
> > >>>>
> > >>>> I agree about the lack of convenience! And Adrien certainly has
> a
> > >>> point about PMD temptations.
> > >>>>
> > >>>> However, I still don't think that a NICs ability to date-stamp a
> > >>> packet is sufficient reason to put a date-stamp in cache line 0
> of
> > >>> the mbuf. Storing only the fast moving 32 bit in cache line 0
> > >>> seems like a good compromise to me.
> > >>>>
> > >>>> Maybe you can find just one more byte, so it can hold 17 minutes
> > >>>> with nanosecond resolution. (I'm joking!)
> > >>>>
> > >>>> Please don't sacrifice the sequence number for the
> > >>>> seconds/hours/days
> > >>> part a timestamp. Maybe it could be configurable to use a 32 bit
> > >>> or 64 bit timestamp.
> > >>>>
> > >>> Do you see both timestamp and sequence numbers being used
> > >>> together? I would have thought that apps would either use one or
> the other?
> > >>> However, your suggestion is workable in any case, to allow the
> > >>> sequence number to overlap just the high 32 bits of the
> timestamp,
> > >>> rather than the low.
> > >>
> > >> In our case, I can foresee sequence numbers used for packet
> > >> processing and timestamps for timing analysis (and possibly for
> > >> packet
> > capturing, when being used).
>
> Great, but right now none of these fields are filled from NIC HW by PMD
> RX function (except RFC for melanox provided by Oleg, but again it is
> pure SW implementation).
> So I would repeat my question: why these fields should stay in the
> first cache-line?
> I understand that it would speed-up some particular application, but
> there are plenty of apps which do use different metadata.
> Let say l2/l3/l4 len - is very useful information for upper layer
> (L3/L4) packet processing.
> Should people who do use it start to push moving that fields into first
> cache-line too?
>
> >For timing analysis, we don’t need long durations, e.g. 4 seconds with
> 32 bit nanosecond resolution suffices.
> > And for packet capturing we are perfectly capable of adding the
> slowly
> >moving 32 bit of the timestamp to our output data stream without
> fetching it from the mbuf.
> > >>
> >
> > We should keep in mind that today we have the seqn field but it is
> not
> > used by any PMD. In case it is implemented, would it be a per-queue
> > sequence number? Is it useful from an application view?
>
> Exactly - it depends from SW that uses that field, right?
> It could be per process / per group of lcores / per port / per queue,
> etc.
> Depending on what are upper layer needs.
>
> >
> > This field is only used by the librte_reorder library, and in my
> > opinion, we should consider moving it in the second cache line since
> > it is not filled by the PMD.
>
> +1
>
> >
> >
> > > For the 32-bit timestamp case, it might be useful to have a
> > > right-shift value passed in to the ethdev driver. If we assume a
> NIC
> > > with nanosecond resolution, (or TSC value with resolution of that
> > > order of magnitude), then the app can choose to have 1 ns
> resolution
> > > with 4 second wraparound, or alternatively 4ns resolution with 16
> > > second wraparound, or even microsecond resolution with wrap around
> of over an hour.
> > > The cost is obviously just a shift op in the driver code per packet
> > > - hopefully with multiple packets done at a time using vector
> operations.
> >
> >
> > About the timestamp, we can manage to find 64 bits in the first cache
> > line, without sacrifying any field we have today.
>
> We can I suppose, but again what for and who will fill it?
> If PMD, then where it will get this information?
> If it is from rte_rdtsc() or clock(), then why upper layer can't do it
> itself?
> Konstantin
>
> >The question is more
> > for the fields we may want to add later.
> >
> > To answer to the question of the size of the timestamp, the first
> > question is to know what is the precision required for the
> > applications using it?
> >
> > I don't quite like the idea of splitting the timestamp in the 2 cache
> > lines, I think it would not be easy to use.
> >
> >
> > Olivier
^ permalink raw reply
* Re: [PATCH] kni: fix build with kernel 4.9
From: Thomas Monjalon @ 2016-10-25 14:22 UTC (permalink / raw)
To: Yigit, Ferruh; +Cc: dev, De Lara Guarch, Pablo
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8973CA0BE47@IRSMSX108.ger.corp.intel.com>
> > compile error:
> > CC [M] .../lib/librte_eal/linuxapp/kni/igb_main.o
> > .../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,
> > ^~~~~~~~~~~~~~~~~~~
> >
> > Linux kernel 4.9 updates API for ndo_set_vf_vlan:
> > Linux: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad
> > support")
> >
> > Use new API for Linux kernels >= 4.9
> >
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Tested-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Bruce Richardson @ 2016-10-25 14:00 UTC (permalink / raw)
To: Declan Doherty
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
In-Reply-To: <674cdf6a-7a92-8d1a-4caa-f2582cf1b733@intel.com>
On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote:
> On 25/10/16 13:57, Bruce Richardson wrote:
> > On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
> > > On 24/10/16 15:51, Jan Blunck wrote:
> > > > On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
> > > > <declan.doherty@intel.com> wrote:
> > > > > On 14/10/16 00:37, Eric Kinzie wrote:
> > > > > >
> > > > > > On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
> > > > > > >
> > > > > > > On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
> > > > > > > >
> > > > > > > > On 07.10.2016 05:02, Eric Kinzie wrote:
> > > > > > > > >
> > > > > > > > > On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> > > > > > > > > >
> > > > > > > > > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > > > > > > > > >
> > > > > > > > > > It is necessary to reconfigure all queues every time because
> > > > > > > > > > configuration
> > > > > > > > > > can be changed.
> > > > > > > > > >
> > > > > > > > > > For example, if we're reconfiguring bonding device with new memory
> > > > > > > > > > pool,
> > > > > > > > > > already configured queues will still use the old one. And if the old
> > > > > > > > > > mempool be freed, application likely will panic in attempt to use
> > > > > > > > > > freed mempool.
> > > > > > > > > >
> > > > > > > > > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > > > > > > > > reconfiguration:
> > > > > > > > > >
> > > > > > > > > > PANIC in rte_mempool_get_ops():
> > > > > > > > > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
> > > > > > > > > > failed
> > > > > > > > > >
> > > > > > > > > > Cc: <stable@dpdk.org>
> > > > > > > > > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> > > > > > > > > > 1 file changed, 2 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > index b20a272..eb5b6d1 100644
> > > > > > > > > > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
> > > > > > > > > > *bonded_eth_dev,
> > > > > > > > > > struct bond_rx_queue *bd_rx_q;
> > > > > > > > > > struct bond_tx_queue *bd_tx_q;
> > > > > > > > > >
> > > > > > > > > > - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> > > > > > > > > > - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> > > > > > > > > > int errval;
> > > > > > > > > > uint16_t q_id;
> > > > > > > > > >
> > > > > > > > > > @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > > > *bonded_eth_dev,
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > /* Setup Rx Queues */
> > > > > > > > > > - /* Use existing queues, if any */
> > > > > > > > > > - for (q_id = old_nb_rx_queues;
> > > > > > > > > > - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > > > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
> > > > > > > > > > q_id++) {
> > > > > > > > > > bd_rx_q = (struct bond_rx_queue
> > > > > > > > > > *)bonded_eth_dev->data->rx_queues[q_id];
> > > > > > > > > >
> > > > > > > > > > errval =
> > > > > > > > > > rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > > > @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > > > *bonded_eth_dev,
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > /* Setup Tx Queues */
> > > > > > > > > > - /* Use existing queues, if any */
> > > > > > > > > > - for (q_id = old_nb_tx_queues;
> > > > > > > > > > - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > > > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
> > > > > > > > > > q_id++) {
> > > > > > > > > > bd_tx_q = (struct bond_tx_queue
> > > > > > > > > > *)bonded_eth_dev->data->tx_queues[q_id];
> > > > > > > > > >
> > > > > > > > > > errval =
> > > > > > > > > > rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > > > --
> > > > > > > > > > 2.7.4
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > NAK
> > > > > > > > >
> > > > > > > > > There are still some users of this code. Let's give them a chance to
> > > > > > > > > comment before removing it.
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Eric,
> > > > > > > >
> > > > > > > > Are these users in CC-list? If not, could you, please, add them?
> > > > > > > > This patch awaits in mail-list already more than a month. I think, it's
> > > > > > > > enough
> > > > > > > > time period for all who wants to say something. Patch fixes a real bug
> > > > > > > > that
> > > > > > > > prevent using of DPDK bonding in all applications that reconfigures
> > > > > > > > devices
> > > > > > > > in runtime including OVS.
> > > > > > > >
> > > > > > > Agreed.
> > > > > > >
> > > > > > > Eric, does reverting this patch cause you problems directly, or is your
> > > > > > > concern
> > > > > > > just with regards to potential impact to others?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > /Bruce
> > > > > >
> > > > > >
> > > > > > This won't impact me directly. The users are CCed (different thread)
> > > > > > and I haven't seen any comment, so I no longer have any objection to
> > > > > > reverting this change.
> > > > > >
> > > > > > Eric
> > > > > >
> > > > >
> > > > > As there has been no further objections and this reinstates the original
> > > > > expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
> > > > > release.
> > > > >
> > > > > Acked-by: Declan Doherty <declan.doherty@intel.com>
> > > >
> > > > Ok, I can revert the revert for us.
> > > >
> > > > Do I read this correctly that you are not interested in fixing this properly?!
> > > >
> > > > Thanks,
> > > > Jan
> > > >
> > >
> > > Jan, sorry I missed the replies from last week due to the way my mail client
> > > was filtering the conversation. Let me have another look at this and I'll
> > > come back to the list.
> > >
> > > Thanks
> > > Declan
> >
> > While this patch has already been applied to dpdk-next-net tree, it
> > appears that there is still some ongoing discussion about it. I'm
> > therefore planning to pull it back out of the tree for rc2. If a
> > subsequent consensus is reached we can see about including it in rc3.
> >
> > Declan, as maintainer, does this seem reasonable to you.
> >
> > Regards,
> > /Bruce
> >
>
>
> Hey Bruce, that seems reasonable, I would like to discuss this further with
> Jan and Ilya.
>
Done. Hopefully consensus on a correct solution for this driver can be
reached soon.
Regards,
/Bruce
^ permalink raw reply
* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Bruce Richardson @ 2016-10-25 13:59 UTC (permalink / raw)
To: Declan Doherty
Cc: Ilya Maximets, dev, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
In-Reply-To: <20161019095525.GK27816@bricha3-MOBL3.ger.corp.intel.com>
On Wed, Oct 19, 2016 at 10:55:25AM +0100, Bruce Richardson wrote:
> On Thu, Oct 06, 2016 at 03:32:36PM +0100, Declan Doherty wrote:
> > On 07/09/16 13:28, Ilya Maximets wrote:
> > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > >
> > > It is necessary to reconfigure all queues every time because configuration
> > > can be changed.
> > >
> >
> > Hey Ilya, this makes sense. I guess this was my original intention but I
> > missed this case in my review of the change.
> >
> > > For example, if we're reconfiguring bonding device with new memory pool,
> > > already configured queues will still use the old one. And if the old
> > > mempool be freed, application likely will panic in attempt to use
> > > freed mempool.
> > >
> > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > reconfiguration:
> > >
> > > PANIC in rte_mempool_get_ops():
> > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
> > >
> > > Cc: <stable@dpdk.org>
> > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > ---
> >
> > Acked-by: Declan Doherty <declan.doherty@intel.com>
>
> Applied to dpdk-next-net/rel_16_11
>
Patch taken out of branch due to on-going discussion in this thread. It
can be re-applied later if consensus is reached that it is ok.
Apologies for any confusion caused by this, but after discussion with
the driver maintainer, we feel this is the safest course for now.
/Bruce
^ 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