Hi Matan,
On Mon, Jul 31, 2017 at 02:15:09PM +0300, Matan Azrad wrote:
> Current mlx4 OFED version has bug which returns error to
> ibv destroy functions when the device was plugged out, in
> spite of the resources were destroyed correctly.
>
> Hence, failsafe PMD was aborted, only in debug mode, when
> it tries to remove the device in plug-out process.
>
> The workaround removed the ibv destroy assertions.
>
> DPDK 18.02 release should work with OFED-4.2 which will
> include the verbs fix to this bug, then, this patch will
> be removed.
>
> Signed-off-by: Matan Azrad <ma...@mellanox.com>
> Cc: sta...@dpdk.org
Since this workaround is needed in order to validate hot-plug with mlx4
compiled in debug mode due to a problem in Verbs, I don't think
sta...@dpdk.org should be involved.
What will be back-ported, once fixed, is the minimum OFED version to install
to properly benefit from hot-plug functionality.
More comments about the patch below.
> ---
> drivers/net/mlx4/mlx4.c | 70
> +++++++++++++++++++++++++++++++++++---------
> drivers/net/mlx4/mlx4_flow.c | 22 ++++++++++----
> 2 files changed, 73 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index 8451f5b..94782c2 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -955,7 +955,10 @@ struct rxq *
> return 0;
> error:
> if (mr_linear != NULL)
> - claim_zero(ibv_dereg_mr(mr_linear));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dereg_mr(mr_linear);
>
> rte_free(elts_linear);
> rte_free(elts);
> @@ -992,7 +995,10 @@ struct rxq *
> txq->elts_linear = NULL;
> txq->mr_linear = NULL;
> if (mr_linear != NULL)
> - claim_zero(ibv_dereg_mr(mr_linear));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dereg_mr(mr_linear);
>
> rte_free(elts_linear);
> if (elts == NULL)
> @@ -1052,9 +1058,15 @@ struct rxq *
> ¶ms));
> }
> if (txq->qp != NULL)
> - claim_zero(ibv_destroy_qp(txq->qp));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_qp(txq->qp);
> if (txq->cq != NULL)
> - claim_zero(ibv_destroy_cq(txq->cq));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_cq(txq->cq);
> if (txq->rd != NULL) {
> struct ibv_exp_destroy_res_domain_attr attr = {
> .comp_mask = 0,
> @@ -1070,7 +1082,10 @@ struct rxq *
> if (txq->mp2mr[i].mp == NULL)
> break;
> assert(txq->mp2mr[i].mr != NULL);
> - claim_zero(ibv_dereg_mr(txq->mp2mr[i].mr));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dereg_mr(txq->mp2mr[i].mr);
> }
> memset(txq, 0, sizeof(*txq));
> }
> @@ -1302,7 +1317,10 @@ static struct ibv_mr *mlx4_mp2mr(struct ibv_pd *,
> struct rte_mempool *)
> DEBUG("%p: MR <-> MP table full, dropping oldest entry.",
> (void *)txq);
> --i;
> - claim_zero(ibv_dereg_mr(txq->mp2mr[0].mr));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dereg_mr(txq->mp2mr[0].mr);
> memmove(&txq->mp2mr[0], &txq->mp2mr[1],
> (sizeof(txq->mp2mr) - sizeof(txq->mp2mr[0])));
> }
> @@ -2355,7 +2373,10 @@ struct txq_mp2mr_mbuf_check_data {
> (void *)rxq,
> (*mac)[0], (*mac)[1], (*mac)[2], (*mac)[3], (*mac)[4], (*mac)[5],
> mac_index, priv->vlan_filter[vlan_index].id);
> - claim_zero(ibv_destroy_flow(rxq->mac_flow[mac_index][vlan_index]));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_flow(rxq->mac_flow[mac_index][vlan_index]);
> rxq->mac_flow[mac_index][vlan_index] = NULL;
> }
>
> @@ -2736,7 +2757,10 @@ struct txq_mp2mr_mbuf_check_data {
> DEBUG("%p: disabling allmulticast mode", (void *)rxq);
> if (rxq->allmulti_flow == NULL)
> return;
> - claim_zero(ibv_destroy_flow(rxq->allmulti_flow));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_flow(rxq->allmulti_flow);
> rxq->allmulti_flow = NULL;
> DEBUG("%p: allmulticast mode disabled", (void *)rxq);
> }
> @@ -2796,7 +2820,10 @@ struct txq_mp2mr_mbuf_check_data {
> DEBUG("%p: disabling promiscuous mode", (void *)rxq);
> if (rxq->promisc_flow == NULL)
> return;
> - claim_zero(ibv_destroy_flow(rxq->promisc_flow));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_flow(rxq->promisc_flow);
> rxq->promisc_flow = NULL;
> DEBUG("%p: promiscuous mode disabled", (void *)rxq);
> }
> @@ -2847,9 +2874,15 @@ struct txq_mp2mr_mbuf_check_data {
> rxq_mac_addrs_del(rxq);
> }
> if (rxq->qp != NULL)
> - claim_zero(ibv_destroy_qp(rxq->qp));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_qp(rxq->qp);
> if (rxq->cq != NULL)
> - claim_zero(ibv_destroy_cq(rxq->cq));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_cq(rxq->cq);
> if (rxq->channel != NULL)
> claim_zero(ibv_destroy_comp_channel(rxq->channel));
> if (rxq->rd != NULL) {
> @@ -2864,7 +2897,10 @@ struct txq_mp2mr_mbuf_check_data {
> &attr));
> }
> if (rxq->mr != NULL)
> - claim_zero(ibv_dereg_mr(rxq->mr));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dereg_mr(rxq->mr);
> memset(rxq, 0, sizeof(*rxq));
> }
>
> @@ -4374,7 +4410,10 @@ struct txq_mp2mr_mbuf_check_data {
> priv_parent_list_cleanup(priv);
> if (priv->pd != NULL) {
> assert(priv->ctx != NULL);
> - claim_zero(ibv_dealloc_pd(priv->pd));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dealloc_pd(priv->pd);
> claim_zero(ibv_close_device(priv->ctx));
> } else
> assert(priv->ctx == NULL);
> @@ -6389,7 +6428,10 @@ struct txq_mp2mr_mbuf_check_data {
> port_error:
> rte_free(priv);
> if (pd)
> - claim_zero(ibv_dealloc_pd(pd));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_dealloc_pd(pd);
> if (ctx)
> claim_zero(ibv_close_device(ctx));
> if (eth_dev)
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index 925c89c..daa62e3 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -799,8 +799,11 @@ struct rte_flow_drop {
> struct rte_flow_drop *fdq = priv->flow_drop_queue;
>
> priv->flow_drop_queue = NULL;
> - claim_zero(ibv_destroy_qp(fdq->qp));
> - claim_zero(ibv_destroy_cq(fdq->cq));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_qp(fdq->qp);
> + ibv_destroy_cq(fdq->cq);
> rte_free(fdq);
> }
> }
> @@ -860,7 +863,10 @@ struct rte_flow_drop {
> priv->flow_drop_queue = fdq;
> return 0;
> err_create_qp:
> - claim_zero(ibv_destroy_cq(cq));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_cq(cq);
> err_create_cq:
> rte_free(fdq);
> err:
> @@ -1200,7 +1206,10 @@ struct rte_flow *
> (void)priv;
> LIST_REMOVE(flow, next);
> if (flow->ibv_flow)
> - claim_zero(ibv_destroy_flow(flow->ibv_flow));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_flow(flow->ibv_flow);
> rte_free(flow->ibv_attr);
> DEBUG("Flow destroyed %p", (void *)flow);
> rte_free(flow);
> @@ -1278,7 +1287,10 @@ struct rte_flow *
> for (flow = LIST_FIRST(&priv->flows);
> flow;
> flow = LIST_NEXT(flow, next)) {
> - claim_zero(ibv_destroy_flow(flow->ibv_flow));
> + /* Current verbs does not allow to check real
> + * errors when the device was plugged out.
> + */
> + ibv_destroy_flow(flow->ibv_flow);
> flow->ibv_flow = NULL;
> DEBUG("Flow %p removed", (void *)flow);
> }
> --
> 1.8.3.1
>
This approach looks way too intrusive. How about making the claim_zero()
definition not fail but still complain when compiled against a broken Verbs
version instead?
#include "mlx4_autoconf.h"
[...]
#ifndef HAVE_BROKEN_VERBS
#define claim_zero(...) assert((__VA_ARGS__) == 0)
#else /* HAVE_BROKEN_VERBS */
#define claim_zero(...) \
(void)(((__VA_ARGS__) == 0) || \
DEBUG("Assertion `" # __VA_ARGS__ "' failed (IGNORED)"))
#endif /* HAVE_BROKEN_VERBS */
You could use auto-config-h.sh to generate the HAVE_BROKEN_VERBS definition
in mlx4_autoconf.h (see mlx4 Makefile) based on some symbol, macro or type
that only exists or doesn't exist yet in problematic releases for instance.
--
Adrien Mazarguil
6WIND
