[openib-general] Re: [PATCH] kDAPL: remove dapl_os_assert()
James Lentini
jlentini at netapp.com
Thu Jun 23 13:55:38 PDT 2005
Why did you remove the asserts in dapl_ib_post_recv() and
dapl_ib_post_send()?
My argument in favor of retaining them is that dapl_evd_wc_to_event()
will crash if the cookie NULL. A BUG_ON will detect this situation
sooner rather than later and therefore make the problem easier to
diagnose. Did I miss something?
On Thu, 23 Jun 2005, Tom Duffy wrote:
tduffy> This patch removes dapl_os_assert(). In most cases, replacing with
tduffy> BUG_ON(!). Some cases, I just called panic() where others I removed the
tduffy> assert all together because the next line dereferences the pointer
tduffy> anyways.
tduffy>
tduffy> Signed-off-by: Tom Duffy <tduffy at sun.com>
tduffy>
tduffy> Index: linux-kernel/dat-provider/dapl_cookie.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_cookie.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_cookie.c (working copy)
tduffy> @@ -184,7 +184,7 @@ u32 dapl_cb_get(struct dapl_cookie_buffe
tduffy> u32 dat_status;
tduffy> int new_head;
tduffy>
tduffy> - dapl_os_assert(NULL != cookie_ptr);
tduffy> + BUG_ON(cookie_ptr == NULL);
tduffy>
tduffy> new_head = (atomic_read(&buffer->head) + 1) % buffer->pool_size;
tduffy>
tduffy> Index: linux-kernel/dat-provider/dapl_openib_util.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_openib_util.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_openib_util.c (working copy)
tduffy> @@ -783,7 +783,7 @@ u32 dapl_ib_get_gid(struct ib_device *hc
tduffy> {
tduffy> int status;
tduffy>
tduffy> - dapl_os_assert(hca);
tduffy> + BUG_ON(!hca);
tduffy>
tduffy> if (gid) {
tduffy> status = ib_query_gid(hca, port, 0, gid);
tduffy> Index: linux-kernel/dat-provider/dapl_openib_qp.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_openib_qp.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_openib_qp.c (working copy)
tduffy> @@ -76,7 +76,6 @@ u32 dapl_ib_qp_alloc(struct dapl_ia *ia_
tduffy>
tduffy> attr = &ep_ptr->param.ep_attr;
tduffy>
tduffy> - dapl_os_assert(ep_ptr->param.pz != NULL);
tduffy> ib_pd_handle = ((struct dapl_pz *)ep_ptr->param.pz)->pd;
tduffy> ib_hca_handle = ia_ptr->hca->ib_hca_handle;
tduffy>
tduffy> Index: linux-kernel/dat-provider/dapl_openib_dto.h
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_openib_dto.h (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_openib_dto.h (working copy)
tduffy> @@ -43,8 +43,6 @@ static inline u32 dapl_ib_post_recv(stru
tduffy> struct ib_sge *sg_list;
tduffy> int status, i, total_len = 0;
tduffy>
tduffy> - dapl_os_assert(NULL != cookie);
tduffy> -
tduffy> sg_list = ep->recv_iov;
tduffy> for (i = 0; i < num_segments; i++, sg_list++) {
tduffy> sg_list->addr = local_iov[i].virtual_address;
tduffy> @@ -84,8 +82,6 @@ static inline u32 dapl_ib_post_send(stru
tduffy> struct ib_sge *sg_list;
tduffy> int status, i, total_len = 0;
tduffy>
tduffy> - dapl_os_assert(NULL != cookie);
tduffy> -
tduffy> sg_list = ep->send_iov;
tduffy> for (i = 0; i < num_segments; i++, sg_list++) {
tduffy> sg_list->addr = local_iov[i].virtual_address;
tduffy> Index: linux-kernel/dat-provider/dapl_ia.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_ia.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_ia.c (working copy)
tduffy> @@ -371,13 +371,13 @@ bail:
tduffy> */
tduffy> void dapl_ia_free(struct dapl_ia *ia)
tduffy> {
tduffy> - dapl_os_assert(ia->async_error_evd == NULL);
tduffy> - dapl_os_assert(list_empty(&ia->lmr_list));
tduffy> - dapl_os_assert(list_empty(&ia->rmr_list));
tduffy> - dapl_os_assert(list_empty(&ia->ep_list));
tduffy> - dapl_os_assert(list_empty(&ia->evd_list));
tduffy> - dapl_os_assert(list_empty(&ia->psp_list));
tduffy> - dapl_os_assert(list_empty(&ia->rsp_list));
tduffy> + BUG_ON(ia->async_error_evd != NULL);
tduffy> + BUG_ON(!list_empty(&ia->lmr_list));
tduffy> + BUG_ON(!list_empty(&ia->rmr_list));
tduffy> + BUG_ON(!list_empty(&ia->ep_list));
tduffy> + BUG_ON(!list_empty(&ia->evd_list));
tduffy> + BUG_ON(!list_empty(&ia->psp_list));
tduffy> + BUG_ON(!list_empty(&ia->rsp_list));
tduffy>
tduffy> dapl_hca_unlink_ia(ia->hca, ia);
tduffy> /* no need to destroy ia->common.lock */
tduffy> Index: linux-kernel/dat-provider/dapl_rmr.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_rmr.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_rmr.c (working copy)
tduffy> @@ -126,8 +126,7 @@ static u64 dapl_rmr_get_address(DAT_REGI
tduffy> * DAT_MEM_TYPE_IA
tduffy> * DAT_MEM_TYPE_BYPASS
tduffy> */
tduffy> - dapl_os_assert(0);
tduffy> - return 0;
tduffy> + panic("unimplemented or unknown memory type\n");
tduffy> }
tduffy> }
tduffy>
tduffy> Index: linux-kernel/dat-provider/dapl_ep.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_ep.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_ep.c (working copy)
tduffy> @@ -675,8 +675,8 @@ u32 dapl_ep_free(struct dat_ep *ep)
tduffy> */
tduffy> (void)dapl_ep_disconnect((struct dat_ep *)ep_ptr,
tduffy> DAT_CLOSE_ABRUPT_FLAG);
tduffy> - dapl_os_assert(ep_ptr->param.ep_state == DAT_EP_STATE_DISCONNECTED ||
tduffy> - ep_ptr->param.ep_state == DAT_EP_STATE_UNCONNECTED);
tduffy> + BUG_ON(ep_ptr->param.ep_state != DAT_EP_STATE_DISCONNECTED &&
tduffy> + ep_ptr->param.ep_state != DAT_EP_STATE_UNCONNECTED);
tduffy>
tduffy> /*
tduffy> * Do verification of parameters and the state change atomically.
tduffy> @@ -1538,11 +1538,11 @@ u32 dapl_ep_modify(struct dat_ep *ep, en
tduffy> * occurred. But they're important to the logic of this routine,
tduffy> * so we check.
tduffy> */
tduffy> - dapl_os_assert(ep1 == ep2);
tduffy> - dapl_os_assert(ep_attr2.max_recv_dtos == ep_attr1.max_recv_dtos);
tduffy> - dapl_os_assert(ep_attr2.max_request_dtos == ep_attr1.max_request_dtos);
tduffy> - dapl_os_assert(ep_attr2.max_recv_iov == ep_attr1.max_recv_iov);
tduffy> - dapl_os_assert(ep_attr2.max_request_iov == ep_attr1.max_request_iov);
tduffy> + BUG_ON(ep1 != ep2);
tduffy> + BUG_ON(ep_attr2.max_recv_dtos != ep_attr1.max_recv_dtos);
tduffy> + BUG_ON(ep_attr2.max_request_dtos != ep_attr1.max_request_dtos);
tduffy> + BUG_ON(ep_attr2.max_recv_iov != ep_attr1.max_recv_iov);
tduffy> + BUG_ON(ep_attr2.max_request_iov != ep_attr1.max_request_iov);
tduffy>
tduffy> copy_of_old_ep = *ep2;
tduffy>
tduffy> @@ -1607,10 +1607,10 @@ u32 dapl_ep_modify(struct dat_ep *ep, en
tduffy> * because the parameter validate routine should protect us,
tduffy> * but it's an important enough point that we assert it.
tduffy> */
tduffy> - dapl_os_assert((ep2->param.ep_state
tduffy> - != DAT_EP_STATE_PASSIVE_CONNECTION_PENDING)
tduffy> - && (ep2->param.ep_state
tduffy> - != DAT_EP_STATE_ACTIVE_CONNECTION_PENDING));
tduffy> + BUG_ON(ep2->param.ep_state ==
tduffy> + DAT_EP_STATE_PASSIVE_CONNECTION_PENDING ||
tduffy> + ep2->param.ep_state ==
tduffy> + DAT_EP_STATE_ACTIVE_CONNECTION_PENDING);
tduffy>
tduffy> new_ep.qp = alloc_ep.qp;
tduffy> }
tduffy> Index: linux-kernel/dat-provider/dapl_cr.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_cr.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_cr.c (working copy)
tduffy> @@ -271,7 +271,7 @@ static u32 dapl_connection_request(struc
tduffy> DAT_EP_STATE_TENTATIVE_CONNECTION_PENDING;
tduffy> } else {
tduffy> /* RSP */
tduffy> - dapl_os_assert(sp->sp.type == DAT_SP_TYPE_RSP);
tduffy> + BUG_ON(sp->sp.type != DAT_SP_TYPE_RSP);
tduffy> ep->param.ep_state =
tduffy> DAT_EP_STATE_PASSIVE_CONNECTION_PENDING;
tduffy> }
tduffy> @@ -516,8 +516,7 @@ void dapl_cr_callback(struct dapl_cm_ctx
tduffy> break;
tduffy> default:
tduffy> evd = NULL;
tduffy> - dapl_os_assert(0); /* shouldn't happen */
tduffy> - break;
tduffy> + panic("unknown callback event\n");
tduffy> }
tduffy>
tduffy> if (evd != NULL)
tduffy> Index: linux-kernel/dat-provider/dapl_evd.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_evd.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_evd.c (working copy)
tduffy> @@ -243,7 +243,7 @@ static u32 dapl_evd_dealloc(struct dapl_
tduffy> u32 status = DAT_SUCCESS;
tduffy> struct dapl_ia *ia;
tduffy>
tduffy> - dapl_os_assert(atomic_read(&evd->evd_ref_count) == 0);
tduffy> + BUG_ON(atomic_read(&evd->evd_ref_count) != 0);
tduffy>
tduffy> /*
tduffy> * Destroy the CQ first, to keep any more callbacks from coming
tduffy> @@ -330,9 +330,9 @@ static void dapl_evd_post_event(struct d
tduffy> event->event_number);
tduffy>
tduffy> status = dapl_rbuf_add(&evd->pending_event_queue, event);
tduffy> - dapl_os_assert(status == DAT_SUCCESS);
tduffy> + BUG_ON(status != DAT_SUCCESS);
tduffy>
tduffy> - dapl_os_assert(evd->evd_state == DAPL_EVD_STATE_OPEN);
tduffy> + BUG_ON(evd->evd_state != DAPL_EVD_STATE_OPEN);
tduffy>
tduffy> if (evd->evd_producer_locking_needed)
tduffy> spin_unlock_irqrestore(&evd->common.lock,
tduffy> @@ -522,10 +522,9 @@ static void dapl_evd_wc_to_event(struct
tduffy> dto_status = dapl_ib_get_dto_status(wc);
tduffy>
tduffy> cookie = (struct dapl_cookie *) (unsigned long) wc->wr_id;
tduffy> - dapl_os_assert(NULL != cookie);
tduffy>
tduffy> ep = cookie->ep;
tduffy> - dapl_os_assert(NULL != ep);
tduffy> + BUG_ON(ep == NULL);
tduffy>
tduffy> event->evd = (struct dat_evd *)evd;
tduffy>
tduffy> @@ -550,26 +549,6 @@ static void dapl_evd_wc_to_event(struct
tduffy> event_data->user_cookie = cookie->val.dto.cookie;
tduffy> event_data->status = dto_status;
tduffy>
tduffy> -#if 0
tduffy> - /* Currently mthca is not setting the opcode in */
tduffy> - /* succesful wc. The opcode will be IB_WC_SEND or */
tduffy> - /* IB_WC_RECV according the is_send bit */
tduffy> - /* We can not check the following assert for now */
tduffy> - if (dto_status == DAT_DTO_SUCCESS) {
tduffy> - enum ib_wc_opcode ib_opcode = wc->opcode;
tduffy> - enum dapl_dto_type dto_type = cookie->val.dto.type;
tduffy> - dapl_os_assert((ib_opcode == IB_WC_SEND &&
tduffy> - dto_type == DAPL_DTO_TYPE_SEND) ||
tduffy> - (ib_opcode == IB_WC_RECV &&
tduffy> - dto_type == DAPL_DTO_TYPE_RECV) ||
tduffy> - (ib_opcide == IB_WC_RDMA_WRITE &&
tduffy> - dto_type == DAPL_DTO_TYPE_RDMA_WRITE) ||
tduffy> - (ib_opcode == IB_WC_RDMA_READ &&
tduffy> - dto_type == DAPL_DTO_TYPE_RDMA_READ));
tduffy> -
tduffy> - }
tduffy> -#endif
tduffy> -
tduffy> if (cookie->val.dto.type == DAPL_DTO_TYPE_SEND ||
tduffy> cookie->val.dto.type == DAPL_DTO_TYPE_RDMA_WRITE) {
tduffy> /* Get size from DTO; CQE value may be off. */
tduffy> @@ -593,7 +572,7 @@ static void dapl_evd_wc_to_event(struct
tduffy> event_data->user_cookie = cookie->val.rmr.cookie;
tduffy>
tduffy> if (dto_status == DAT_DTO_SUCCESS) {
tduffy> - dapl_os_assert(wc->opcode == IB_WC_BIND_MW);
tduffy> + BUG_ON(wc->opcode != IB_WC_BIND_MW);
tduffy> event_data->status = DAT_RMR_BIND_SUCCESS;
tduffy> } else {
tduffy> dapl_dbg_log(DAPL_DBG_TYPE_DTO_COMP_ERR,
tduffy> @@ -607,10 +586,7 @@ static void dapl_evd_wc_to_event(struct
tduffy> break;
tduffy> }
tduffy> default:
tduffy> - {
tduffy> - dapl_os_assert(!"Invalid Operation type");
tduffy> - break;
tduffy> - }
tduffy> + panic("Invalid Operation type\n");
tduffy> } /* end switch */
tduffy> }
tduffy>
tduffy> @@ -652,7 +628,7 @@ void dapl_evd_qp_async_error_callback(st
tduffy> ep->param.ep_state = DAT_EP_STATE_DISCONNECTED;
tduffy> }
tduffy>
tduffy> - dapl_os_assert(async_evd != NULL);
tduffy> + BUG_ON(async_evd == NULL);
tduffy>
tduffy> status = dapl_ib_get_async_event(cause, &async_event);
tduffy> if (status == DAT_SUCCESS) {
tduffy> @@ -792,9 +768,7 @@ void dapl_evd_connection_callback(struct
tduffy> default:
tduffy> spin_unlock_irqrestore(&ep->common.lock, ep->common.flags);
tduffy> evd = NULL;
tduffy> -
tduffy> - dapl_os_assert(0); /* shouldn't happen */
tduffy> - break;
tduffy> + panic("bad event\n");
tduffy> }
tduffy>
tduffy> /*
tduffy> @@ -838,7 +812,7 @@ static void dapl_evd_dto_callback(struct
tduffy>
tduffy> evd = (struct dapl_evd *)user_context;
tduffy>
tduffy> - dapl_os_assert(evd->cq == cq);
tduffy> + BUG_ON(evd->cq != cq);
tduffy>
tduffy> /* Read once. */
tduffy> state = *(volatile enum dapl_evd_state *)&evd->evd_state;
tduffy> Index: linux-kernel/dat-provider/dapl_util.h
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_util.h (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_util.h (working copy)
tduffy> @@ -48,15 +48,6 @@
tduffy> #include <asm/system.h>
tduffy> #endif
tduffy>
tduffy> -#define dapl_os_assert(expression) \
tduffy> - do { \
tduffy> - if (!(expression)) { \
tduffy> - panic("ASSERTION fail in %s:%i:%s\n", \
tduffy> - __FILE__, __LINE__, __func__); \
tduffy> - } \
tduffy> - } while (0)
tduffy> -
tduffy> -
tduffy> /* dapl_os_atomic_assign
tduffy> *
tduffy> * assign 'new_value' to '*v' if the current value
tduffy> Index: linux-kernel/dat-provider/dapl_sp.c
tduffy> ===================================================================
tduffy> --- linux-kernel/dat-provider/dapl_sp.c (revision 2701)
tduffy> +++ linux-kernel/dat-provider/dapl_sp.c (working copy)
tduffy> @@ -64,7 +64,7 @@ static struct dapl_sp *dapl_sp_alloc(str
tduffy>
tduffy> void dapl_sp_dealloc(struct dapl_sp *sp)
tduffy> {
tduffy> - dapl_os_assert(list_empty(&sp->cr_list));
tduffy> + BUG_ON(!list_empty(&sp->cr_list));
tduffy>
tduffy> kfree(sp);
tduffy> }
tduffy>
tduffy>
More information about the general
mailing list