From owner-freebsd-hackers@FreeBSD.ORG Tue May 13 19:09:34 2008 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2EA511065673; Tue, 13 May 2008 19:09:34 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from hosted.kievnet.com (hosted.kievnet.com [193.138.144.10]) by mx1.freebsd.org (Postfix) with ESMTP id B82938FC15; Tue, 13 May 2008 19:09:33 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from localhost ([127.0.0.1] helo=edge.pp.kiev.ua) by hosted.kievnet.com with esmtpa (Exim 4.62) (envelope-from ) id 1JvzsS-0000iw-Oj; Tue, 13 May 2008 22:09:32 +0300 Message-ID: <4829E76B.8010204@icyb.net.ua> Date: Tue, 13 May 2008 22:09:31 +0300 From: Andriy Gapon User-Agent: Thunderbird 2.0.0.12 (X11/20080320) MIME-Version: 1.0 To: Kostik Belousov References: <480E4269.2090604@icyb.net.ua> <480FBAB9.1000904@icyb.net.ua> <48103F36.6060707@icyb.net.ua> <200804240811.26183.jhb@freebsd.org> <4810FD1E.70602@icyb.net.ua> <20080425095009.GD18958@deviant.kiev.zoral.com.ua> <4811E6BC.4060306@icyb.net.ua> <20080425143646.GF18958@deviant.kiev.zoral.com.ua> <48275C0C.2040601@icyb.net.ua> <20080511214833.GB18958@deviant.kiev.zoral.com.ua> <4829E658.3000605@icyb.net.ua> In-Reply-To: <4829E658.3000605@icyb.net.ua> Content-Type: multipart/mixed; boundary="------------000802000206000606030404" Cc: freebsd-hackers@freebsd.org Subject: Re: devctl (alike?) for devfs X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 May 2008 19:09:34 -0000 This is a multi-part message in MIME format. --------------000802000206000606030404 Content-Type: text/plain; charset=KOI8-U; format=flowed Content-Transfer-Encoding: 7bit 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 --------------000802000206000606030404 Content-Type: text/plain; name="devfs-notify.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="devfs-notify.diff" 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 #include #include +#include #include #include #include @@ -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); } --------------000802000206000606030404--