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
-------------- next part --------------
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);
 	}