* [PATCH v4] net/i40e: fix the hash filter invalid calculation in X722
From: Jeff Guo @ 2016-10-25 2:26 UTC (permalink / raw)
To: helin.zhang, jingjing.wu; +Cc: dev, jia.guo
In-Reply-To: <1476931738-44140-1-git-send-email-jia.guo@intel.com>
When verifying the Hash filtering on X722, we found a problem that
the hash value in descriptor is incorrect. The root caused is X722
uses different way of hash key word selection comparing with X710/XL710.
This patch fixes it by setting X722 specific key selection.
Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4:
refine commit log
---
drivers/net/i40e/i40e_ethdev.c | 60 +++++++++++++++++++++++++++++++++---------
1 file changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index db5f808..ca515dd 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -211,6 +211,14 @@
#define I40E_REG_INSET_L3_SRC_IP4 0x0001800000000000ULL
/* Destination IPv4 address */
#define I40E_REG_INSET_L3_DST_IP4 0x0000001800000000ULL
+/* Source IPv4 address for X722 */
+#define I40E_X722_REG_INSET_L3_SRC_IP4 0x0006000000000000ULL
+/* Destination IPv4 address for X722 */
+#define I40E_X722_REG_INSET_L3_DST_IP4 0x0000060000000000ULL
+/* IPv4 Protocol for X722 */
+#define I40E_X722_REG_INSET_L3_IP4_PROTO 0x0010000000000000ULL
+/* IPv4 Time to Live for X722 */
+#define I40E_X722_REG_INSET_L3_IP4_TTL 0x0010000000000000ULL
/* IPv4 Type of Service (TOS) */
#define I40E_REG_INSET_L3_IP4_TOS 0x0040000000000000ULL
/* IPv4 Protocol */
@@ -7568,25 +7576,23 @@ i40e_parse_input_set(uint64_t *inset,
* and vice versa
*/
static uint64_t
-i40e_translate_input_set_reg(uint64_t input)
+i40e_translate_input_set_reg(enum i40e_mac_type type, uint64_t input)
{
uint64_t val = 0;
uint16_t i;
- static const struct {
+ struct inset_map {
uint64_t inset;
uint64_t inset_reg;
- } inset_map[] = {
+ };
+
+ static const struct inset_map inset_map_common[] = {
{I40E_INSET_DMAC, I40E_REG_INSET_L2_DMAC},
{I40E_INSET_SMAC, I40E_REG_INSET_L2_SMAC},
{I40E_INSET_VLAN_OUTER, I40E_REG_INSET_L2_OUTER_VLAN},
{I40E_INSET_VLAN_INNER, I40E_REG_INSET_L2_INNER_VLAN},
{I40E_INSET_LAST_ETHER_TYPE, I40E_REG_INSET_LAST_ETHER_TYPE},
- {I40E_INSET_IPV4_SRC, I40E_REG_INSET_L3_SRC_IP4},
- {I40E_INSET_IPV4_DST, I40E_REG_INSET_L3_DST_IP4},
{I40E_INSET_IPV4_TOS, I40E_REG_INSET_L3_IP4_TOS},
- {I40E_INSET_IPV4_PROTO, I40E_REG_INSET_L3_IP4_PROTO},
- {I40E_INSET_IPV4_TTL, I40E_REG_INSET_L3_IP4_TTL},
{I40E_INSET_IPV6_SRC, I40E_REG_INSET_L3_SRC_IP6},
{I40E_INSET_IPV6_DST, I40E_REG_INSET_L3_DST_IP6},
{I40E_INSET_IPV6_TC, I40E_REG_INSET_L3_IP6_TC},
@@ -7615,13 +7621,40 @@ i40e_translate_input_set_reg(uint64_t input)
{I40E_INSET_FLEX_PAYLOAD_W8, I40E_REG_INSET_FLEX_PAYLOAD_WORD8},
};
+ /* some different registers map in x722*/
+ static const struct inset_map inset_map_diff_x722[] = {
+ {I40E_INSET_IPV4_SRC, I40E_X722_REG_INSET_L3_SRC_IP4},
+ {I40E_INSET_IPV4_DST, I40E_X722_REG_INSET_L3_DST_IP4},
+ {I40E_INSET_IPV4_PROTO, I40E_X722_REG_INSET_L3_IP4_PROTO},
+ {I40E_INSET_IPV4_TTL, I40E_X722_REG_INSET_L3_IP4_TTL},
+ };
+
+ static const struct inset_map inset_map_diff_not_x722[] = {
+ {I40E_INSET_IPV4_SRC, I40E_REG_INSET_L3_SRC_IP4},
+ {I40E_INSET_IPV4_DST, I40E_REG_INSET_L3_DST_IP4},
+ {I40E_INSET_IPV4_PROTO, I40E_REG_INSET_L3_IP4_PROTO},
+ {I40E_INSET_IPV4_TTL, I40E_REG_INSET_L3_IP4_TTL},
+ };
+
if (input == 0)
return val;
/* Translate input set to register aware inset */
- for (i = 0; i < RTE_DIM(inset_map); i++) {
- if (input & inset_map[i].inset)
- val |= inset_map[i].inset_reg;
+ if (type == I40E_MAC_X722) {
+ for (i = 0; i < RTE_DIM(inset_map_diff_x722); i++) {
+ if (input & inset_map_diff_x722[i].inset)
+ val |= inset_map_diff_x722[i].inset_reg;
+ }
+ } else {
+ for (i = 0; i < RTE_DIM(inset_map_diff_not_x722); i++) {
+ if (input & inset_map_diff_not_x722[i].inset)
+ val |= inset_map_diff_not_x722[i].inset_reg;
+ }
+ }
+
+ for (i = 0; i < RTE_DIM(inset_map_common); i++) {
+ if (input & inset_map_common[i].inset)
+ val |= inset_map_common[i].inset_reg;
}
return val;
@@ -7712,7 +7745,8 @@ i40e_filter_input_set_init(struct i40e_pf *pf)
I40E_INSET_MASK_NUM_REG);
if (num < 0)
return;
- inset_reg = i40e_translate_input_set_reg(input_set);
+ inset_reg = i40e_translate_input_set_reg(hw->mac.type,
+ input_set);
i40e_check_write_reg(hw, I40E_PRTQF_FD_INSET(pctype, 0),
(uint32_t)(inset_reg & UINT32_MAX));
@@ -7802,7 +7836,7 @@ i40e_hash_filter_inset_select(struct i40e_hw *hw,
if (num < 0)
return -EINVAL;
- inset_reg |= i40e_translate_input_set_reg(input_set);
+ inset_reg |= i40e_translate_input_set_reg(hw->mac.type, input_set);
i40e_check_write_reg(hw, I40E_GLQF_HASH_INSET(0, pctype),
(uint32_t)(inset_reg & UINT32_MAX));
@@ -7880,7 +7914,7 @@ i40e_fdir_filter_inset_select(struct i40e_pf *pf,
if (num < 0)
return -EINVAL;
- inset_reg |= i40e_translate_input_set_reg(input_set);
+ inset_reg |= i40e_translate_input_set_reg(hw->mac.type, input_set);
i40e_check_write_reg(hw, I40E_PRTQF_FD_INSET(pctype, 0),
(uint32_t)(inset_reg & UINT32_MAX));
--
1.9.3
^ permalink raw reply related
* [PATCH v4] net/i40e: fix hash filter invalid issue in X722
From: Jeff Guo @ 2016-10-25 2:42 UTC (permalink / raw)
To: helin.zhang, jingjing.wu; +Cc: dev, jia.guo
In-Reply-To: <1476931738-44140-1-git-send-email-jia.guo@intel.com>
When verifying the Hash filtering on X722, we found a problem that
the hash value in descriptor is incorrect. The root caused is X722
uses different way of hash key word selection comparing with X710/XL710.
This patch fixes it by setting X722 specific key selection.
Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4:
refine commit log
---
drivers/net/i40e/i40e_ethdev.c | 60 +++++++++++++++++++++++++++++++++---------
1 file changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index db5f808..ca515dd 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -211,6 +211,14 @@
#define I40E_REG_INSET_L3_SRC_IP4 0x0001800000000000ULL
/* Destination IPv4 address */
#define I40E_REG_INSET_L3_DST_IP4 0x0000001800000000ULL
+/* Source IPv4 address for X722 */
+#define I40E_X722_REG_INSET_L3_SRC_IP4 0x0006000000000000ULL
+/* Destination IPv4 address for X722 */
+#define I40E_X722_REG_INSET_L3_DST_IP4 0x0000060000000000ULL
+/* IPv4 Protocol for X722 */
+#define I40E_X722_REG_INSET_L3_IP4_PROTO 0x0010000000000000ULL
+/* IPv4 Time to Live for X722 */
+#define I40E_X722_REG_INSET_L3_IP4_TTL 0x0010000000000000ULL
/* IPv4 Type of Service (TOS) */
#define I40E_REG_INSET_L3_IP4_TOS 0x0040000000000000ULL
/* IPv4 Protocol */
@@ -7568,25 +7576,23 @@ i40e_parse_input_set(uint64_t *inset,
* and vice versa
*/
static uint64_t
-i40e_translate_input_set_reg(uint64_t input)
+i40e_translate_input_set_reg(enum i40e_mac_type type, uint64_t input)
{
uint64_t val = 0;
uint16_t i;
- static const struct {
+ struct inset_map {
uint64_t inset;
uint64_t inset_reg;
- } inset_map[] = {
+ };
+
+ static const struct inset_map inset_map_common[] = {
{I40E_INSET_DMAC, I40E_REG_INSET_L2_DMAC},
{I40E_INSET_SMAC, I40E_REG_INSET_L2_SMAC},
{I40E_INSET_VLAN_OUTER, I40E_REG_INSET_L2_OUTER_VLAN},
{I40E_INSET_VLAN_INNER, I40E_REG_INSET_L2_INNER_VLAN},
{I40E_INSET_LAST_ETHER_TYPE, I40E_REG_INSET_LAST_ETHER_TYPE},
- {I40E_INSET_IPV4_SRC, I40E_REG_INSET_L3_SRC_IP4},
- {I40E_INSET_IPV4_DST, I40E_REG_INSET_L3_DST_IP4},
{I40E_INSET_IPV4_TOS, I40E_REG_INSET_L3_IP4_TOS},
- {I40E_INSET_IPV4_PROTO, I40E_REG_INSET_L3_IP4_PROTO},
- {I40E_INSET_IPV4_TTL, I40E_REG_INSET_L3_IP4_TTL},
{I40E_INSET_IPV6_SRC, I40E_REG_INSET_L3_SRC_IP6},
{I40E_INSET_IPV6_DST, I40E_REG_INSET_L3_DST_IP6},
{I40E_INSET_IPV6_TC, I40E_REG_INSET_L3_IP6_TC},
@@ -7615,13 +7621,40 @@ i40e_translate_input_set_reg(uint64_t input)
{I40E_INSET_FLEX_PAYLOAD_W8, I40E_REG_INSET_FLEX_PAYLOAD_WORD8},
};
+ /* some different registers map in x722*/
+ static const struct inset_map inset_map_diff_x722[] = {
+ {I40E_INSET_IPV4_SRC, I40E_X722_REG_INSET_L3_SRC_IP4},
+ {I40E_INSET_IPV4_DST, I40E_X722_REG_INSET_L3_DST_IP4},
+ {I40E_INSET_IPV4_PROTO, I40E_X722_REG_INSET_L3_IP4_PROTO},
+ {I40E_INSET_IPV4_TTL, I40E_X722_REG_INSET_L3_IP4_TTL},
+ };
+
+ static const struct inset_map inset_map_diff_not_x722[] = {
+ {I40E_INSET_IPV4_SRC, I40E_REG_INSET_L3_SRC_IP4},
+ {I40E_INSET_IPV4_DST, I40E_REG_INSET_L3_DST_IP4},
+ {I40E_INSET_IPV4_PROTO, I40E_REG_INSET_L3_IP4_PROTO},
+ {I40E_INSET_IPV4_TTL, I40E_REG_INSET_L3_IP4_TTL},
+ };
+
if (input == 0)
return val;
/* Translate input set to register aware inset */
- for (i = 0; i < RTE_DIM(inset_map); i++) {
- if (input & inset_map[i].inset)
- val |= inset_map[i].inset_reg;
+ if (type == I40E_MAC_X722) {
+ for (i = 0; i < RTE_DIM(inset_map_diff_x722); i++) {
+ if (input & inset_map_diff_x722[i].inset)
+ val |= inset_map_diff_x722[i].inset_reg;
+ }
+ } else {
+ for (i = 0; i < RTE_DIM(inset_map_diff_not_x722); i++) {
+ if (input & inset_map_diff_not_x722[i].inset)
+ val |= inset_map_diff_not_x722[i].inset_reg;
+ }
+ }
+
+ for (i = 0; i < RTE_DIM(inset_map_common); i++) {
+ if (input & inset_map_common[i].inset)
+ val |= inset_map_common[i].inset_reg;
}
return val;
@@ -7712,7 +7745,8 @@ i40e_filter_input_set_init(struct i40e_pf *pf)
I40E_INSET_MASK_NUM_REG);
if (num < 0)
return;
- inset_reg = i40e_translate_input_set_reg(input_set);
+ inset_reg = i40e_translate_input_set_reg(hw->mac.type,
+ input_set);
i40e_check_write_reg(hw, I40E_PRTQF_FD_INSET(pctype, 0),
(uint32_t)(inset_reg & UINT32_MAX));
@@ -7802,7 +7836,7 @@ i40e_hash_filter_inset_select(struct i40e_hw *hw,
if (num < 0)
return -EINVAL;
- inset_reg |= i40e_translate_input_set_reg(input_set);
+ inset_reg |= i40e_translate_input_set_reg(hw->mac.type, input_set);
i40e_check_write_reg(hw, I40E_GLQF_HASH_INSET(0, pctype),
(uint32_t)(inset_reg & UINT32_MAX));
@@ -7880,7 +7914,7 @@ i40e_fdir_filter_inset_select(struct i40e_pf *pf,
if (num < 0)
return -EINVAL;
- inset_reg |= i40e_translate_input_set_reg(input_set);
+ inset_reg |= i40e_translate_input_set_reg(hw->mac.type, input_set);
i40e_check_write_reg(hw, I40E_PRTQF_FD_INSET(pctype, 0),
(uint32_t)(inset_reg & UINT32_MAX));
--
1.9.3
^ permalink raw reply related
* Re: [PATCH] doc: announce ABI change for ethtool app enhance
From: Xing, Beilei @ 2016-10-25 2:53 UTC (permalink / raw)
To: Yang, Qiming, dev@dpdk.org; +Cc: Yang, Qiming
In-Reply-To: <1475983002-72869-1-git-send-email-qiming.yang@intel.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiming Yang
> Sent: Sunday, October 9, 2016 11:17 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>
> Subject: [dpdk-dev] [PATCH] doc: announce ABI change for ethtool app
> enhance
>
> This patch adds a notice that the ABI change for ethtool app to get the NIC
> firmware version in the 17.02 release.
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> ---
> doc/guides/rel_notes/deprecation.rst | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 845d2aa..60bd7ed 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -62,3 +62,7 @@ Deprecation Notices
> * API will change for ``rte_port_source_params`` and
> ``rte_port_sink_params``
> structures. The member ``file_name`` data type will be changed from
> ``char *`` to ``const char *``. This change targets release 16.11.
> +
> +* In 17.02 ABI change is planned: the ``rte_eth_dev_info`` structure
> + will be extended with a new member ``fw_version`` in order to store
> + the NIC firmware version.
> --
> 2.7.4
Acked-by: Beilei Xing<beilei.xing@intel.com>
^ permalink raw reply
* Re: 16.07.1 stable patches review and test
From: Xu, Qian Q @ 2016-10-25 3:30 UTC (permalink / raw)
To: Yuanhan Liu, dpdk stable, Chen, WeichunX, Wei, FangfangX,
Pei, Yulong, Yao, Lei A
Cc: dev@dpdk.org
In-Reply-To: <20161014092646.GD16751@yliu-dev.sh.intel.com>
Yuanhan
So we will get the package for testing on Oct.26th, right? Thx.
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yuanhan Liu
Sent: Friday, October 14, 2016 5:27 PM
To: dpdk stable <stable@dpdk.org>
Cc: dev@dpdk.org
Subject: [dpdk-dev] 16.07.1 stable patches review and test
Hi,
I have applied most of bug fixing patches (listed below) to the 16.07 stable branch at
http://dpdk.org/browse/dpdk-stable/
Please help reviewing and testing. The planned date for the final release is 26th, Oct. Before that, please shout if anyone has objections with these patches being applied.
Thanks.
--yliu
---
Alejandro Lucero (1):
net/nfp: fix copying MAC address
Aleksey Katargin (1):
table: fix symbol exports
Alex Zelezniak (1):
net/ixgbe: fix VF reset to apply to correct VF
Ali Volkan Atli (1):
net/e1000: fix returned number of available Rx descriptors
Arek Kusztal (1):
app/test: fix verification of digest for GCM
Beilei Xing (2):
net/i40e: fix dropping packets with ethertype 0x88A8
net/i40e: fix parsing QinQ packets type
Bruce Richardson (1):
net/mlx: fix debug build with gcc 6.1
Christian Ehrhardt (1):
examples/ip_pipeline: fix Python interpreter
Deepak Kumar Jain (2):
crypto/null: fix key size increment value
crypto/qat: fix FreeBSD build
Dror Birkman (1):
net/pcap: fix memory leak in jumbo frames
Ferruh Yigit (2):
app/testpmd: fix help of MTU set commmand
pmdinfogen: fix clang build
Gary Mussar (1):
tools: fix virtio interface name when binding
Gowrishankar Muthukrishnan (1):
examples/ip_pipeline: fix lcore mapping for ppc64
Hiroyuki Mikita (1):
sched: fix releasing enqueued packets
James Poole (1):
app/testpmd: fix timeout in Rx queue flushing
Jianfeng Tan (3):
net/virtio_user: fix first queue pair without multiqueue
net/virtio_user: fix wrong sequence of messages
net/virtio_user: fix error management during init
Jim Harris (1):
contigmem: zero all pages during mmap
John Daley (1):
net/enic: fix bad L4 checksum flag on ICMP packets
Karmarkar Suyash (1):
timer: fix lag delay
Maciej Czekaj (1):
mem: fix crash on hugepage mapping error
Nelson Escobar (1):
net/enic: fix freeing memory for descriptor ring
Olivier Matz (4):
app/testpmd: fix crash when mempool allocation fails
tools: fix json output of pmdinfo
mbuf: fix error handling on pool creation
mem: fix build with -O1
Pablo de Lara (3):
hash: fix ring size
hash: fix false zero signature key hit lookup
crypto: fix build with icc
Qi Zhang (1):
net/i40e/base: fix UDP packet header
Rich Lane (1):
net/i40e: fix null pointer dereferences when using VMDq+RSS
Weiliang Luo (1):
mempool: fix corruption due to invalid handler
Xiao Wang (5):
net/fm10k: fix MAC address removal from switch
net/ixgbe/base: fix pointer check
net/ixgbe/base: fix check for NACK
net/ixgbe/base: fix possible corruption of shadow RAM
net/ixgbe/base: fix skipping PHY config
Yangchao Zhou (1):
pci: fix memory leak when detaching device
Yury Kylulin (2):
net/ixgbe: fix mbuf leak during Rx queue release
net/i40e: fix mbuf leak during Rx queue release
Zhiyong Yang (1):
net/virtio: fix xstats name
^ permalink raw reply
* [PATCH v2 1/2] net/i40e: fix link status change interrupt
From: Qiming Yang @ 2016-10-25 4:19 UTC (permalink / raw)
To: dev; +Cc: jingjing.wu, helin.zhang, bruce.richardson, Qiming Yang
Previously, link status interrupt in i40e is achieved by checking
LINK_STAT_CHANGE_MASK in PFINT_ICR0 register which is provided only
for diagnostic use. Instead, drivers need to get the link status
change notification by using LSE (Link Status Event).
This patch enables LSE and calls LSC callback when the event is
received. This patch also removes the processing on
LINK_STAT_CHANGE_MASK.
Fixes: 4861cde46116 ("i40e: new poll mode driver")
Signed-off-by: Qiming Yang <qiming.yang@intel.com>
---
drivers/net/i40e/i40e_ethdev.c | 97 +++++++++---------------------------------
1 file changed, 19 insertions(+), 78 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0aeb70..9c1f542 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -108,7 +108,6 @@
I40E_PFINT_ICR0_ENA_GRST_MASK | \
I40E_PFINT_ICR0_ENA_PCI_EXCEPTION_MASK | \
I40E_PFINT_ICR0_ENA_STORM_DETECT_MASK | \
- I40E_PFINT_ICR0_ENA_LINK_STAT_CHANGE_MASK | \
I40E_PFINT_ICR0_ENA_HMC_ERR_MASK | \
I40E_PFINT_ICR0_ENA_PE_CRITERR_MASK | \
I40E_PFINT_ICR0_ENA_VFLR_MASK | \
@@ -1768,6 +1767,16 @@ i40e_dev_start(struct rte_eth_dev *dev)
if (dev->data->dev_conf.intr_conf.lsc != 0)
PMD_INIT_LOG(INFO, "lsc won't enable because of"
" no intr multiplex\n");
+ } else if (dev->data->dev_conf.intr_conf.lsc != 0) {
+ ret = i40e_aq_set_phy_int_mask(hw,
+ ~(I40E_AQ_EVENT_LINK_UPDOWN |
+ I40E_AQ_EVENT_MODULE_QUAL_FAIL |
+ I40E_AQ_EVENT_MEDIA_NA), NULL);
+ if (ret != I40E_SUCCESS)
+ PMD_DRV_LOG(WARNING, "Fail to set phy mask");
+
+ /* Call get_link_info aq commond to enable LSE */
+ i40e_dev_link_update(dev, 0);
}
/* enable uio intr after callback register */
@@ -1984,6 +1993,7 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
struct rte_eth_link link, old;
int status;
unsigned rep_cnt = MAX_REPEAT_TIME;
+ bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
memset(&link, 0, sizeof(link));
memset(&old, 0, sizeof(old));
@@ -1992,7 +2002,8 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
do {
/* Get link status information from hardware */
- status = i40e_aq_get_link_info(hw, false, &link_status, NULL);
+ status = i40e_aq_get_link_info(hw, enable_lse,
+ &link_status, NULL);
if (status != I40E_SUCCESS) {
link.link_speed = ETH_SPEED_NUM_100M;
link.link_duplex = ETH_LINK_FULL_DUPLEX;
@@ -5442,6 +5453,12 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
info.msg_buf,
info.msg_len);
break;
+ case i40e_aqc_opc_get_link_status:
+ ret = i40e_dev_link_update(dev, 0);
+ if (!ret)
+ _rte_eth_dev_callback_process(dev,
+ RTE_ETH_EVENT_INTR_LSC);
+ break;
default:
PMD_DRV_LOG(ERR, "Request %u is not supported yet",
opcode);
@@ -5451,57 +5468,6 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
rte_free(info.msg_buf);
}
-/*
- * Interrupt handler is registered as the alarm callback for handling LSC
- * interrupt in a definite of time, in order to wait the NIC into a stable
- * state. Currently it waits 1 sec in i40e for the link up interrupt, and
- * no need for link down interrupt.
- */
-static void
-i40e_dev_interrupt_delayed_handler(void *param)
-{
- struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
- struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
- uint32_t icr0;
-
- /* read interrupt causes again */
- icr0 = I40E_READ_REG(hw, I40E_PFINT_ICR0);
-
-#ifdef RTE_LIBRTE_I40E_DEBUG_DRIVER
- if (icr0 & I40E_PFINT_ICR0_ECC_ERR_MASK)
- PMD_DRV_LOG(ERR, "ICR0: unrecoverable ECC error\n");
- if (icr0 & I40E_PFINT_ICR0_MAL_DETECT_MASK)
- PMD_DRV_LOG(ERR, "ICR0: malicious programming detected\n");
- if (icr0 & I40E_PFINT_ICR0_GRST_MASK)
- PMD_DRV_LOG(INFO, "ICR0: global reset requested\n");
- if (icr0 & I40E_PFINT_ICR0_PCI_EXCEPTION_MASK)
- PMD_DRV_LOG(INFO, "ICR0: PCI exception\n activated\n");
- if (icr0 & I40E_PFINT_ICR0_STORM_DETECT_MASK)
- PMD_DRV_LOG(INFO, "ICR0: a change in the storm control "
- "state\n");
- if (icr0 & I40E_PFINT_ICR0_HMC_ERR_MASK)
- PMD_DRV_LOG(ERR, "ICR0: HMC error\n");
- if (icr0 & I40E_PFINT_ICR0_PE_CRITERR_MASK)
- PMD_DRV_LOG(ERR, "ICR0: protocol engine critical error\n");
-#endif /* RTE_LIBRTE_I40E_DEBUG_DRIVER */
-
- if (icr0 & I40E_PFINT_ICR0_VFLR_MASK) {
- PMD_DRV_LOG(INFO, "INT:VF reset detected\n");
- i40e_dev_handle_vfr_event(dev);
- }
- if (icr0 & I40E_PFINT_ICR0_ADMINQ_MASK) {
- PMD_DRV_LOG(INFO, "INT:ADMINQ event\n");
- i40e_dev_handle_aq_msg(dev);
- }
-
- /* handle the link up interrupt in an alarm callback */
- i40e_dev_link_update(dev, 0);
- _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC);
-
- i40e_pf_enable_irq0(hw);
- rte_intr_enable(&(dev->pci_dev->intr_handle));
-}
-
/**
* Interrupt handler triggered by NIC for handling
* specific interrupt.
@@ -5558,31 +5524,6 @@ i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
PMD_DRV_LOG(INFO, "ICR0: adminq event");
i40e_dev_handle_aq_msg(dev);
}
-
- /* Link Status Change interrupt */
- if (icr0 & I40E_PFINT_ICR0_LINK_STAT_CHANGE_MASK) {
-#define I40E_US_PER_SECOND 1000000
- struct rte_eth_link link;
-
- PMD_DRV_LOG(INFO, "ICR0: link status changed\n");
- memset(&link, 0, sizeof(link));
- rte_i40e_dev_atomic_read_link_status(dev, &link);
- i40e_dev_link_update(dev, 0);
-
- /*
- * For link up interrupt, it needs to wait 1 second to let the
- * hardware be a stable state. Otherwise several consecutive
- * interrupts can be observed.
- * For link down interrupt, no need to wait.
- */
- if (!link.link_status && rte_eal_alarm_set(I40E_US_PER_SECOND,
- i40e_dev_interrupt_delayed_handler, (void *)dev) >= 0)
- return;
- else
- _rte_eth_dev_callback_process(dev,
- RTE_ETH_EVENT_INTR_LSC);
- }
-
done:
/* Enable interrupt */
i40e_pf_enable_irq0(hw);
--
2.5.5
^ permalink raw reply related
* [PATCH v2 2/2] net/i40e: fix VF bonded device link down
From: Qiming Yang @ 2016-10-25 4:19 UTC (permalink / raw)
To: dev; +Cc: jingjing.wu, helin.zhang, bruce.richardson, Qiming Yang
In-Reply-To: <1477369146-63317-1-git-send-email-qiming.yang@intel.com>
If VF device is used as slave of a bond device, it will be polled
periodically through alarm. Interrupt is involved here. And then
VF will send I40E_VIRTCHNL_OP_GET_LINK_STAT message to
PF to query the status. The response is handled by interrupt
callback. Interrupt is involved here again. That's why bond
device cannot bring up.
This patch removes I40E_VIRTCHNL_OP_GET_LINK_STAT
message. Link status in VF driver will be updated when PF driver
notify it, and VF stores this link status locally. VF driver just
returns the local status when being required.
Fixes: 4861cde46116 ("i40e: new poll mode driver")
Signed-off-by: Qiming Yang <qiming.yang@intel.com>
---
drivers/net/i40e/i40e_ethdev.c | 22 ++++++++++-
drivers/net/i40e/i40e_ethdev.h | 4 +-
drivers/net/i40e/i40e_ethdev_vf.c | 81 +++++++++++++--------------------------
drivers/net/i40e/i40e_pf.c | 33 ++++++++--------
drivers/net/i40e/i40e_pf.h | 3 +-
5 files changed, 67 insertions(+), 76 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 9c1f542..13060db 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -5418,6 +5418,24 @@ i40e_dev_handle_vfr_event(struct rte_eth_dev *dev)
}
static void
+i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev)
+{
+ struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+ struct i40e_virtchnl_pf_event event;
+ int i;
+
+ event.event = I40E_VIRTCHNL_EVENT_LINK_CHANGE;
+ event.event_data.link_event.link_status =
+ dev->data->dev_link.link_status;
+ event.event_data.link_event.link_speed =
+ dev->data->dev_link.link_speed;
+
+ for (i = 0; i < pf->vf_num; i++)
+ i40e_pf_host_send_msg_to_vf(&pf->vfs[i], I40E_VIRTCHNL_OP_EVENT,
+ I40E_SUCCESS, (uint8_t *)&event, sizeof(event));
+}
+
+static void
i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
{
struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -5455,9 +5473,11 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
break;
case i40e_aqc_opc_get_link_status:
ret = i40e_dev_link_update(dev, 0);
- if (!ret)
+ if (!ret) {
+ i40e_notify_all_vfs_link_status(dev);
_rte_eth_dev_callback_process(dev,
RTE_ETH_EVENT_INTR_LSC);
+ }
break;
default:
PMD_DRV_LOG(ERR, "Request %u is not supported yet",
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 92c8fad..61dfa93 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -599,7 +599,9 @@ int i40e_hash_filter_inset_select(struct i40e_hw *hw,
struct rte_eth_input_set_conf *conf);
int i40e_fdir_filter_inset_select(struct i40e_pf *pf,
struct rte_eth_input_set_conf *conf);
-
+int i40e_pf_host_send_msg_to_vf(struct i40e_pf_vf *vf, uint32_t opcode,
+ uint32_t retval, uint8_t *msg,
+ uint16_t msglen);
void i40e_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
struct rte_eth_rxq_info *qinfo);
void i40e_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index a616ae0..ba63a7f 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -126,8 +126,6 @@ static void i40evf_dev_promiscuous_enable(struct rte_eth_dev *dev);
static void i40evf_dev_promiscuous_disable(struct rte_eth_dev *dev);
static void i40evf_dev_allmulticast_enable(struct rte_eth_dev *dev);
static void i40evf_dev_allmulticast_disable(struct rte_eth_dev *dev);
-static int i40evf_get_link_status(struct rte_eth_dev *dev,
- struct rte_eth_link *link);
static int i40evf_init_vlan(struct rte_eth_dev *dev);
static int i40evf_dev_rx_queue_start(struct rte_eth_dev *dev,
uint16_t rx_queue_id);
@@ -1084,31 +1082,6 @@ i40evf_del_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
return err;
}
-static int
-i40evf_get_link_status(struct rte_eth_dev *dev, struct rte_eth_link *link)
-{
- struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
- int err;
- struct vf_cmd_info args;
- struct rte_eth_link *new_link;
-
- args.ops = (enum i40e_virtchnl_ops)I40E_VIRTCHNL_OP_GET_LINK_STAT;
- args.in_args = NULL;
- args.in_args_size = 0;
- args.out_buffer = vf->aq_resp;
- args.out_size = I40E_AQ_BUF_SZ;
- err = i40evf_execute_vf_cmd(dev, &args);
- if (err) {
- PMD_DRV_LOG(ERR, "fail to execute command OP_GET_LINK_STAT");
- return err;
- }
-
- new_link = (struct rte_eth_link *)args.out_buffer;
- (void)rte_memcpy(link, new_link, sizeof(*link));
-
- return 0;
-}
-
static const struct rte_pci_id pci_id_i40evf_map[] = {
{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_VF) },
{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_VF_HV) },
@@ -2166,35 +2139,33 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
* DPDK pf host provide interfacet to acquire link status
* while Linux driver does not
*/
- if (vf->version_major == I40E_DPDK_VERSION_MAJOR)
- i40evf_get_link_status(dev, &new_link);
- else {
- /* Linux driver PF host */
- switch (vf->link_speed) {
- case I40E_LINK_SPEED_100MB:
- new_link.link_speed = ETH_SPEED_NUM_100M;
- break;
- case I40E_LINK_SPEED_1GB:
- new_link.link_speed = ETH_SPEED_NUM_1G;
- break;
- case I40E_LINK_SPEED_10GB:
- new_link.link_speed = ETH_SPEED_NUM_10G;
- break;
- case I40E_LINK_SPEED_20GB:
- new_link.link_speed = ETH_SPEED_NUM_20G;
- break;
- case I40E_LINK_SPEED_40GB:
- new_link.link_speed = ETH_SPEED_NUM_40G;
- break;
- default:
- new_link.link_speed = ETH_SPEED_NUM_100M;
- break;
- }
- /* full duplex only */
- new_link.link_duplex = ETH_LINK_FULL_DUPLEX;
- new_link.link_status = vf->link_up ? ETH_LINK_UP :
- ETH_LINK_DOWN;
+
+ /* Linux driver PF host */
+ switch (vf->link_speed) {
+ case I40E_LINK_SPEED_100MB:
+ new_link.link_speed = ETH_SPEED_NUM_100M;
+ break;
+ case I40E_LINK_SPEED_1GB:
+ new_link.link_speed = ETH_SPEED_NUM_1G;
+ break;
+ case I40E_LINK_SPEED_10GB:
+ new_link.link_speed = ETH_SPEED_NUM_10G;
+ break;
+ case I40E_LINK_SPEED_20GB:
+ new_link.link_speed = ETH_SPEED_NUM_20G;
+ break;
+ case I40E_LINK_SPEED_40GB:
+ new_link.link_speed = ETH_SPEED_NUM_40G;
+ break;
+ default:
+ new_link.link_speed = ETH_SPEED_NUM_100M;
+ break;
}
+ /* full duplex only */
+ new_link.link_duplex = ETH_LINK_FULL_DUPLEX;
+ new_link.link_status = vf->link_up ? ETH_LINK_UP :
+ ETH_LINK_DOWN;
+
i40evf_dev_atomic_write_link_status(dev, &new_link);
return 0;
diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index d5b2d45..350f6a0 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -250,7 +250,7 @@ i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
return ret;
}
-static int
+int
i40e_pf_host_send_msg_to_vf(struct i40e_pf_vf *vf,
uint32_t opcode,
uint32_t retval,
@@ -847,18 +847,6 @@ i40e_pf_host_process_cmd_get_stats(struct i40e_pf_vf *vf)
return I40E_SUCCESS;
}
-static void
-i40e_pf_host_process_cmd_get_link_status(struct i40e_pf_vf *vf)
-{
- struct rte_eth_dev *dev = I40E_VSI_TO_ETH_DEV(vf->pf->main_vsi);
-
- /* Update link status first to acquire latest link change */
- i40e_dev_link_update(dev, 1);
- i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_GET_LINK_STAT,
- I40E_SUCCESS, (uint8_t *)&dev->data->dev_link,
- sizeof(struct rte_eth_link));
-}
-
static int
i40e_pf_host_process_cmd_cfg_vlan_offload(
struct i40e_pf_vf *vf,
@@ -909,6 +897,20 @@ send_msg:
return ret;
}
+static void
+i40e_notify_vf_link_status(struct rte_eth_dev *dev, struct i40e_pf_vf *vf)
+{
+ struct i40e_virtchnl_pf_event event;
+
+ event.event = I40E_VIRTCHNL_EVENT_LINK_CHANGE;
+ event.event_data.link_event.link_status =
+ dev->data->dev_link.link_status;
+ event.event_data.link_event.link_speed =
+ dev->data->dev_link.link_speed;
+ i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_EVENT,
+ I40E_SUCCESS, (uint8_t *)&event, sizeof(event));
+}
+
void
i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
uint16_t abs_vf_id, uint32_t opcode,
@@ -964,6 +966,7 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
case I40E_VIRTCHNL_OP_ENABLE_QUEUES:
PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received");
i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen);
+ i40e_notify_vf_link_status(dev, vf);
break;
case I40E_VIRTCHNL_OP_DISABLE_QUEUES:
PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received");
@@ -993,10 +996,6 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
PMD_DRV_LOG(INFO, "OP_GET_STATS received");
i40e_pf_host_process_cmd_get_stats(vf);
break;
- case I40E_VIRTCHNL_OP_GET_LINK_STAT:
- PMD_DRV_LOG(INFO, "OP_GET_LINK_STAT received");
- i40e_pf_host_process_cmd_get_link_status(vf);
- break;
case I40E_VIRTCHNL_OP_CFG_VLAN_OFFLOAD:
PMD_DRV_LOG(INFO, "OP_CFG_VLAN_OFFLOAD received");
i40e_pf_host_process_cmd_cfg_vlan_offload(vf, msg, msglen);
diff --git a/drivers/net/i40e/i40e_pf.h b/drivers/net/i40e/i40e_pf.h
index 9c01829..cddc45c 100644
--- a/drivers/net/i40e/i40e_pf.h
+++ b/drivers/net/i40e/i40e_pf.h
@@ -59,9 +59,8 @@ enum i40e_virtchnl_ops_dpdk {
* Keep some gap between Linux PF commands and
* DPDK PF extended commands.
*/
- I40E_VIRTCHNL_OP_GET_LINK_STAT = I40E_VIRTCHNL_OP_VERSION +
+ I40E_VIRTCHNL_OP_CFG_VLAN_OFFLOAD = I40E_VIRTCHNL_OP_VERSION +
I40E_DPDK_OFFSET,
- I40E_VIRTCHNL_OP_CFG_VLAN_OFFLOAD,
I40E_VIRTCHNL_OP_CFG_VLAN_PVID,
I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EXT,
};
--
2.5.5
^ permalink raw reply related
* Re: [PATCH v5 06/21] eal/soc: introduce very essential SoC infra definitions
From: Shreyansh Jain @ 2016-10-25 5:36 UTC (permalink / raw)
To: Jan Viktorin; +Cc: david.marchand, dev, thomas.monjalon, Hemant Agrawal
In-Reply-To: <20161024182105.002bee08@pcviktorin.fit.vutbr.cz>
Hello Jan,
On Monday 24 October 2016 09:51 PM, Jan Viktorin wrote:
> On Mon, 24 Oct 2016 17:29:25 +0530
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>
>> From: Jan Viktorin <viktorin@rehivetech.com>
>>
>> Define initial structures and functions for the SoC infrastructure.
>> This patch supports only a very minimal functions for now.
>> More features will be added in the following commits.
>>
>> Includes rte_device/rte_driver inheritance of
>> rte_soc_device/rte_soc_driver.
>>
>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>> app/test/Makefile | 1 +
>> app/test/test_soc.c | 90 +++++++++++++++++++++
>> lib/librte_eal/common/Makefile | 2 +-
>> lib/librte_eal/common/eal_private.h | 4 +
>> lib/librte_eal/common/include/rte_soc.h | 138 ++++++++++++++++++++++++++++++++
>> 5 files changed, 234 insertions(+), 1 deletion(-)
>> create mode 100644 app/test/test_soc.c
>> create mode 100644 lib/librte_eal/common/include/rte_soc.h
>>
>> diff --git a/app/test/Makefile b/app/test/Makefile
>
> [...]
>
>> +++ b/lib/librte_eal/common/include/rte_soc.h
>> @@ -0,0 +1,138 @@
>
> [...]
>
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +#include <inttypes.h>
>> +#include <string.h>
>> +
>> +#include <rte_dev.h>
>> +#include <rte_debug.h>
>> +
>> +struct rte_soc_id {
>> + const char *compatible; /**< OF compatible specification */
>> + uint64_t priv_data; /**< SoC Driver specific data */
>
> Do you expect this to be a pointer?
A 64 bit entry, which can be typecasted to pointer by implementations,
if required. Or, it might as well remain as a 64bit entry as ID.
>
>> +};
>> +
>
> [...]
>
>> +
>> +/**
>> + * Initialization function for the driver called during SoC probing.
>> + */
>> +typedef int (soc_devinit_t)(struct rte_soc_driver *, struct rte_soc_device *);
>> +
>> +/**
>> + * Uninitialization function for the driver called during hotplugging.
>> + */
>> +typedef int (soc_devuninit_t)(struct rte_soc_device *);
>> +
>> +/**
>> + * A structure describing a SoC driver.
>> + */
>> +struct rte_soc_driver {
>> + TAILQ_ENTRY(rte_soc_driver) next; /**< Next in list */
>> + struct rte_driver driver; /**< Inherit core driver. */
>> + soc_devinit_t *devinit; /**< Device initialization */
>> + soc_devuninit_t *devuninit; /**< Device uninitialization */
>
> Shouldn't those functions be named probe/remove?
Indeed. I think there was a comment on v4 as well - I thought I had
fixed it but it seems I have mixed up my patches. I will send v6
immediately with this fixed. Thanks for pointing out.
>
>> + const struct rte_soc_id *id_table; /**< ID table, NULL terminated */
>> +};
>> +
>
> [...]
>
>> +#endif
>
>
>
-
Shreyansh
^ permalink raw reply
* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Ilya Maximets @ 2016-10-25 6:26 UTC (permalink / raw)
To: Jan Blunck
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable, Dyasly Sergey, bruce.richardson
In-Reply-To: <CALe+Z03uH+0WLxJAKyL5q2g+P1VJ2HhH0D1B7WKOhKA9_tK+fQ@mail.gmail.com>
On 24.10.2016 17:54, Jan Blunck wrote:
> On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
>> On 18.10.2016 18:19, Jan Blunck wrote:
>>> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>> On 18.10.2016 15:28, Jan Blunck wrote:
>>>>> If the application already configured queues the PMD should not
>>>>> silently claim ownership and reset them.
>>>>>
>>>>> What exactly is the problem when changing MTU? This works fine from
>>>>> what I can tell.
>>>>
>>>> Following scenario leads to APP PANIC:
>>>>
>>>> 1. mempool_1 = rte_mempool_create()
>>>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>>>> 3. rte_eth_dev_start(bond0);
>>>> 4. mempool_2 = rte_mempool_create();
>>>> 5. rte_eth_dev_stop(bond0);
>>>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>>>> 7. rte_eth_dev_start(bond0);
>>>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>>>> 8. rte_mempool_free(mempool_1);
>>>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>>>> PANIC in rte_mempool_get_ops():
>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>
>>>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>>>> Bug is easily reproducible.
>>>>
>>>
>>> I see. I'm not 100% that this is expected to work without leaking the
>>> driver's queues though. The driver is allowed to do allocations in
>>> its rx_queue_setup() function that are being freed via
>>> rx_queue_release() later. But rx_queue_release() is only called if you
>>> reconfigure the
>>> device with 0 queues.
It's not true. Drivers usually calls 'rx_queue_release()' inside
'rx_queue_setup()' function while reallocating the already allocated
queue. (See ixgbe driver for example). Also all HW queues are
usually destroyed inside 'eth_dev_stop()' and reallocated in
'eth_dev_start()' or '{rx,tx}_queue_setup()'.
So, there is no leaks at all.
>>> From what I understand there is no other way to
>>> reconfigure a device to use another mempool.
>>>
>>> But ... even that wouldn't work with the bonding driver right now: the
>>> bonding master only configures the slaves during startup. I can put
>>> that on my todo list.
No, bonding master reconfigures new slaves in 'rte_eth_bond_slave_add()'
if needed.
>>> Coming back to your original problem: changing the MTU for the bond
>>> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
>>> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
>>> jumbo_frame / enable_scatter accordingly). This does work without a
>>> call to rte_eth_rx_queue_setup().
>>
>> Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
>> reconfiguration will require to have mempools with huge mbufs (9KB)
>> for all ports from the start. This is unacceptable because leads to
>> significant performance regressions because of fast cache exhausting.
>> Also this will require big work to rewrite OVS reconfiguration code
>> this way.
>> Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
>> also can't be changed in runtime.
>>
>>
>> I'm not fully understand what is the use case for this 'reusing' code.
>> Could you, please, describe situation where this behaviour is necessary?
>
> The device that is added to the bond was used before and therefore
> already has allocated queues. Therefore we reuse the existing queues
> of the devices instead of borrowing the queues of the bond device. If
> the slave is removed from the bond again there is no need to allocate
> the queues again.
>
> Hope that clarifies the usecase
So, As I see, this is just an optimization that leads to differently
configured queues of same device and possible application crash if the
old configuration doesn't valid any more.
With revert applied in your usecase while adding the device to bond
it's queues will be automatically reconfigured according to configuration
of the bond device. If the slave is removed from the bond all its'
queues will remain as configured by bond. You can reconfigure them if
needed. I guess, that in your case configuration of slave devices,
actually, matches configuration of bond device. In that case slave
will remain in the same state after removing from bond as it was
before adding.
Best regards, Ilya Maximets.
>>>>>
>>>>> On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> 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
^ permalink raw reply
* Re: rte_eth_tx_burst() hangs because of nofreedescriptors
From: yingzhi @ 2016-10-25 7:10 UTC (permalink / raw)
To: Zhang, Helin; +Cc: dev@dpdk.org
Hi Helin,
I'm doing comparison tests today, (with fragmented and small traffic).
On the other hand, I checked some example code from DPDK source:
load_balancer:
app_lcore_io_tx:
n_pkts = rte_eth_tx_burst(
port,
0,
lp->tx.mbuf_out[port].array,
(uint16_t) n_mbufs);
if (unlikely(n_pkts < n_mbufs)) {
uint32_t k;
for (k = n_pkts; k < n_mbufs; k ++) {
struct rte_mbuf *pkt_to_free = lp->tx.mbuf_out[port].array[k];
rte_pktmbuf_free(pkt_to_free);
}
}
And fragmentation example
http://dpdk.org/doc/api-2.2/ip_fragmentation_2main_8c-example.html
ret = rte_eth_tx_burst(port, queueid, m_table, n);
if (unlikely(ret < n)) {
do {
rte_pktmbuf_free(m_table[ret]);
} while (++ret < n);
}
They works basically the same as in my app, is there any other way to free segmented packets?
The question is:
if pkt_mbuf next pointer is not none, rte_pktmbuf_free() will free all the packet of that linklist, but due to packet re-order, I guess those packets may not sequentially stores in the mbuf array? So how can we correctly free those fragmented packets if rte_eth_tx_burst() didn't sent out them so we need to manually free?
Thanks
------------------ Original ------------------
From: "Zhang, Helin";<helin.zhang@intel.com>;
Date: Tue, Oct 25, 2016 09:05 AM
To: "yingzhi"<kaitoy@qq.com>;
Cc: "dev@dpdk.org"<dev@dpdk.org>;
Subject: RE: RE: [dpdk-dev] rte_kni_tx_burst() hangs because of nofreedescriptors
From: yingzhi [mailto:kaitoy@qq.com]
Sent: Monday, October 24, 2016 6:39 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: RE: [dpdk-dev] rte_kni_tx_burst() hangs because of no freedescriptors
Hi Helin,
Thanks for your response, to answer your questions:
1. we send only one packet each time calling rte_kni_tx_burst(), which means the last argument is 1.
2. it returns 0 because the free mbuf function inside tx_burst will not free any mbuf:
if (txq->nb_tx_free < txq->tx_free_thresh)
ixgbe_tx_free_bufs(txq);
after this operation, the txq->nb_tx_free is not increased and keeps to be "0" eventually.
I did some tests today, I commented out this section of ixgbe_rxtx_vec_common.h -> ixgbe_tx_free_bufs
status = txq->tx_ring[txq->tx_next_dd].wb.status;
if (!(status & IXGBE_ADVTXD_STAT_DD))
return 0;
After ignoring DD bit check, our app runs for about 6 hours without issue. I suspect there is something wrong in my program set the DD bit somewhere. One of the possible cause currently I suspect is as far as I know, rte_pktmbuf_free(mbuf.array[k]) will free the mbuf of the packet and any fragmented packets following by it. But in our application such as below code snippet:
auto nb_tx = rte_eth_tx_burst(port, queue, mbuf.array, (uint16_t) nb_rx);
if (unlikely(nb_tx < nb_rx)) {
for (unsigned k = nb_tx; k < nb_rx; k++) {
rte_pktmbuf_free(mbuf.array[k]);
}
}
[Zhang, Helin] it seems above code piece has memory leak, if the buffer is chained. After all memory leaked, then the issue comes. Please try to check if this is the root cause!
In this case if there are fragmented packets and failed transmission, may cause a mbuf be freed multiple times.
Above is just my suspect, need to do some tests later today or tomorrow.
Thanks
------------------ Original ------------------
From: "Zhang, Helin";<helin.zhang@intel.com>;
Date: Mon, Oct 24, 2016 11:33 AM
To: "yingzhi"<kaitoy@qq.com>;
Cc: "dev@dpdk.org"<dev@dpdk.org>;
Subject: RE: [dpdk-dev] rte_kni_tx_burst() hangs because of no freedescriptors
Hi Yingzhi
Thank you for the reporting! The description is not so clear at least for me.
Please help to narrown down the issue by youself.
How many packets would it have for calling TX function?
Why it would return 0 after calling TX function? No memory? Or return from else? Have you found anything?
Regards,
Helin
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of yingzhi
> Sent: Sunday, October 23, 2016 9:30 PM
> To: users; dev@dpdk.org
> Subject: [dpdk-dev] rte_kni_tx_burst() hangs because of no free descriptors
>
> -
> Hi Experts,
>
> Background:
>
> We are using DPDK to develop a LoadBalancer following below logic: When
> a new packet is received:
> 1. if the dst_addr is management IP, forward to KNI. 2. if the dst_addr is in
> VIP list, select backend and forward(modify dst mac address). 3. otherwise
> drop the packet.
>
> At this stage, we use one single thread for KNI forwarding and another for
> VIP forwarding(forward to eth).
>
> DPDK version: 16.07
> NIC: 82599ES 10-Gigabit SFI/SFP+ Network Connection
> Linux: 14.04.1-Ubuntu x64
>
> Promblem description:
>
> The program runs correctly for sometime(around 2 hours for 400Mb traffic).
> But it it will hang. When problem happens, rte_eth_tx_burst() will not able to
> send out any packets(always returns 0). We tracked into that function and
> noticed it is actually calling ixgbe driver's ixgbe_xmit_pkts_vec() function in
> our environment, because we use default tx queue configuration, after
> printing some info, we found if the free function works fine:
> tx_rs_thresh: 32, tx_free_thresh: 32, nb_tx_free: 31
>
> it will trigger free and make 32 more free descriptors:
> tx_rs_thresh: 32, tx_free_thresh: 32, nb_tx_free: 62
>
> but when something going wrong, it will no longer free anything:
> tx_rs_thresh: 32, tx_free_thresh: 32, nb_tx_free: 0 tx_rs_thresh: 32,
> tx_free_thresh: 32, nb_tx_free: 0
>
> It may related with the DD flag of the descriptor but we are not quite sure.
>
> Our program logic:
>
> create two mbuf pools on socket 0, one for rx_queue and one for kni. (all
> lcore threads runs on socket0)
>
> init kni interface with rte_kni_alloc()
>
>
> init one NIC interface with
> rte_eth_dev_configure(); rte_eth_rx_queue_setup();
> rte_eth_tx_queue_setup(); rte_eth_dev_start();
>
>
>
> in the eth main loop: (code is simplified)
> while(1) { n = rte_eth_rx_burst(packets); for (i = 0; i < n; ++i) { if
> (SEND_TO_KNI) { m = rte_kni_tx_burst(packets[i]); if (m != 1))
> { rte_pktmbuf_free(packets[i]); } } if (SEND_TO_ETH)
> { // after modify the packet m = rte_eth_tx_burst(packets[i]);
> if (m != 1)) { rte_pktmbuf_free(packets[i]); } } //
> otherwise drop the packet rte_pktmbuf_free(packets[i]); } }
>
>
> Please advise if I'm using DPDK in a wrong way. Sorry if I missed something
> basic, I'm new to DPDK.
>
> Thanks in advance
> Best regards
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 8:53 UTC (permalink / raw)
To: Morten Brørup; +Cc: Wiles, Keith, dev, Olivier Matz
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8B2@smartserver.smartshare.dk>
On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
> Comments inline.
>
> Med venlig hilsen / kind regards
> - Morten Brørup
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Monday, October 24, 2016 6:26 PM
> > To: Wiles, Keith
> > Cc: Morten Brørup; dev@dpdk.org; Olivier Matz
> > Subject: Re: [dpdk-dev] mbuf changes
> >
> > 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:
> > > >
> > > > First of all: Thanks for a great DPDK Userspace 2016!
> > > >
> > > >
> > > >
> > > > Continuing the Userspace discussion about Olivier Matz’s proposed mbuf
> > changes...
> >
> > Thanks for keeping the discussion going!
> > > >
> > > >
> > > >
> > > > 1.
> > > >
> > > > Stephen Hemminger had a noteworthy general comment about keeping
> > metadata for the NIC in the appropriate section of the mbuf: Metadata
> > generated by the NIC’s RX handler belongs in the first cache line, and
> > metadata required by the NIC’s TX handler belongs in the second cache line.
> > This also means that touching the second cache line on ingress should be
> > avoided if possible; and Bruce Richardson mentioned that for this reason m-
> > >next was zeroed on free().
> > > >
> > Thinking about it, I suspect there are more fields we can reset on free
> > to save time on alloc. Refcnt, as discussed below is one of them, but so
> > too could be the nb_segs field and possibly others.
> Yes. Consider the use of rte_pktmbuf_reset() or add a rte_pktmbuf_prealloc() for this purpose.
>
Possibly. We just want to make sure that we don't reset too much either!
:-)
>
> > > > 2.
> > > >
> > > > There seemed to be consensus that the size of m->refcnt should match
> > the size of m->port because a packet could be duplicated on all physical
> > ports for L3 multicast and L2 flooding.
> > > >
> > > > Furthermore, although a single physical machine (i.e. a single server)
> > with 255 physical ports probably doesn’t exist, it might contain more than
> > 255 virtual machines with a virtual port each, so it makes sense extending
> > these mbuf fields from 8 to 16 bits.
> > >
> > > I thought we also talked about removing the m->port from the mbuf as it
> > is not really needed.
> > >
> > Yes, this was mentioned, and also the option of moving the port value to
> > the second cacheline, but it appears that NXP are using the port value
> > in their NIC drivers for passing in metadata, so we'd need their
> > agreement on any move (or removal).
> >
> If a single driver instance services multiple ports, so the ports are not polled individually, the m->port member will be required to identify the port. In that case, it might also used elsewhere in the ingress path, and thus appropriate having in the first cache line. So yes, this needs further discussion with NXP.
>
>
> > > > 3.
> > > >
> > > > Someone (Bruce Richardson?) suggested moving m->refcnt and m->port to
> > the second cache line, which then generated questions from the audience
> > about the real life purpose of m->port, and if m->port could be removed
> > from the mbuf structure.
> > > >
> > > >
> > > >
> > > > 4.
> > > >
> > > > I suggested using offset -1 for m->refcnt, so m->refcnt becomes 0 on
> > first allocation. This is based on the assumption that other mbuf fields
> > must be zeroed at alloc()/free() anyway, so zeroing m->refcnt is cheaper
> > than setting it to 1.
> > > >
> > > > Furthermore (regardless of m->refcnt offset), I suggested that it is
> > not required to modify m->refcnt when allocating and freeing the mbuf, thus
> > saving one write operation on both alloc() and free(). However, this
> > assumes that m->refcnt debugging, e.g. underrun detection, is not required.
> >
> > I don't think it really matters what sentinal value is used for the
> > refcnt because it can't be blindly assigned on free like other fields.
> > However, I think 0 as first reference value becomes more awkward
> > than 1, because we need to deal with underflow. Consider the situation
> > where we have two references to the mbuf, so refcnt is 1, and both are
> > freed at the same time. Since the refcnt is not-zero, then both cores
> > will do an atomic decrement simultaneously giving a refcnt of -1. We can
> > then set this back to zero before freeing, however, I'd still prefer to
> > have refcnt be an accurate value so that it always stays positive, and
> > we can still set it to "one" on free to avoid having to set on alloc.
> >
> Good example. It explains very clearly why my suggested optimization is tricky. My optimization is targeting the most likely scenario where m->refcnt is only "one", but at the expense of some added complexity for the unlikely scenarios. (I only suggested 0 as "one" because I assumed the value could be zeroed faster than set to 1, so forget about that, if you like.)
>
> I still argue that if we keep m->refcnt at "one" value while in the free pool, a write operation can be avoided at both free() and alloc(). Here is a conceptual example (using m->refcnt==1 for "one"):
>
Yes, I agree with you on this.
> void __rte_mbuf_raw_free(struct rte_mbuf *m)
> {
> RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> rte_mempool_put(m->pool, m);
> }
>
> void __rte_pktmbuf_free_seg_inner(struct rte_mbuf *m)
> {
> /* if this is an indirect mbuf, it is detached. */
> if (RTE_MBUF_INDIRECT(m))
> rte_pktmbuf_detach(m);
> m->next = NULL; /* Preload to avoid setting in alloc(). */
> __rte_mbuf_raw_free(m);
> }
>
> void rte_pktmbuf_free_seg(struct rte_mbuf *m)
> {
> if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> /* We have the last reference, so we return it to the free pool. */
> __rte_pktmbuf_free_seg_inner(m);
> } else {
> /* More references, so decrement the refcnt. */
> if (unlikely(rte_mbuf_refcnt_update(m, -1) == 0)) {
> /* Someone raced to decrement the refcnt before us,
> so now we have the last reference,
> so we return it to the free pool. */
> rte_mbuf_refcnt_set(m, 1); /* Preload to avoid in alloc(). */
> __rte_pktmbuf_free_seg_inner(m);
> }
> }
> }
>
>
> > Also, if we set refcnt on free rather than alloc, it does set itself up
> > as a good candidate for moving to the second cacheline. Fast-path
> > processing does not normally update the value.
> >
> Agreed. But also consider that TX handling should be optimized too. And this includes free() processing and preloading values for alloc(); this should only be done in free() instead of alloc() if it provides a total performance improvement. We should not improve RX handling performance if the TX handling performance decreases much more.
Agreed.
>
>
> > > > 5.
> > > >
> > > > And here’s something new to think about:
> > > >
> > > > m->next already reveals if there are more segments to a packet. Which
> > purpose does m->nb_segs serve that is not already covered by m->next?
> >
> > It is duplicate info, but nb_segs can be used to check the validity of
> > the next pointer without having to read the second mbuf cacheline.
> >
> > Whether it's worth having is something I'm happy enough to discuss,
> > though.
> If anybody needs to check for additional segments (m->next != NULL) without hitting the second cache line, a single bit in the first cache line will suffice. (If this is a common case, they probably need to read the m->next value anyway, and then it would optimally belong in the first cache line. I am just mentioning this. It's not my preference.)
>
>
> > 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?
> Speaking for myself, a high resolution RX timestamp is "nice to have", so I like it, but I would hate to sacrifice the sequence number for it. Generally, I would assume that the sequence number used for reordering is more important than a timestamp, except for applications dealing with timing measurements or high precision packet capturing.
>
> Conversion of timestamps from one unit to another can be relatively expensive, so I recommend keeping timestamps in whatever resolution the NIC hardware provides, leaving the conversion to e.g. nanoseconds up to the applications that use timestamps. A lot of protocols use opaque timestamps, e.g. RTP, so this would not be exceptional. Since the timestamp belongs in the first cache line, I suggest we limit it to 32 bit. This can be the least significant 32 bit of a 64 bit NIC hardware timestamp, leaving it up to the application to keep track of the most significant 32 bit by some other means. If it's counting nanoseconds, 32 bit can hold 4 seconds.
>
I worry that 32-bits is not enough. If we do use a 32-bit value, I also
think that in many cases we can't just take the low 32-bits. For a 2GHz
IA CPU the rdtsc() value would wrap around in just 2 seconds if
truncated to 32 bits, for instance. In a case like that, we may be
better to take 32-bits from somewhere in the middle - trading accuracy
for better wrap-around time.
/Bruce
^ permalink raw reply
* Re: rte_kni_tx_burst() hangs because of no freedescriptors
From: Ananyev, Konstantin @ 2016-10-25 9:20 UTC (permalink / raw)
To: Zhang, Helin, yingzhi; +Cc: dev@dpdk.org
In-Reply-To: <F35DEAC7BCE34641BA9FAC6BCA4A12E717F2D4AF@SHSMSX103.ccr.corp.intel.com>
Hi Helin,
>
> From: yingzhi [mailto:kaitoy@qq.com]
> Sent: Monday, October 24, 2016 6:39 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: RE: [dpdk-dev] rte_kni_tx_burst() hangs because of no freedescriptors
>
> Hi Helin,
>
> Thanks for your response, to answer your questions:
> 1. we send only one packet each time calling rte_kni_tx_burst(), which means the last argument is 1.
> 2. it returns 0 because the free mbuf function inside tx_burst will not free any mbuf:
>
> if (txq->nb_tx_free < txq->tx_free_thresh)
> ixgbe_tx_free_bufs(txq);
>
> after this operation, the txq->nb_tx_free is not increased and keeps to be "0" eventually.
>
> I did some tests today, I commented out this section of ixgbe_rxtx_vec_common.h -> ixgbe_tx_free_bufs
>
> status = txq->tx_ring[txq->tx_next_dd].wb.status;
> if (!(status & IXGBE_ADVTXD_STAT_DD))
> return 0;
>
> After ignoring DD bit check, our app runs for about 6 hours without issue. I suspect there is something wrong in my program set the DD bit
> somewhere. One of the possible cause currently I suspect is as far as I know, rte_pktmbuf_free(mbuf.array[k]) will free the mbuf of the
> packet and any fragmented packets following by it. But in our application such as below code snippet:
>
> auto nb_tx = rte_eth_tx_burst(port, queue, mbuf.array, (uint16_t) nb_rx);
> if (unlikely(nb_tx < nb_rx)) {
> for (unsigned k = nb_tx; k < nb_rx; k++) {
> rte_pktmbuf_free(mbuf.array[k]);
> }
> }
> [Zhang, Helin] it seems above code piece has memory leak, if the buffer is chained. After all memory leaked, then the issue comes. Please
> try to check if this is the root cause!
>
> In this case if there are fragmented packets and failed transmission, may cause a mbuf be freed multiple times.
Yes rte_pktmbuf_free() will free all chained segments for that packet.
The code above seems correct to me
(if of course .you don't try to free these packets again somewhere later).
Can you clarify where do you think is a memory leak here?
Konstantin
>
> Above is just my suspect, need to do some tests later today or tomorrow.
>
> Thanks
> ------------------ Original ------------------
> From: "Zhang, Helin";<helin.zhang@intel.com>;
> Date: Mon, Oct 24, 2016 11:33 AM
> To: "yingzhi"<kaitoy@qq.com>;
> Cc: "dev@dpdk.org"<dev@dpdk.org>;
> Subject: RE: [dpdk-dev] rte_kni_tx_burst() hangs because of no freedescriptors
>
> Hi Yingzhi
>
> Thank you for the reporting! The description is not so clear at least for me.
> Please help to narrown down the issue by youself.
> How many packets would it have for calling TX function?
> Why it would return 0 after calling TX function? No memory? Or return from else? Have you found anything?
>
> Regards,
> Helin
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org]<mailto:[mailto:dev-bounces@dpdk.org]> On Behalf Of yingzhi
> > Sent: Sunday, October 23, 2016 9:30 PM
> > To: users; dev@dpdk.org<mailto:dev@dpdk.org>
> > Subject: [dpdk-dev] rte_kni_tx_burst() hangs because of no free descriptors
> >
> > -
> > Hi Experts,
> >
> > Background:
> >
> > We are using DPDK to develop a LoadBalancer following below logic: When
> > a new packet is received:
> > 1. if the dst_addr is management IP, forward to KNI. 2. if the dst_addr is in
> > VIP list, select backend and forward(modify dst mac address). 3. otherwise
> > drop the packet.
> >
> > At this stage, we use one single thread for KNI forwarding and another for
> > VIP forwarding(forward to eth).
> >
> > DPDK version: 16.07
> > NIC: 82599ES 10-Gigabit SFI/SFP+ Network Connection
> > Linux: 14.04.1-Ubuntu x64
> >
> > Promblem description:
> >
> > The program runs correctly for sometime(around 2 hours for 400Mb traffic).
> > But it it will hang. When problem happens, rte_eth_tx_burst() will not able to
> > send out any packets(always returns 0). We tracked into that function and
> > noticed it is actually calling ixgbe driver's ixgbe_xmit_pkts_vec() function in
> > our environment, because we use default tx queue configuration, after
> > printing some info, we found if the free function works fine:
> > tx_rs_thresh: 32, tx_free_thresh: 32, nb_tx_free: 31
> >
> > it will trigger free and make 32 more free descriptors:
> > tx_rs_thresh: 32, tx_free_thresh: 32, nb_tx_free: 62
> >
> > but when something going wrong, it will no longer free anything:
> > tx_rs_thresh: 32, tx_free_thresh: 32, nb_tx_free: 0 tx_rs_thresh: 32,
> > tx_free_thresh: 32, nb_tx_free: 0
> >
> > It may related with the DD flag of the descriptor but we are not quite sure.
> >
> > Our program logic:
> >
> > create two mbuf pools on socket 0, one for rx_queue and one for kni. (all
> > lcore threads runs on socket0)
> >
> > init kni interface with rte_kni_alloc()
> >
> >
> > init one NIC interface with
> > rte_eth_dev_configure(); rte_eth_rx_queue_setup();
> > rte_eth_tx_queue_setup(); rte_eth_dev_start();
> >
> >
> >
> > in the eth main loop: (code is simplified)
> > while(1) { n = rte_eth_rx_burst(packets); for (i = 0; i < n; ++i) { if
> > (SEND_TO_KNI) { m = rte_kni_tx_burst(packets[i]); if (m != 1))
> > { rte_pktmbuf_free(packets[i]); } } if (SEND_TO_ETH)
> > { // after modify the packet m = rte_eth_tx_burst(packets[i]);
> > if (m != 1)) { rte_pktmbuf_free(packets[i]); } } //
> > otherwise drop the packet rte_pktmbuf_free(packets[i]); } }
> >
> >
> > Please advise if I'm using DPDK in a wrong way. Sorry if I missed something
> > basic, I'm new to DPDK.
> >
> > Thanks in advance
> > Best regards
^ permalink raw reply
* Re: mbuf changes
From: Adrien Mazarguil @ 2016-10-25 9:39 UTC (permalink / raw)
To: Bruce Richardson
Cc: Wiles, Keith, Morten Brørup, dev@dpdk.org, Olivier Matz,
Oleg Kuporosov
In-Reply-To: <20161024162538.GA34988@bricha3-MOBL3.ger.corp.intel.com>
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:
[...]
> > > 5.
> > >
> > > And here’s something new to think about:
> > >
> > > m->next already reveals if there are more segments to a packet. Which purpose does m->nb_segs serve that is not already covered by m->next?
>
> It is duplicate info, but nb_segs can be used to check the validity of
> the next pointer without having to read the second mbuf cacheline.
>
> Whether it's worth having is something I'm happy enough to discuss,
> though.
Although slower in some cases than a full blown "next packet" pointer,
nb_segs can also be conveniently abused to link several packets and their
segments in the same list without wasting space.
> 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
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: mbuf changes
From: Ananyev, Konstantin @ 2016-10-25 10:02 UTC (permalink / raw)
To: Richardson, Bruce, Morten Brørup
Cc: Wiles, Keith, dev@dpdk.org, Olivier Matz
In-Reply-To: <20161025085353.GA33668@bricha3-MOBL3.ger.corp.intel.com>
Hi everyone,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Tuesday, October 25, 2016 9:54 AM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>
> Subject: Re: [dpdk-dev] mbuf changes
>
> On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
> > Comments inline.
> >
> > Med venlig hilsen / kind regards
> > - Morten Brørup
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > > Sent: Monday, October 24, 2016 6:26 PM
> > > To: Wiles, Keith
> > > Cc: Morten Brørup; dev@dpdk.org; Olivier Matz
> > > Subject: Re: [dpdk-dev] mbuf changes
> > >
> > > 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:
> > > > >
> > > > > First of all: Thanks for a great DPDK Userspace 2016!
> > > > >
> > > > >
> > > > >
> > > > > Continuing the Userspace discussion about Olivier Matz’s proposed mbuf
> > > changes...
> > >
> > > Thanks for keeping the discussion going!
> > > > >
> > > > >
> > > > >
> > > > > 1.
> > > > >
> > > > > Stephen Hemminger had a noteworthy general comment about keeping
> > > metadata for the NIC in the appropriate section of the mbuf: Metadata
> > > generated by the NIC’s RX handler belongs in the first cache line, and
> > > metadata required by the NIC’s TX handler belongs in the second cache line.
> > > This also means that touching the second cache line on ingress should be
> > > avoided if possible; and Bruce Richardson mentioned that for this reason m-
> > > >next was zeroed on free().
> > > > >
> > > Thinking about it, I suspect there are more fields we can reset on free
> > > to save time on alloc. Refcnt, as discussed below is one of them, but so
> > > too could be the nb_segs field and possibly others.
> > Yes. Consider the use of rte_pktmbuf_reset() or add a rte_pktmbuf_prealloc() for this purpose.
> >
> Possibly. We just want to make sure that we don't reset too much either!
> :-)
>
> >
> > > > > 2.
> > > > >
> > > > > There seemed to be consensus that the size of m->refcnt should match
> > > the size of m->port because a packet could be duplicated on all physical
> > > ports for L3 multicast and L2 flooding.
> > > > >
> > > > > Furthermore, although a single physical machine (i.e. a single server)
> > > with 255 physical ports probably doesn’t exist, it might contain more than
> > > 255 virtual machines with a virtual port each, so it makes sense extending
> > > these mbuf fields from 8 to 16 bits.
> > > >
> > > > I thought we also talked about removing the m->port from the mbuf as it
> > > is not really needed.
> > > >
> > > Yes, this was mentioned, and also the option of moving the port value to
> > > the second cacheline, but it appears that NXP are using the port value
> > > in their NIC drivers for passing in metadata, so we'd need their
> > > agreement on any move (or removal).
> > >
> > If a single driver instance services multiple ports, so the ports are not polled individually, the m->port member will be required to identify
> the port. In that case, it might also used elsewhere in the ingress path, and thus appropriate having in the first cache line.
Ok, but these days most of devices have multiple rx queues.
So identify the RX source properly you need not only port, but port+queue (at least 3 bytes).
But I suppose better to wait for NXP input here.
>So yes, this needs
> further discussion with NXP.
> >
> >
> > > > > 3.
> > > > >
> > > > > Someone (Bruce Richardson?) suggested moving m->refcnt and m->port to
> > > the second cache line, which then generated questions from the audience
> > > about the real life purpose of m->port, and if m->port could be removed
> > > from the mbuf structure.
> > > > >
> > > > >
> > > > >
> > > > > 4.
> > > > >
> > > > > I suggested using offset -1 for m->refcnt, so m->refcnt becomes 0 on
> > > first allocation. This is based on the assumption that other mbuf fields
> > > must be zeroed at alloc()/free() anyway, so zeroing m->refcnt is cheaper
> > > than setting it to 1.
> > > > >
> > > > > Furthermore (regardless of m->refcnt offset), I suggested that it is
> > > not required to modify m->refcnt when allocating and freeing the mbuf, thus
> > > saving one write operation on both alloc() and free(). However, this
> > > assumes that m->refcnt debugging, e.g. underrun detection, is not required.
> > >
> > > I don't think it really matters what sentinal value is used for the
> > > refcnt because it can't be blindly assigned on free like other fields.
> > > However, I think 0 as first reference value becomes more awkward
> > > than 1, because we need to deal with underflow. Consider the situation
> > > where we have two references to the mbuf, so refcnt is 1, and both are
> > > freed at the same time. Since the refcnt is not-zero, then both cores
> > > will do an atomic decrement simultaneously giving a refcnt of -1. We can
> > > then set this back to zero before freeing, however, I'd still prefer to
> > > have refcnt be an accurate value so that it always stays positive, and
> > > we can still set it to "one" on free to avoid having to set on alloc.
> > >
> > Good example. It explains very clearly why my suggested optimization is tricky. My optimization is targeting the most likely scenario
> where m->refcnt is only "one", but at the expense of some added complexity for the unlikely scenarios. (I only suggested 0 as "one"
> because I assumed the value could be zeroed faster than set to 1, so forget about that, if you like.)
> >
> > I still argue that if we keep m->refcnt at "one" value while in the free pool, a write operation can be avoided at both free() and alloc().
> Here is a conceptual example (using m->refcnt==1 for "one"):
> >
> Yes, I agree with you on this.
>
> > void __rte_mbuf_raw_free(struct rte_mbuf *m)
> > {
> > RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> > rte_mempool_put(m->pool, m);
> > }
> >
> > void __rte_pktmbuf_free_seg_inner(struct rte_mbuf *m)
> > {
> > /* if this is an indirect mbuf, it is detached. */
> > if (RTE_MBUF_INDIRECT(m))
> > rte_pktmbuf_detach(m);
> > m->next = NULL; /* Preload to avoid setting in alloc(). */
> > __rte_mbuf_raw_free(m);
> > }
> >
> > void rte_pktmbuf_free_seg(struct rte_mbuf *m)
> > {
> > if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> > /* We have the last reference, so we return it to the free pool. */
> > __rte_pktmbuf_free_seg_inner(m);
> > } else {
> > /* More references, so decrement the refcnt. */
> > if (unlikely(rte_mbuf_refcnt_update(m, -1) == 0)) {
> > /* Someone raced to decrement the refcnt before us,
> > so now we have the last reference,
> > so we return it to the free pool. */
> > rte_mbuf_refcnt_set(m, 1); /* Preload to avoid in alloc(). */
> > __rte_pktmbuf_free_seg_inner(m);
> > }
> > }
> > }
> >
> >
> > > Also, if we set refcnt on free rather than alloc, it does set itself up
> > > as a good candidate for moving to the second cacheline. Fast-path
> > > processing does not normally update the value.
> > >
> > Agreed. But also consider that TX handling should be optimized too. And this includes free() processing and preloading values for alloc();
> this should only be done in free() instead of alloc() if it provides a total performance improvement. We should not improve RX handling
> performance if the TX handling performance decreases much more.
>
> Agreed.
> >
> >
> > > > > 5.
> > > > >
> > > > > And here’s something new to think about:
> > > > >
> > > > > m->next already reveals if there are more segments to a packet. Which
> > > purpose does m->nb_segs serve that is not already covered by m->next?
> > >
> > > It is duplicate info, but nb_segs can be used to check the validity of
> > > the next pointer without having to read the second mbuf cacheline.
> > >
> > > Whether it's worth having is something I'm happy enough to discuss,
> > > though.
> > If anybody needs to check for additional segments (m->next != NULL) without hitting the second cache line, a single bit in the first cache
> line will suffice. (If this is a common case, they probably need to read the m->next value anyway, and then it would optimally belong in the
> first cache line. I am just mentioning this. It's not my preference.)
> >
> >
> > > 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?
> > Speaking for myself, a high resolution RX timestamp is "nice to have", so I like it, but I would hate to sacrifice the sequence number for it.
> Generally, I would assume that the sequence number used for reordering is more important than a timestamp, except for applications
> dealing with timing measurements or high precision packet capturing.
> >
> > Conversion of timestamps from one unit to another can be relatively expensive, so I recommend keeping timestamps in whatever
> resolution the NIC hardware provides, leaving the conversion to e.g. nanoseconds up to the applications that use timestamps. A lot of
> protocols use opaque timestamps, e.g. RTP, so this would not be exceptional. Since the timestamp belongs in the first cache line, I suggest
> we limit it to 32 bit. This can be the least significant 32 bit of a 64 bit NIC hardware timestamp, leaving it up to the application to keep track
> of the most significant 32 bit by some other means. If it's counting nanoseconds, 32 bit can hold 4 seconds.
> >
>
> I worry that 32-bits is not enough. If we do use a 32-bit value, I also
> think that in many cases we can't just take the low 32-bits. For a 2GHz
> IA CPU the rdtsc() value would wrap around in just 2 seconds if
> truncated to 32 bits, for instance. In a case like that, we may be
> better to take 32-bits from somewhere in the middle - trading accuracy
> for better wrap-around time.
I also think that 32bit for timestamps would not be enough.
Though as we agree that first cache line should be for fields filled by fast-path RX,
I suppose timestamp (and seqn) should be in the second line and shouldn't be filled by PMD itself.
About what should be stored here (tsc value/system clock in ns/something else) -
I think it is better left to particular SW that would fill/use that field to decide.
For me personally tsc seems like the most appropriate.
Konstantin
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 10:11 UTC (permalink / raw)
To: Adrien Mazarguil, Bruce Richardson
Cc: Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <20161025093915.GJ5733@6wind.com>
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:
> [...]
> > > > 5.
> > > >
> > > > And here’s something new to think about:
> > > >
> > > > m->next already reveals if there are more segments to a packet.
> Which purpose does m->nb_segs serve that is not already covered by m-
> >next?
> >
> > It is duplicate info, but nb_segs can be used to check the validity
> of
> > the next pointer without having to read the second mbuf cacheline.
> >
> > Whether it's worth having is something I'm happy enough to discuss,
> > though.
>
> Although slower in some cases than a full blown "next packet" pointer,
> nb_segs can also be conveniently abused to link several packets and
> their segments in the same list without wasting space.
I don’t understand that; can you please elaborate? Are you abusing m->nb_segs as an index into an array in your application? If that is the case, and it is endorsed by the community, we should get rid of m->nb_segs and add a member for application specific use instead.
> > 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.
>
> --
> Adrien Mazarguil
> 6WIND
^ permalink raw reply
* Re: [PATCH v4] net/i40e: fix hash filter invalid issue in X722
From: Wu, Jingjing @ 2016-10-25 10:22 UTC (permalink / raw)
To: Guo, Jia, Zhang, Helin; +Cc: dev@dpdk.org
In-Reply-To: <1477363356-112272-1-git-send-email-jia.guo@intel.com>
> -----Original Message-----
> From: Guo, Jia
> Sent: Tuesday, October 25, 2016 10:43 AM
> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>
> Subject: [PATCH v4] net/i40e: fix hash filter invalid issue in X722
>
> When verifying the Hash filtering on X722, we found a problem that
> the hash value in descriptor is incorrect. The root caused is X722
> uses different way of hash key word selection comparing with X710/XL710.
> This patch fixes it by setting X722 specific key selection.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
If we use default configuration on HW, this issue will not exist. But
our software sets the default value through i40e_filter_input_set_init.
So I think fix line need to be the commit which introduced input set
changing feature.
Acked-by: Jingjing Wu <jingjing.wu@intel.com> with additional line:
Fixes: commit 98f055707685 ("i40e: configure input fields for RSS or flow director ")
Thanks
Jingjing
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 10:22 UTC (permalink / raw)
To: Ananyev, Konstantin, Richardson, Bruce; +Cc: Wiles, Keith, dev, Olivier Matz
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CC6A7@irsmsx105.ger.corp.intel.com>
Comments inline (at the end).
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Tuesday, October 25, 2016 12:03 PM
> To: Richardson, Bruce; Morten Brørup
> Cc: Wiles, Keith; dev@dpdk.org; Olivier Matz
> Subject: Re: [dpdk-dev] mbuf changes
>
> Hi everyone,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Tuesday, October 25, 2016 9:54 AM
> > To: Morten Brørup <mb@smartsharesystems.com>
> > Cc: Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Olivier Matz
> > <olivier.matz@6wind.com>
> > Subject: Re: [dpdk-dev] mbuf changes
> >
> > On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
> > > Comments inline.
> > >
> > > Med venlig hilsen / kind regards
> > > - Morten Brørup
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> > > > Richardson
> > > > Sent: Monday, October 24, 2016 6:26 PM
> > > > To: Wiles, Keith
> > > > Cc: Morten Brørup; dev@dpdk.org; Olivier Matz
> > > > Subject: Re: [dpdk-dev] mbuf changes
> > > >
> > > > 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:
> > > > > >
> > > > > > 2.
> > > > > >
> > > > > > There seemed to be consensus that the size of m->refcnt
> should
> > > > > > match
> > > > the size of m->port because a packet could be duplicated on all
> > > > physical ports for L3 multicast and L2 flooding.
> > > > > >
> > > > > > Furthermore, although a single physical machine (i.e. a
> single
> > > > > > server)
> > > > with 255 physical ports probably doesn’t exist, it might contain
> > > > more than
> > > > 255 virtual machines with a virtual port each, so it makes sense
> > > > extending these mbuf fields from 8 to 16 bits.
> > > > >
> > > > > I thought we also talked about removing the m->port from the
> > > > > mbuf as it
> > > > is not really needed.
> > > > >
> > > > Yes, this was mentioned, and also the option of moving the port
> > > > value to the second cacheline, but it appears that NXP are using
> > > > the port value in their NIC drivers for passing in metadata, so
> > > > we'd need their agreement on any move (or removal).
> > > >
> > > If a single driver instance services multiple ports, so the ports
> > > are not polled individually, the m->port member will be required to
> > > identify
> > the port. In that case, it might also used elsewhere in the ingress
> path, and thus appropriate having in the first cache line.
>
> Ok, but these days most of devices have multiple rx queues.
> So identify the RX source properly you need not only port, but
> port+queue (at least 3 bytes).
> But I suppose better to wait for NXP input here.
>
> >So yes, this needs
> > further discussion with NXP.
It seems that this concept might be somewhat similar to the Flow Director information, which already has plenty of bits in the first cache line. You should consider if this can be somehow folded into the m->hash union.
-Morten
^ permalink raw reply
* Re: [PATCH v2] net/i40e: fix fdir configure failed issue in X710
From: Wu, Jingjing @ 2016-10-25 10:28 UTC (permalink / raw)
To: Guo, Jia, Zhang, Helin; +Cc: dev@dpdk.org
In-Reply-To: <1477362341-90630-1-git-send-email-jia.guo@intel.com>
> -----Original Message-----
> From: Guo, Jia
> Sent: Tuesday, October 25, 2016 10:26 AM
> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>
> Subject: [PATCH v2] net/i40e: fix fdir configure failed issue in X710
>
> Because of some register is only supported by X722, such as I40E_GLQF_FD_PCTYPES,
> so it need to use the mac type to distinguish the behavior of X722 from X710 and other
> NICs, or it would result X710 functional failed.
>
> Fixes: 8c5cb3c11513 (“net/i40e: add packet type translation for X722”)
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
Thanks
Jingjing
^ permalink raw reply
* Re: [PATCH v2 1/2] net/i40e: fix link status change interrupt
From: Wu, Jingjing @ 2016-10-25 10:43 UTC (permalink / raw)
To: Yang, Qiming, dev@dpdk.org; +Cc: Zhang, Helin, Richardson, Bruce
In-Reply-To: <1477369146-63317-1-git-send-email-qiming.yang@intel.com>
> -----Original Message-----
> From: Yang, Qiming
> Sent: Tuesday, October 25, 2016 12:19 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Yang, Qiming <qiming.yang@intel.com>
> Subject: [PATCH v2 1/2] net/i40e: fix link status change interrupt
>
> Previously, link status interrupt in i40e is achieved by checking
> LINK_STAT_CHANGE_MASK in PFINT_ICR0 register which is provided only
> for diagnostic use. Instead, drivers need to get the link status
> change notification by using LSE (Link Status Event).
>
> This patch enables LSE and calls LSC callback when the event is
> received. This patch also removes the processing on
> LINK_STAT_CHANGE_MASK.
>
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
^ permalink raw reply
* Re: [PATCH v2 2/2] net/i40e: fix VF bonded device link down
From: Wu, Jingjing @ 2016-10-25 10:45 UTC (permalink / raw)
To: Yang, Qiming, dev@dpdk.org; +Cc: Zhang, Helin, Richardson, Bruce
In-Reply-To: <1477369146-63317-2-git-send-email-qiming.yang@intel.com>
> -----Original Message-----
> From: Yang, Qiming
> Sent: Tuesday, October 25, 2016 12:19 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Yang, Qiming <qiming.yang@intel.com>
> Subject: [PATCH v2 2/2] net/i40e: fix VF bonded device link down
>
> If VF device is used as slave of a bond device, it will be polled
> periodically through alarm. Interrupt is involved here. And then
> VF will send I40E_VIRTCHNL_OP_GET_LINK_STAT message to
> PF to query the status. The response is handled by interrupt
> callback. Interrupt is involved here again. That's why bond
> device cannot bring up.
>
> This patch removes I40E_VIRTCHNL_OP_GET_LINK_STAT
> message. Link status in VF driver will be updated when PF driver
> notify it, and VF stores this link status locally. VF driver just
> returns the local status when being required.
>
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
Thanks.
BTW, next time, don't forget to note your changes comparing the previous version.
It will make reviewers' life easy. :)
/Jingjing
^ permalink raw reply
* Re: mbuf changes
From: Adrien Mazarguil @ 2016-10-25 11:04 UTC (permalink / raw)
To: Morten Brørup
Cc: Bruce Richardson, Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8B7@smartserver.smartshare.dk>
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:
> > [...]
> > > > > 5.
> > > > >
> > > > > And here’s something new to think about:
> > > > >
> > > > > m->next already reveals if there are more segments to a packet.
> > Which purpose does m->nb_segs serve that is not already covered by m-
> > >next?
> > >
> > > It is duplicate info, but nb_segs can be used to check the validity
> > of
> > > the next pointer without having to read the second mbuf cacheline.
> > >
> > > Whether it's worth having is something I'm happy enough to discuss,
> > > though.
> >
> > Although slower in some cases than a full blown "next packet" pointer,
> > nb_segs can also be conveniently abused to link several packets and
> > their segments in the same list without wasting space.
>
> I don’t understand that; can you please elaborate? Are you abusing m->nb_segs as an index into an array in your application? If that is the case, and it is endorsed by the community, we should get rid of m->nb_segs and add a member for application specific use instead.
Well, that's just an idea, I'm not aware of any application using this,
however the ability to link several packets with segments seems
useful to me (e.g. buffering packets). Here's a diagram:
.-----------. .-----------. .-----------. .-----------. .------
| pkt 0 | | seg 1 | | seg 2 | | pkt 1 | | pkt 2
| next --->| next --->| next --->| next --->| ...
| nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
`-----------' `-----------' `-----------' `-----------' `------
> > > 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 with
out 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.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 11:13 UTC (permalink / raw)
To: Adrien Mazarguil
Cc: Morten Brørup, Wiles, Keith, dev, Olivier Matz,
Oleg Kuporosov
In-Reply-To: <20161025110444.GK5733@6wind.com>
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:
> > > [...]
> > > > > > 5.
> > > > > >
> > > > > > And here’s something new to think about:
> > > > > >
> > > > > > m->next already reveals if there are more segments to a packet.
> > > Which purpose does m->nb_segs serve that is not already covered by m-
> > > >next?
> > > >
> > > > It is duplicate info, but nb_segs can be used to check the validity
> > > of
> > > > the next pointer without having to read the second mbuf cacheline.
> > > >
> > > > Whether it's worth having is something I'm happy enough to discuss,
> > > > though.
> > >
> > > Although slower in some cases than a full blown "next packet" pointer,
> > > nb_segs can also be conveniently abused to link several packets and
> > > their segments in the same list without wasting space.
> >
> > I don’t understand that; can you please elaborate? Are you abusing m->nb_segs as an index into an array in your application? If that is the case, and it is endorsed by the community, we should get rid of m->nb_segs and add a member for application specific use instead.
>
> Well, that's just an idea, I'm not aware of any application using this,
> however the ability to link several packets with segments seems
> useful to me (e.g. buffering packets). Here's a diagram:
>
> .-----------. .-----------. .-----------. .-----------. .------
> | pkt 0 | | seg 1 | | seg 2 | | pkt 1 | | pkt 2
> | next --->| next --->| next --->| next --->| ...
> | nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
> `-----------' `-----------' `-----------' `-----------' `------
>
> > > > 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 wi
thout 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. However,
nb_segs may be a good candidate for demotion, along with possibly the
port value, or the reference count.
/Bruce
^ permalink raw reply
* Re: mbuf changes
From: Shreyansh Jain @ 2016-10-25 11:54 UTC (permalink / raw)
To: Bruce Richardson
Cc: Wiles, Keith, Morten Brørup, dev@dpdk.org, Olivier Matz
In-Reply-To: <20161024162538.GA34988@bricha3-MOBL3.ger.corp.intel.com>
On Monday 24 October 2016 09:55 PM, 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:
>>>
>>> First of all: Thanks for a great DPDK Userspace 2016!
>>>
>>>
>>>
>>> Continuing the Userspace discussion about Olivier Matz’s proposed mbuf changes...
>
> Thanks for keeping the discussion going!
>>>
>>>
>>>
>>> 1.
>>>
>>> Stephen Hemminger had a noteworthy general comment about keeping metadata for the NIC in the appropriate section of the mbuf: Metadata generated by the NIC’s RX handler belongs in the first cache line, and metadata required by the NIC’s TX handler belongs in the second cache line. This also means that touching the second cache line on ingress should be avoided if possible; and Bruce Richardson mentioned that for this reason m->next was zeroed on free().
>>>
> Thinking about it, I suspect there are more fields we can reset on free
> to save time on alloc. Refcnt, as discussed below is one of them, but so
> too could be the nb_segs field and possibly others.
>
>>>
>>>
>>> 2.
>>>
>>> There seemed to be consensus that the size of m->refcnt should match the size of m->port because a packet could be duplicated on all physical ports for L3 multicast and L2 flooding.
>>>
>>> Furthermore, although a single physical machine (i.e. a single server) with 255 physical ports probably doesn’t exist, it might contain more than 255 virtual machines with a virtual port each, so it makes sense extending these mbuf fields from 8 to 16 bits.
>>
>> I thought we also talked about removing the m->port from the mbuf as it is not really needed.
>>
> Yes, this was mentioned, and also the option of moving the port value to
> the second cacheline, but it appears that NXP are using the port value
> in their NIC drivers for passing in metadata, so we'd need their
> agreement on any move (or removal).
I am not sure where NXP's NIC came into picture on this, but now that it
is highlighted, this field is required for libevent implementation [1].
A scheduler sending an event, which can be a packet, would only have
information of a flow_id. From this matching it back to a port, without
mbuf->port, would be very difficult (costly). There may be way around
this but at least in current proposal I think port would be important to
have - even if in second cache line.
But, off the top of my head, as of now it is not being used for any
specific purpose in NXP's PMD implementation.
Even the SoC patches don't necessarily rely on it except using it
because it is available.
@Bruce: where did you get the NXP context here from?
[1] http://dpdk.org/ml/archives/dev/2016-October/048592.html
>
>>>
>>>
>>>
>>> 3.
>>>
>>> Someone (Bruce Richardson?) suggested moving m->refcnt and m->port to the second cache line, which then generated questions from the audience about the real life purpose of m->port, and if m->port could be removed from the mbuf structure.
>>>
>>>
>>>
>>> 4.
>>>
>>> I suggested using offset -1 for m->refcnt, so m->refcnt becomes 0 on first allocation. This is based on the assumption that other mbuf fields must be zeroed at alloc()/free() anyway, so zeroing m->refcnt is cheaper than setting it to 1.
>>>
>>> Furthermore (regardless of m->refcnt offset), I suggested that it is not required to modify m->refcnt when allocating and freeing the mbuf, thus saving one write operation on both alloc() and free(). However, this assumes that m->refcnt debugging, e.g. underrun detection, is not required.
>
> I don't think it really matters what sentinal value is used for the
> refcnt because it can't be blindly assigned on free like other fields.
> However, I think 0 as first reference value becomes more awkward
> than 1, because we need to deal with underflow. Consider the situation
> where we have two references to the mbuf, so refcnt is 1, and both are
> freed at the same time. Since the refcnt is not-zero, then both cores
> will do an atomic decrement simultaneously giving a refcnt of -1. We can
> then set this back to zero before freeing, however, I'd still prefer to
> have refcnt be an accurate value so that it always stays positive, and
> we can still set it to "one" on free to avoid having to set on alloc.
>
> Also, if we set refcnt on free rather than alloc, it does set itself up
> as a good candidate for moving to the second cacheline. Fast-path
> processing does not normally update the value.
>
>>>
>>>
>>>
>>> 5.
>>>
>>> And here’s something new to think about:
>>>
>>> m->next already reveals if there are more segments to a packet. Which purpose does m->nb_segs serve that is not already covered by m->next?
>
> It is duplicate info, but nb_segs can be used to check the validity of
> the next pointer without having to read the second mbuf cacheline.
>
> Whether it's worth having is something I'm happy enough to discuss,
> though.
>
> 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?
>
> /Bruce
>
--
-
Shreyansh
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 12:05 UTC (permalink / raw)
To: Shreyansh Jain
Cc: Wiles, Keith, Morten Brørup, dev@dpdk.org, Olivier Matz,
alejandro.lucero
In-Reply-To: <f2d7f77f-dc67-fce1-183a-7b57a60e4342@nxp.com>
On Tue, Oct 25, 2016 at 05:24:28PM +0530, Shreyansh Jain wrote:
> On Monday 24 October 2016 09:55 PM, 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:
> > > >
> > > > First of all: Thanks for a great DPDK Userspace 2016!
> > > >
> > > >
> > > >
> > > > Continuing the Userspace discussion about Olivier Matz’s proposed mbuf changes...
> >
> > Thanks for keeping the discussion going!
> > > >
> > > >
> > > >
> > > > 1.
> > > >
> > > > Stephen Hemminger had a noteworthy general comment about keeping metadata for the NIC in the appropriate section of the mbuf: Metadata generated by the NIC’s RX handler belongs in the first cache line, and metadata required by the NIC’s TX handler belongs in the second cache line. This also means that touching the second cache line on ingress should be avoided if possible; and Bruce Richardson mentioned that for this reason m->next was zeroed on free().
> > > >
> > Thinking about it, I suspect there are more fields we can reset on free
> > to save time on alloc. Refcnt, as discussed below is one of them, but so
> > too could be the nb_segs field and possibly others.
> >
> > > >
> > > >
> > > > 2.
> > > >
> > > > There seemed to be consensus that the size of m->refcnt should match the size of m->port because a packet could be duplicated on all physical ports for L3 multicast and L2 flooding.
> > > >
> > > > Furthermore, although a single physical machine (i.e. a single server) with 255 physical ports probably doesn’t exist, it might contain more than 255 virtual machines with a virtual port each, so it makes sense extending these mbuf fields from 8 to 16 bits.
> > >
> > > I thought we also talked about removing the m->port from the mbuf as it is not really needed.
> > >
> > Yes, this was mentioned, and also the option of moving the port value to
> > the second cacheline, but it appears that NXP are using the port value
> > in their NIC drivers for passing in metadata, so we'd need their
> > agreement on any move (or removal).
>
> I am not sure where NXP's NIC came into picture on this, but now that it is
> highlighted, this field is required for libevent implementation [1].
>
> A scheduler sending an event, which can be a packet, would only have
> information of a flow_id. From this matching it back to a port, without
> mbuf->port, would be very difficult (costly). There may be way around this
> but at least in current proposal I think port would be important to have -
> even if in second cache line.
>
> But, off the top of my head, as of now it is not being used for any specific
> purpose in NXP's PMD implementation.
>
> Even the SoC patches don't necessarily rely on it except using it because it
> is available.
>
> @Bruce: where did you get the NXP context here from?
>
Oh, I'm just mis-remembering. :-( It was someone else who was looking for
this - Netronome, perhaps?
CC'ing Alejandro in the hope I'm remembering correctly second time
round!
/Bruce
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 12:16 UTC (permalink / raw)
To: Bruce Richardson, Adrien Mazarguil
Cc: Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <20161025111357.GA43504@bricha3-MOBL3.ger.corp.intel.com>
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:
> > > > [...]
> > > > > > > 5.
> > > > > > >
> > > > > > > And here’s something new to think about:
> > > > > > >
> > > > > > > m->next already reveals if there are more segments to a
> packet.
> > > > Which purpose does m->nb_segs serve that is not already covered
> by
> > > > m-
> > > > >next?
> > > > >
> > > > > It is duplicate info, but nb_segs can be used to check the
> > > > > validity
> > > > of
> > > > > the next pointer without having to read the second mbuf
> cacheline.
> > > > >
> > > > > Whether it's worth having is something I'm happy enough to
> > > > > discuss, though.
> > > >
> > > > Although slower in some cases than a full blown "next packet"
> > > > pointer, nb_segs can also be conveniently abused to link several
> > > > packets and their segments in the same list without wasting
> space.
> > >
> > > I don’t understand that; can you please elaborate? Are you abusing
> m->nb_segs as an index into an array in your application? If that is
> the case, and it is endorsed by the community, we should get rid of m-
> >nb_segs and add a member for application specific use instead.
> >
> > Well, that's just an idea, I'm not aware of any application using
> > this, however the ability to link several packets with segments seems
> > useful to me (e.g. buffering packets). Here's a diagram:
> >
> > .-----------. .-----------. .-----------. .-----------. .---
> ---
> > | pkt 0 | | seg 1 | | seg 2 | | pkt 1 | |
> pkt 2
> > | next --->| next --->| next --->| next --->|
> ...
> > | nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
> > `-----------' `-----------' `-----------' `-----------' `---
> ---
I see. It makes it possible to refer to a burst of packets (with segments or not) by a single mbuf reference, as an alternative to the current design pattern of using an array and length (struct rte_mbuf **mbufs, unsigned count).
This would require implementation in the PMDs etc.
And even in this case, m->nb_segs does not need to be an integer, but could be replaced by a single bit indicating if the segment is a continuation of a packet or the beginning (alternatively the end) of a packet, i.e. the bit can be set for either the first or the last segment in the packet.
It is an almost equivalent alternative to the fundamental design pattern of using an array of mbuf with count, which is widely implemented in DPDK. And m->next still lives in the second cache line, so I don't see any gain by this.
I still don't get how m->nb_segs can be abused without m->next.
> > > > > 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.
> However, nb_segs may be a good candidate for demotion, along with
> possibly the port value, or the reference count.
>
> /Bruce
>
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 12:20 UTC (permalink / raw)
To: Morten Brørup
Cc: Adrien Mazarguil, Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8BA@smartserver.smartshare.dk>
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:
> > > > > [...]
> > > > > > > > 5.
> > > > > > > >
> > > > > > > > And here’s something new to think about:
> > > > > > > >
> > > > > > > > m->next already reveals if there are more segments to a
> > packet.
> > > > > Which purpose does m->nb_segs serve that is not already covered
> > by
> > > > > m-
> > > > > >next?
> > > > > >
> > > > > > It is duplicate info, but nb_segs can be used to check the
> > > > > > validity
> > > > > of
> > > > > > the next pointer without having to read the second mbuf
> > cacheline.
> > > > > >
> > > > > > Whether it's worth having is something I'm happy enough to
> > > > > > discuss, though.
> > > > >
> > > > > Although slower in some cases than a full blown "next packet"
> > > > > pointer, nb_segs can also be conveniently abused to link several
> > > > > packets and their segments in the same list without wasting
> > space.
> > > >
> > > > I don’t understand that; can you please elaborate? Are you abusing
> > m->nb_segs as an index into an array in your application? If that is
> > the case, and it is endorsed by the community, we should get rid of m-
> > >nb_segs and add a member for application specific use instead.
> > >
> > > Well, that's just an idea, I'm not aware of any application using
> > > this, however the ability to link several packets with segments seems
> > > useful to me (e.g. buffering packets). Here's a diagram:
> > >
> > > .-----------. .-----------. .-----------. .-----------. .---
> > ---
> > > | pkt 0 | | seg 1 | | seg 2 | | pkt 1 | |
> > pkt 2
> > > | next --->| next --->| next --->| next --->|
> > ...
> > > | nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
> > > `-----------' `-----------' `-----------' `-----------' `---
> > ---
>
> I see. It makes it possible to refer to a burst of packets (with segments or not) by a single mbuf reference, as an alternative to the current design pattern of using an array and length (struct rte_mbuf **mbufs, unsigned count).
>
> This would require implementation in the PMDs etc.
>
> And even in this case, m->nb_segs does not need to be an integer, but could be replaced by a single bit indicating if the segment is a continuation of a packet or the beginning (alternatively the end) of a packet, i.e. the bit can be set for either the first or the last segment in the packet.
>
> It is an almost equivalent alternative to the fundamental design pattern of using an array of mbuf with count, which is widely implemented in DPDK. And m->next still lives in the second cache line, so I don't see any gain by this.
>
> I still don't get how m->nb_segs can be abused without m->next.
>
>
> > > > > > 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.
/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