Messages in this thread | | | Date | Fri, 17 Dec 2010 17:18:29 -0800 | From | Andrew Morton <> | Subject | Re: [PATCHv6 07/16] pps: move idr stuff to pps.c |
| |
On Sat, 18 Dec 2010 04:07:38 +0300 Alexander Gordeev <lasaine@lvk.cs.msu.su> wrote:
> > > + if (err < 0) > > > + return err; > > > > The IDR interface really sucks :( > > > > What this code should be doing is > > > > retry: > > if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) > > return -ENOMEM; > > spin_lock_irq(&pps_idr_lock); > > err = idr_get_new(&pps_idr, pps, &pps->id); > > spin_unlock_irq(&pps_idr_lock); > > if (err < 0) { > > if (err == -EAGAIN) > > goto retry; > > return err; > > } > > > > this way it correctly handles the case where the idr_pre_get() > > succeeded in precharging the pool, but some other task cam in and stole > > your reservation. > > Yeah, I see. Maybe switching from spin lock to mutex and protecting the > whole thing with it can do? Like this: > > ... > mutex_lock(&pps_idr_lock); > if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) { > mutex_unlock(&pps_idr_lock); > return -ENOMEM; > } > err = idr_get_new(&pps_idr, pps, &pps->id); > mutex_unlock(&pps_idr_lock); > > if (err < 0) > return err; > ...
That works so, as long as no code path will take pps_idr_lock in the page allocator direct-reclaim path.
That's unlikely to be happening in the PPS driver of course. It's conceivable that some filesystems might want to read the time when playing with file timestamps in the direct-reclaim path, but I assume pps_idr_lock wouldn't be taken on any read-system-time paths.
| |