| Message ID | 1481306104-19352-9-git-send-email-tatyana.e.nikolova@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Dec 09, 2016 at 11:55:02AM -0600, Tatyana Nikolova wrote: > From: Shiraz Saleem <shiraz.saleem@intel.com> > > Debug prints for error paths are off by default. User > has the option to turn them on by setting environment > variable I40IW_DEBUG in command line. > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> Hi Tatyana, This patch duplicates already existing code in most of providers and libraries in rdma-core, while two of our main goals for creating this consolidated library were simplification for users and reduce code duplication. It will be very beneficial if you: 1. Use and promote general pr_debug(..), srp_tools has nice piece of code, to be general code. 2. Create one and general place for all rdma-core's environment variables. This infrastructure will allow us in the future create better manual of all variables supported and ensure easy change if neeeded. Thanks > --- > providers/i40iw/i40iw_umain.c | 11 ++++++--- > providers/i40iw/i40iw_umain.h | 7 ++++++ > providers/i40iw/i40iw_uverbs.c | 52 +++++++++++++++++++++++------------------- > 3 files changed, 44 insertions(+), 26 deletions(-) > > diff --git a/providers/i40iw/i40iw_umain.c b/providers/i40iw/i40iw_umain.c > index 1756e65..a204859 100644 > --- a/providers/i40iw/i40iw_umain.c > +++ b/providers/i40iw/i40iw_umain.c > @@ -46,7 +46,7 @@ > #include "i40iw_umain.h" > #include "i40iw-abi.h" > > -unsigned int i40iw_debug_level; > +unsigned int i40iw_dbg; > > #include <sys/types.h> > #include <sys/stat.h> > @@ -184,7 +184,7 @@ static struct ibv_context *i40iw_ualloc_context(struct ibv_device *ibdev, int cm > return &iwvctx->ibv_ctx; > > err_free: > - fprintf(stderr, PFX "%s: failed to allocate context for device.\n", __func__); > + i40iw_debug("failed to allocate context for device.\n"); > free(iwvctx); > > return NULL; > @@ -216,6 +216,7 @@ static struct ibv_device_ops i40iw_udev_ops = { > struct ibv_device *i40iw_driver_init(const char *uverbs_sys_path, int abi_version) > { > char value[16]; > + char *env_val; > struct i40iw_udevice *dev; > unsigned int vendor, device; > int i; > @@ -236,9 +237,13 @@ struct ibv_device *i40iw_driver_init(const char *uverbs_sys_path, int abi_versio > > return NULL; > found: > + env_val = getenv("I40IW_DEBUG"); > + if (env_val) > + i40iw_dbg = atoi(env_val); > + > dev = malloc(sizeof(*dev)); > if (!dev) { > - fprintf(stderr, PFX "%s: failed to allocate memory for device object\n", __func__); > + i40iw_debug("failed to allocate memory for device object\n"); > return NULL; > } > > diff --git a/providers/i40iw/i40iw_umain.h b/providers/i40iw/i40iw_umain.h > index 13d3da8..889c006 100644 > --- a/providers/i40iw/i40iw_umain.h > +++ b/providers/i40iw/i40iw_umain.h > @@ -72,6 +72,13 @@ > #define I40E_DB_SHADOW_AREA_SIZE 64 > #define I40E_DB_CQ_OFFSET 0x40 > > +extern unsigned int i40iw_dbg; > +#define i40iw_debug(fmt, args...) \ > +do { \ > + if (i40iw_dbg) \ > + fprintf(stderr, PFX "%s: " fmt, __FUNCTION__, ##args); \ > +} while (0) > + > enum i40iw_uhca_type { > INTEL_i40iw > }; > diff --git a/providers/i40iw/i40iw_uverbs.c b/providers/i40iw/i40iw_uverbs.c > index f6d9196..464900b 100644 > --- a/providers/i40iw/i40iw_uverbs.c > +++ b/providers/i40iw/i40iw_uverbs.c > @@ -65,7 +65,7 @@ int i40iw_uquery_device(struct ibv_context *context, struct ibv_device_attr *att > > ret = ibv_cmd_query_device(context, attr, &i40iw_fw_ver, &cmd, sizeof(cmd)); > if (ret) { > - fprintf(stderr, PFX "%s: query device failed and returned status code: %d\n", __func__, ret); > + i40iw_debug("query device failed and returned status code: %d\n", ret); > return ret; > } > > @@ -165,7 +165,7 @@ struct ibv_mr *i40iw_ureg_mr(struct ibv_pd *pd, void *addr, size_t length, int a > if (ibv_cmd_reg_mr(pd, addr, length, (uintptr_t)addr, > access, mr, &cmd.ibv_cmd, sizeof(cmd), > &resp, sizeof(resp))) { > - fprintf(stderr, PFX "%s: Failed to register memory\n", __func__); > + i40iw_debug("Failed to register memory\n"); > free(mr); > return NULL; > } > @@ -264,7 +264,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context *context, int cqe, > &iwucq->mr, ®_mr_cmd.ibv_cmd, sizeof(reg_mr_cmd), ®_mr_resp, > sizeof(reg_mr_resp)); > if (ret) { > - fprintf(stderr, PFX "%s: failed to pin memory for CQ\n", __func__); > + i40iw_debug("failed to pin memory for CQ\n"); > goto err; > } > > @@ -274,7 +274,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context *context, int cqe, > &resp.ibv_resp, sizeof(resp)); > if (ret) { > ibv_cmd_dereg_mr(&iwucq->mr); > - fprintf(stderr, PFX "%s: failed to create CQ\n", __func__); > + i40iw_debug("failed to create CQ\n"); > goto err; > } > > @@ -286,7 +286,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context *context, int cqe, > if (!ret) > return &iwucq->ibv_cq; > else > - fprintf(stderr, PFX "%s: failed to initialze CQ, status %d\n", __func__, ret); > + i40iw_debug("failed to initialze CQ, status %d\n", ret); > err: > if (info.cq_base) > free(info.cq_base); > @@ -307,11 +307,11 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) > > ret = pthread_spin_destroy(&iwucq->lock); > if (ret) > - return ret; > + goto err; > > ret = ibv_cmd_destroy_cq(cq); > if (ret) > - return ret; > + goto err; > > ibv_cmd_dereg_mr(&iwucq->mr); > > @@ -319,6 +319,9 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) > free(iwucq); > > return 0; > +err: > + i40iw_debug("failed to destroy CQ, status %d\n", ret); > + return ret; > } > > /** > @@ -344,7 +347,7 @@ int i40iw_upoll_cq(struct ibv_cq *cq, int num_entries, struct ibv_wc *entry) > if (ret == I40IW_ERR_QUEUE_EMPTY) { > break; > } else if (ret) { > - fprintf(stderr, PFX "%s: Error polling CQ, status %d\n", __func__, ret); > + i40iw_debug("Error polling CQ, status %d\n", ret); > if (!cqe_count) > /* Indicate error */ > cqe_count = -1; > @@ -519,7 +522,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, > info->sq = memalign(I40IW_HW_PAGE_SIZE, totalqpsize); > > if (!info->sq) { > - fprintf(stderr, PFX "%s: failed to allocate memory for SQ\n", __func__); > + i40iw_debug("failed to allocate memory for SQ\n"); > return 0; > } > > @@ -535,7 +538,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, > IBV_ACCESS_LOCAL_WRITE, &iwuqp->mr, ®_mr_cmd.ibv_cmd, > sizeof(reg_mr_cmd), ®_mr_resp, sizeof(reg_mr_resp)); > if (ret) { > - fprintf(stderr, PFX "%s: failed to pin memory for SQ\n", __func__); > + i40iw_debug("failed to pin memory for SQ\n"); > free(info->sq); > return 0; > } > @@ -545,7 +548,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, > ret = ibv_cmd_create_qp(pd, &iwuqp->ibv_qp, attr, &cmd.ibv_cmd, sizeof(cmd), > &resp->ibv_resp, sizeof(struct i40iw_ucreate_qp_resp)); > if (ret) { > - fprintf(stderr, PFX "%s: failed to create QP, status %d\n", __func__, ret); > + i40iw_debug("failed to create QP, status %d\n", ret); > ibv_cmd_dereg_mr(&iwuqp->mr); > free(info->sq); > return 0; > @@ -565,7 +568,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, > map = mmap(NULL, I40IW_HW_PAGE_SIZE, PROT_WRITE | PROT_READ, MAP_SHARED, > pd->context->cmd_fd, offset); > if (map == MAP_FAILED) { > - fprintf(stderr, PFX "%s: failed to map push page, errno %d\n", __func__, errno); > + i40iw_debug("failed to map push page, errno %d\n", errno); > info->push_wqe = NULL; > info->push_db = NULL; > } else { > @@ -575,7 +578,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, > map = mmap(NULL, I40IW_HW_PAGE_SIZE, PROT_WRITE | PROT_READ, MAP_SHARED, > pd->context->cmd_fd, offset); > if (map == MAP_FAILED) { > - fprintf(stderr, PFX "%s: failed to map push doorbell, errno %d\n", __func__, errno); > + i40iw_debug("failed to map push doorbell, errno %d\n", errno); > munmap(info->push_wqe, I40IW_HW_PAGE_SIZE); > info->push_wqe = NULL; > info->push_db = NULL; > @@ -639,7 +642,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr > int sq_attr, rq_attr; > > if (attr->qp_type != IBV_QPT_RC) { > - fprintf(stderr, PFX "%s: failed to create QP, unsupported QP type: 0x%x\n", __func__, attr->qp_type); > + i40iw_debug("failed to create QP, unsupported QP type: 0x%x\n", attr->qp_type); > return NULL; > } > > @@ -658,8 +661,8 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr > /* Sanity check QP size before proceeding */ > sqdepth = i40iw_qp_get_qdepth(sq_attr, attr->cap.max_send_sge, attr->cap.max_inline_data); > if (!sqdepth) { > - fprintf(stderr, PFX "%s: invalid SQ attributes, max_send_wr=%d max_send_sge=%d\n", > - __func__, attr->cap.max_send_wr, attr->cap.max_send_sge); > + i40iw_debug("invalid SQ attributes, max_send_wr=%d max_send_sge=%d\n", > + attr->cap.max_send_wr, attr->cap.max_send_sge); > return NULL; > } > > @@ -691,13 +694,13 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr > info.sq_wrtrk_array = calloc(sqdepth, sizeof(*info.sq_wrtrk_array)); > > if (!info.sq_wrtrk_array) { > - fprintf(stderr, PFX "%s: failed to allocate memory for SQ work array\n", __func__); > + i40iw_debug("failed to allocate memory for SQ work array\n"); > goto err_destroy_lock; > } > > info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); > if (!info.rq_wrid_array) { > - fprintf(stderr, PFX "%s: failed to allocate memory for RQ work array\n", __func__); > + i40iw_debug("failed to allocate memory for RQ work array\n"); > goto err_free_sq_wrtrk; > } > > @@ -706,7 +709,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr > status = i40iw_vmapped_qp(iwuqp, pd, attr, &resp, sqdepth, rqdepth, &info); > > if (!status) { > - fprintf(stderr, PFX "%s: failed to map QP\n", __func__); > + i40iw_debug("failed to map QP\n"); > goto err_free_rq_wrid; > } > info.qp_id = resp.qp_id; > @@ -772,11 +775,11 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) > > ret = pthread_spin_destroy(&iwuqp->lock); > if (ret) > - return ret; > + goto err; > > ret = i40iw_destroy_vmapped_qp(iwuqp, iwuqp->qp.sq_base); > if (ret) > - return ret; > + goto err; > > if (iwuqp->qp.sq_wrtrk_array) > free(iwuqp->qp.sq_wrtrk_array); > @@ -792,6 +795,9 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) > free(iwuqp); > > return 0; > +err: > + i40iw_debug("failed to destroy QP, status %d\n", ret); > + return ret; > } > > /** > @@ -916,7 +922,7 @@ int i40iw_upost_send(struct ibv_qp *ib_qp, struct ibv_send_wr *ib_wr, struct ibv > default: > /* error */ > err = -EINVAL; > - fprintf(stderr, PFX "%s: post work request failed, invalid opcode: 0x%x\n", __func__, ib_wr->opcode); > + i40iw_debug("post work request failed, invalid opcode: 0x%x\n", ib_wr->opcode); > break; > } > > @@ -960,7 +966,7 @@ int i40iw_upost_recv(struct ibv_qp *ib_qp, struct ibv_recv_wr *ib_wr, struct ibv > post_recv.sg_list = sg_list; > ret = iwuqp->qp.ops.iw_post_receive(&iwuqp->qp, &post_recv); > if (ret) { > - fprintf(stderr, PFX "%s: failed to post receives, status %d\n", __func__, ret); > + i40iw_debug("failed to post receives, status %d\n", ret); > if (ret == I40IW_ERR_QP_TOOMANY_WRS_POSTED) > err = -ENOMEM; > else > -- > 1.8.5.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Leon, Please see inline. > -----Original Message----- > From: Leon Romanovsky [mailto:leonro@mellanox.com] > Sent: Saturday, December 10, 2016 8:34 AM > To: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com> > Cc: jgunthorpe@obsidianresearch.com; dledford@redhat.com; linux- > rdma@vger.kernel.org; e1000-rdma@lists.sourceforge.net > Subject: Re: [PATCH V2 08/10] i40iw: Control debug error prints using env > variable > > On Fri, Dec 09, 2016 at 11:55:02AM -0600, Tatyana Nikolova wrote: > > From: Shiraz Saleem <shiraz.saleem@intel.com> > > > > Debug prints for error paths are off by default. User has the option > > to turn them on by setting environment variable I40IW_DEBUG in > command > > line. > > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> > > Hi Tatyana, > > This patch duplicates already existing code in most of providers and libraries > in rdma-core, while two of our main goals for creating this consolidated > library were simplification for users and reduce code duplication. > > It will be very beneficial if you: > 1. Use and promote general pr_debug(..), srp_tools has nice piece of code, > to be general code. [Tatyana Nikolova] The debug/error printing macros available in rdma-core use different mechanisms to report information, for instance, they set/check one or more variables, or they use a bit mask to enable debug level. They also print to different outputs: stderr/stdout, debug files or syslog. By promoting general code, do you mean that we need to implement common functions and replace all of the currently used debug macros in rdma-core with the common ones? Which one of the different debug mechanisms should be used? Please, provide details on what you are asking for. > 2. Create one and general place for all rdma-core's environment variables. [Tatyana Nikolova] Are you suggesting a new common header file with defines for all environmental variables in rdma-core? For example: # cat rdma-core/util/env_vars.h #define MLX5_CQE_SIZE "MLX5_CQE_SIZE" #define MLX5_SCATTER_TO_CQE "MLX5_SCATTER_TO_CQE" #define MLX5_SRQ_SIGNATURE "MLX5_SRQ_SIGNATURE" #define MLX5_QP_SIGNATURE "MLX5_QP_SIGNATURE" #define MLX5_LOCAL_CPUS "MLX5_LOCAL_CPUS" #define MLX5_STALL_CQ_POLL "MLX5_STALL_CQ_POLL" #define MLX5_STALL_NUM_LOOP "MLX5_STALL_NUM_LOOP" Please explain and provide an example if possible. Thank you, Tatyana > > This infrastructure will allow us in the future create better manual of all > variables supported and ensure easy change if neeeded. > > Thanks > > > --- > > providers/i40iw/i40iw_umain.c | 11 ++++++--- > > providers/i40iw/i40iw_umain.h | 7 ++++++ > > providers/i40iw/i40iw_uverbs.c | 52 > > +++++++++++++++++++++++------------------- > > 3 files changed, 44 insertions(+), 26 deletions(-) > > > > diff --git a/providers/i40iw/i40iw_umain.c > > b/providers/i40iw/i40iw_umain.c index 1756e65..a204859 100644 > > --- a/providers/i40iw/i40iw_umain.c > > +++ b/providers/i40iw/i40iw_umain.c > > @@ -46,7 +46,7 @@ > > #include "i40iw_umain.h" > > #include "i40iw-abi.h" > > > > -unsigned int i40iw_debug_level; > > +unsigned int i40iw_dbg; > > > > #include <sys/types.h> > > #include <sys/stat.h> > > @@ -184,7 +184,7 @@ static struct ibv_context > *i40iw_ualloc_context(struct ibv_device *ibdev, int cm > > return &iwvctx->ibv_ctx; > > > > err_free: > > - fprintf(stderr, PFX "%s: failed to allocate context for device.\n", > __func__); > > + i40iw_debug("failed to allocate context for device.\n"); > > free(iwvctx); > > > > return NULL; > > @@ -216,6 +216,7 @@ static struct ibv_device_ops i40iw_udev_ops = { > > struct ibv_device *i40iw_driver_init(const char *uverbs_sys_path, int > > abi_version) { > > char value[16]; > > + char *env_val; > > struct i40iw_udevice *dev; > > unsigned int vendor, device; > > int i; > > @@ -236,9 +237,13 @@ struct ibv_device *i40iw_driver_init(const char > > *uverbs_sys_path, int abi_versio > > > > return NULL; > > found: > > + env_val = getenv("I40IW_DEBUG"); > > + if (env_val) > > + i40iw_dbg = atoi(env_val); > > + > > dev = malloc(sizeof(*dev)); > > if (!dev) { > > - fprintf(stderr, PFX "%s: failed to allocate memory for device > object\n", __func__); > > + i40iw_debug("failed to allocate memory for device > object\n"); > > return NULL; > > } > > > > diff --git a/providers/i40iw/i40iw_umain.h > > b/providers/i40iw/i40iw_umain.h index 13d3da8..889c006 100644 > > --- a/providers/i40iw/i40iw_umain.h > > +++ b/providers/i40iw/i40iw_umain.h > > @@ -72,6 +72,13 @@ > > #define I40E_DB_SHADOW_AREA_SIZE 64 > > #define I40E_DB_CQ_OFFSET 0x40 > > > > +extern unsigned int i40iw_dbg; > > +#define i40iw_debug(fmt, args...) \ > > +do { \ > > + if (i40iw_dbg) \ > > + fprintf(stderr, PFX "%s: " fmt, __FUNCTION__, ##args); \ } > while > > +(0) > > + > > enum i40iw_uhca_type { > > INTEL_i40iw > > }; > > diff --git a/providers/i40iw/i40iw_uverbs.c > > b/providers/i40iw/i40iw_uverbs.c index f6d9196..464900b 100644 > > --- a/providers/i40iw/i40iw_uverbs.c > > +++ b/providers/i40iw/i40iw_uverbs.c > > @@ -65,7 +65,7 @@ int i40iw_uquery_device(struct ibv_context *context, > > struct ibv_device_attr *att > > > > ret = ibv_cmd_query_device(context, attr, &i40iw_fw_ver, &cmd, > sizeof(cmd)); > > if (ret) { > > - fprintf(stderr, PFX "%s: query device failed and returned > status code: %d\n", __func__, ret); > > + i40iw_debug("query device failed and returned status code: > %d\n", > > +ret); > > return ret; > > } > > > > @@ -165,7 +165,7 @@ struct ibv_mr *i40iw_ureg_mr(struct ibv_pd *pd, > void *addr, size_t length, int a > > if (ibv_cmd_reg_mr(pd, addr, length, (uintptr_t)addr, > > access, mr, &cmd.ibv_cmd, sizeof(cmd), > > &resp, sizeof(resp))) { > > - fprintf(stderr, PFX "%s: Failed to register memory\n", > __func__); > > + i40iw_debug("Failed to register memory\n"); > > free(mr); > > return NULL; > > } > > @@ -264,7 +264,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context > *context, int cqe, > > &iwucq->mr, ®_mr_cmd.ibv_cmd, > sizeof(reg_mr_cmd), ®_mr_resp, > > sizeof(reg_mr_resp)); > > if (ret) { > > - fprintf(stderr, PFX "%s: failed to pin memory for CQ\n", > __func__); > > + i40iw_debug("failed to pin memory for CQ\n"); > > goto err; > > } > > > > @@ -274,7 +274,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context > *context, int cqe, > > &resp.ibv_resp, sizeof(resp)); > > if (ret) { > > ibv_cmd_dereg_mr(&iwucq->mr); > > - fprintf(stderr, PFX "%s: failed to create CQ\n", __func__); > > + i40iw_debug("failed to create CQ\n"); > > goto err; > > } > > > > @@ -286,7 +286,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context > *context, int cqe, > > if (!ret) > > return &iwucq->ibv_cq; > > else > > - fprintf(stderr, PFX "%s: failed to initialze CQ, status %d\n", > __func__, ret); > > + i40iw_debug("failed to initialze CQ, status %d\n", ret); > > err: > > if (info.cq_base) > > free(info.cq_base); > > @@ -307,11 +307,11 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) > > > > ret = pthread_spin_destroy(&iwucq->lock); > > if (ret) > > - return ret; > > + goto err; > > > > ret = ibv_cmd_destroy_cq(cq); > > if (ret) > > - return ret; > > + goto err; > > > > ibv_cmd_dereg_mr(&iwucq->mr); > > > > @@ -319,6 +319,9 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) > > free(iwucq); > > > > return 0; > > +err: > > + i40iw_debug("failed to destroy CQ, status %d\n", ret); > > + return ret; > > } > > > > /** > > @@ -344,7 +347,7 @@ int i40iw_upoll_cq(struct ibv_cq *cq, int > num_entries, struct ibv_wc *entry) > > if (ret == I40IW_ERR_QUEUE_EMPTY) { > > break; > > } else if (ret) { > > - fprintf(stderr, PFX "%s: Error polling CQ, status > %d\n", __func__, ret); > > + i40iw_debug("Error polling CQ, status %d\n", ret); > > if (!cqe_count) > > /* Indicate error */ > > cqe_count = -1; > > @@ -519,7 +522,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > *iwuqp, struct ibv_pd *pd, > > info->sq = memalign(I40IW_HW_PAGE_SIZE, totalqpsize); > > > > if (!info->sq) { > > - fprintf(stderr, PFX "%s: failed to allocate memory for SQ\n", > __func__); > > + i40iw_debug("failed to allocate memory for SQ\n"); > > return 0; > > } > > > > @@ -535,7 +538,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > *iwuqp, struct ibv_pd *pd, > > IBV_ACCESS_LOCAL_WRITE, &iwuqp->mr, > ®_mr_cmd.ibv_cmd, > > sizeof(reg_mr_cmd), ®_mr_resp, > sizeof(reg_mr_resp)); > > if (ret) { > > - fprintf(stderr, PFX "%s: failed to pin memory for SQ\n", > __func__); > > + i40iw_debug("failed to pin memory for SQ\n"); > > free(info->sq); > > return 0; > > } > > @@ -545,7 +548,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > *iwuqp, struct ibv_pd *pd, > > ret = ibv_cmd_create_qp(pd, &iwuqp->ibv_qp, attr, &cmd.ibv_cmd, > sizeof(cmd), > > &resp->ibv_resp, sizeof(struct > i40iw_ucreate_qp_resp)); > > if (ret) { > > - fprintf(stderr, PFX "%s: failed to create QP, status %d\n", > __func__, ret); > > + i40iw_debug("failed to create QP, status %d\n", ret); > > ibv_cmd_dereg_mr(&iwuqp->mr); > > free(info->sq); > > return 0; > > @@ -565,7 +568,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > *iwuqp, struct ibv_pd *pd, > > map = mmap(NULL, I40IW_HW_PAGE_SIZE, PROT_WRITE | > PROT_READ, MAP_SHARED, > > pd->context->cmd_fd, offset); > > if (map == MAP_FAILED) { > > - fprintf(stderr, PFX "%s: failed to map push page, > errno %d\n", __func__, errno); > > + i40iw_debug("failed to map push page, errno %d\n", > errno); > > info->push_wqe = NULL; > > info->push_db = NULL; > > } else { > > @@ -575,7 +578,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > *iwuqp, struct ibv_pd *pd, > > map = mmap(NULL, I40IW_HW_PAGE_SIZE, > PROT_WRITE | PROT_READ, MAP_SHARED, > > pd->context->cmd_fd, offset); > > if (map == MAP_FAILED) { > > - fprintf(stderr, PFX "%s: failed to map push > doorbell, errno %d\n", __func__, errno); > > + i40iw_debug("failed to map push doorbell, > errno %d\n", errno); > > munmap(info->push_wqe, > I40IW_HW_PAGE_SIZE); > > info->push_wqe = NULL; > > info->push_db = NULL; > > @@ -639,7 +642,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > struct ibv_qp_init_attr *attr > > int sq_attr, rq_attr; > > > > if (attr->qp_type != IBV_QPT_RC) { > > - fprintf(stderr, PFX "%s: failed to create QP, unsupported QP > type: 0x%x\n", __func__, attr->qp_type); > > + i40iw_debug("failed to create QP, unsupported QP type: > 0x%x\n", > > +attr->qp_type); > > return NULL; > > } > > > > @@ -658,8 +661,8 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > struct ibv_qp_init_attr *attr > > /* Sanity check QP size before proceeding */ > > sqdepth = i40iw_qp_get_qdepth(sq_attr, attr->cap.max_send_sge, > attr->cap.max_inline_data); > > if (!sqdepth) { > > - fprintf(stderr, PFX "%s: invalid SQ attributes, > max_send_wr=%d max_send_sge=%d\n", > > - __func__, attr->cap.max_send_wr, attr- > >cap.max_send_sge); > > + i40iw_debug("invalid SQ attributes, max_send_wr=%d > max_send_sge=%d\n", > > + attr->cap.max_send_wr, attr->cap.max_send_sge); > > return NULL; > > } > > > > @@ -691,13 +694,13 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd > *pd, struct ibv_qp_init_attr *attr > > info.sq_wrtrk_array = calloc(sqdepth, sizeof(*info.sq_wrtrk_array)); > > > > if (!info.sq_wrtrk_array) { > > - fprintf(stderr, PFX "%s: failed to allocate memory for SQ work > array\n", __func__); > > + i40iw_debug("failed to allocate memory for SQ work > array\n"); > > goto err_destroy_lock; > > } > > > > info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); > > if (!info.rq_wrid_array) { > > - fprintf(stderr, PFX "%s: failed to allocate memory for RQ > work array\n", __func__); > > + i40iw_debug("failed to allocate memory for RQ work > array\n"); > > goto err_free_sq_wrtrk; > > } > > > > @@ -706,7 +709,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > struct ibv_qp_init_attr *attr > > status = i40iw_vmapped_qp(iwuqp, pd, attr, &resp, sqdepth, > rqdepth, > > &info); > > > > if (!status) { > > - fprintf(stderr, PFX "%s: failed to map QP\n", __func__); > > + i40iw_debug("failed to map QP\n"); > > goto err_free_rq_wrid; > > } > > info.qp_id = resp.qp_id; > > @@ -772,11 +775,11 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) > > > > ret = pthread_spin_destroy(&iwuqp->lock); > > if (ret) > > - return ret; > > + goto err; > > > > ret = i40iw_destroy_vmapped_qp(iwuqp, iwuqp->qp.sq_base); > > if (ret) > > - return ret; > > + goto err; > > > > if (iwuqp->qp.sq_wrtrk_array) > > free(iwuqp->qp.sq_wrtrk_array); > > @@ -792,6 +795,9 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) > > free(iwuqp); > > > > return 0; > > +err: > > + i40iw_debug("failed to destroy QP, status %d\n", ret); > > + return ret; > > } > > > > /** > > @@ -916,7 +922,7 @@ int i40iw_upost_send(struct ibv_qp *ib_qp, struct > ibv_send_wr *ib_wr, struct ibv > > default: > > /* error */ > > err = -EINVAL; > > - fprintf(stderr, PFX "%s: post work request failed, > invalid opcode: 0x%x\n", __func__, ib_wr->opcode); > > + i40iw_debug("post work request failed, invalid > opcode: 0x%x\n", > > +ib_wr->opcode); > > break; > > } > > > > @@ -960,7 +966,7 @@ int i40iw_upost_recv(struct ibv_qp *ib_qp, struct > ibv_recv_wr *ib_wr, struct ibv > > post_recv.sg_list = sg_list; > > ret = iwuqp->qp.ops.iw_post_receive(&iwuqp->qp, > &post_recv); > > if (ret) { > > - fprintf(stderr, PFX "%s: failed to post receives, status > %d\n", __func__, ret); > > + i40iw_debug("failed to post receives, status %d\n", > ret); > > if (ret == I40IW_ERR_QP_TOOMANY_WRS_POSTED) > > err = -ENOMEM; > > else > > -- > > 1.8.5.2 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > in the body of a message to majordomo@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 13, 2016 at 08:02:09PM +0000, Nikolova, Tatyana E wrote: > Hi Leon, > > Please see inline. > > > -----Original Message----- > > From: Leon Romanovsky [mailto:leonro@mellanox.com] > > Sent: Saturday, December 10, 2016 8:34 AM > > To: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com> > > Cc: jgunthorpe@obsidianresearch.com; dledford@redhat.com; linux- > > rdma@vger.kernel.org; e1000-rdma@lists.sourceforge.net > > Subject: Re: [PATCH V2 08/10] i40iw: Control debug error prints using env > > variable > > > > On Fri, Dec 09, 2016 at 11:55:02AM -0600, Tatyana Nikolova wrote: > > > From: Shiraz Saleem <shiraz.saleem@intel.com> > > > > > > Debug prints for error paths are off by default. User has the option > > > to turn them on by setting environment variable I40IW_DEBUG in > > command > > > line. > > > > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> > > > > Hi Tatyana, > > > > This patch duplicates already existing code in most of providers and libraries > > in rdma-core, while two of our main goals for creating this consolidated > > library were simplification for users and reduce code duplication. > > > > It will be very beneficial if you: > > 1. Use and promote general pr_debug(..), srp_tools has nice piece of code, > > to be general code. > > [Tatyana Nikolova] The debug/error printing macros available in rdma-core use different mechanisms to report information, for instance, they set/check one or more variables, or they use a bit mask to enable debug level. They also print to different outputs: stderr/stdout, debug files or syslog. At the end, all these prints are for debug. It is hard to see any objections to see output from them in one place. > > By promoting general code, do you mean that we need to implement common functions and replace all of the currently used debug macros in rdma-core with the common ones? Yes. > > Which one of the different debug mechanisms should be used? The most common, all providers copy/paste the same code. > > Please, provide details on what you are asking for. > > > 2. Create one and general place for all rdma-core's environment variables. > > [Tatyana Nikolova] Are you suggesting a new common header file with defines for all environmental variables in rdma-core? > > For example: > # cat rdma-core/util/env_vars.h > > #define MLX5_CQE_SIZE "MLX5_CQE_SIZE" > #define MLX5_SCATTER_TO_CQE "MLX5_SCATTER_TO_CQE" > #define MLX5_SRQ_SIGNATURE "MLX5_SRQ_SIGNATURE" > #define MLX5_QP_SIGNATURE "MLX5_QP_SIGNATURE" > #define MLX5_LOCAL_CPUS "MLX5_LOCAL_CPUS" > #define MLX5_STALL_CQ_POLL "MLX5_STALL_CQ_POLL" > #define MLX5_STALL_NUM_LOOP "MLX5_STALL_NUM_LOOP" > > Please explain and provide an example if possible. I had something different in mind. The lib is interested in ENV facilities will have something like that in their init function. ... struct i40w_env *i40w_env; i40w_env = get_env_vars(I40W_ENV); ... in rdma-core/util/env.h|c #define SET_VAR(type, var, field) \ (struct ##name*)env->field = get_env_var(...) void *get_env_vars(enum typ) { void *env; switch(type) { case I40W_ENV: env = malloc(sizeof(struct i40w_env)); .... SET_VAR(i40w_env, "I40W_DEBUG", debug); ... } > > Thank you, > Tatyana > > > > > This infrastructure will allow us in the future create better manual of all > > variables supported and ensure easy change if neeeded. > > > > Thanks > > > > > --- > > > providers/i40iw/i40iw_umain.c | 11 ++++++--- > > > providers/i40iw/i40iw_umain.h | 7 ++++++ > > > providers/i40iw/i40iw_uverbs.c | 52 > > > +++++++++++++++++++++++------------------- > > > 3 files changed, 44 insertions(+), 26 deletions(-) > > > > > > diff --git a/providers/i40iw/i40iw_umain.c > > > b/providers/i40iw/i40iw_umain.c index 1756e65..a204859 100644 > > > --- a/providers/i40iw/i40iw_umain.c > > > +++ b/providers/i40iw/i40iw_umain.c > > > @@ -46,7 +46,7 @@ > > > #include "i40iw_umain.h" > > > #include "i40iw-abi.h" > > > > > > -unsigned int i40iw_debug_level; > > > +unsigned int i40iw_dbg; > > > > > > #include <sys/types.h> > > > #include <sys/stat.h> > > > @@ -184,7 +184,7 @@ static struct ibv_context > > *i40iw_ualloc_context(struct ibv_device *ibdev, int cm > > > return &iwvctx->ibv_ctx; > > > > > > err_free: > > > - fprintf(stderr, PFX "%s: failed to allocate context for device.\n", > > __func__); > > > + i40iw_debug("failed to allocate context for device.\n"); > > > free(iwvctx); > > > > > > return NULL; > > > @@ -216,6 +216,7 @@ static struct ibv_device_ops i40iw_udev_ops = { > > > struct ibv_device *i40iw_driver_init(const char *uverbs_sys_path, int > > > abi_version) { > > > char value[16]; > > > + char *env_val; > > > struct i40iw_udevice *dev; > > > unsigned int vendor, device; > > > int i; > > > @@ -236,9 +237,13 @@ struct ibv_device *i40iw_driver_init(const char > > > *uverbs_sys_path, int abi_versio > > > > > > return NULL; > > > found: > > > + env_val = getenv("I40IW_DEBUG"); > > > + if (env_val) > > > + i40iw_dbg = atoi(env_val); > > > + > > > dev = malloc(sizeof(*dev)); > > > if (!dev) { > > > - fprintf(stderr, PFX "%s: failed to allocate memory for device > > object\n", __func__); > > > + i40iw_debug("failed to allocate memory for device > > object\n"); > > > return NULL; > > > } > > > > > > diff --git a/providers/i40iw/i40iw_umain.h > > > b/providers/i40iw/i40iw_umain.h index 13d3da8..889c006 100644 > > > --- a/providers/i40iw/i40iw_umain.h > > > +++ b/providers/i40iw/i40iw_umain.h > > > @@ -72,6 +72,13 @@ > > > #define I40E_DB_SHADOW_AREA_SIZE 64 > > > #define I40E_DB_CQ_OFFSET 0x40 > > > > > > +extern unsigned int i40iw_dbg; > > > +#define i40iw_debug(fmt, args...) \ > > > +do { \ > > > + if (i40iw_dbg) \ > > > + fprintf(stderr, PFX "%s: " fmt, __FUNCTION__, ##args); \ } > > while > > > +(0) > > > + > > > enum i40iw_uhca_type { > > > INTEL_i40iw > > > }; > > > diff --git a/providers/i40iw/i40iw_uverbs.c > > > b/providers/i40iw/i40iw_uverbs.c index f6d9196..464900b 100644 > > > --- a/providers/i40iw/i40iw_uverbs.c > > > +++ b/providers/i40iw/i40iw_uverbs.c > > > @@ -65,7 +65,7 @@ int i40iw_uquery_device(struct ibv_context *context, > > > struct ibv_device_attr *att > > > > > > ret = ibv_cmd_query_device(context, attr, &i40iw_fw_ver, &cmd, > > sizeof(cmd)); > > > if (ret) { > > > - fprintf(stderr, PFX "%s: query device failed and returned > > status code: %d\n", __func__, ret); > > > + i40iw_debug("query device failed and returned status code: > > %d\n", > > > +ret); > > > return ret; > > > } > > > > > > @@ -165,7 +165,7 @@ struct ibv_mr *i40iw_ureg_mr(struct ibv_pd *pd, > > void *addr, size_t length, int a > > > if (ibv_cmd_reg_mr(pd, addr, length, (uintptr_t)addr, > > > access, mr, &cmd.ibv_cmd, sizeof(cmd), > > > &resp, sizeof(resp))) { > > > - fprintf(stderr, PFX "%s: Failed to register memory\n", > > __func__); > > > + i40iw_debug("Failed to register memory\n"); > > > free(mr); > > > return NULL; > > > } > > > @@ -264,7 +264,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context > > *context, int cqe, > > > &iwucq->mr, ®_mr_cmd.ibv_cmd, > > sizeof(reg_mr_cmd), ®_mr_resp, > > > sizeof(reg_mr_resp)); > > > if (ret) { > > > - fprintf(stderr, PFX "%s: failed to pin memory for CQ\n", > > __func__); > > > + i40iw_debug("failed to pin memory for CQ\n"); > > > goto err; > > > } > > > > > > @@ -274,7 +274,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context > > *context, int cqe, > > > &resp.ibv_resp, sizeof(resp)); > > > if (ret) { > > > ibv_cmd_dereg_mr(&iwucq->mr); > > > - fprintf(stderr, PFX "%s: failed to create CQ\n", __func__); > > > + i40iw_debug("failed to create CQ\n"); > > > goto err; > > > } > > > > > > @@ -286,7 +286,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context > > *context, int cqe, > > > if (!ret) > > > return &iwucq->ibv_cq; > > > else > > > - fprintf(stderr, PFX "%s: failed to initialze CQ, status %d\n", > > __func__, ret); > > > + i40iw_debug("failed to initialze CQ, status %d\n", ret); > > > err: > > > if (info.cq_base) > > > free(info.cq_base); > > > @@ -307,11 +307,11 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) > > > > > > ret = pthread_spin_destroy(&iwucq->lock); > > > if (ret) > > > - return ret; > > > + goto err; > > > > > > ret = ibv_cmd_destroy_cq(cq); > > > if (ret) > > > - return ret; > > > + goto err; > > > > > > ibv_cmd_dereg_mr(&iwucq->mr); > > > > > > @@ -319,6 +319,9 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) > > > free(iwucq); > > > > > > return 0; > > > +err: > > > + i40iw_debug("failed to destroy CQ, status %d\n", ret); > > > + return ret; > > > } > > > > > > /** > > > @@ -344,7 +347,7 @@ int i40iw_upoll_cq(struct ibv_cq *cq, int > > num_entries, struct ibv_wc *entry) > > > if (ret == I40IW_ERR_QUEUE_EMPTY) { > > > break; > > > } else if (ret) { > > > - fprintf(stderr, PFX "%s: Error polling CQ, status > > %d\n", __func__, ret); > > > + i40iw_debug("Error polling CQ, status %d\n", ret); > > > if (!cqe_count) > > > /* Indicate error */ > > > cqe_count = -1; > > > @@ -519,7 +522,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > > *iwuqp, struct ibv_pd *pd, > > > info->sq = memalign(I40IW_HW_PAGE_SIZE, totalqpsize); > > > > > > if (!info->sq) { > > > - fprintf(stderr, PFX "%s: failed to allocate memory for SQ\n", > > __func__); > > > + i40iw_debug("failed to allocate memory for SQ\n"); > > > return 0; > > > } > > > > > > @@ -535,7 +538,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > > *iwuqp, struct ibv_pd *pd, > > > IBV_ACCESS_LOCAL_WRITE, &iwuqp->mr, > > ®_mr_cmd.ibv_cmd, > > > sizeof(reg_mr_cmd), ®_mr_resp, > > sizeof(reg_mr_resp)); > > > if (ret) { > > > - fprintf(stderr, PFX "%s: failed to pin memory for SQ\n", > > __func__); > > > + i40iw_debug("failed to pin memory for SQ\n"); > > > free(info->sq); > > > return 0; > > > } > > > @@ -545,7 +548,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > > *iwuqp, struct ibv_pd *pd, > > > ret = ibv_cmd_create_qp(pd, &iwuqp->ibv_qp, attr, &cmd.ibv_cmd, > > sizeof(cmd), > > > &resp->ibv_resp, sizeof(struct > > i40iw_ucreate_qp_resp)); > > > if (ret) { > > > - fprintf(stderr, PFX "%s: failed to create QP, status %d\n", > > __func__, ret); > > > + i40iw_debug("failed to create QP, status %d\n", ret); > > > ibv_cmd_dereg_mr(&iwuqp->mr); > > > free(info->sq); > > > return 0; > > > @@ -565,7 +568,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > > *iwuqp, struct ibv_pd *pd, > > > map = mmap(NULL, I40IW_HW_PAGE_SIZE, PROT_WRITE | > > PROT_READ, MAP_SHARED, > > > pd->context->cmd_fd, offset); > > > if (map == MAP_FAILED) { > > > - fprintf(stderr, PFX "%s: failed to map push page, > > errno %d\n", __func__, errno); > > > + i40iw_debug("failed to map push page, errno %d\n", > > errno); > > > info->push_wqe = NULL; > > > info->push_db = NULL; > > > } else { > > > @@ -575,7 +578,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp > > *iwuqp, struct ibv_pd *pd, > > > map = mmap(NULL, I40IW_HW_PAGE_SIZE, > > PROT_WRITE | PROT_READ, MAP_SHARED, > > > pd->context->cmd_fd, offset); > > > if (map == MAP_FAILED) { > > > - fprintf(stderr, PFX "%s: failed to map push > > doorbell, errno %d\n", __func__, errno); > > > + i40iw_debug("failed to map push doorbell, > > errno %d\n", errno); > > > munmap(info->push_wqe, > > I40IW_HW_PAGE_SIZE); > > > info->push_wqe = NULL; > > > info->push_db = NULL; > > > @@ -639,7 +642,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > > struct ibv_qp_init_attr *attr > > > int sq_attr, rq_attr; > > > > > > if (attr->qp_type != IBV_QPT_RC) { > > > - fprintf(stderr, PFX "%s: failed to create QP, unsupported QP > > type: 0x%x\n", __func__, attr->qp_type); > > > + i40iw_debug("failed to create QP, unsupported QP type: > > 0x%x\n", > > > +attr->qp_type); > > > return NULL; > > > } > > > > > > @@ -658,8 +661,8 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > > struct ibv_qp_init_attr *attr > > > /* Sanity check QP size before proceeding */ > > > sqdepth = i40iw_qp_get_qdepth(sq_attr, attr->cap.max_send_sge, > > attr->cap.max_inline_data); > > > if (!sqdepth) { > > > - fprintf(stderr, PFX "%s: invalid SQ attributes, > > max_send_wr=%d max_send_sge=%d\n", > > > - __func__, attr->cap.max_send_wr, attr- > > >cap.max_send_sge); > > > + i40iw_debug("invalid SQ attributes, max_send_wr=%d > > max_send_sge=%d\n", > > > + attr->cap.max_send_wr, attr->cap.max_send_sge); > > > return NULL; > > > } > > > > > > @@ -691,13 +694,13 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd > > *pd, struct ibv_qp_init_attr *attr > > > info.sq_wrtrk_array = calloc(sqdepth, sizeof(*info.sq_wrtrk_array)); > > > > > > if (!info.sq_wrtrk_array) { > > > - fprintf(stderr, PFX "%s: failed to allocate memory for SQ work > > array\n", __func__); > > > + i40iw_debug("failed to allocate memory for SQ work > > array\n"); > > > goto err_destroy_lock; > > > } > > > > > > info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); > > > if (!info.rq_wrid_array) { > > > - fprintf(stderr, PFX "%s: failed to allocate memory for RQ > > work array\n", __func__); > > > + i40iw_debug("failed to allocate memory for RQ work > > array\n"); > > > goto err_free_sq_wrtrk; > > > } > > > > > > @@ -706,7 +709,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, > > struct ibv_qp_init_attr *attr > > > status = i40iw_vmapped_qp(iwuqp, pd, attr, &resp, sqdepth, > > rqdepth, > > > &info); > > > > > > if (!status) { > > > - fprintf(stderr, PFX "%s: failed to map QP\n", __func__); > > > + i40iw_debug("failed to map QP\n"); > > > goto err_free_rq_wrid; > > > } > > > info.qp_id = resp.qp_id; > > > @@ -772,11 +775,11 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) > > > > > > ret = pthread_spin_destroy(&iwuqp->lock); > > > if (ret) > > > - return ret; > > > + goto err; > > > > > > ret = i40iw_destroy_vmapped_qp(iwuqp, iwuqp->qp.sq_base); > > > if (ret) > > > - return ret; > > > + goto err; > > > > > > if (iwuqp->qp.sq_wrtrk_array) > > > free(iwuqp->qp.sq_wrtrk_array); > > > @@ -792,6 +795,9 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) > > > free(iwuqp); > > > > > > return 0; > > > +err: > > > + i40iw_debug("failed to destroy QP, status %d\n", ret); > > > + return ret; > > > } > > > > > > /** > > > @@ -916,7 +922,7 @@ int i40iw_upost_send(struct ibv_qp *ib_qp, struct > > ibv_send_wr *ib_wr, struct ibv > > > default: > > > /* error */ > > > err = -EINVAL; > > > - fprintf(stderr, PFX "%s: post work request failed, > > invalid opcode: 0x%x\n", __func__, ib_wr->opcode); > > > + i40iw_debug("post work request failed, invalid > > opcode: 0x%x\n", > > > +ib_wr->opcode); > > > break; > > > } > > > > > > @@ -960,7 +966,7 @@ int i40iw_upost_recv(struct ibv_qp *ib_qp, struct > > ibv_recv_wr *ib_wr, struct ibv > > > post_recv.sg_list = sg_list; > > > ret = iwuqp->qp.ops.iw_post_receive(&iwuqp->qp, > > &post_recv); > > > if (ret) { > > > - fprintf(stderr, PFX "%s: failed to post receives, status > > %d\n", __func__, ret); > > > + i40iw_debug("failed to post receives, status %d\n", > > ret); > > > if (ret == I40IW_ERR_QP_TOOMANY_WRS_POSTED) > > > err = -ENOMEM; > > > else > > > -- > > > 1.8.5.2 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > > in the body of a message to majordomo@vger.kernel.org More > > majordomo > > > info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 14, 2016 at 07:11:11PM +0200, Leon Romanovsky wrote: > > > This patch duplicates already existing code in most of providers and libraries > > > in rdma-core, while two of our main goals for creating this consolidated > > > library were simplification for users and reduce code duplication. > > > > > > It will be very beneficial if you: > > > 1. Use and promote general pr_debug(..), srp_tools has nice piece of code, > > > to be general code. > > > > [Tatyana Nikolova] The debug/error printing macros available in > > rdma-core use different mechanisms to report information, for > > instance, they set/check one or more variables, or they use a bit > > mask to enable debug level. They also print to different outputs: > > stderr/stdout, debug files or syslog. > > At the end, all these prints are for debug. It is hard to see any > objections to see output from them in one place. Yes, let us just use stderr for now for provider debugging. If someone wants syslog then that can be a later patch. It makes no sense that there are difference here. > in rdma-core/util/env.h|c > > #define SET_VAR(type, var, field) \ > (struct ##name*)env->field = get_env_var(...) > > void *get_env_vars(enum typ) > { > void *env; > switch(type) { > case I40W_ENV: > env = malloc(sizeof(struct i40w_env)); > .... > SET_VAR(i40w_env, "I40W_DEBUG", debug); > ... > } Why? I was thinking more like a standard: VERBS_PROVIDER_DEBUG=qp,ah,blah parser since other than mlx5 that is what providers use env vars for. I'm not sure I agree at all with what mlx5 is doing with tuning parameters via env vars :\ Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/2016 4:21 PM, Jason Gunthorpe wrote: > On Wed, Dec 14, 2016 at 07:11:11PM +0200, Leon Romanovsky wrote: > >>>> This patch duplicates already existing code in most of providers and libraries >>>> in rdma-core, while two of our main goals for creating this consolidated >>>> library were simplification for users and reduce code duplication. >>>> >>>> It will be very beneficial if you: >>>> 1. Use and promote general pr_debug(..), srp_tools has nice piece of code, >>>> to be general code. >>> >>> [Tatyana Nikolova] The debug/error printing macros available in >>> rdma-core use different mechanisms to report information, for >>> instance, they set/check one or more variables, or they use a bit >>> mask to enable debug level. They also print to different outputs: >>> stderr/stdout, debug files or syslog. >> >> At the end, all these prints are for debug. It is hard to see any >> objections to see output from them in one place. > > Yes, let us just use stderr for now for provider debugging. If someone > wants syslog then that can be a later patch. It makes no sense that > there are difference here. > >> in rdma-core/util/env.h|c >> >> #define SET_VAR(type, var, field) \ >> (struct ##name*)env->field = get_env_var(...) >> >> void *get_env_vars(enum typ) >> { >> void *env; >> switch(type) { >> case I40W_ENV: >> env = malloc(sizeof(struct i40w_env)); >> .... >> SET_VAR(i40w_env, "I40W_DEBUG", debug); >> ... >> } > > Why? > > I was thinking more like a standard: > > VERBS_PROVIDER_DEBUG=qp,ah,blah > > parser since other than mlx5 that is what providers use env vars for. > > I'm not sure I agree at all with what mlx5 is doing with tuning > parameters via env vars :\ Options (not necessarily tuning though) have long been enabled via env vars for shared libraries. That really isn't uncommon. Env vars are easy to set on a per-app or per-user or per-container basis. The other option, config files, are not so easy to separate out. And one thing we might need this for in the future is to reserve QPs for IPoIB use so we can actually have containers with consistent IPoIB device hw addresses by using GID + Fixed QP number.
On Wed, Dec 14, 2016 at 05:36:26PM -0500, Doug Ledford wrote: > > I'm not sure I agree at all with what mlx5 is doing with tuning > > parameters via env vars :\ > > Options (not necessarily tuning though) have long been enabled via > env Sure, I was more concerned about using env vars for tuning. Setting log level/etc seems OK Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 14, 2016 at 02:21:03PM -0700, Jason Gunthorpe wrote: > On Wed, Dec 14, 2016 at 07:11:11PM +0200, Leon Romanovsky wrote: > > > > > This patch duplicates already existing code in most of providers and libraries > > > > in rdma-core, while two of our main goals for creating this consolidated > > > > library were simplification for users and reduce code duplication. > > > > > > > > It will be very beneficial if you: > > > > 1. Use and promote general pr_debug(..), srp_tools has nice piece of code, > > > > to be general code. > > > > > > [Tatyana Nikolova] The debug/error printing macros available in > > > rdma-core use different mechanisms to report information, for > > > instance, they set/check one or more variables, or they use a bit > > > mask to enable debug level. They also print to different outputs: > > > stderr/stdout, debug files or syslog. > > > > At the end, all these prints are for debug. It is hard to see any > > objections to see output from them in one place. > > Yes, let us just use stderr for now for provider debugging. If someone > wants syslog then that can be a later patch. It makes no sense that > there are difference here. > > > in rdma-core/util/env.h|c > > > > #define SET_VAR(type, var, field) \ > > (struct ##name*)env->field = get_env_var(...) > > > > void *get_env_vars(enum typ) > > { > > void *env; > > switch(type) { > > case I40W_ENV: > > env = malloc(sizeof(struct i40w_env)); > > .... > > SET_VAR(i40w_env, "I40W_DEBUG", debug); > > ... > > } > > Why? It will give common place for all different variables and debug was an example. My request from Tatyana was to come with 2 common mechanisms: 1. Common debug prints. 2. Common place for all various getenv() calls. And the pseudo-code above was example of second mechsnism. > > I was thinking more like a standard: > > VERBS_PROVIDER_DEBUG=qp,ah,blah > > parser since other than mlx5 that is what providers use env vars for. It is not providers only, but many other libraries in rdma-core are using env variables for tuning (libibverbs/librdmacm/rdma-ndd). > > I'm not sure I agree at all with what mlx5 is doing with tuning > parameters via env vars :\ > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 08:48:07AM +0200, Leon Romanovsky wrote: > > I was thinking more like a standard: > > > > VERBS_PROVIDER_DEBUG=qp,ah,blah > > > > parser since other than mlx5 that is what providers use env vars for. > > It is not providers only, but many other libraries in rdma-core are > using env variables for tuning (libibverbs/librdmacm/rdma-ndd). Other things have different requirements, eg all the *daemons* should be using the system log and use a command line arg to enable debug, rdma-ndd is doing it OK for instance. I think it is reasonable to focus on small tasks, considering how much there is to do. In this case hoisting only the provider debug mechanism to util seems like enough and fixes the ugly problem with i40iw where it unconditionally prints debug information to stderr.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 09:54:20AM -0700, Jason Gunthorpe wrote: > On Thu, Dec 15, 2016 at 08:48:07AM +0200, Leon Romanovsky wrote: > > > > I was thinking more like a standard: > > > > > > VERBS_PROVIDER_DEBUG=qp,ah,blah > > > > > > parser since other than mlx5 that is what providers use env vars for. > > > > It is not providers only, but many other libraries in rdma-core are > > using env variables for tuning (libibverbs/librdmacm/rdma-ndd). > > Other things have different requirements, eg all the *daemons* should > be using the system log and use a command line arg to enable debug, > rdma-ndd is doing it OK for instance. I had in mind much simplier infrastucture, just add pr_debug(..) call and allow every provider to place in any place in their code. My main point is that I want to see all ENV variables in one place. > > I think it is reasonable to focus on small tasks, considering how much > there is to do. In this case hoisting only the provider debug > mechanism to util seems like enough and fixes the ugly problem with > i40iw where it unconditionally prints debug information to stderr.. > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 08:35:37PM +0200, Leon Romanovsky wrote: > I had in mind much simplier infrastucture, just add pr_debug(..) call > and allow every provider to place in any place in their code. I think the bitmask thing has to be hoisted too. > My main point is that I want to see all ENV variables in one place. Why? I really don't want to see util/ files include provider headers, for instance, so I don't like your SET() macro idea.. At this point I'd settle for having all ENV vars *documented* in one place :( Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 11:50:34AM -0700, Jason Gunthorpe wrote: > On Thu, Dec 15, 2016 at 08:35:37PM +0200, Leon Romanovsky wrote: > > > I had in mind much simplier infrastucture, just add pr_debug(..) call > > and allow every provider to place in any place in their code. > > I think the bitmask thing has to be hoisted too. > > > My main point is that I want to see all ENV variables in one place. > > Why? There are two reasons: 1. Easy to document and for curious users to spot all possible variables. 2. It helps to potential authors to reuse variables instead of inventing their own. > > I really don't want to see util/ files include provider headers, for > instance, so I don't like your SET() macro idea.. > > At this point I'd settle for having all ENV vars *documented* in one > place :( See, reason #1 :) > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Leon, Please see inline. > -----Original Message----- > From: Leon Romanovsky [mailto:leonro@mellanox.com] > Sent: Thursday, December 15, 2016 1:18 PM > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Cc: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com>; > dledford@redhat.com; linux-rdma@vger.kernel.org; e1000- > rdma@lists.sourceforge.net > Subject: Re: [PATCH V2 08/10] i40iw: Control debug error prints using env > variable > > On Thu, Dec 15, 2016 at 11:50:34AM -0700, Jason Gunthorpe wrote: > > On Thu, Dec 15, 2016 at 08:35:37PM +0200, Leon Romanovsky wrote: > > > > > I had in mind much simplier infrastucture, just add pr_debug(..) > > > call and allow every provider to place in any place in their code. > > > > I think the bitmask thing has to be hoisted too. > > > > > My main point is that I want to see all ENV variables in one place. > > > > Why? > > There are two reasons: > 1. Easy to document and for curious users to spot all possible variables. [Tatyana Nikolova] Thank you for providing an example for the environment variables. IMO the malloc implementation brings unnecessary overhead in terms of memory and code. A text file containing all environment variables and updated every time a new environment variable is added sounds like a good idea to keep them in one place. > 2. It helps to potential authors to reuse variables instead of inventing their > own. [Tatyana Nikolova] Jason gave an example with " VERBS_PROVIDER_DEBUG=qp,ah,blah". Should all of the environment variables, enabling other features in rdma-core use a similar approach? Current environment variables are provider specific. How are they going to be reused without renaming? Renaming may create issues with applications and scripts already using the existing ones. > > > > I really don't want to see util/ files include provider headers, for > > instance, so I don't like your SET() macro idea.. > > > > At this point I'd settle for having all ENV vars *documented* in one > > place :( > > See, reason #1 :) > > > > > Jason > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > in the body of a message to majordomo@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 07:55:18PM +0000, Nikolova, Tatyana E wrote: > Current environment variables are provider specific. How are they > going to be reused without renaming? Renaming may create issues with > applications and scripts already using the existing ones. If we just focus on provider debug prints I'd have the common code parse all of the provider env var possibilities to enable the global bitmask value. Add and recommend a new value like VERBS_PROVIDER_DEBUG in the man page. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 07:55:18PM +0000, Nikolova, Tatyana E wrote: > Hi Leon, > > Please see inline. > > > -----Original Message----- > > From: Leon Romanovsky [mailto:leonro@mellanox.com] > > Sent: Thursday, December 15, 2016 1:18 PM > > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > Cc: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com>; > > dledford@redhat.com; linux-rdma@vger.kernel.org; e1000- > > rdma@lists.sourceforge.net > > Subject: Re: [PATCH V2 08/10] i40iw: Control debug error prints using env > > variable > > > > On Thu, Dec 15, 2016 at 11:50:34AM -0700, Jason Gunthorpe wrote: > > > On Thu, Dec 15, 2016 at 08:35:37PM +0200, Leon Romanovsky wrote: > > > > > > > I had in mind much simplier infrastucture, just add pr_debug(..) > > > > call and allow every provider to place in any place in their code. > > > > > > I think the bitmask thing has to be hoisted too. > > > > > > > My main point is that I want to see all ENV variables in one place. > > > > > > Why? > > > > There are two reasons: > > 1. Easy to document and for curious users to spot all possible variables. > > [Tatyana Nikolova] Thank you for providing an example for the environment variables. IMO the malloc implementation brings unnecessary overhead in terms of memory and code. It is in init phase and it will take the same amount of space as other global varibles now. i40w will have one ENV entry now -> it will have one field in that struct -> malloc will be the same. > > A text file containing all environment variables and updated every time a new environment variable is added sounds like a good idea to keep them in one place. If it will be one place and won't requre from us to remind and remember to update the same information in multiple places, I'm ok. > > > 2. It helps to potential authors to reuse variables instead of inventing their > > own. > > [Tatyana Nikolova] > > Jason gave an example with " VERBS_PROVIDER_DEBUG=qp,ah,blah". Should all of the environment variables, enabling other features in rdma-core use a similar approach? I think that Jason is over designing it, but it is better to ask him about it. > > Current environment variables are provider specific. How are they going to be reused without renaming? Renaming may create issues with applications and scripts already using the existing ones. #define NEW_VARIABLE OLD_VARIABLE > > > > > > > I really don't want to see util/ files include provider headers, for > > > instance, so I don't like your SET() macro idea.. > > > > > > At this point I'd settle for having all ENV vars *documented* in one > > > place :( > > > > See, reason #1 :) > > > > > > > > Jason > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > > in the body of a message to majordomo@vger.kernel.org More > > majordomo > > > info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 10:19:17PM +0200, Leon Romanovsky wrote: > > Jason gave an example with " > > VERBS_PROVIDER_DEBUG=qp,ah,blah". Should all of the environment > > variables, enabling other features in rdma-core use a similar > > approach? > > I think that Jason is over designing it, but it is better to ask him > about it. Well, this is what mlx5 does for debugging and I want to see it move to the common API. Are you saying we can get rid of the classifications?? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 15, 2016 at 09:26:32PM -0700, Jason Gunthorpe wrote: > On Thu, Dec 15, 2016 at 10:19:17PM +0200, Leon Romanovsky wrote: > > > > Jason gave an example with " > > > VERBS_PROVIDER_DEBUG=qp,ah,blah". Should all of the environment > > > variables, enabling other features in rdma-core use a similar > > > approach? > > > > I think that Jason is over designing it, but it is better to ask him > > about it. > > Well, this is what mlx5 does for debugging and I want to see it move > to the common API. Are you saying we can get rid of the classifications?? This is my personal opinion and Yishai can provide official opinion. I see litle value in current implementation of debug mask, it is protected by compilation flag. The enablement of this will require to recompile code and to set env variable prior execution of application => it will print for all CQ/QP. The same can be achieved without debug mask. Thanks > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/2016 10:19 AM, Leon Romanovsky wrote: > On Thu, Dec 15, 2016 at 09:26:32PM -0700, Jason Gunthorpe wrote: >> On Thu, Dec 15, 2016 at 10:19:17PM +0200, Leon Romanovsky wrote: >> >>>> Jason gave an example with " >>>> VERBS_PROVIDER_DEBUG=qp,ah,blah". Should all of the environment >>>> variables, enabling other features in rdma-core use a similar >>>> approach? >>> >>> I think that Jason is over designing it, but it is better to ask him >>> about it. >> >> Well, this is what mlx5 does for debugging and I want to see it move >> to the common API. Are you saying we can get rid of the classifications?? > > This is my personal opinion and Yishai can provide official opinion. > Useful debug mode should have classification and granularity, let's preserve this ability. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/providers/i40iw/i40iw_umain.c b/providers/i40iw/i40iw_umain.c index 1756e65..a204859 100644 --- a/providers/i40iw/i40iw_umain.c +++ b/providers/i40iw/i40iw_umain.c @@ -46,7 +46,7 @@ #include "i40iw_umain.h" #include "i40iw-abi.h" -unsigned int i40iw_debug_level; +unsigned int i40iw_dbg; #include <sys/types.h> #include <sys/stat.h> @@ -184,7 +184,7 @@ static struct ibv_context *i40iw_ualloc_context(struct ibv_device *ibdev, int cm return &iwvctx->ibv_ctx; err_free: - fprintf(stderr, PFX "%s: failed to allocate context for device.\n", __func__); + i40iw_debug("failed to allocate context for device.\n"); free(iwvctx); return NULL; @@ -216,6 +216,7 @@ static struct ibv_device_ops i40iw_udev_ops = { struct ibv_device *i40iw_driver_init(const char *uverbs_sys_path, int abi_version) { char value[16]; + char *env_val; struct i40iw_udevice *dev; unsigned int vendor, device; int i; @@ -236,9 +237,13 @@ struct ibv_device *i40iw_driver_init(const char *uverbs_sys_path, int abi_versio return NULL; found: + env_val = getenv("I40IW_DEBUG"); + if (env_val) + i40iw_dbg = atoi(env_val); + dev = malloc(sizeof(*dev)); if (!dev) { - fprintf(stderr, PFX "%s: failed to allocate memory for device object\n", __func__); + i40iw_debug("failed to allocate memory for device object\n"); return NULL; } diff --git a/providers/i40iw/i40iw_umain.h b/providers/i40iw/i40iw_umain.h index 13d3da8..889c006 100644 --- a/providers/i40iw/i40iw_umain.h +++ b/providers/i40iw/i40iw_umain.h @@ -72,6 +72,13 @@ #define I40E_DB_SHADOW_AREA_SIZE 64 #define I40E_DB_CQ_OFFSET 0x40 +extern unsigned int i40iw_dbg; +#define i40iw_debug(fmt, args...) \ +do { \ + if (i40iw_dbg) \ + fprintf(stderr, PFX "%s: " fmt, __FUNCTION__, ##args); \ +} while (0) + enum i40iw_uhca_type { INTEL_i40iw }; diff --git a/providers/i40iw/i40iw_uverbs.c b/providers/i40iw/i40iw_uverbs.c index f6d9196..464900b 100644 --- a/providers/i40iw/i40iw_uverbs.c +++ b/providers/i40iw/i40iw_uverbs.c @@ -65,7 +65,7 @@ int i40iw_uquery_device(struct ibv_context *context, struct ibv_device_attr *att ret = ibv_cmd_query_device(context, attr, &i40iw_fw_ver, &cmd, sizeof(cmd)); if (ret) { - fprintf(stderr, PFX "%s: query device failed and returned status code: %d\n", __func__, ret); + i40iw_debug("query device failed and returned status code: %d\n", ret); return ret; } @@ -165,7 +165,7 @@ struct ibv_mr *i40iw_ureg_mr(struct ibv_pd *pd, void *addr, size_t length, int a if (ibv_cmd_reg_mr(pd, addr, length, (uintptr_t)addr, access, mr, &cmd.ibv_cmd, sizeof(cmd), &resp, sizeof(resp))) { - fprintf(stderr, PFX "%s: Failed to register memory\n", __func__); + i40iw_debug("Failed to register memory\n"); free(mr); return NULL; } @@ -264,7 +264,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context *context, int cqe, &iwucq->mr, ®_mr_cmd.ibv_cmd, sizeof(reg_mr_cmd), ®_mr_resp, sizeof(reg_mr_resp)); if (ret) { - fprintf(stderr, PFX "%s: failed to pin memory for CQ\n", __func__); + i40iw_debug("failed to pin memory for CQ\n"); goto err; } @@ -274,7 +274,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context *context, int cqe, &resp.ibv_resp, sizeof(resp)); if (ret) { ibv_cmd_dereg_mr(&iwucq->mr); - fprintf(stderr, PFX "%s: failed to create CQ\n", __func__); + i40iw_debug("failed to create CQ\n"); goto err; } @@ -286,7 +286,7 @@ struct ibv_cq *i40iw_ucreate_cq(struct ibv_context *context, int cqe, if (!ret) return &iwucq->ibv_cq; else - fprintf(stderr, PFX "%s: failed to initialze CQ, status %d\n", __func__, ret); + i40iw_debug("failed to initialze CQ, status %d\n", ret); err: if (info.cq_base) free(info.cq_base); @@ -307,11 +307,11 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) ret = pthread_spin_destroy(&iwucq->lock); if (ret) - return ret; + goto err; ret = ibv_cmd_destroy_cq(cq); if (ret) - return ret; + goto err; ibv_cmd_dereg_mr(&iwucq->mr); @@ -319,6 +319,9 @@ int i40iw_udestroy_cq(struct ibv_cq *cq) free(iwucq); return 0; +err: + i40iw_debug("failed to destroy CQ, status %d\n", ret); + return ret; } /** @@ -344,7 +347,7 @@ int i40iw_upoll_cq(struct ibv_cq *cq, int num_entries, struct ibv_wc *entry) if (ret == I40IW_ERR_QUEUE_EMPTY) { break; } else if (ret) { - fprintf(stderr, PFX "%s: Error polling CQ, status %d\n", __func__, ret); + i40iw_debug("Error polling CQ, status %d\n", ret); if (!cqe_count) /* Indicate error */ cqe_count = -1; @@ -519,7 +522,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, info->sq = memalign(I40IW_HW_PAGE_SIZE, totalqpsize); if (!info->sq) { - fprintf(stderr, PFX "%s: failed to allocate memory for SQ\n", __func__); + i40iw_debug("failed to allocate memory for SQ\n"); return 0; } @@ -535,7 +538,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, IBV_ACCESS_LOCAL_WRITE, &iwuqp->mr, ®_mr_cmd.ibv_cmd, sizeof(reg_mr_cmd), ®_mr_resp, sizeof(reg_mr_resp)); if (ret) { - fprintf(stderr, PFX "%s: failed to pin memory for SQ\n", __func__); + i40iw_debug("failed to pin memory for SQ\n"); free(info->sq); return 0; } @@ -545,7 +548,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, ret = ibv_cmd_create_qp(pd, &iwuqp->ibv_qp, attr, &cmd.ibv_cmd, sizeof(cmd), &resp->ibv_resp, sizeof(struct i40iw_ucreate_qp_resp)); if (ret) { - fprintf(stderr, PFX "%s: failed to create QP, status %d\n", __func__, ret); + i40iw_debug("failed to create QP, status %d\n", ret); ibv_cmd_dereg_mr(&iwuqp->mr); free(info->sq); return 0; @@ -565,7 +568,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, map = mmap(NULL, I40IW_HW_PAGE_SIZE, PROT_WRITE | PROT_READ, MAP_SHARED, pd->context->cmd_fd, offset); if (map == MAP_FAILED) { - fprintf(stderr, PFX "%s: failed to map push page, errno %d\n", __func__, errno); + i40iw_debug("failed to map push page, errno %d\n", errno); info->push_wqe = NULL; info->push_db = NULL; } else { @@ -575,7 +578,7 @@ static int i40iw_vmapped_qp(struct i40iw_uqp *iwuqp, struct ibv_pd *pd, map = mmap(NULL, I40IW_HW_PAGE_SIZE, PROT_WRITE | PROT_READ, MAP_SHARED, pd->context->cmd_fd, offset); if (map == MAP_FAILED) { - fprintf(stderr, PFX "%s: failed to map push doorbell, errno %d\n", __func__, errno); + i40iw_debug("failed to map push doorbell, errno %d\n", errno); munmap(info->push_wqe, I40IW_HW_PAGE_SIZE); info->push_wqe = NULL; info->push_db = NULL; @@ -639,7 +642,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr int sq_attr, rq_attr; if (attr->qp_type != IBV_QPT_RC) { - fprintf(stderr, PFX "%s: failed to create QP, unsupported QP type: 0x%x\n", __func__, attr->qp_type); + i40iw_debug("failed to create QP, unsupported QP type: 0x%x\n", attr->qp_type); return NULL; } @@ -658,8 +661,8 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr /* Sanity check QP size before proceeding */ sqdepth = i40iw_qp_get_qdepth(sq_attr, attr->cap.max_send_sge, attr->cap.max_inline_data); if (!sqdepth) { - fprintf(stderr, PFX "%s: invalid SQ attributes, max_send_wr=%d max_send_sge=%d\n", - __func__, attr->cap.max_send_wr, attr->cap.max_send_sge); + i40iw_debug("invalid SQ attributes, max_send_wr=%d max_send_sge=%d\n", + attr->cap.max_send_wr, attr->cap.max_send_sge); return NULL; } @@ -691,13 +694,13 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr info.sq_wrtrk_array = calloc(sqdepth, sizeof(*info.sq_wrtrk_array)); if (!info.sq_wrtrk_array) { - fprintf(stderr, PFX "%s: failed to allocate memory for SQ work array\n", __func__); + i40iw_debug("failed to allocate memory for SQ work array\n"); goto err_destroy_lock; } info.rq_wrid_array = calloc(rqdepth, sizeof(*info.rq_wrid_array)); if (!info.rq_wrid_array) { - fprintf(stderr, PFX "%s: failed to allocate memory for RQ work array\n", __func__); + i40iw_debug("failed to allocate memory for RQ work array\n"); goto err_free_sq_wrtrk; } @@ -706,7 +709,7 @@ struct ibv_qp *i40iw_ucreate_qp(struct ibv_pd *pd, struct ibv_qp_init_attr *attr status = i40iw_vmapped_qp(iwuqp, pd, attr, &resp, sqdepth, rqdepth, &info); if (!status) { - fprintf(stderr, PFX "%s: failed to map QP\n", __func__); + i40iw_debug("failed to map QP\n"); goto err_free_rq_wrid; } info.qp_id = resp.qp_id; @@ -772,11 +775,11 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) ret = pthread_spin_destroy(&iwuqp->lock); if (ret) - return ret; + goto err; ret = i40iw_destroy_vmapped_qp(iwuqp, iwuqp->qp.sq_base); if (ret) - return ret; + goto err; if (iwuqp->qp.sq_wrtrk_array) free(iwuqp->qp.sq_wrtrk_array); @@ -792,6 +795,9 @@ int i40iw_udestroy_qp(struct ibv_qp *qp) free(iwuqp); return 0; +err: + i40iw_debug("failed to destroy QP, status %d\n", ret); + return ret; } /** @@ -916,7 +922,7 @@ int i40iw_upost_send(struct ibv_qp *ib_qp, struct ibv_send_wr *ib_wr, struct ibv default: /* error */ err = -EINVAL; - fprintf(stderr, PFX "%s: post work request failed, invalid opcode: 0x%x\n", __func__, ib_wr->opcode); + i40iw_debug("post work request failed, invalid opcode: 0x%x\n", ib_wr->opcode); break; } @@ -960,7 +966,7 @@ int i40iw_upost_recv(struct ibv_qp *ib_qp, struct ibv_recv_wr *ib_wr, struct ibv post_recv.sg_list = sg_list; ret = iwuqp->qp.ops.iw_post_receive(&iwuqp->qp, &post_recv); if (ret) { - fprintf(stderr, PFX "%s: failed to post receives, status %d\n", __func__, ret); + i40iw_debug("failed to post receives, status %d\n", ret); if (ret == I40IW_ERR_QP_TOOMANY_WRS_POSTED) err = -ENOMEM; else