* [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return @ 2017-07-31 11:15 Matan Azrad 2017-07-31 14:17 ` Adrien Mazarguil 0 siblings, 1 reply; 9+ messages in thread From: Matan Azrad @ 2017-07-31 11:15 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: dev, Thomas Monjalon, Olga Shern, stable 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 <matan@mellanox.com> Cc: stable@dpdk.org --- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return 2017-07-31 11:15 [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return Matan Azrad @ 2017-07-31 14:17 ` Adrien Mazarguil 2017-07-31 16:56 ` Matan Azrad 0 siblings, 1 reply; 9+ messages in thread From: Adrien Mazarguil @ 2017-07-31 14:17 UTC (permalink / raw) To: Matan Azrad; +Cc: dev, Thomas Monjalon, Olga Shern, stable 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 <matan@mellanox.com> > Cc: stable@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 stable@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return 2017-07-31 14:17 ` Adrien Mazarguil @ 2017-07-31 16:56 ` Matan Azrad 2017-08-01 9:42 ` Adrien Mazarguil 0 siblings, 1 reply; 9+ messages in thread From: Matan Azrad @ 2017-07-31 16:56 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: dev, Thomas Monjalon, Olga Shern, stable Hi Adrien > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Monday, July 31, 2017 5:17 PM > To: Matan Azrad <matan@mellanox.com> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Olga Shern > <olgas@mellanox.com>; stable@dpdk.org > Subject: Re: [PATCH] net/mlx4: workaround to verbs wrong error return > > 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 <matan@mellanox.com> > > Cc: stable@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 > stable@dpdk.org should be involved. > Ok I'll remove it. > 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. > I agree with the dependence on broken verbs but there are other places in mlx4 code which use claim_zero assertion, So this suggestion will hurt other validations. What's about to create new define depend on broken verbs for the specific assertions? It will be still intrusive but more accurate. > -- > Adrien Mazarguil > 6WIND ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return 2017-07-31 16:56 ` Matan Azrad @ 2017-08-01 9:42 ` Adrien Mazarguil 2017-08-01 10:12 ` Matan Azrad 0 siblings, 1 reply; 9+ messages in thread From: Adrien Mazarguil @ 2017-08-01 9:42 UTC (permalink / raw) To: Matan Azrad; +Cc: dev, Thomas Monjalon, Olga Shern, stable Hi Matan, On Mon, Jul 31, 2017 at 04:56:33PM +0000, Matan Azrad wrote: > Hi Adrien > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Monday, July 31, 2017 5:17 PM > > To: Matan Azrad <matan@mellanox.com> > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Olga Shern > > <olgas@mellanox.com>; stable@dpdk.org > > Subject: Re: [PATCH] net/mlx4: workaround to verbs wrong error return > > > > 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 <matan@mellanox.com> > > > Cc: stable@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 > > stable@dpdk.org should be involved. > > > > Ok I'll remove it. > > > 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. > > > > I agree with the dependence on broken verbs but > there are other places in mlx4 code which use claim_zero assertion, > So this suggestion will hurt other validations. Well, half broken is no better than completely broken in my opinion, so while Verbs is being repaired, users debugging the mlx4 PMD will temporarily get debug traces without the ensuing abort(). At least the behavior will be consistent. Think about it, they already have to go out of their way to enable CONFIG_RTE_LIBRTE_MLX4_DEBUG, if they know they aren't using hot-plug but still use a buggy Verbs version, they can disable HAVE_BROKEN_VERBS to revert to the normal behavior. > What's about to create new define depend on broken verbs for the specific assertions? > It will be still intrusive but more accurate. One reason I prefer the code to remain unchanged is that I'm currently refactoring the entire PMD. Maintaining the above patch (picking the right ibv_*() calls that return a consistent value) will be difficult and an intrusive patch won't be reverted easily once Verbs is fixed. All these claim_zero() checks ensure the PMD destroys Verbs resources in the proper order (e.g. a flow before the QP it is associated with). If the return value of any of these cannot be relied on, it's useless to only check some of them. Moreover if ibv_destroy_something() wrongly returns an error when the device is unplugged, I think this can happen to the calls not part of your patch, i.e. all of them, so working around it at the macro definition level makes sense. If you don't know what symbol can be relied on in OFED 4.2 to define HAVE_BROKEN_VERBS (which is just an example, you can use another name BTW), maybe you can add a compilation option to enable manually in case of trouble? Something verbose like: CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS_ASSERT=n Which will have to be documented. -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return 2017-08-01 9:42 ` Adrien Mazarguil @ 2017-08-01 10:12 ` Matan Azrad 2017-08-01 10:50 ` Adrien Mazarguil 0 siblings, 1 reply; 9+ messages in thread From: Matan Azrad @ 2017-08-01 10:12 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: dev, Thomas Monjalon, Olga Shern, stable Hi Adrien > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Tuesday, August 1, 2017 12:42 PM > To: Matan Azrad <matan@mellanox.com> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Olga Shern > <olgas@mellanox.com>; stable@dpdk.org > Subject: Re: [PATCH] net/mlx4: workaround to verbs wrong error return > > Hi Matan, > > On Mon, Jul 31, 2017 at 04:56:33PM +0000, Matan Azrad wrote: > > Hi Adrien > > > > > -----Original Message----- > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > > Sent: Monday, July 31, 2017 5:17 PM > > > To: Matan Azrad <matan@mellanox.com> > > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Olga > Shern > > > <olgas@mellanox.com>; stable@dpdk.org > > > Subject: Re: [PATCH] net/mlx4: workaround to verbs wrong error > > > return > > > > > > 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 <matan@mellanox.com> > > > > Cc: stable@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 > > > stable@dpdk.org should be involved. > > > > > > > Ok I'll remove it. > > > > > 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. > > > > > > > I agree with the dependence on broken verbs but there are other places > > in mlx4 code which use claim_zero assertion, So this suggestion will > > hurt other validations. > > Well, half broken is no better than completely broken in my opinion, so while > Verbs is being repaired, users debugging the mlx4 PMD will temporarily get > debug traces without the ensuing abort(). At least the behavior will be > consistent. > > Think about it, they already have to go out of their way to enable > CONFIG_RTE_LIBRTE_MLX4_DEBUG, if they know they aren't using hot-plug > but still use a buggy Verbs version, they can disable HAVE_BROKEN_VERBS to > revert to the normal behavior. > priv_flow_validate and priv_mac_addr_add functions calls also are wrapped by claim_zero, These are not ibv_destroy functions and don't depend only in broken verbs, The user want to be aborted in those cases otherwise he would have put there trace print as you suggest. > > What's about to create new define depend on broken verbs for the specific > assertions? > > It will be still intrusive but more accurate. > > One reason I prefer the code to remain unchanged is that I'm currently > refactoring the entire PMD. Maintaining the above patch (picking the right > ibv_*() calls that return a consistent value) will be difficult and an intrusive > patch won't be reverted easily once Verbs is fixed. > You can just find all claim_zero_new and replace it with claim_zero. > All these claim_zero() checks ensure the PMD destroys Verbs resources in > the proper order (e.g. a flow before the QP it is associated with). If the > return value of any of these cannot be relied on, it's useless to only check > some of them. priv_flow_validate and priv_mac_addr_add functions are not destroy verbs resources. > > Moreover if ibv_destroy_something() wrongly returns an error when the > device is unplugged, I think this can happen to the calls not part of your > patch, i.e. all of them, so working around it at the macro definition level > makes sense. I checked with failsafe tests and found that only the specific destroy functions are problematic. > > If you don't know what symbol can be relied on in OFED 4.2 to define > HAVE_BROKEN_VERBS (which is just an example, you can use another name > BTW), maybe you can add a compilation option to enable manually in case of > trouble? Something verbose like: > > CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS_ASSERT=n > > Which will have to be documented. > > -- > Adrien Mazarguil > 6WIND ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return 2017-08-01 10:12 ` Matan Azrad @ 2017-08-01 10:50 ` Adrien Mazarguil 2017-08-01 12:18 ` Matan Azrad 2017-08-02 19:00 ` [dpdk-dev] [PATCH v2] " Matan Azrad 0 siblings, 2 replies; 9+ messages in thread From: Adrien Mazarguil @ 2017-08-01 10:50 UTC (permalink / raw) To: Matan Azrad; +Cc: dev, Thomas Monjalon, Olga Shern, stable Hi Matan, (snipping a bit of unnecessary context) On Tue, Aug 01, 2017 at 10:12:29AM +0000, Matan Azrad wrote: [...] > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] [...] > > On Mon, Jul 31, 2017 at 04:56:33PM +0000, Matan Azrad wrote: [...] > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] [...] > > > > On Mon, Jul 31, 2017 at 02:15:09PM +0300, Matan Azrad wrote: [...] > > > > > @@ -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. > > > > > > > > > > I agree with the dependence on broken verbs but there are other places > > > in mlx4 code which use claim_zero assertion, So this suggestion will > > > hurt other validations. > > > > Well, half broken is no better than completely broken in my opinion, so while > > Verbs is being repaired, users debugging the mlx4 PMD will temporarily get > > debug traces without the ensuing abort(). At least the behavior will be > > consistent. > > > > Think about it, they already have to go out of their way to enable > > CONFIG_RTE_LIBRTE_MLX4_DEBUG, if they know they aren't using hot-plug > > but still use a buggy Verbs version, they can disable HAVE_BROKEN_VERBS to > > revert to the normal behavior. > > > > priv_flow_validate and priv_mac_addr_add functions calls also are wrapped by claim_zero, > These are not ibv_destroy functions and don't depend only in broken verbs, > The user want to be aborted in those cases otherwise he would have put there trace print > as you suggest. As the only exceptions, if you had to change something it would be these instance in order to be less intrusive. But I suggest you not to since, again, this is a workaround for a problem that is not under PMD control. > > > What's about to create new define depend on broken verbs for the specific > > assertions? > > > It will be still intrusive but more accurate. > > > > One reason I prefer the code to remain unchanged is that I'm currently > > refactoring the entire PMD. Maintaining the above patch (picking the right > > ibv_*() calls that return a consistent value) will be difficult and an intrusive > > patch won't be reverted easily once Verbs is fixed. > > > > You can just find all claim_zero_new and replace it with claim_zero. So what if I'm adding new ibv_destroy_qp() calls, can I expect them to work consistently or will each of them have to be validated against a possible assert() failure during hot-plug? Note that ibv_destroy_qp() is only one example among many, the work you've done for this patch will have to be performed every time new code is added. I don't find it particularly convenient. > > All these claim_zero() checks ensure the PMD destroys Verbs resources in > > the proper order (e.g. a flow before the QP it is associated with). If the > > return value of any of these cannot be relied on, it's useless to only check > > some of them. > > > priv_flow_validate and priv_mac_addr_add functions are not destroy verbs resources. Right, and that's not a problem. Unfortunate users still get a nice debugging message in the unlikely case of a failure for these. > > Moreover if ibv_destroy_something() wrongly returns an error when the > > device is unplugged, I think this can happen to the calls not part of your > > patch, i.e. all of them, so working around it at the macro definition level > > makes sense. > > I checked with failsafe tests and found that only the specific destroy functions are problematic. What about other applications and corner cases, such as when the device is unplugged while the PMD is being initialized? Since the control path is bound by system calls, the PMD might actually sleep for a non negligible amount of time (there is really no upper limit to how long) and the device could disappear at any point. Subsequent ibv_*() calls would return unexpected errors. I'm sure you cannot verify all corner cases, so let's avoid them. > > If you don't know what symbol can be relied on in OFED 4.2 to define > > HAVE_BROKEN_VERBS (which is just an example, you can use another name > > BTW), maybe you can add a compilation option to enable manually in case of > > trouble? Something verbose like: > > > > CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS_ASSERT=n > > > > Which will have to be documented. What about the above suggestion? -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return 2017-08-01 10:50 ` Adrien Mazarguil @ 2017-08-01 12:18 ` Matan Azrad 2017-08-02 19:00 ` [dpdk-dev] [PATCH v2] " Matan Azrad 1 sibling, 0 replies; 9+ messages in thread From: Matan Azrad @ 2017-08-01 12:18 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: dev, Thomas Monjalon, Olga Shern, stable > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Tuesday, August 1, 2017 1:51 PM > To: Matan Azrad <matan@mellanox.com> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Olga Shern > <olgas@mellanox.com>; stable@dpdk.org > Subject: Re: [PATCH] net/mlx4: workaround to verbs wrong error return > > Hi Matan, > > (snipping a bit of unnecessary context) > > On Tue, Aug 01, 2017 at 10:12:29AM +0000, Matan Azrad wrote: > [...] > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > [...] > > > On Mon, Jul 31, 2017 at 04:56:33PM +0000, Matan Azrad wrote: > [...] > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > [...] > > > > > On Mon, Jul 31, 2017 at 02:15:09PM +0300, Matan Azrad wrote: > [...] > > > > > > @@ -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. > > > > > > > > > > > > > I agree with the dependence on broken verbs but there are other > > > > places in mlx4 code which use claim_zero assertion, So this > > > > suggestion will hurt other validations. > > > > > > Well, half broken is no better than completely broken in my opinion, > > > so while Verbs is being repaired, users debugging the mlx4 PMD will > > > temporarily get debug traces without the ensuing abort(). At least > > > the behavior will be consistent. > > > > > > Think about it, they already have to go out of their way to enable > > > CONFIG_RTE_LIBRTE_MLX4_DEBUG, if they know they aren't using > > > hot-plug but still use a buggy Verbs version, they can disable > > > HAVE_BROKEN_VERBS to revert to the normal behavior. > > > > > > > priv_flow_validate and priv_mac_addr_add functions calls also are > > wrapped by claim_zero, These are not ibv_destroy functions and don't > > depend only in broken verbs, The user want to be aborted in those > > cases otherwise he would have put there trace print as you suggest. > > As the only exceptions, if you had to change something it would be these > instance in order to be less intrusive. But I suggest you not to since, again, > this is a workaround for a problem that is not under PMD control. > > > > > What's about to create new define depend on broken verbs for the > > > > specific > > > assertions? > > > > It will be still intrusive but more accurate. > > > > > > One reason I prefer the code to remain unchanged is that I'm > > > currently refactoring the entire PMD. Maintaining the above patch > > > (picking the right > > > ibv_*() calls that return a consistent value) will be difficult and > > > an intrusive patch won't be reverted easily once Verbs is fixed. > > > > > > > You can just find all claim_zero_new and replace it with claim_zero. > > So what if I'm adding new ibv_destroy_qp() calls, can I expect them to work > consistently or will each of them have to be validated against a possible > assert() failure during hot-plug? > > Note that ibv_destroy_qp() is only one example among many, the work > you've done for this patch will have to be performed every time new code is > added. I don't find it particularly convenient. > > > > All these claim_zero() checks ensure the PMD destroys Verbs > > > resources in the proper order (e.g. a flow before the QP it is > > > associated with). If the return value of any of these cannot be > > > relied on, it's useless to only check some of them. > > > > > > priv_flow_validate and priv_mac_addr_add functions are not destroy > verbs resources. > > Right, and that's not a problem. Unfortunate users still get a nice debugging > message in the unlikely case of a failure for these. > I don't think the user want debugging message for this functions - he want to be aborted and to stop the program. He could add DEBUG prints if he had want, those behaviors are really different. > > > Moreover if ibv_destroy_something() wrongly returns an error when > > > the device is unplugged, I think this can happen to the calls not > > > part of your patch, i.e. all of them, so working around it at the > > > macro definition level makes sense. > > > > I checked with failsafe tests and found that only the specific destroy > functions are problematic. > > What about other applications and corner cases, such as when the device is > unplugged while the PMD is being initialized? Since the control path is bound > by system calls, the PMD might actually sleep for a non negligible amount of > time (there is really no upper limit to how long) and the device could > disappear at any point. Subsequent ibv_*() calls would return unexpected > errors. > > I'm sure you cannot verify all corner cases, so let's avoid them. > > > > If you don't know what symbol can be relied on in OFED 4.2 to define > > > HAVE_BROKEN_VERBS (which is just an example, you can use another > > > name BTW), maybe you can add a compilation option to enable manually > > > in case of trouble? Something verbose like: > > > > > > CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS_ASSERT=n > > > > > > Which will have to be documented. > > What about the above suggestion? > So, I don't think there is perfect solution to this issue. I will take your suggestion to depend claim_zero in verbs version. Firstly, I will check if I can get the information for broken verbs from somewhere, If not, I will use compilation option. > -- > Adrien Mazarguil > 6WIND ^ permalink raw reply [flat|nested] 9+ messages in thread
* [dpdk-dev] [PATCH v2] net/mlx4: workaround to verbs wrong error return 2017-08-01 10:50 ` Adrien Mazarguil 2017-08-01 12:18 ` Matan Azrad @ 2017-08-02 19:00 ` Matan Azrad 2017-08-03 21:04 ` Thomas Monjalon 1 sibling, 1 reply; 9+ messages in thread From: Matan Azrad @ 2017-08-02 19:00 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: dev 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 added option to replace all claim_zero assertions with debugging messages, by the way, this option affects non ibv destroy assertions. DPDK 18.02 release should work with Mellanox OFED-4.2 which will include the verbs fix to this bug, then, this patch can be removed. Signed-off-by: Matan Azrad <matan@mellanox.com> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com> --- config/common_base | 1 + doc/guides/nics/mlx4.rst | 8 ++++++++ drivers/net/mlx4/Makefile | 4 ++++ drivers/net/mlx4/mlx4.h | 6 ++++++ 4 files changed, 19 insertions(+) This v2 is not perfect but satisfy. diff --git a/config/common_base b/config/common_base index 7805605..5e97a08 100644 --- a/config/common_base +++ b/config/common_base @@ -213,6 +213,7 @@ CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y # CONFIG_RTE_LIBRTE_MLX4_PMD=n CONFIG_RTE_LIBRTE_MLX4_DEBUG=n +CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS=n CONFIG_RTE_LIBRTE_MLX4_SGE_WR_N=4 CONFIG_RTE_LIBRTE_MLX4_MAX_INLINE=0 CONFIG_RTE_LIBRTE_MLX4_TX_MP_CACHE=8 diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst index d5bf2b3..f8885b2 100644 --- a/doc/guides/nics/mlx4.rst +++ b/doc/guides/nics/mlx4.rst @@ -119,6 +119,14 @@ These options can be modified in the ``.config`` file. adds additional run-time checks and debugging messages at the cost of lower performance. +- ``CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS`` (default **n**) + + Mellanox OFED versions earlier than 4.2 may return false errors from + Verbs object destruction APIs after the device is plugged out. + Enabling this option replaces assertion checks that cause the program + to abort with harmless debugging messages as a workaround. + Relevant only when CONFIG_RTE_LIBRTE_MLX4_DEBUG is enabled. + - ``CONFIG_RTE_LIBRTE_MLX4_SGE_WR_N`` (default **4**) Number of scatter/gather elements (SGEs) per work request (WR). Lowering diff --git a/drivers/net/mlx4/Makefile b/drivers/net/mlx4/Makefile index 755c8a4..c045bd7 100644 --- a/drivers/net/mlx4/Makefile +++ b/drivers/net/mlx4/Makefile @@ -84,6 +84,10 @@ ifdef CONFIG_RTE_LIBRTE_MLX4_SOFT_COUNTERS CFLAGS += -DMLX4_PMD_SOFT_COUNTERS=$(CONFIG_RTE_LIBRTE_MLX4_SOFT_COUNTERS) endif +ifeq ($(CONFIG_RTE_LIBRTE_MLX4_DEBUG_BROKEN_VERBS),y) +CFLAGS += -DMLX4_PMD_DEBUG_BROKEN_VERBS +endif + include $(RTE_SDK)/mk/rte.lib.mk # Generate and clean-up mlx4_autoconf.h. diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index a2e0ae7..c0ade4f 100644 --- a/drivers/net/mlx4/mlx4.h +++ b/drivers/net/mlx4/mlx4.h @@ -182,7 +182,13 @@ enum { (DEBUG__(__VA_ARGS__), 0) \ })[0]) #define DEBUG(...) DEBUG_(__VA_ARGS__, '\n') +#ifndef MLX4_PMD_DEBUG_BROKEN_VERBS #define claim_zero(...) assert((__VA_ARGS__) == 0) +#else /* MLX4_PMD_DEBUG_BROKEN_VERBS */ +#define claim_zero(...) \ + (void)(((__VA_ARGS__) == 0) || \ + DEBUG("Assertion `(" # __VA_ARGS__ ") == 0' failed (IGNORED).")) +#endif /* MLX4_PMD_DEBUG_BROKEN_VERBS */ #define claim_nonzero(...) assert((__VA_ARGS__) != 0) #define claim_positive(...) assert((__VA_ARGS__) >= 0) #else /* NDEBUG */ -- 1.8.3.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/mlx4: workaround to verbs wrong error return 2017-08-02 19:00 ` [dpdk-dev] [PATCH v2] " Matan Azrad @ 2017-08-03 21:04 ` Thomas Monjalon 0 siblings, 0 replies; 9+ messages in thread From: Thomas Monjalon @ 2017-08-03 21:04 UTC (permalink / raw) To: Matan Azrad; +Cc: dev, Adrien Mazarguil 02/08/2017 21:00, Matan Azrad: > 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 added option to replace all claim_zero > assertions with debugging messages, by the way, this option > affects non ibv destroy assertions. > > DPDK 18.02 release should work with Mellanox OFED-4.2 which will > include the verbs fix to this bug, then, this patch can > be removed. > > Signed-off-by: Matan Azrad <matan@mellanox.com> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com> Applied, thanks ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-03 21:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-31 11:15 [dpdk-dev] [PATCH] net/mlx4: workaround to verbs wrong error return Matan Azrad 2017-07-31 14:17 ` Adrien Mazarguil 2017-07-31 16:56 ` Matan Azrad 2017-08-01 9:42 ` Adrien Mazarguil 2017-08-01 10:12 ` Matan Azrad 2017-08-01 10:50 ` Adrien Mazarguil 2017-08-01 12:18 ` Matan Azrad 2017-08-02 19:00 ` [dpdk-dev] [PATCH v2] " Matan Azrad 2017-08-03 21:04 ` Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).