From: Or Gerlitz <ogerlitz@voltaire.com>
To: Sean Hefty <sean.hefty@intel.com>
Cc: linux-kernel@vger.kernel.org, openib-general@openib.org
Subject: Re: [openib-general] [PATCH 5/6] iser RDMA CM (CMA) and IB verbs interaction
Date: Thu, 04 May 2006 16:06:09 +0300 [thread overview]
Message-ID: <4459FC41.3070700@voltaire.com> (raw)
In-Reply-To: <4459FB04.3090302@voltaire.com>
Or Gerlitz wrote:
> Or Gerlitz wrote:
>> Sean Hefty wrote:
>>>> +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()?
>> It seems that indeed a race is possible here, i am rethinking now on
>> the implementation of the ib connection states moves, thanks for
>> pointing this.
> Following a review and the clarification i have got from you re cma
> callbacks serialization, i have committed this change which removes
> unneeded state checks from two flows (disconnect handler and connect error)
This is the actual fix to the possible races you were pointing on,
thanks for your feedback.
Or.
r6924 | ogerlitz | 2006-05-04 16:03:21 +0300 (Thu, 04 May 2006) | 7 lines
changed iser ib conn state management to be done with an int variable
keeping the state and a lock. When a related race is possible the lock
is used to check (comp) or change (comp_exch) the state. When no race
can happen the state is just examined or changed.
Signed-off-by: Or Gerlitz <ogerlitz@voltaire.com
$ diffstat /tmp/6900-6924
iscsi_iser.c | 13 +++------
iscsi_iser.h | 6 +++-
iser_initiator.c | 6 ++--
iser_verbs.c | 77
++++++++++++++++++++++++++++++++++++++-----------------
4 files changed, 67 insertions(+), 35 deletions(-)
Index: iscsi_iser.h
===================================================================
--- iscsi_iser.h (revision 6900)
+++ iscsi_iser.h (revision 6924)
@@ -235,7 +235,8 @@ struct iser_device {
struct iser_conn
{
struct iscsi_iser_conn *iser_conn; /* iser conn for upcalls */
- atomic_t state; /* rdma connection state */
+ enum iser_ib_conn_state state; /* rdma connection state */
+ spinlock_t lock; /* used for state changes */
struct iser_device *device; /* device context */
struct rdma_cm_id *cma_id; /* CMA ID */
struct ib_qp *qp; /* QP */
@@ -352,4 +353,7 @@ void iser_unreg_mem(struct iser_mem_reg
int iser_post_recv(struct iser_desc *rx_desc);
int iser_post_send(struct iser_desc *tx_desc);
+
+int iser_conn_state_comp(struct iser_conn *ib_conn,
+ enum iser_ib_conn_state comp);
#endif
Index: iser_verbs.c
===================================================================
--- iser_verbs.c (revision 6900)
+++ iser_verbs.c (revision 6924)
@@ -287,6 +287,30 @@ static void iser_device_try_release(stru
mutex_unlock(&ig.device_list_mutex);
}
+int iser_conn_state_comp(struct iser_conn *ib_conn,
+ enum iser_ib_conn_state comp)
+{
+ int ret;
+
+ spin_lock_bh(&ib_conn->lock);
+ ret = (ib_conn->state == comp);
+ spin_unlock_bh(&ib_conn->lock);
+ return ret;
+}
+
+static int iser_conn_state_comp_exch(struct iser_conn *ib_conn,
+ enum iser_ib_conn_state comp,
+ enum iser_ib_conn_state exch)
+{
+ int ret;
+
+ spin_lock_bh(&ib_conn->lock);
+ if ((ret = (ib_conn->state == comp)))
+ ib_conn->state = exch;
+ spin_unlock_bh(&ib_conn->lock);
+ return ret;
+}
+
/**
* triggers start of the disconnect procedures and wait for them to be
done
*/
@@ -294,12 +318,17 @@ void iser_conn_terminate(struct iser_con
{
int err = 0;
- atomic_set(&ib_conn->state, ISER_CONN_TERMINATING);
- err = rdma_disconnect(ib_conn->cma_id);
- if (err)
- iser_bug("Failed to disconnect, conn: 0x%p err %d\n",ib_conn,err);
+ if (iser_conn_state_comp_exch(ib_conn, ISER_CONN_UP,
+ ISER_CONN_TERMINATING)) {
+ err = rdma_disconnect(ib_conn->cma_id);
+ if (err)
+ iser_err("Failed to disconnect, conn: 0x%p err %d\n",
+ ib_conn,err);
+
+ }
+
wait_event_interruptible(ib_conn->wait,
- (atomic_read(&ib_conn->state) == ISER_CONN_DOWN));
+ ib_conn->state == ISER_CONN_DOWN);
iser_conn_release(ib_conn);
}
@@ -309,7 +338,7 @@ static void iser_connect_error(struct rd
struct iser_conn *ib_conn;
ib_conn = (struct iser_conn *)cma_id->context;
- atomic_set(&ib_conn->state, ISER_CONN_DOWN);
+ ib_conn->state = ISER_CONN_DOWN;
wake_up_interruptible(&ib_conn->wait);
}
@@ -369,7 +398,7 @@ static void iser_connected_handler(struc
struct iser_conn *ib_conn;
ib_conn = (struct iser_conn *)cma_id->context;
- atomic_set(&ib_conn->state, ISER_CONN_UP);
+ ib_conn->state = ISER_CONN_UP;
wake_up_interruptible(&ib_conn->wait);
}
@@ -380,17 +409,17 @@ static void iser_disconnected_handler(st
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_UP) {
- atomic_set(&ib_conn->state, ISER_CONN_TERMINATING);
+ /* getting here when the state is UP means that the conn is being *
+ * terminated asynchronously from the iSCSI layer's perspective. */
+ if (iser_conn_state_comp_exch(ib_conn, ISER_CONN_UP,
+ 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);
+ ib_conn->state = ISER_CONN_DOWN;
wake_up_interruptible(&ib_conn->wait);
}
}
@@ -444,13 +473,14 @@ int iser_conn_init(struct iser_conn **ib
iser_err("can't alloc memory for struct iser_conn\n");
return -ENOMEM;
}
- atomic_set(&ib_conn->state, ISER_CONN_INIT);
+ ib_conn->state = ISER_CONN_INIT;
init_waitqueue_head(&ib_conn->wait);
atomic_set(&ib_conn->post_recv_buf_count, 0);
atomic_set(&ib_conn->post_send_buf_count, 0);
INIT_WORK(&ib_conn->comperror_work, iser_comp_error_worker,
ib_conn);
INIT_LIST_HEAD(&ib_conn->conn_list);
+ spin_lock_init(&ib_conn->lock);
*ibconn = ib_conn;
return 0;
@@ -477,7 +507,7 @@ int iser_connect(struct iser_conn *ib_
iser_err("connecting to: %d.%d.%d.%d, port 0x%x\n",
NIPQUAD(dst_addr->sin_addr), dst_addr->sin_port);
- atomic_set(&ib_conn->state, ISER_CONN_PENDING);
+ ib_conn->state = ISER_CONN_PENDING;
ib_conn->cma_id = rdma_create_id(iser_cma_handler,
(void *)ib_conn,
@@ -498,9 +528,9 @@ int iser_connect(struct iser_conn *ib_
if (!non_blocking) {
wait_event_interruptible(ib_conn->wait,
- atomic_read(&ib_conn->state) != ISER_CONN_PENDING);
+ (ib_conn->state != ISER_CONN_PENDING));
- if (atomic_read(&ib_conn->state) != ISER_CONN_UP) {
+ if (ib_conn->state != ISER_CONN_UP) {
err = -EIO;
goto connect_failure;
}
@@ -514,7 +544,7 @@ int iser_connect(struct iser_conn *ib_
id_failure:
ib_conn->cma_id = NULL;
addr_failure:
- atomic_set(&ib_conn->state, ISER_CONN_DOWN);
+ ib_conn->state = ISER_CONN_DOWN;
connect_failure:
iser_conn_release(ib_conn);
return err;
@@ -527,7 +557,7 @@ void iser_conn_release(struct iser_conn
{
struct iser_device *device = ib_conn->device;
- BUG_ON(atomic_read(&ib_conn->state) != ISER_CONN_DOWN);
+ BUG_ON(ib_conn->state != ISER_CONN_DOWN);
mutex_lock(&ig.connlist_mutex);
list_del(&ib_conn->conn_list);
@@ -719,16 +749,17 @@ static void iser_comp_error_worker(void
{
struct iser_conn *ib_conn = data;
- if (atomic_read(&ib_conn->state) == ISER_CONN_UP) {
- atomic_set(&ib_conn->state, ISER_CONN_TERMINATING);
+ /* getting here when the state is UP means that the conn is being *
+ * terminated asynchronously from the iSCSI layer's perspective. */
+ if (iser_conn_state_comp_exch(ib_conn, ISER_CONN_UP,
+ ISER_CONN_TERMINATING))
iscsi_conn_failure(ib_conn->iser_conn->iscsi_conn,
ISCSI_ERR_CONN_FAILED);
- }
/* complete the termination process if disconnect event was delivered *
* note there are no more non completed posts to the QP */
if (ib_conn->disc_evt_flag) {
- atomic_set(&ib_conn->state, ISER_CONN_DOWN);
+ ib_conn->state = ISER_CONN_DOWN;
wake_up_interruptible(&ib_conn->wait);
}
}
Index: iser_initiator.c
===================================================================
--- iser_initiator.c (revision 6900)
+++ iser_initiator.c (revision 6924)
@@ -370,7 +370,7 @@ int iser_send_command(struct iscsi_conn
struct iscsi_cmd *hdr = ctask->hdr;
struct scsi_cmnd *sc = ctask->sc;
- if (atomic_read(&iser_conn->ib_conn->state) != ISER_CONN_UP) {
+ if (!iser_conn_state_comp(iser_conn->ib_conn, ISER_CONN_UP)) {
iser_err("Failed to send, conn: 0x%p is not up\n", iser_conn->ib_conn);
return -EPERM;
}
@@ -454,7 +454,7 @@ int iser_send_data_out(struct iscsi_conn
unsigned int itt;
int err = 0;
- if (atomic_read(&iser_conn->ib_conn->state) != ISER_CONN_UP) {
+ if (!iser_conn_state_comp(iser_conn->ib_conn, ISER_CONN_UP)) {
iser_err("Failed to send, conn: 0x%p is not up\n", iser_conn->ib_conn);
return -EPERM;
}
@@ -528,7 +528,7 @@ int iser_send_control(struct iscsi_conn
struct iser_regd_buf *regd_buf;
struct iser_device *device;
- if (atomic_read(&iser_conn->ib_conn->state) != ISER_CONN_UP) {
+ if (!iser_conn_state_comp(iser_conn->ib_conn, ISER_CONN_UP)) {
iser_err("Failed to send, conn: 0x%p is not up\n", iser_conn->ib_conn);
return -EPERM;
}
Index: iscsi_iser.c
===================================================================
--- iscsi_iser.c (revision 6900)
+++ iscsi_iser.c (revision 6924)
@@ -649,13 +649,13 @@ iscsi_iser_ep_poll(__u64 ep_handle, int
return -EINVAL;
rc = wait_event_interruptible_timeout(ib_conn->wait,
- atomic_read(&ib_conn->state) == ISER_CONN_UP,
+ ib_conn->state == ISER_CONN_UP,
msecs_to_jiffies(timeout_ms));
/* if conn establishment failed, return error code to iscsi */
if (!rc &&
- (atomic_read(&ib_conn->state) == ISER_CONN_TERMINATING ||
- atomic_read(&ib_conn->state) == ISER_CONN_DOWN))
+ (ib_conn->state == ISER_CONN_TERMINATING ||
+ ib_conn->state == ISER_CONN_DOWN))
rc = -1;
iser_err("ib conn %p rc = %d\n", ib_conn, rc);
@@ -676,12 +676,9 @@ iscsi_iser_ep_disconnect(__u64 ep_handle
if (!ib_conn)
return;
- iser_err("ib conn %p state %d\n",ib_conn, atomic_read(&ib_conn->state));
+ iser_err("ib conn %p state %d\n",ib_conn, ib_conn->state);
- if (atomic_read(&ib_conn->state) == ISER_CONN_UP)
- iser_conn_terminate(ib_conn);
- else
- iser_conn_release(ib_conn);
+ iser_conn_terminate(ib_conn);
}
static struct scsi_host_template iscsi_iser_sht = {
next prev parent reply other threads:[~2006-05-04 13:06 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-04-27 12:30 [PATCH 0/6] iSER (iSCSI Extensions for RDMA) initiator Or Gerlitz
2006-04-27 12:30 ` [PATCH 1/6] iSER's Makefile and Kconfig Or Gerlitz
2006-04-27 12:31 ` [PATCH 2/6] iscsi_iser header file Or Gerlitz
2006-04-27 12:31 ` [PATCH 3/6] open iscsi iser transport provider code Or Gerlitz
2006-04-27 12:32 ` [PATCH 4/6] iser initiator Or Gerlitz
2006-04-27 12:32 ` [PATCH 5/6] iser RDMA CM (CMA) and IB verbs interaction Or Gerlitz
2006-04-27 12:33 ` [PATCH 6/6] iser handling of memory for RDMA Or Gerlitz
2006-04-28 23:05 ` [openib-general] [PATCH 5/6] iser RDMA CM (CMA) and IB verbsinteraction Sean Hefty
2006-04-30 12:30 ` Or Gerlitz
2006-05-01 13:02 ` Or Gerlitz
2006-05-04 13:00 ` [openib-general] [PATCH 5/6] iser RDMA CM (CMA) and IB verbs interaction Or Gerlitz
2006-05-04 13:06 ` Or Gerlitz [this message]
2006-04-27 17:01 ` [PATCH 3/6] open iscsi iser transport provider code Stephen Hemminger
2006-04-27 16:58 ` [PATCH 2/6] iscsi_iser header file Stephen Hemminger
2006-04-27 12:40 ` [PATCH 1/6] iSER's Makefile and Kconfig Jan-Benedict Glaw
2006-04-27 12:44 ` Or Gerlitz
2006-05-01 18:32 ` [PATCH 0/6] iSER (iSCSI Extensions for RDMA) initiator Roland Dreier
2006-05-02 7:56 ` Or Gerlitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4459FC41.3070700@voltaire.com \
--to=ogerlitz@voltaire.com \
--cc=linux-kernel@vger.kernel.org \
--cc=openib-general@openib.org \
--cc=sean.hefty@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.