[openib-general] Re: [PATCH] sdp_conn_put/sdp_conn_hold race

[Libor Michalek]

On Thu, Jul 21, 2005 at 02:40:50AM +0300, Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <limichal at cisco.com>:
> > Subject: Re: [PATCH] sdp_conn_put/sdp_conn_hold race
> > 
> > On Wed, Jul 20, 2005 at 09:44:18PM +0300, Michael S. Tsirkin wrote:
> > > Quoting r. Michael S. Tsirkin <mst at mellanox.co.il>:
> > > > The current sdp_conn_put/sdp_conn_hold implementation
> > > > seems to be subject to the following race condition:
> > > 
> > > Libor, did you have the time to review this patch?
> > 
> >   Yes, looks good. The only question I had was about making
> > sdp_conn_put an inline, and calling to destruct only on the
> > last decrement.
> 
> conn_put must first do
> spin_lock_irqsave(&dev_root_s.sock_lock, flags);
> 
> so that would require making dev_root_s extern instead of static.

  You're right, I overlooked the spinlock declaration. 

> Right. How about we merge it, and then its easier to discuss
> patches on top of it? BTW, my limited benchmarking showed
> no effect on performance.

  Yes, performance looks the same to me as well. I checked it in
as is. Also, I was going to follow it up with this patch making
sdp_conn_table_remove a void, since all the error conditions
checked should never occur.

-Libor

Index: ulp/sdp/sdp_proto.h
===================================================================
--- ulp/sdp/sdp_proto.h	(revision 2873)
+++ ulp/sdp/sdp_proto.h	(working copy)
@@ -258,8 +258,6 @@
 			 off_t start_index,
 			 long *end_index);
 
-int sdp_conn_table_remove(struct sdp_sock *conn);
-
 struct sdp_sock *sdp_conn_table_lookup(s32 entry);
 
 struct sdp_sock *sdp_conn_alloc(int priority);
Index: ulp/sdp/sdp_conn.c
===================================================================
--- ulp/sdp/sdp_conn.c	(revision 2888)
+++ ulp/sdp/sdp_conn.c	(working copy)
@@ -572,28 +572,20 @@
 /*
  * sdp_conn_table_remove - remove a connection from the connection table
  */
-int sdp_conn_table_remove(struct sdp_sock *conn)
+static void sdp_conn_table_remove(struct sdp_sock *conn)
 {
-	int result = 0;
 	/*
 	 * validate entry
 	 */
-	if (SDP_DEV_SK_INVALID == conn->hashent)
-		goto done;
-
-	if (conn->hashent < 0 || conn != dev_root_s.sk_array[conn->hashent]) {
-		result = -ERANGE;
-		goto done;
-	}
+	BUG_ON(SDP_DEV_SK_INVALID == conn->hashent);
+	BUG_ON(conn->hashent < 0);
+	BUG_ON(conn != dev_root_s.sk_array[conn->hashent]);
 	/*
 	 * drop entry
 	 */
 	dev_root_s.sk_array[conn->hashent] = NULL;
 	dev_root_s.sk_entry--;
 	conn->hashent = SDP_DEV_SK_INVALID;
-
-done:
-	return result;
 }
 
 /*
@@ -691,11 +683,7 @@
 		return;
 	}
 
-	result = sdp_conn_table_remove(conn);
-	if (result < 0)
-		sdp_dbg_warn(conn, "Error <%d> removing connection <%u:%u>",
-			     result, dev_root_s.sk_entry,
-			     dev_root_s.sk_size);
+	sdp_conn_table_remove(conn);
 
 	spin_unlock_irqrestore(&dev_root_s.sock_lock, flags);