On Fri, Jan 09, 2009 at 09:23:39PM +0300, Evgeniy Polyakov wrote:

quoted text
> On Fri, Jan 09, 2009 at 07:14:46PM +0100, Joerg Roedel (joro@8bytes.org) wrote:

> > > > +static struct hash_bucket *get_hash_bucket(struct dma_debug_entry *entry,

> > > > +					   unsigned long *flags)

> > > > +{

> > > > +	int idx = hash_fn(entry);

> > > > +	unsigned long __flags;

> > > > +

> > > > +	spin_lock_irqsave(&dma_entry_hash[idx].lock, __flags);

> > > > +	*flags = __flags;

> > > > +	return &dma_entry_hash[idx];

> > > > +}

> > > > +

> > > > +/*

> > > > + * Give up exclusive access to the hash bucket

> > > > + */

> > > > +static void put_hash_bucket(struct hash_bucket *bucket,

> > > > +			    unsigned long *flags)

> > > > +{

> > > > +	unsigned long __flags = *flags;

> > > > +

> > > > +	spin_unlock_irqrestore(&bucket->lock, __flags);

> > > > +}

> > >

> > > Why do you need such ugly helpers?

> >

> > Because everything else I thought about here was even more ugly. But

> > maybe you have a better idea? I tried to lock directly in the debug_

> > functions. But this is ugly and unnecessary code duplication.

>

> I believe that having direct locking in the debug_ functions is not a

> duplication, anyone will have a direct vision on the locking and hash

> array dereference, and this will be just one additional line compared to

> the get_* call and the same number of lines for the put :)

Even more additional lines because of the additional variables needed in

every function. Anyway, I try it and if it does not look good I will

keep that change ;)
Joerg

--

To unsubscribe from this list: send the line "unsubscribe netdev" in

the body of a message to majordomo@vger.kernel.org

More majordomo info at  http://vger.kernel.org/majordomo-info.html