>+static int iser_free_device_ib_res(struct iser_device *device)
>+{
>+ BUG_ON(device->mr == NULL);
>+
>+ tasklet_kill(&device->cq_tasklet);
>+
>+ (void)ib_dereg_mr(device->mr);
>+ (void)ib_destroy_cq(device->cq);
>+ (void)ib_dealloc_pd(device->pd);
>+
>+ device->mr = NULL;
>+ device->cq = NULL;
>+ device->pd = NULL;
>+ return 0;
>+}
Can you eliminate the return code?
>+static int iser_free_ib_conn_res(struct iser_conn *ib_conn)
>+{
>+ BUG_ON(ib_conn == NULL);
>+
>+ iser_err("freeing conn %p cma_id %p fmr pool %p qp %p\n",
>+ ib_conn, ib_conn->cma_id,
>+ ib_conn->fmr_pool, ib_conn->qp);
>+
>+ /* qp is created only once both addr & route are resolved */
>+ if (ib_conn->fmr_pool != NULL)
>+ ib_destroy_fmr_pool(ib_conn->fmr_pool);
>+
>+ if (ib_conn->qp != NULL)
>+ rdma_destroy_qp(ib_conn->cma_id);
>+
>+ if (ib_conn->cma_id != NULL)
>+ rdma_destroy_id(ib_conn->cma_id);
Are the NULL checks needed above? Neither iser_create_device_ib_res() or
iser_create_ib_conn_res() set the values to NULL if an error occurred.
>+
>+ ib_conn->fmr_pool = NULL;
>+ ib_conn->qp = NULL;
>+ ib_conn->cma_id = NULL;
>+ kfree(ib_conn->page_vec);
>+
>+ return 0;
>+}
>+
>+/**
>+ * based on the resolved device node GUID see if there already allocated
>+ * device for this device. If there's no such, create one.
>+ */
>+static
>+struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
>+{
>+ struct list_head *p_list;
>+ struct iser_device *device = NULL;
>+
>+ mutex_lock(&ig.device_list_mutex);
>+
>+ p_list = ig.device_list.next;
>+ while (p_list != &ig.device_list) {
>+ device = list_entry(p_list, struct iser_device, ig_list);
>+ /* find if there's a match using the node GUID */
>+ if (device->ib_device->node_guid == cma_id->device->node_guid)
>+ break;
>+ }
>+
>+ if (device == NULL) {
>+ device = kzalloc(sizeof *device, GFP_KERNEL);
>+ if (device == NULL)
>+ goto end;
goto out; // see below
>+ /* assign this device to the device */
>+ device->ib_device = cma_id->device;
>+ /* init the device and link it into ig device list */
>+ if (iser_create_device_ib_res(device)) {
>+ kfree(device);
>+ device = NULL;
>+ goto end;
>+ }
>+ list_add(&device->ig_list, &ig.device_list);
>+ }
>+end:
>+ BUG_ON(device == NULL);
>+ device->refcount++;
out:
>+ mutex_unlock(&ig.device_list_mutex);
>+ return device;
>+}
>+
>+static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
>+{
>+ struct iser_conn *ib_conn;
>+
>+ ib_conn = (struct iser_conn *)cma_id->context;
>+ ib_conn->disc_evt_flag = 1;
>+
>+ /* If this event is unsolicited this means that the conn is being */
>+ /* terminated asynchronously from the iSCSI layer's perspective. */
>+ if (atomic_read(&ib_conn->state) == ISER_CONN_PENDING) {
>+ atomic_set(&ib_conn->state, ISER_CONN_DOWN);
>+ wake_up_interruptible(&ib_conn->wait);
>+ } else {
>+ if (atomic_read(&ib_conn->state) == ISER_CONN_UP) {
>+ atomic_set(&ib_conn->state, ISER_CONN_TERMINATING);
>+ iscsi_conn_failure(ib_conn->iser_conn->iscsi_conn,
>+ ISCSI_ERR_CONN_FAILED);
>+ }
>+ /* Complete the termination process if no posts are pending */
>+ if ((atomic_read(&ib_conn->post_recv_buf_count) == 0) &&
>+ (atomic_read(&ib_conn->post_send_buf_count) == 0)) {
>+ atomic_set(&ib_conn->state, ISER_CONN_DOWN);
>+ wake_up_interruptible(&ib_conn->wait);
>+ }
>+ }
Are there races here between reading ib_conn->state and setting it? Could it
have changed in between the atomic_read() and atomic_set()?
>+ src = (struct sockaddr *)src_addr;
>+ dst = (struct sockaddr *)dst_addr;
>+ err = rdma_resolve_addr(ib_conn->cma_id, src, dst, 1000);
>+ if (err) {
>+ iser_err("rdma_resolve_addr failed: %d\n", err);
>+ goto addr_failure;
>+ }
>+
>+ if (!non_blocking) {
>+ wait_event_interruptible(ib_conn->wait,
>+ atomic_read(&ib_conn->state) != ISER_CONN_PENDING);
>+
>+ if (atomic_read(&ib_conn->state) != ISER_CONN_UP) {
>+ err = -EIO;
>+ goto connect_failure;
>+ }
>+ }
>+
>+ mutex_lock(&ig.connlist_mutex);
>+ list_add(&ib_conn->conn_list, &ig.connlist);
>+ mutex_unlock(&ig.connlist_mutex);
Not sure if there's a race here or not, but rdma_resolve_addr() will result in a
callback from a separate thread. That callback could occur before the ib_conn
is added to the ig.connlist. Do you assume that ib_conn is in the connlist in
any of the callbacks?
>+int iser_post_recv(struct iser_desc *rx_desc)
>+{
>+ int ib_ret, ret_val = 0;
>+ struct ib_recv_wr recv_wr, *recv_wr_failed;
>+ struct ib_sge iov[2];
>+ struct iser_conn *ib_conn;
>+ struct iser_dto *recv_dto = &rx_desc->dto;
>+
>+ /* Retrieve conn */
>+ ib_conn = recv_dto->conn->ib_conn;
>+
>+ iser_dto_to_iov(recv_dto, iov, 2);
>+
>+ recv_wr.next = NULL;
>+ recv_wr.sg_list = iov;
>+ recv_wr.num_sge = recv_dto->regd_vector_len;
>+ recv_wr.wr_id = (unsigned long)rx_desc;
Nit - position of "=" signs above is weird.
>+static void iser_comp_error_worker(void *data)
>+{
>+ struct iser_conn *ib_conn = data;
>+
>+ if (atomic_read(&ib_conn->state) == ISER_CONN_UP) {
>+ atomic_set(&ib_conn->state, ISER_CONN_TERMINATING);
>+ iscsi_conn_failure(ib_conn->iser_conn->iscsi_conn,
>+ ISCSI_ERR_CONN_FAILED);
>+ }
Potential race reading/setting state?
- Sean
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
