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@xxxxxxxxxxxx] > > Sent: Saturday, December 10, 2016 8:34 AM > > To: Nikolova, Tatyana E <tatyana.e.nikolova@xxxxxxxxx> > > Cc: jgunthorpe@xxxxxxxxxxxxxxxxxxxx; dledford@xxxxxxxxxx; linux- > > rdma@xxxxxxxxxxxxxxx; e1000-rdma@xxxxxxxxxxxxxxxxxxxxx > > 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@xxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature