Re: devctl (alike?) for devfs

[Andriy Gapon]

on 13/05/2008 22:04 Andriy Gapon said the following:

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.

Oops! I attached the older patch.
New patch is here.

--
Andriy Gapon

diff --git a/sys/kern/kern_conf.c b/sys/kern/kern_conf.c
index 1db25f8..7106182 100644
--- a/sys/kern/kern_conf.c
+++ b/sys/kern/kern_conf.c
@@ -30,6 +30,7 @@
 #include <sys/param.h>
 #include <sys/kernel.h>
 #include <sys/systm.h>
+#include <sys/bus.h>
 #include <sys/bio.h>
 #include <sys/lock.h>
 #include <sys/mutex.h>
@@ -63,6 +64,8 @@ static struct cdev_priv_list cdevp_free_list =
     TAILQ_HEAD_INITIALIZER(cdevp_free_list);
 static SLIST_HEAD(free_cdevsw, cdevsw) cdevsw_gt_post_list =
     SLIST_HEAD_INITIALIZER();
+static LIST_HEAD(cdev_notif, cdev) cdev_notif_list =
+    LIST_HEAD_INITIALIZER(cdev_notif_list);
 
 void
 dev_lock(void)
@@ -82,8 +85,10 @@ dev_unlock_and_free(void)
 {
        struct cdev_priv_list cdp_free;
        struct free_cdevsw csw_free;
+       struct cdev_notif cd_notif;
        struct cdev_priv *cdp;
        struct cdevsw *csw;
+       struct cdev *cd;
 
        mtx_assert(&devmtx, MA_OWNED);
 
@@ -95,10 +100,23 @@ dev_unlock_and_free(void)
        TAILQ_CONCAT(&cdp_free, &cdevp_free_list, cdp_list);
        csw_free = cdevsw_gt_post_list;
        SLIST_INIT(&cdevsw_gt_post_list);
+       /* XXX How to do the following nicely? */
+       cd_notif = cdev_notif_list;
+       if (!LIST_EMPTY(&cd_notif))
+               LIST_FIRST(&cd_notif)->si_siblings.le_prev = 
&LIST_FIRST(&cd_notif);
+       LIST_INIT(&cdev_notif_list);
 
        mtx_unlock(&devmtx);
 
+       while ((cd = LIST_FIRST(&cd_notif)) != NULL) {
+               if (!cold)
+                       devctl_notify("DEVFS", cd->si_name, "DESTROY", NULL);
+               LIST_REMOVE(cd, si_siblings);
+               dev_rel(cd);
+       }
        while ((cdp = TAILQ_FIRST(&cdp_free)) != NULL) {
+               if (!cold)
+                       devctl_notify("DEVFS", cdp->cdp_c.si_name, "DESTROY", 
NULL);
                TAILQ_REMOVE(&cdp_free, cdp, cdp_list);
                devfs_free(&cdp->cdp_c);
        }
@@ -706,6 +724,10 @@ make_dev_credv(int flags, struct cdevsw *devsw, int 
minornr,
        devfs_create(dev);
        clean_unrhdrl(devfs_inos);
        dev_unlock_and_free();
+
+       if (!cold)
+               devctl_notify("DEVFS", dev->si_name, "CREATE", NULL);
+
        return (dev);
 }
 
@@ -794,6 +816,10 @@ make_dev_alias(struct cdev *pdev, const char *fmt, ...)
        clean_unrhdrl(devfs_inos);
        dev_unlock();
        dev_depends(pdev, dev);
+
+       if (!cold)
+               devctl_notify("DEVFS", dev->si_name, "CREATE", NULL);
+
        return (dev);
 }
 
@@ -861,6 +887,9 @@ destroy_devl(struct cdev *dev)
 
        if (dev->si_refcount > 0) {
                LIST_INSERT_HEAD(&dead_cdevsw.d_devs, dev, si_list);
+               /* (re-/ab-)use si_siblings for destroy notification list */
+               LIST_INSERT_HEAD(&cdev_notif_list, dev, si_siblings);
+               dev_refl(dev);  /* Avoid race with dev_rel() */
        } else {
                dev_free_devlocked(dev);
        }

_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"