Messages in this thread | | | Date | Fri, 12 Jun 2009 12:51:51 -0500 | From | Jack Steiner <> | Subject | Re: [Patch 02/12] GRU - add user request to explicitly unload a gru context |
| |
On Mon, Jun 08, 2009 at 04:05:25PM -0700, Andrew Morton wrote: > On Mon, 08 Jun 2009 12:16:50 -0500 > steiner@sgi.com wrote: > > > /* > > + * Free all kernel contexts that are not currently in use. > > + * Returns 0 if all freed, else number of inuse context. > > + */ > > +static int gru_free_kernel_contexts(void) > > +{ > > + struct gru_blade_state *bs; > > + struct gru_thread_state *kgts; > > + int bid, ret = 0; > > + > > + for (bid = 0; bid < GRU_MAX_BLADES; bid++) { > > + bs = gru_base[bid]; > > + if (!bs) > > + continue; > > + if (down_write_trylock(&bs->bs_kgts_sema)) { > > trylocks are always lame - they add a rarely-executed code path where > bugs can lurk. They're often an admission that the locking is screwed > up. > > I don't know if the latter is true here, but it would be helpful to add > a comment explaining what's going on, and why this unusual and > troublesome locking primitive is being used.
Agree that trylock is frequently a crappy way to avoid a real fix for potential ABBA deadlocks. However, in this case no potential locking inversion is being avoided. The code in gru_free_kernel_contexts() is simply trying to free any non-busy contexts where "busy" is defined as locked. Contexts that are "busy" are simply skipped.
Also, this is a path that is rarely used. It exists primarily for stress testing.
I'll add comments to code to make this clearer.
> > > + kgts = bs->bs_kgts; > > + if (kgts && kgts->ts_gru) > > + gru_unload_context(kgts, 0); > > + kfree(kgts); > > + bs->bs_kgts = NULL; > > + up_write(&bs->bs_kgts_sema); > > nit: the kfree() can be moved outside the locked region.
Yuck. Done...
> > > + } else { > > + ret++; > > + } > > + } > > + return ret; > > +}
--- jack
| |