Re: [PATCHv6 07/16] pps: move idr stuff to pps.c

Alexander Gordeev
2010-12-18T01:07:48Z

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

В Fri, 17 Dec 2010 16:13:28 -0800
Andrew Morton <akpm@linux-foundation.org> пишет:

> On Fri, 17 Dec 2010 22:54:31 +0300
> Alexander Gordeev <lasaine@lvk.cs.msu.su> wrote:
>
> > Since now idr is only used to manage char device id's and not used in
> > kernel API anymore it should be moved to pps.c. This also makes it
> > possible to release id only at actual device freeing so nobody can
> > register a pps device with the same id while our device is not freed
> > yet.
> >
> > ...
> >
> > int pps_register_cdev(struct pps_device *pps)
> > {
> > int err;
> > dev_t devt;
> >
> > + /* Get new ID for the new PPS source */
> > + if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
> > + return -ENOMEM;
> > +
> > + /* Now really allocate the PPS source.
> > + * After idr_get_new() calling the new source will be freely available
> > + * into the kernel.
> > + */
> > + spin_lock_irq(&pps_idr_lock);
> > + err = idr_get_new(&pps_idr, pps, &pps->id);
> > + spin_unlock_irq(&pps_idr_lock);
> > +
> > + 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;
...

--
Alexander

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]