Re: devctl (alike?) for devfs

[Kostik Belousov]

On Tue, May 13, 2008 at 10:04:56PM +0300, Andriy Gapon wrote:
> on 12/05/2008 00:48 Kostik Belousov said the following:
> >No, we do not have a leak, but we have somewhat non-obvious behaviour.
> >
> >The cdev structure is freed only after the last reference to cdev is
> >gone. Typical holder of the reference is the devfs vnode. In the normal
> >usage, the vnode is present until both the device is destroyed _and_
> >devfs_populate_loop() run is performed. This function actually reclaim
> >the vnodes for destroyed devices, that causes last reference to cdev to
> >be dropped and memory freed.
> >
> >The populate loop is called syncronously from the upper levels. The
> >easiest method to trigger it is to do ls /dev, since it is called from
> >the devfs_lookupx().
> 
> Thank you for the explanation! ls did trigger DESTROY notifications.
> But this arbitrary delay between device entry going away and 
> notification about about it is a little bit "uncool".
> 
> So I re-wrote the patch to post notifications about deleted devices with 
> si_refcount > 0 in a fashion similar to how si_refcount=0 devices are 
> freed. I put those cdev-s onto a special list (re-/ab-using si_siblings 
> LIST link field) and bump their si_refcount to prevent them from getting 
> destroyed before the notification is sent (it would need si_name).
> Then, in dev_unlock_and_free() I send notifications and call dev_rel on 
> the devices.
> 
> I am not entirely satisfied with the code:
> 1. I don't like the way I move LIST elements from one head to the other, 
> this should be a macro in queue.h
> 2. I am not sure about the names I picked
> 3. I slightly don't like a fact that parent-child destroy notifications 
> are sent in reverse order because of my simplistic LIST usage.

I looked at your previous patch, and it seems it is much simpler to
do drop the devmtx once more then to try to abuse free lists.
In the destroy_devl(), after the 

        while (dev->si_threadcount != 0) {
                /* Use unique dummy wait ident */
                msleep(&csw, &devmtx, PRIBIO, "devdrn", hz / 10);
        }

loop, add

        mtx_unlock(&devmtx);
        if (!cold)
                devctl_notify("DEVFS", dev->si_name, "DESTROY", NULL);
        mtx_lock(&devmtx);

pgpkiZQWEwIyj.pgp
Description: PGP signature