Re: [PATCH 1/2] dma-debug: add dma_debug_resize_entries() toadjust the number of dma_debug_entries

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:

> On Tue, 14 Apr 2009 12:58:23 +0200
> Ingo Molnar <mingo@xxxxxxx> wrote:
> 
> > 
> > * FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:
> > 
> > > +int dma_debug_resize_entries(u32 num_entries)
> > > +{
> > > +	int i, delta, ret = 0;
> > > +	unsigned long flags;
> > > +	struct dma_debug_entry *entry;
> > > +
> > > +	spin_lock_irqsave(&free_entries_lock, flags);
> > > +
> > > +	if (nr_total_entries < num_entries) {
> > > +		delta = num_entries - nr_total_entries;
> > > +
> > > +		for (i = 0; i < delta; i++) {
> > > +			entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
> > > +			if (!entry)
> > > +				break;
> > 
> > hm, using GFP_ATOMIC within a spinlock is not a very nice thing to 
> > do in general. While this is boot-only and the GFP_ATOMIC will 
> > likely succeed, this could become non-boot functionality and then 
> > it's exposed to the momentary VM pressure situation that might make 
> > GFP_ATOMIC fail.
> > 
> > Please fix this to be GFP_KERNEL.
> 
> Ok, fixed though I'm not sure it matters. It's unlikely that this 
> could become non-boot functionality so I chose a simple way.
> 
> I've attached a fixed patch. It's unlikely that this function is 
> called concurrently so I don't try hard.

thanks, it looks nice and clean now.

GFP_ATOMIC (mis-)use is prominently detected by Andrew's 
barf-o-meter so even though it's probably fine here it's still 
better to just not get into the habit. People would then see it in 
an otherwise tidy looking piece of code and think it's an example to 
follow.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/