* Re: [PATCH v2] doc: add mlx5 release notes
From: Mcnamara, John @ 2016-11-04 9:28 UTC (permalink / raw)
To: Nelio Laranjeiro, dev@dpdk.org; +Cc: Adrien Mazarguil
In-Reply-To: <b3884e8811837a97492d7820edea3351b5407d8a.1478180639.git.nelio.laranjeiro@6wind.com>
> -----Original Message-----
> From: Nelio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Thursday, November 3, 2016 1:46 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John <john.mcnamara@intel.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>
> Subject: [PATCH v2] doc: add mlx5 release notes
>
> Add list of tested and validated NICs too.
>
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Hi Nelio,
Nice to see such an extensive list of tested platforms and NICs.
I don't know if it is generally okay to update only part of a patchset
but apart from that:
Acked-by: John McNamara <john.mcnamara@intel.com>
^ permalink raw reply
* [PATCH] net/virtio: cache Rx/Tx offload enabled check
From: Yuanhan Liu @ 2016-11-04 9:28 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, Olivier Matz
It's not a good idea to do Rx/Tx offload enabled check at the data
path. Instead, we could do the check at init stage and store the
result, so that we could avoid the check again and again at the
critical datapath.
Cc: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++
drivers/net/virtio/virtio_pci.h | 2 ++
drivers/net/virtio/virtio_rxtx.c | 30 ++++--------------------------
3 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b388134..708c0e0 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1141,6 +1141,22 @@ rx_func_get(struct rte_eth_dev *eth_dev)
eth_dev->rx_pkt_burst = &virtio_recv_pkts;
}
+static inline int
+rx_offload_enabled(struct virtio_hw *hw)
+{
+ return vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM) ||
+ vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
+ vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
+}
+
+static inline int
+tx_offload_enabled(struct virtio_hw *hw)
+{
+ return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) ||
+ vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) ||
+ vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
+}
+
/* reset device and renegotiate features if needed */
static int
virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
@@ -1161,6 +1177,9 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
if (virtio_negotiate_features(hw, req_features) < 0)
return -1;
+ hw->tx_offload = tx_offload_enabled(hw);
+ hw->rx_offload = rx_offload_enabled(hw);
+
/* If host does not support status then disable LSC */
if (!vtpci_with_feature(hw, VIRTIO_NET_F_STATUS))
eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 0c5ed31..442eed2 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -264,6 +264,8 @@ struct virtio_hw {
struct virtio_net_config *dev_cfg;
const struct virtio_pci_ops *vtpci_ops;
void *virtio_user_dev;
+ int tx_offload;
+ int rx_offload;
};
/*
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index b4c4aa4..ba46595 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -250,14 +250,6 @@ virtio_tso_fix_cksum(struct rte_mbuf *m)
}
}
-static inline int
-tx_offload_enabled(struct virtio_hw *hw)
-{
- return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) ||
- vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) ||
- vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
-}
-
static inline void
virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
uint16_t needed, int use_indirect, int can_push)
@@ -270,9 +262,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
uint16_t head_idx, idx;
uint16_t head_size = vq->hw->vtnet_hdr_size;
struct virtio_net_hdr *hdr;
- int offload;
- offload = tx_offload_enabled(vq->hw);
head_idx = vq->vq_desc_head_idx;
idx = head_idx;
dxp = &vq->vq_descx[idx];
@@ -286,7 +276,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
hdr = (struct virtio_net_hdr *)
rte_pktmbuf_prepend(cookie, head_size);
/* if offload disabled, it is not zeroed below, do it now */
- if (offload == 0)
+ if (vq->hw->tx_offload == 0)
memset(hdr, 0, head_size);
} else if (use_indirect) {
/* setup tx ring slot to point to indirect
@@ -318,7 +308,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
}
/* Checksum Offload / TSO */
- if (offload) {
+ if (vq->hw->tx_offload) {
if (cookie->ol_flags & PKT_TX_TCP_SEG)
cookie->ol_flags |= PKT_TX_TCP_CKSUM;
@@ -798,14 +788,6 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
return 0;
}
-static inline int
-rx_offload_enabled(struct virtio_hw *hw)
-{
- return vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM) ||
- vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
- vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
-}
-
#define VIRTIO_MBUF_BURST_SZ 64
#define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
uint16_t
@@ -821,7 +803,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
int error;
uint32_t i, nb_enqueued;
uint32_t hdr_size;
- int offload;
struct virtio_net_hdr *hdr;
nb_used = VIRTQUEUE_NUSED(vq);
@@ -840,7 +821,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
nb_rx = 0;
nb_enqueued = 0;
hdr_size = hw->vtnet_hdr_size;
- offload = rx_offload_enabled(hw);
for (i = 0; i < num ; i++) {
rxm = rcv_pkts[i];
@@ -871,7 +851,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
if (hw->vlan_strip)
rte_vlan_strip(rxm);
- if (offload && virtio_rx_offload(rxm, hdr) < 0) {
+ if (hw->rx_offload && virtio_rx_offload(rxm, hdr) < 0) {
virtio_discard_rxbuf(vq, rxm);
rxvq->stats.errors++;
continue;
@@ -936,7 +916,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
uint16_t extra_idx;
uint32_t seg_res;
uint32_t hdr_size;
- int offload;
nb_used = VIRTQUEUE_NUSED(vq);
@@ -952,7 +931,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
extra_idx = 0;
seg_res = 0;
hdr_size = hw->vtnet_hdr_size;
- offload = rx_offload_enabled(hw);
while (i < nb_used) {
struct virtio_net_hdr_mrg_rxbuf *header;
@@ -998,7 +976,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
rx_pkts[nb_rx] = rxm;
prev = rxm;
- if (offload && virtio_rx_offload(rxm, &header->hdr) < 0) {
+ if (hw->rx_offload && virtio_rx_offload(rxm, &header->hdr) < 0) {
virtio_discard_rxbuf(vq, rxm);
rxvq->stats.errors++;
continue;
--
2.8.1
^ permalink raw reply related
* Re: [PATCH 1/2] arch/arm: fix file descriptors leakage when getting CPU features
From: Jianbo Liu @ 2016-11-04 9:20 UTC (permalink / raw)
To: Jan Viktorin; +Cc: dev, chaozhu
In-Reply-To: <20161104082423.1f5a7a1c@jvn>
Hi Jan,
On 4 November 2016 at 15:24, <viktorin@rehivetech.com> wrote:
> Hello Jianbo Liu,
>
> thank you, a good catch!
>
> Can you please git blame for the commit introducing the issue and add
> the "Fixes:" tag as described in [1]?
>
> Same for ppc.
>
I will send v2 soon.
Thanks!
> Regards
> Jan
>
> [1] http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body
>
> On Fri, 4 Nov 2016 11:59:08 +0530
> Jianbo Liu <jianbo.liu@linaro.org> wrote:
>
>> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
>
> Acked-by: Jan Viktorin <viktorin@rehivetech.com>
^ permalink raw reply
* [PATCH v4 2/2] net/i40e: fix VF bonded device link down
From: Qiming Yang @ 2016-11-04 9:10 UTC (permalink / raw)
To: dev; +Cc: jingjing.wu, bruce.richardson, Qiming Yang
In-Reply-To: <1478250651-37307-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>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
---
v3 changes:
* resolved the conflict with other changes, rework based
on version: 16.11-rc2
v4 changes:
*fixed compilation error with icc
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 078c581..fbaec23 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -5441,6 +5441,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 =
+ (enum i40e_aq_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);
@@ -5478,9 +5496,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, NULL);
+ }
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 24b8580..298cef4 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -609,7 +609,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 4b835cb..aa306d6 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) },
@@ -2146,35 +2119,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..ddfc140 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 =
+ (enum i40e_aq_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
* [PATCH v4 1/2] net/i40e: fix link status change interrupt
From: Qiming Yang @ 2016-11-04 9:10 UTC (permalink / raw)
To: dev; +Cc: jingjing.wu, bruce.richardson, Qiming Yang
In-Reply-To: <1477628339-5816-2-git-send-email-qiming.yang@intel.com>
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>
---
v3 changes:
* resolved the conflict with other changes, rework based
on version: 16.11-rc2
drivers/net/i40e/i40e_ethdev.c | 96 +++++++++---------------------------------
1 file changed, 19 insertions(+), 77 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index bb81b15..078c581 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 | \
@@ -1777,6 +1776,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 */
@@ -1995,6 +2004,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));
@@ -2003,7 +2013,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;
@@ -5465,6 +5476,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, NULL);
+ break;
default:
PMD_DRV_LOG(ERR, "Request %u is not supported yet",
opcode);
@@ -5474,57 +5491,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, NULL);
-
- i40e_pf_enable_irq0(hw);
- rte_intr_enable(&(dev->pci_dev->intr_handle));
-}
-
/**
* Interrupt handler triggered by NIC for handling
* specific interrupt.
@@ -5582,30 +5548,6 @@ i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
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, NULL);
- }
-
done:
/* Enable interrupt */
i40e_pf_enable_irq0(hw);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH 8/8] net/virtio: remove started field
From: Maxime Coquelin @ 2016-11-04 8:46 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: Ilya Maximets
In-Reply-To: <1478189400-14606-9-git-send-email-yuanhan.liu@linux.intel.com>
On 11/03/2016 05:10 PM, Yuanhan Liu wrote:
> The "hw->started" field was introduced to stop touching queues
> on restart. We never touches queues on restart any more, thus
> it's safe to remove this flag.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 15 ++-------------
> drivers/net/virtio/virtio_pci.h | 1 -
> 2 files changed, 2 insertions(+), 14 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 7/8] net/virtio: complete init stage at the right place
From: Maxime Coquelin @ 2016-11-04 8:44 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: Ilya Maximets
In-Reply-To: <1478189400-14606-8-git-send-email-yuanhan.liu@linux.intel.com>
On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> Invoking vtpci_reinit_complete() at port start stage doesn't make any
> sense, instead, it should be done at the end of dev init stage.
>
> So move it here.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Makes sense:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 6/8] net/virtio: move queue configure code to proper place
From: Maxime Coquelin @ 2016-11-04 8:39 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: Ilya Maximets
In-Reply-To: <1478189400-14606-7-git-send-email-yuanhan.liu@linux.intel.com>
On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> The only piece of code of virtio_dev_rxtx_start() is actually doing
> queue configure/setup work. So, move it to corresponding queue_setup
> callback.
>
> Once that is done, virtio_dev_rxtx_start() becomes an empty function,
> thus it's being removed.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 2 -
> drivers/net/virtio/virtio_ethdev.h | 2 -
> drivers/net/virtio/virtio_rxtx.c | 187 ++++++++++++++++---------------------
> 3 files changed, 79 insertions(+), 112 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 5/8] net/virtio: initiate vring at init stage
From: Maxime Coquelin @ 2016-11-04 8:34 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: Ilya Maximets
In-Reply-To: <1478189400-14606-6-git-send-email-yuanhan.liu@linux.intel.com>
On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> virtio_dev_vring_start() is actually doing the vring initiation job.
> And the vring initiation job should be done at the dev init stage, as
> stated with great details in former commit.
>
> So move it there, and rename it to virtio_init_vring().
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 32 +++++++++++++++++++++++++++++++-
> drivers/net/virtio/virtio_rxtx.c | 32 --------------------------------
> 2 files changed, 31 insertions(+), 33 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 4/8] net/virtio: allocate queue at init stage
From: Maxime Coquelin @ 2016-11-04 8:25 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: Ilya Maximets
In-Reply-To: <1478189400-14606-5-git-send-email-yuanhan.liu@linux.intel.com>
On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> Queue allocation should be done once, since the queue related info (such
> as vring addreess) will only be informed to the vhost-user backend once
> without virtio device reset.
>
> That means, if you allocate queues again after the vhost-user negotiation,
> the vhost-user backend will not be informed any more. Leading to a state
> that the vring info mismatches between virtio PMD driver and vhost-backend:
> the driver switches to the new address has just been allocated, while the
> vhost-backend still sticks to the old address has been assigned in the init
> stage.
>
> Unfortunately, that is exactly how the virtio driver is coded so far: queue
> allocation is done at queue_setup stage (when rte_eth_tx/rx_queue_setup is
> invoked). This is wrong, because queue_setup can be invoked several times.
> For example,
>
> $ start_testpmd.sh ... --txq=1 --rxq=1 ...
> > port stop 0
> > port config all txq 1 # just trigger the queue_setup callback again
> > port config all rxq 1
> > port start 0
>
> The right way to do is allocate the queues in the init stage, so that the
> vring info could be persistent with the vhost-user backend.
>
> Besides that, we should allocate max_queue pairs the device supports, but
> not nr queue pairs firstly configured, to make following case work.
>
> $ start_testpmd.sh ... --txq=1 --rxq=1 ...
> > port stop 0
> > port config all txq 2
> > port config all rxq 2
> > port start 0
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 105 +++++++++++++++++++++++--------------
> drivers/net/virtio/virtio_ethdev.h | 8 ---
> drivers/net/virtio/virtio_pci.h | 2 +
> drivers/net/virtio/virtio_rxtx.c | 38 +++++++-------
> 4 files changed, 85 insertions(+), 68 deletions(-)
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 1/8] net/virtio: revert "virtio: fix restart"
From: Maxime Coquelin @ 2016-11-04 8:10 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: stable, Ilya Maximets
In-Reply-To: <1478189400-14606-2-git-send-email-yuanhan.liu@linux.intel.com>
On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
> manually addressed.
>
> Kyle reported an issue with above commit
>
> qemu-kvm: Guest moved used index from 5 to 1
>
> with following steps,
>
> 1) Start my virtio interfaces
> 2) Send some traffic into/out of the interfaces
> 3) Stop the interfaces
> 4) Start the interfaces
> 5) Send some more traffic
>
> And here are some quotes from Kyle's analysis,
>
> Prior to the patch, if an interface were stopped then started, without
> restarting the application, the queues would be left as-is, because
> hw->started would be set to 1. Now, calling stop sets hw->started to 0,
> which means the next call to start will "touch the queues". This is the
> unintended side-effect that causes the problem.
>
> Fixes: 9a0615af7746 ("virtio: fix restart")
>
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> Cc: <stable@dpdk.org>
> Reported-by: Kyle Larose <klarose@sandvine.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 1/8] net/virtio: revert "virtio: fix restart"
From: Maxime Coquelin @ 2016-11-04 8:09 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, stable, Ilya Maximets
In-Reply-To: <20161104020005.GV16751@yliu-dev.sh.intel.com>
On 11/04/2016 03:00 AM, Yuanhan Liu wrote:
> On Thu, Nov 03, 2016 at 09:36:52PM +0100, Maxime Coquelin wrote:
>> Hi Yuanhan,
>>
>> On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
>>> This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
>>> manually addressed.
>>>
>>> Kyle reported an issue with above commit
>>>
>>> qemu-kvm: Guest moved used index from 5 to 1
>>>
>>> with following steps,
>>>
>>> 1) Start my virtio interfaces
>>> 2) Send some traffic into/out of the interfaces
>>> 3) Stop the interfaces
>>> 4) Start the interfaces
>>> 5) Send some more traffic
>>>
>>> And here are some quotes from Kyle's analysis,
>>>
>>> Prior to the patch, if an interface were stopped then started, without
>>> restarting the application, the queues would be left as-is, because
>>> hw->started would be set to 1. Now, calling stop sets hw->started to 0,
>>> which means the next call to start will "touch the queues". This is the
>>> unintended side-effect that causes the problem.
>>
>> Maybe a good idea to explain what is the problem the revert aims to fix.
>
> It aims to fix the issue, by "not touching the queues" on restart.
>
>> It does not seem to be clearly stated in the commit message.
>
> I was thinking the quote from Kyle is enough. How about following supplement:
>
> We should not touch the queues once the init is done, otherwise, the
> vring state of virtio PMD driver and vhost-user would be inconsistent,
> leading some issue like above.
>
> Thus this patch is reverted.
>
> Better now?
Yes, this is much clearer from my PoV.
Thanks!
Maxime
>
> --yliu
>
^ permalink raw reply
* Re: [PATCH 4/8] net/virtio: allocate queue at init stage
From: Maxime Coquelin @ 2016-11-04 8:08 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, Ilya Maximets
In-Reply-To: <20161104015048.GS16751@yliu-dev.sh.intel.com>
On 11/04/2016 02:50 AM, Yuanhan Liu wrote:
> On Thu, Nov 03, 2016 at 10:11:43PM +0100, Maxime Coquelin wrote:
>>
>>
>> On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
>>> Queue allocation should be done once, since the queue related info (such
>>> as vring addreess) will only be informed to the vhost-user backend once
>>> without virtio device reset.
>>>
>>> That means, if you allocate queues again after the vhost-user negotiation,
>>> the vhost-user backend will not be informed any more. Leading to a state
>>> that the vring info mismatches between virtio PMD driver and vhost-backend:
>>> the driver switches to the new address has just been allocated, while the
>>> vhost-backend still sticks to the old address has been assigned in the init
>>> stage.
>>>
>>> Unfortunately, that is exactly how the virtio driver is coded so far: queue
>>> allocation is done at queue_setup stage (when rte_eth_tx/rx_queue_setup is
>>> invoked). This is wrong, because queue_setup can be invoked several times.
>>> For example,
>>>
>>> $ start_testpmd.sh ... --txq=1 --rxq=1 ...
>>> > port stop 0
>>> > port config all txq 1 # just trigger the queue_setup callback again
>>> > port config all rxq 1
>>> > port start 0
>>>
>>> The right way to do is allocate the queues in the init stage, so that the
>>> vring info could be persistent with the vhost-user backend.
>>>
>>> Besides that, we should allocate max_queue pairs the device supports, but
>>> not nr queue pairs firstly configured, to make following case work.
>> I understand, but how much memory overhead does that represent?
>
> We are allocating max queue pairs the device supports, but not the
> virtio-net spec supports, which, as you stated, would be too much.
Oh, ok. In this case this is good to me.
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Maxime Coquelin @ 2016-11-04 7:59 UTC (permalink / raw)
To: Wang, Zhihong, Yuanhan Liu
Cc: stephen@networkplumber.org, Pierre Pfister (ppfister),
Xie, Huawei, dev@dpdk.org, vkaplans@redhat.com, mst@redhat.com
In-Reply-To: <17d285a9-818c-b060-8969-daccb052dc1f@redhat.com>
On 11/04/2016 08:57 AM, Maxime Coquelin wrote:
> Hi Zhihong,
>
> On 11/04/2016 08:20 AM, Wang, Zhihong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Thursday, November 3, 2016 4:11 PM
>>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
>>> <yuanhan.liu@linux.intel.com>
>>> Cc: stephen@networkplumber.org; Pierre Pfister (ppfister)
>>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>; dev@dpdk.org;
>>> vkaplans@redhat.com; mst@redhat.com
>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>> support
>>> to the TX path
>>>
>>>
>>>
>>> On 11/02/2016 11:51 AM, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 10/31/2016 11:01 AM, Wang, Zhihong wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>>>> Sent: Friday, October 28, 2016 3:42 PM
>>>>>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
>>>>>> <yuanhan.liu@linux.intel.com>
>>>>>> Cc: stephen@networkplumber.org; Pierre Pfister (ppfister)
>>>>>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>;
>>> dev@dpdk.org;
>>>>>> vkaplans@redhat.com; mst@redhat.com
>>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>>>>> support
>>>>>> to the TX path
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 10/28/2016 02:49 AM, Wang, Zhihong wrote:
>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
>>>>>>>>> Sent: Thursday, October 27, 2016 6:46 PM
>>>>>>>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>>> Cc: Wang, Zhihong <zhihong.wang@intel.com>;
>>>>>>>>> stephen@networkplumber.org; Pierre Pfister (ppfister)
>>>>>>>>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>;
>>>>>> dev@dpdk.org;
>>>>>>>>> vkaplans@redhat.com; mst@redhat.com
>>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>>>>> support
>>>>>>>>> to the TX path
>>>>>>>>>
>>>>>>>>> On Thu, Oct 27, 2016 at 12:35:11PM +0200, Maxime Coquelin wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 10/27/2016 12:33 PM, Yuanhan Liu wrote:
>>>>>>>>>>>>> On Thu, Oct 27, 2016 at 11:10:34AM +0200, Maxime Coquelin
>>>>>> wrote:
>>>>>>>>>>>>>>> Hi Zhihong,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
>>>>>>>>>>>>>>>>> Hi Maxime,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Seems indirect desc feature is causing serious
>>> performance
>>>>>>>>>>>>>>>>> degradation on Haswell platform, about 20% drop for both
>>>>>>>>>>>>>>>>> mrg=on and mrg=off (--txqflags=0xf00, non-vector
>>> version),
>>>>>>>>>>>>>>>>> both iofwd and macfwd.
>>>>>>>>>>>>>>> I tested PVP (with macswap on guest) and Txonly/Rxonly on
>>> an
>>>>>> Ivy
>>>>>>>>> Bridge
>>>>>>>>>>>>>>> platform, and didn't faced such a drop.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I was actually wondering that may be the cause. I tested it
>>>>>>>>>>>>> with
>>>>>>>>>>>>> my IvyBridge server as well, I saw no drop.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Maybe you should find a similar platform (Haswell) and have a
>>>>>>>>>>>>> try?
>>>>>>>>>>> Yes, that's why I asked Zhihong whether he could test Txonly in
>>>>>>>>>>> guest
>>>>>> to
>>>>>>>>>>> see if issue is reproducible like this.
>>>>>>>>>
>>>>>>>>> I have no Haswell box, otherwise I could do a quick test for you.
>>>>>>>>> IIRC,
>>>>>>>>> he tried to disable the indirect_desc feature, then the
>>>>>>>>> performance
>>>>>>>>> recovered. So, it's likely the indirect_desc is the culprit here.
>>>>>>>>>
>>>>>>>>>>> I will be easier for me to find an Haswell machine if it has not
>>>>>>>>>>> to be
>>>>>>>>>>> connected back to back to and HW/SW packet generator.
>>>>>>> In fact simple loopback test will also do, without pktgen.
>>>>>>>
>>>>>>> Start testpmd in both host and guest, and do "start" in one
>>>>>>> and "start tx_first 32" in another.
>>>>>>>
>>>>>>> Perf drop is about 24% in my test.
>>>>>>>
>>>>>>
>>>>>> Thanks, I never tried this test.
>>>>>> I managed to find an Haswell platform (Intel(R) Xeon(R) CPU
>>>>>> E5-2699 v3
>>>>>> @ 2.30GHz), and can reproduce the problem with the loop test you
>>>>>> mention. I see a performance drop about 10% (8.94Mpps/8.08Mpps).
>>>>>> Out of curiosity, what are the numbers you get with your setup?
>>>>>
>>>>> Hi Maxime,
>>>>>
>>>>> Let's align our test case to RC2, mrg=on, loopback, on Haswell.
>>>>> My results below:
>>>>> 1. indirect=1: 5.26 Mpps
>>>>> 2. indirect=0: 6.54 Mpps
>>>>>
>>>>> It's about 24% drop.
>>>> OK, so on my side, same setup on Haswell:
>>>> 1. indirect=1: 7.44 Mpps
>>>> 2. indirect=0: 8.18 Mpps
>>>>
>>>> Still 10% drop in my case with mrg=on.
>>>>
>>>> The strange thing with both of our figures is that this is below from
>>>> what I obtain with my SandyBridge machine. The SB cpu freq is 4%
>>>> higher,
>>>> but that doesn't explain the gap between the measurements.
>>>>
>>>> I'm continuing the investigations on my side.
>>>> Maybe we should fix a deadline, and decide do disable indirect in
>>>> Virtio PMD if root cause not identified/fixed at some point?
>>>>
>>>> Yuanhan, what do you think?
>>>
>>> I have done some measurements using perf, and know understand better
>>> what happens.
>>>
>>> With indirect descriptors, I can see a cache miss when fetching the
>>> descriptors in the indirect table. Actually, this is expected, so
>>> we prefetch the first desc as soon as possible, but still not soon
>>> enough to make it transparent.
>>> In direct descriptors case, the desc in the virtqueue seems to be
>>> remain in the cache from its previous use, so we have a hit.
>>>
>>> That said, in realistic use-case, I think we should not have a hit,
>>> even with direct descriptors.
>>> Indeed, the test case use testpmd on guest side with the forwarding set
>>> in IO mode. It means the packet content is never accessed by the guest.
>>>
>>> In my experiments, I am used to set the "macswap" forwarding mode, which
>>> swaps src and dest MAC addresses in the packet. I find it more
>>> realistic, because I don't see the point in sending packets to the guest
>>> if it is not accessed (not even its header).
>>>
>>> I tried again the test case, this time with setting the forwarding mode
>>> to macswap in the guest. This time, I get same performance with both
>>> direct and indirect (indirect even a little better with a small
>>> optimization, consisting in prefetching the 2 first descs
>>> systematically as we know there are contiguous).
>>
>>
>> Hi Maxime,
>>
>> I did a little more macswap test and found out more stuff here:
> Thanks for doing more tests.
>
>>
>> 1. I did loopback test on another HSW machine with the same H/W,
>> and indirect_desc on and off seems have close perf
>>
>> 2. So I checked the gcc version:
>>
>> * Previous: gcc version 6.2.1 20160916 (Fedora 24)
>>
>> * New: gcc version 5.4.0 20160609 (Ubuntu 16.04.1 LTS)
>
> On my side, I tested with RHEL7.3:
> - gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)
>
> It certainly contains some backports from newer GCC versions.
>
>>
>> On previous one indirect_desc has 20% drop
>>
>> 3. Then I compiled binary on Ubuntu and scp to Fedora, and as
>> expected I got the same perf as on Ubuntu, and the perf gap
>> disappeared, so gcc is definitely one factor here
>>
>> 4. Then I use the Ubuntu binary on Fedora for PVP test, then the
>> perf gap comes back again and the same with the Fedora binary
>> results, indirect_desc causes about 20% drop
>
> Let me know if I understand correctly:
> Loopback test with macswap:
> - gcc version 6.2.1 : 20% perf drop
> - gcc version 5.4.0 : No drop
>
> PVP test with macswap:
> - gcc version 6.2.1 : 20% perf drop
> - gcc version 5.4.0 : 20% perf drop
I forgot to ask, did you recompile only host, or both host and guest
testmpd's in your test?
>
>>
>> So in all, could you try PVP traffic on HSW to see how it works?
> Sadly, the HSW machine I borrowed does not have other device connected
> back to back on its 10G port. I can only test PVP with SNB machines
> currently.
>
>>
>>
>>>
>>> Do you agree we should assume that the packet (header or/and buf) will
>>> always be accessed by the guest application?
>>> If so, do you agree we should keep indirect descs enabled, and maybe
>>> update the test cases?
>>
>>
>> I agree with you that mac/macswap test is more realistic and makes
>> more sense for real applications.
>
> Thanks,
> Maxime
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Maxime Coquelin @ 2016-11-04 7:57 UTC (permalink / raw)
To: Wang, Zhihong, Yuanhan Liu
Cc: stephen@networkplumber.org, Pierre Pfister (ppfister),
Xie, Huawei, dev@dpdk.org, vkaplans@redhat.com, mst@redhat.com
In-Reply-To: <8F6C2BD409508844A0EFC19955BE09414E7DC40F@SHSMSX103.ccr.corp.intel.com>
Hi Zhihong,
On 11/04/2016 08:20 AM, Wang, Zhihong wrote:
>
>
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Thursday, November 3, 2016 4:11 PM
>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
>> <yuanhan.liu@linux.intel.com>
>> Cc: stephen@networkplumber.org; Pierre Pfister (ppfister)
>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>; dev@dpdk.org;
>> vkaplans@redhat.com; mst@redhat.com
>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support
>> to the TX path
>>
>>
>>
>> On 11/02/2016 11:51 AM, Maxime Coquelin wrote:
>>>
>>>
>>> On 10/31/2016 11:01 AM, Wang, Zhihong wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>>> Sent: Friday, October 28, 2016 3:42 PM
>>>>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
>>>>> <yuanhan.liu@linux.intel.com>
>>>>> Cc: stephen@networkplumber.org; Pierre Pfister (ppfister)
>>>>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>;
>> dev@dpdk.org;
>>>>> vkaplans@redhat.com; mst@redhat.com
>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>>>> support
>>>>> to the TX path
>>>>>
>>>>>
>>>>>
>>>>> On 10/28/2016 02:49 AM, Wang, Zhihong wrote:
>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
>>>>>>>> Sent: Thursday, October 27, 2016 6:46 PM
>>>>>>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> Cc: Wang, Zhihong <zhihong.wang@intel.com>;
>>>>>>>> stephen@networkplumber.org; Pierre Pfister (ppfister)
>>>>>>>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>;
>>>>> dev@dpdk.org;
>>>>>>>> vkaplans@redhat.com; mst@redhat.com
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>>>> support
>>>>>>>> to the TX path
>>>>>>>>
>>>>>>>> On Thu, Oct 27, 2016 at 12:35:11PM +0200, Maxime Coquelin wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 10/27/2016 12:33 PM, Yuanhan Liu wrote:
>>>>>>>>>>>> On Thu, Oct 27, 2016 at 11:10:34AM +0200, Maxime Coquelin
>>>>> wrote:
>>>>>>>>>>>>>> Hi Zhihong,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
>>>>>>>>>>>>>>>> Hi Maxime,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Seems indirect desc feature is causing serious
>> performance
>>>>>>>>>>>>>>>> degradation on Haswell platform, about 20% drop for both
>>>>>>>>>>>>>>>> mrg=on and mrg=off (--txqflags=0xf00, non-vector
>> version),
>>>>>>>>>>>>>>>> both iofwd and macfwd.
>>>>>>>>>>>>>> I tested PVP (with macswap on guest) and Txonly/Rxonly on
>> an
>>>>> Ivy
>>>>>>>> Bridge
>>>>>>>>>>>>>> platform, and didn't faced such a drop.
>>>>>>>>>>>>
>>>>>>>>>>>> I was actually wondering that may be the cause. I tested it with
>>>>>>>>>>>> my IvyBridge server as well, I saw no drop.
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe you should find a similar platform (Haswell) and have a
>>>>>>>>>>>> try?
>>>>>>>>>> Yes, that's why I asked Zhihong whether he could test Txonly in
>>>>>>>>>> guest
>>>>> to
>>>>>>>>>> see if issue is reproducible like this.
>>>>>>>>
>>>>>>>> I have no Haswell box, otherwise I could do a quick test for you.
>>>>>>>> IIRC,
>>>>>>>> he tried to disable the indirect_desc feature, then the performance
>>>>>>>> recovered. So, it's likely the indirect_desc is the culprit here.
>>>>>>>>
>>>>>>>>>> I will be easier for me to find an Haswell machine if it has not
>>>>>>>>>> to be
>>>>>>>>>> connected back to back to and HW/SW packet generator.
>>>>>> In fact simple loopback test will also do, without pktgen.
>>>>>>
>>>>>> Start testpmd in both host and guest, and do "start" in one
>>>>>> and "start tx_first 32" in another.
>>>>>>
>>>>>> Perf drop is about 24% in my test.
>>>>>>
>>>>>
>>>>> Thanks, I never tried this test.
>>>>> I managed to find an Haswell platform (Intel(R) Xeon(R) CPU E5-2699 v3
>>>>> @ 2.30GHz), and can reproduce the problem with the loop test you
>>>>> mention. I see a performance drop about 10% (8.94Mpps/8.08Mpps).
>>>>> Out of curiosity, what are the numbers you get with your setup?
>>>>
>>>> Hi Maxime,
>>>>
>>>> Let's align our test case to RC2, mrg=on, loopback, on Haswell.
>>>> My results below:
>>>> 1. indirect=1: 5.26 Mpps
>>>> 2. indirect=0: 6.54 Mpps
>>>>
>>>> It's about 24% drop.
>>> OK, so on my side, same setup on Haswell:
>>> 1. indirect=1: 7.44 Mpps
>>> 2. indirect=0: 8.18 Mpps
>>>
>>> Still 10% drop in my case with mrg=on.
>>>
>>> The strange thing with both of our figures is that this is below from
>>> what I obtain with my SandyBridge machine. The SB cpu freq is 4% higher,
>>> but that doesn't explain the gap between the measurements.
>>>
>>> I'm continuing the investigations on my side.
>>> Maybe we should fix a deadline, and decide do disable indirect in
>>> Virtio PMD if root cause not identified/fixed at some point?
>>>
>>> Yuanhan, what do you think?
>>
>> I have done some measurements using perf, and know understand better
>> what happens.
>>
>> With indirect descriptors, I can see a cache miss when fetching the
>> descriptors in the indirect table. Actually, this is expected, so
>> we prefetch the first desc as soon as possible, but still not soon
>> enough to make it transparent.
>> In direct descriptors case, the desc in the virtqueue seems to be
>> remain in the cache from its previous use, so we have a hit.
>>
>> That said, in realistic use-case, I think we should not have a hit,
>> even with direct descriptors.
>> Indeed, the test case use testpmd on guest side with the forwarding set
>> in IO mode. It means the packet content is never accessed by the guest.
>>
>> In my experiments, I am used to set the "macswap" forwarding mode, which
>> swaps src and dest MAC addresses in the packet. I find it more
>> realistic, because I don't see the point in sending packets to the guest
>> if it is not accessed (not even its header).
>>
>> I tried again the test case, this time with setting the forwarding mode
>> to macswap in the guest. This time, I get same performance with both
>> direct and indirect (indirect even a little better with a small
>> optimization, consisting in prefetching the 2 first descs
>> systematically as we know there are contiguous).
>
>
> Hi Maxime,
>
> I did a little more macswap test and found out more stuff here:
Thanks for doing more tests.
>
> 1. I did loopback test on another HSW machine with the same H/W,
> and indirect_desc on and off seems have close perf
>
> 2. So I checked the gcc version:
>
> * Previous: gcc version 6.2.1 20160916 (Fedora 24)
>
> * New: gcc version 5.4.0 20160609 (Ubuntu 16.04.1 LTS)
On my side, I tested with RHEL7.3:
- gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)
It certainly contains some backports from newer GCC versions.
>
> On previous one indirect_desc has 20% drop
>
> 3. Then I compiled binary on Ubuntu and scp to Fedora, and as
> expected I got the same perf as on Ubuntu, and the perf gap
> disappeared, so gcc is definitely one factor here
>
> 4. Then I use the Ubuntu binary on Fedora for PVP test, then the
> perf gap comes back again and the same with the Fedora binary
> results, indirect_desc causes about 20% drop
Let me know if I understand correctly:
Loopback test with macswap:
- gcc version 6.2.1 : 20% perf drop
- gcc version 5.4.0 : No drop
PVP test with macswap:
- gcc version 6.2.1 : 20% perf drop
- gcc version 5.4.0 : 20% perf drop
>
> So in all, could you try PVP traffic on HSW to see how it works?
Sadly, the HSW machine I borrowed does not have other device connected
back to back on its 10G port. I can only test PVP with SNB machines
currently.
>
>
>>
>> Do you agree we should assume that the packet (header or/and buf) will
>> always be accessed by the guest application?
>> If so, do you agree we should keep indirect descs enabled, and maybe
>> update the test cases?
>
>
> I agree with you that mac/macswap test is more realistic and makes
> more sense for real applications.
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Maxime Coquelin @ 2016-11-04 7:41 UTC (permalink / raw)
To: Xu, Qian Q, Wang, Zhihong, Yuanhan Liu
Cc: mst@redhat.com, dev@dpdk.org, vkaplans@redhat.com
In-Reply-To: <82F45D86ADE5454A95A89742C8D1410E3923865A@shsmsx102.ccr.corp.intel.com>
Hi,
On 11/04/2016 07:18 AM, Xu, Qian Q wrote:
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Thursday, November 3, 2016 4:11 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Cc: mst@redhat.com; dev@dpdk.org; vkaplans@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support to the TX path
>
>
>>
>> The strange thing with both of our figures is that this is below from
>> what I obtain with my SandyBridge machine. The SB cpu freq is 4%
>> higher, but that doesn't explain the gap between the measurements.
>>
>> I'm continuing the investigations on my side.
>> Maybe we should fix a deadline, and decide do disable indirect in
>> Virtio PMD if root cause not identified/fixed at some point?
>>
>> Yuanhan, what do you think?
>
> I have done some measurements using perf, and know understand better what happens.
>
> With indirect descriptors, I can see a cache miss when fetching the descriptors in the indirect table. Actually, this is expected, so we prefetch the first desc as soon as possible, but still not soon enough to make it transparent.
> In direct descriptors case, the desc in the virtqueue seems to be remain in the cache from its previous use, so we have a hit.
>
> That said, in realistic use-case, I think we should not have a hit, even with direct descriptors.
> Indeed, the test case use testpmd on guest side with the forwarding set in IO mode. It means the packet content is never accessed by the guest.
>
> In my experiments, I am used to set the "macswap" forwarding mode, which swaps src and dest MAC addresses in the packet. I find it more realistic, because I don't see the point in sending packets to the guest if it is not accessed (not even its header).
>
> I tried again the test case, this time with setting the forwarding mode to macswap in the guest. This time, I get same performance with both direct and indirect (indirect even a little better with a small optimization, consisting in prefetching the 2 first descs systematically as we know there are contiguous).
>
> Do you agree we should assume that the packet (header or/and buf) will always be accessed by the guest application?
> ----Maybe it's true in many real use case. But we also need ensure the performance for "io fwd" has no performance drop. As I know, OVS-DPDK team will do the performance benchmark based on "IO fwd" for virtio part, so they will also see some performance drop. And we just thought if it's possible to make the feature default off then if someone wanted to use it can turn it on. People can choose if they want to use the feature, just like vhost dequeue zero copy.
OVS adds an overhead compared to testpmd on host, and its cache
utilization might have the same effect as doing macswap.
Do you know who could test with OVS? I would be interested in the
results.
And the difference today with zero-copy is that it can be enabled at
runtime, whereas we can only do it at build time on guest side for
indirect.
It can be disabled in QEMU command line though.
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 2/2] arch/ppc: fix file descriptors leakage when getting CPU features
From: viktorin @ 2016-11-04 7:26 UTC (permalink / raw)
To: Jianbo Liu; +Cc: dev, chaozhu
In-Reply-To: <1478240949-13487-2-git-send-email-jianbo.liu@linaro.org>
Please, add the Fixes tag.
On Fri, 4 Nov 2016 11:59:09 +0530
Jianbo Liu <jianbo.liu@linaro.org> wrote:
> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
Acked-by: Jan Viktorin <viktorin@rehivetech.com>
^ permalink raw reply
* Re: [PATCH 1/2] arch/arm: fix file descriptors leakage when getting CPU features
From: viktorin @ 2016-11-04 7:24 UTC (permalink / raw)
To: Jianbo Liu; +Cc: dev, chaozhu
In-Reply-To: <1478240949-13487-1-git-send-email-jianbo.liu@linaro.org>
Hello Jianbo Liu,
thank you, a good catch!
Can you please git blame for the commit introducing the issue and add
the "Fixes:" tag as described in [1]?
Same for ppc.
Regards
Jan
[1] http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body
On Fri, 4 Nov 2016 11:59:08 +0530
Jianbo Liu <jianbo.liu@linaro.org> wrote:
> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
Acked-by: Jan Viktorin <viktorin@rehivetech.com>
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Wang, Zhihong @ 2016-11-04 7:20 UTC (permalink / raw)
To: Maxime Coquelin, Yuanhan Liu
Cc: stephen@networkplumber.org, Pierre Pfister (ppfister),
Xie, Huawei, dev@dpdk.org, vkaplans@redhat.com, mst@redhat.com
In-Reply-To: <eb2dc51c-8ada-cca4-0a51-7c50fd25361e@redhat.com>
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, November 3, 2016 4:11 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: stephen@networkplumber.org; Pierre Pfister (ppfister)
> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>; dev@dpdk.org;
> vkaplans@redhat.com; mst@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support
> to the TX path
>
>
>
> On 11/02/2016 11:51 AM, Maxime Coquelin wrote:
> >
> >
> > On 10/31/2016 11:01 AM, Wang, Zhihong wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >>> Sent: Friday, October 28, 2016 3:42 PM
> >>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
> >>> <yuanhan.liu@linux.intel.com>
> >>> Cc: stephen@networkplumber.org; Pierre Pfister (ppfister)
> >>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>;
> dev@dpdk.org;
> >>> vkaplans@redhat.com; mst@redhat.com
> >>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
> >>> support
> >>> to the TX path
> >>>
> >>>
> >>>
> >>> On 10/28/2016 02:49 AM, Wang, Zhihong wrote:
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> >>>>>> Sent: Thursday, October 27, 2016 6:46 PM
> >>>>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>> Cc: Wang, Zhihong <zhihong.wang@intel.com>;
> >>>>>> stephen@networkplumber.org; Pierre Pfister (ppfister)
> >>>>>> <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>;
> >>> dev@dpdk.org;
> >>>>>> vkaplans@redhat.com; mst@redhat.com
> >>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
> >>> support
> >>>>>> to the TX path
> >>>>>>
> >>>>>> On Thu, Oct 27, 2016 at 12:35:11PM +0200, Maxime Coquelin wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 10/27/2016 12:33 PM, Yuanhan Liu wrote:
> >>>>>>>>>> On Thu, Oct 27, 2016 at 11:10:34AM +0200, Maxime Coquelin
> >>> wrote:
> >>>>>>>>>>>> Hi Zhihong,
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
> >>>>>>>>>>>>>> Hi Maxime,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Seems indirect desc feature is causing serious
> performance
> >>>>>>>>>>>>>> degradation on Haswell platform, about 20% drop for both
> >>>>>>>>>>>>>> mrg=on and mrg=off (--txqflags=0xf00, non-vector
> version),
> >>>>>>>>>>>>>> both iofwd and macfwd.
> >>>>>>>>>>>> I tested PVP (with macswap on guest) and Txonly/Rxonly on
> an
> >>> Ivy
> >>>>>> Bridge
> >>>>>>>>>>>> platform, and didn't faced such a drop.
> >>>>>>>>>>
> >>>>>>>>>> I was actually wondering that may be the cause. I tested it with
> >>>>>>>>>> my IvyBridge server as well, I saw no drop.
> >>>>>>>>>>
> >>>>>>>>>> Maybe you should find a similar platform (Haswell) and have a
> >>>>>>>>>> try?
> >>>>>>>> Yes, that's why I asked Zhihong whether he could test Txonly in
> >>>>>>>> guest
> >>> to
> >>>>>>>> see if issue is reproducible like this.
> >>>>>>
> >>>>>> I have no Haswell box, otherwise I could do a quick test for you.
> >>>>>> IIRC,
> >>>>>> he tried to disable the indirect_desc feature, then the performance
> >>>>>> recovered. So, it's likely the indirect_desc is the culprit here.
> >>>>>>
> >>>>>>>> I will be easier for me to find an Haswell machine if it has not
> >>>>>>>> to be
> >>>>>>>> connected back to back to and HW/SW packet generator.
> >>>> In fact simple loopback test will also do, without pktgen.
> >>>>
> >>>> Start testpmd in both host and guest, and do "start" in one
> >>>> and "start tx_first 32" in another.
> >>>>
> >>>> Perf drop is about 24% in my test.
> >>>>
> >>>
> >>> Thanks, I never tried this test.
> >>> I managed to find an Haswell platform (Intel(R) Xeon(R) CPU E5-2699 v3
> >>> @ 2.30GHz), and can reproduce the problem with the loop test you
> >>> mention. I see a performance drop about 10% (8.94Mpps/8.08Mpps).
> >>> Out of curiosity, what are the numbers you get with your setup?
> >>
> >> Hi Maxime,
> >>
> >> Let's align our test case to RC2, mrg=on, loopback, on Haswell.
> >> My results below:
> >> 1. indirect=1: 5.26 Mpps
> >> 2. indirect=0: 6.54 Mpps
> >>
> >> It's about 24% drop.
> > OK, so on my side, same setup on Haswell:
> > 1. indirect=1: 7.44 Mpps
> > 2. indirect=0: 8.18 Mpps
> >
> > Still 10% drop in my case with mrg=on.
> >
> > The strange thing with both of our figures is that this is below from
> > what I obtain with my SandyBridge machine. The SB cpu freq is 4% higher,
> > but that doesn't explain the gap between the measurements.
> >
> > I'm continuing the investigations on my side.
> > Maybe we should fix a deadline, and decide do disable indirect in
> > Virtio PMD if root cause not identified/fixed at some point?
> >
> > Yuanhan, what do you think?
>
> I have done some measurements using perf, and know understand better
> what happens.
>
> With indirect descriptors, I can see a cache miss when fetching the
> descriptors in the indirect table. Actually, this is expected, so
> we prefetch the first desc as soon as possible, but still not soon
> enough to make it transparent.
> In direct descriptors case, the desc in the virtqueue seems to be
> remain in the cache from its previous use, so we have a hit.
>
> That said, in realistic use-case, I think we should not have a hit,
> even with direct descriptors.
> Indeed, the test case use testpmd on guest side with the forwarding set
> in IO mode. It means the packet content is never accessed by the guest.
>
> In my experiments, I am used to set the "macswap" forwarding mode, which
> swaps src and dest MAC addresses in the packet. I find it more
> realistic, because I don't see the point in sending packets to the guest
> if it is not accessed (not even its header).
>
> I tried again the test case, this time with setting the forwarding mode
> to macswap in the guest. This time, I get same performance with both
> direct and indirect (indirect even a little better with a small
> optimization, consisting in prefetching the 2 first descs
> systematically as we know there are contiguous).
Hi Maxime,
I did a little more macswap test and found out more stuff here:
1. I did loopback test on another HSW machine with the same H/W,
and indirect_desc on and off seems have close perf
2. So I checked the gcc version:
* Previous: gcc version 6.2.1 20160916 (Fedora 24)
* New: gcc version 5.4.0 20160609 (Ubuntu 16.04.1 LTS)
On previous one indirect_desc has 20% drop
3. Then I compiled binary on Ubuntu and scp to Fedora, and as
expected I got the same perf as on Ubuntu, and the perf gap
disappeared, so gcc is definitely one factor here
4. Then I use the Ubuntu binary on Fedora for PVP test, then the
perf gap comes back again and the same with the Fedora binary
results, indirect_desc causes about 20% drop
So in all, could you try PVP traffic on HSW to see how it works?
>
> Do you agree we should assume that the packet (header or/and buf) will
> always be accessed by the guest application?
> If so, do you agree we should keep indirect descs enabled, and maybe
> update the test cases?
I agree with you that mac/macswap test is more realistic and makes
more sense for real applications.
Thanks
Zhihong
>
> Thanks,
> Maxime
^ permalink raw reply
* Re: [PATCH v2] scripts: show fixes with release version of bug
From: Yuanhan Liu @ 2016-11-04 6:58 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
In-Reply-To: <1475245122-23719-1-git-send-email-thomas.monjalon@6wind.com>
On Fri, Sep 30, 2016 at 04:18:42PM +0200, Thomas Monjalon wrote:
> This script can help to find commits to backport in stable branches.
>
> Fixes are found if there is the word "fix" in the headline or
> if there is a tag Fixes: or Reverts: in the message.
> Chained fixes of fixes are explored to find the oldest origin.
> Fixes of not released bugs are ignored.
Thomas, that saves my life! It helps a lot when I'm trying to pick
commits for 16.07.2. So,
Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Thanks for the patch (and sorry for late reply).
--yliu
^ permalink raw reply
* Re: dpdk16.11 RC2 package ipv4 reassembly example can't work
From: Lu, Wenzhuo @ 2016-11-04 6:36 UTC (permalink / raw)
To: Adrien Mazarguil
Cc: Ananyev, Konstantin, Liu, Yu Y, Chen, WeichunX, Xu, HuilongX,
dev@dpdk.org
In-Reply-To: <20161102152111.GD5733@6wind.com>
Hi Adrien,
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, November 2, 2016 11:21 PM
> To: Lu, Wenzhuo
> Cc: Ananyev, Konstantin; Liu, Yu Y; Chen, WeichunX; Xu, HuilongX;
> dev@dpdk.org
> Subject: Re: dpdk16.11 RC2 package ipv4 reassembly example can't work
>
> Hi all,
>
> On Wed, Nov 02, 2016 at 08:39:31AM +0000, Lu, Wenzhuo wrote:
> > Correct the typo of receiver.
> >
> > Hi Adrien,
> > The change from struct ip_frag_pkt pkt[0] to struct ip_frag_pkt pkt[] will
> make IP reassembly not working. I think this is not the root cause. Maybe
> Konstantin can give us some idea.
> > But I notice one thing, you change some from [0] to [], but others just add
> '__extension__'. I believe if you add '__extension__' for struct ip_frag_pkt pkt[0],
> we'll not hit this issue. Just curious why you use 2 ways to resolve the same
> problem.
>
> I've used the __extension__ method whenever the C99 syntax could not work
> due to invalid usage in the code, e.g. a flexible array cannot be the only member
> of a struct, you cannot make arrays out of structures that contain such fields,
> while there is no such constraint with the GNU syntax.
>
> For example see __extension__ uint8_t action_data[0] in struct
> rte_pipeline_table_entry. The C99 could not be used because of
> test_table_acl.c:
>
> struct rte_pipeline_table_entry entries[5];
>
> If replacing ip_frag_pkt[] with __extension__ ip_frag_pkt pkt[0] in rte_ip_frag.h
> solves the issue, either some code is breaking some constraint somewhere or
> this change broke the ABI (unlikely considering a simple recompilation should
> have taken care of the issue). I did not notice any change in sizeof(struct
> rte_ip_frag_tbl) nor offsetof(struct rte_ip_frag_tbl, pkt) on my setup, perhaps
> the compilation flags used in your test affect them somehow.
Thanks for your explanation. I also checked sizeof(struct rte_ip_frag_tbl). I don't see any change either.
>
> Can you confirm whether only reverting this particular field solves the issue?
Yes. ip_frag_pkt pkt[0] or even ip_frag_pkt pkt[1] can work but ip_frag_pkt pkt[] cannot :(
Do you like the idea of changing the ip_frag_pkt[] to __extension__ ip_frag_pkt pkt[0]?
>
> > From: Xu, HuilongX
> > Sent: Wednesday, November 2, 2016 4:29 PM
> > To: drien.mazarguil@6wind.com
> > Cc: Ananyev, Konstantin; Liu, Yu Y; Chen, WeichunX; Lu, Wenzhuo; Xu,
> > HuilongX
> > Subject: dpdk16.11 RC2 package ipv4 reassembly example can't work
> >
> > Hi mazarguil,
> > I find ip reassembly example can't work with dpdk16.11 rc2 package.
> > But when I reset dpdk code before
> 347a1e037fd323e6c2af55d17f7f0dc4bfe1d479, it works ok.
> > Could you have time to check this issue, thanks a lot.
> > Unzip password: intel123
> >
> > Test detail info:
> >
> > os&kernel:4.2.3-300.fc23.x86_64
> > gcc version:5.3.1 20160406 (Red Hat 5.3.1-6) (GCC)
> > NIC:03:00.0 Ethernet controller [0200]: Intel Corporation Ethernet
> > Connection X552/X557-AT 10GBASE-T [8086:15ad] and
> > 84:00.0 Ethernet controller [0200]: Intel Corporation 82599ES
> > 10-Gigabit SFI/SFP+ Network Connection [8086:10fb] (rev 01)
> > package: dpdk16.11.rc2.tar.gz
> > test steps:
> > 1. build and install dpdk
> > 2. build ip_reassembly example
> > 3. run ip_reassembly
> > ./examples/ip_reassembly/build/ip_reassembly -c 0x2 -n 4 - -p 0x1
> > --maxflows=1024 --flowttl=10s 4. set tester port mtu ip link set mtu
> > 9000 dev ens160f1 5. setup scapy on tester and send packet scapy pcap
> > = rdpcap("file.pcap") sendp(pcap, iface="ens160f1") 6. sniff packet on
> > tester and check packet test result:
> > dpdk16.04 reassembly packet successful but dpdk16.11 reassembly pack failed.
> >
> > comments:
> > file.pcap: send packets pcap file
> > tcpdump_16.04_reassembly_successful.pcap: sniff packets by tcpdump on
> 16.04.
> > tcpdump_reset_code_reassembly_failed.pcap: sniff packets by tcpdump on
> > 16.11
> > reset_code_reassembly_successful_.jpg: reassembly a packets successful
> > detail info
> > dpdk16.11_reassembly_failed.jpg: reassembly a packets failed detail
> > info
> >
>
> --
> Adrien Mazarguil
> 6WIND
^ permalink raw reply
* [PATCH 2/2] arch/ppc: fix file descriptors leakage when getting CPU features
From: Jianbo Liu @ 2016-11-04 6:29 UTC (permalink / raw)
To: dev, chaozhu, viktorin; +Cc: Jianbo Liu
In-Reply-To: <1478240949-13487-1-git-send-email-jianbo.liu@linaro.org>
Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
---
lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
index a8147c8..fcf96e0 100644
--- a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
+++ b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
@@ -116,6 +116,7 @@ rte_cpu_get_features(hwcap_registers_t out)
else if (auxv.a_type == AT_HWCAP2)
out[REG_HWCAP2] = auxv.a_un.a_val;
}
+ close(auxv_fd);
}
/*
--
2.4.11
^ permalink raw reply related
* [PATCH 1/2] arch/arm: fix file descriptors leakage when getting CPU features
From: Jianbo Liu @ 2016-11-04 6:29 UTC (permalink / raw)
To: dev, chaozhu, viktorin; +Cc: Jianbo Liu
Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
---
lib/librte_eal/common/arch/arm/rte_cpuflags.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_eal/common/arch/arm/rte_cpuflags.c b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
index 23240ef..79160a6 100644
--- a/lib/librte_eal/common/arch/arm/rte_cpuflags.c
+++ b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
@@ -148,6 +148,7 @@ rte_cpu_get_features(hwcap_registers_t out)
out[REG_PLATFORM] = 0x0001;
}
}
+ close(auxv_fd);
}
/*
--
2.4.11
^ permalink raw reply related
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Xu, Qian Q @ 2016-11-04 6:18 UTC (permalink / raw)
To: Maxime Coquelin, Wang, Zhihong, Yuanhan Liu
Cc: mst@redhat.com, dev@dpdk.org, vkaplans@redhat.com
In-Reply-To: <eb2dc51c-8ada-cca4-0a51-7c50fd25361e@redhat.com>
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
Sent: Thursday, November 3, 2016 4:11 PM
To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: mst@redhat.com; dev@dpdk.org; vkaplans@redhat.com
Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support to the TX path
>
> The strange thing with both of our figures is that this is below from
> what I obtain with my SandyBridge machine. The SB cpu freq is 4%
> higher, but that doesn't explain the gap between the measurements.
>
> I'm continuing the investigations on my side.
> Maybe we should fix a deadline, and decide do disable indirect in
> Virtio PMD if root cause not identified/fixed at some point?
>
> Yuanhan, what do you think?
I have done some measurements using perf, and know understand better what happens.
With indirect descriptors, I can see a cache miss when fetching the descriptors in the indirect table. Actually, this is expected, so we prefetch the first desc as soon as possible, but still not soon enough to make it transparent.
In direct descriptors case, the desc in the virtqueue seems to be remain in the cache from its previous use, so we have a hit.
That said, in realistic use-case, I think we should not have a hit, even with direct descriptors.
Indeed, the test case use testpmd on guest side with the forwarding set in IO mode. It means the packet content is never accessed by the guest.
In my experiments, I am used to set the "macswap" forwarding mode, which swaps src and dest MAC addresses in the packet. I find it more realistic, because I don't see the point in sending packets to the guest if it is not accessed (not even its header).
I tried again the test case, this time with setting the forwarding mode to macswap in the guest. This time, I get same performance with both direct and indirect (indirect even a little better with a small optimization, consisting in prefetching the 2 first descs systematically as we know there are contiguous).
Do you agree we should assume that the packet (header or/and buf) will always be accessed by the guest application?
----Maybe it's true in many real use case. But we also need ensure the performance for "io fwd" has no performance drop. As I know, OVS-DPDK team will do the performance benchmark based on "IO fwd" for virtio part, so they will also see some performance drop. And we just thought if it's possible to make the feature default off then if someone wanted to use it can turn it on. People can choose if they want to use the feature, just like vhost dequeue zero copy.
If so, do you agree we should keep indirect descs enabled, and maybe update the test cases?
Thanks,
Maxime
^ permalink raw reply
* Re: Best Practices for PMD Verification before Upstream Requests
From: Remy Horton @ 2016-11-04 5:41 UTC (permalink / raw)
To: Shepard Siegel, Thomas Monjalon; +Cc: dev
In-Reply-To: <CAMMLSKAr+drh0NDimAoZN=9oT2zmnVsthnwhD5tEMY2kE6iMGw@mail.gmail.com>
On 02/11/2016 20:21, Shepard Siegel wrote:
[..]
> Almost a year into our DPDK development, we have shipped an alpha version
> of our "Arkville" product. We've thankful for all the support from this
> Most everyone has suggested "get your code upstream ASAP"; but our
> team is cut from the "if it isn't tested, it doesn't work" cloth. We now
> have some solid miles on our Arkville PMD driver "ark" with 16.07. Mostly
> testpmd and a suite of user apps; dts not so much, only because our use
> case is a little different.
To me that sounds good enough for a v1 patch.
> One question that
> came up is "Should we do a thorough port and regression against 16.11 as a
> precursor to up streaming at 17.02?". Constructive feedback always welcome!
It is helpful, although bear in mind there is only so much you'll be
able to test. Patches sent to the mailing list are picked up by
patchwork for automated testing, so you'll find out quite quickly if
you've broken something.
And avoid top-posting.. :)
..Remy
^ 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