Re: [openib-general] [PATCH] Encapsulate finding of id in sa_query.c

[Krishna Kumar]

On Fri, 5 Nov 2004, Roland Dreier wrote:

 Thanks but I'm not going to apply this.  I prefer to have the locking
 and the idr lookup be explicit (and it's only done in two places so
 the cleanup is pretty minimal).

Actually three places ... And IMO, it does make the locking code look
cleaner, eg, the original code (with multiple unlocks) :

void ib_sa_cancel_query(int id, struct ib_sa_query *query)
{
unsigned long flags;

spin_lock_irqsave(idr_lock, flags);
if (idr_find(query_idr, query-id) != query) {
spin_unlock_irqrestore(idr_lock, flags);
return;
}
spin_unlock_irqrestore(idr_lock, flags);

ib_cancel_mad(query-port-agent, query-id);
}

now becomes :

void ib_sa_cancel_query(int id, struct ib_sa_query *query)
{
if (ib_sa_find_idr(id) == query)
ib_cancel_mad(query-port-agent, query-id);
}

with the find:

static inline struct ib_sa_query *ib_sa_find_idr(int id)
{
struct ib_sa_query  *query
unsigned long   flags;

spin_lock_irqsave(idr_lock, flags);
query = idr_find(query_idr, id);
spin_unlock_irqrestore(idr_lock, flags);
return query;
}

thx,

- KK

___
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

Re: [openib-general] [PATCH] Encapsulate finding of id in sa_query.c

[Roland Dreier]

Actually looking at this code one more time:

spin_lock_irqsave(idr_lock, flags);
if (idr_find(query_idr, query-id) != query) {
spin_unlock_irqrestore(idr_lock, flags);
return;
}
spin_unlock_irqrestore(idr_lock, flags);

ib_cancel_mad(query-port-agent, query-id);

I realize that it has a race.  I check that the query is still around
inside the spinlock, but the query could complete and be freed in
between the unlock and the call to ib_cancel_mad().  I'll have to add
some reference counting...

 - R.
___
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

Re: [openib-general] [PATCH] Encapsulate finding of id in sa_query.c

[Krishna Kumar]

Good catch Sean.

Yes, but since it is a race (hence uncommon), isn't it enough to let the
ib_cancel_mad handle it ? It drops out if find_send_by_wr_id fails to find
this entry.

- KK

On Mon, 8 Nov 2004, Roland Dreier wrote:

 Actually looking at this code one more time:

   spin_lock_irqsave(idr_lock, flags);
   if (idr_find(query_idr, query-id) != query) {
   spin_unlock_irqrestore(idr_lock, flags);
   return;
   }
   spin_unlock_irqrestore(idr_lock, flags);

   ib_cancel_mad(query-port-agent, query-id);

 I realize that it has a race.  I check that the query is still around
 inside the spinlock, but the query could complete and be freed in
 between the unlock and the call to ib_cancel_mad().  I'll have to add
 some reference counting...

  - R.


___
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general