On Thu, Jul 21, 2005 at 02:40:50AM +0300, Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <[EMAIL PROTECTED]>:
> > 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 <[EMAIL PROTECTED]>:
> > > > 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);
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
