From owner-freebsd-arch@FreeBSD.ORG Mon Jul 7 00:38:21 2014 Return-Path: Delivered-To: arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 11CFCE88; Mon, 7 Jul 2014 00:38:21 +0000 (UTC) Received: from gw.catspoiler.org (gw.catspoiler.org [75.1.14.242]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DBB3525DF; Mon, 7 Jul 2014 00:38:20 +0000 (UTC) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id s670cCkb022030; Sun, 6 Jul 2014 17:38:16 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <201407070038.s670cCkb022030@gw.catspoiler.org> Date: Sun, 6 Jul 2014 17:38:12 -0700 (PDT) From: Don Lewis Subject: [patch] axe RF_TIMESHARE? To: arch@FreeBSD.org, jhb@FreeBSD.org MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Jul 2014 00:38:21 -0000 When he was reviewing my previous batch of subr_rman.c patches, John Baldwin suggested getting rid of the RF_TIMESHARE feature. There is nothing in the tree that uses it, and he didn't think that anything ever used it. Getting rid of RF_TIMESHARE gets rid of quite a bit of complexity and unbloats the the size of the code on i386 by more than 500 bytes. Before: text data bss dec hex filename 9663 376 28 10067 2753 subr_rman.o After: text data bss dec hex filename 9128 376 28 9532 253c subr_rman.o The int_rman_activate_resource() and int_rman_deactivate_resource() functions become trivial and I elected to manually inline both. It looks to me like the special handling of RF_ACTIVE isn't needed in reserve_resource_bound() isn't needed and doesn't have to be deferred to the end of the function. The comments seemed to indicate that this code was iffy for RF_TIMESHARE in any case. Should we nuke RF_TIMESHARE? Should this change be MFC'ed, and if so, how far back? Comments on the attached patch? Index: sys/kern/subr_rman.c =================================================================== --- sys/kern/subr_rman.c (revision 268273) +++ sys/kern/subr_rman.c (working copy) @@ -109,9 +109,6 @@ struct rman_head rman_head; static struct mtx rman_mtx; /* mutex to protect rman_head */ -static int int_rman_activate_resource(struct rman *rm, struct resource_i *r, - struct resource_i **whohas); -static int int_rman_deactivate_resource(struct resource_i *r); static int int_rman_release_resource(struct rman *rm, struct resource_i *r); static __inline struct resource_i * @@ -321,7 +318,7 @@ /* Not supported for shared resources. */ r = rr->__r_i; - if (r->r_flags & (RF_TIMESHARE | RF_SHAREABLE)) + if (r->r_flags & RF_SHAREABLE) return (EINVAL); /* @@ -434,7 +431,7 @@ return (0); } -#define SHARE_TYPE(f) (f & (RF_SHAREABLE | RF_TIMESHARE | RF_PREFETCHABLE)) +#define SHARE_TYPE(f) (f & (RF_SHAREABLE | RF_PREFETCHABLE)) struct resource * rman_reserve_resource_bound(struct rman *rm, u_long start, u_long end, @@ -451,10 +448,9 @@ "length %#lx, flags %u, device %s\n", rm->rm_descr, start, end, count, flags, dev == NULL ? "" : device_get_nameunit(dev))); - KASSERT((flags & (RF_WANTED | RF_FIRSTSHARE)) == 0, + KASSERT((flags & RF_FIRSTSHARE) == 0, ("invalid flags %#x", flags)); - new_rflags = (flags & ~(RF_ACTIVE | RF_WANTED | RF_FIRSTSHARE)) | - RF_ALLOCATED; + new_rflags = (flags & ~RF_FIRSTSHARE) | RF_ALLOCATED; mtx_lock(rm->rm_mtx); @@ -600,7 +596,7 @@ * additional work, but this does not seem warranted.) */ DPRINTF(("no unshared regions found\n")); - if ((flags & (RF_SHAREABLE | RF_TIMESHARE)) == 0) + if ((flags & RF_SHAREABLE) == 0) goto out; for (s = r; s && s->r_end <= end; s = TAILQ_NEXT(s, r_link)) { @@ -635,25 +631,11 @@ goto out; } } - /* * We couldn't find anything. */ + out: - /* - * If the user specified RF_ACTIVE in flags, we attempt to atomically - * activate the resource. If this fails, we release the resource - * and indicate overall failure. (This behavior probably doesn't - * make sense for RF_TIMESHARE-type resources.) - */ - if (rv && (flags & RF_ACTIVE) != 0) { - struct resource_i *whohas; - if (int_rman_activate_resource(rm, rv, &whohas)) { - int_rman_release_resource(rm, rv); - rv = NULL; - } - } - mtx_unlock(rm->rm_mtx); return (rv == NULL ? NULL : &rv->r_r); } @@ -667,91 +649,17 @@ dev)); } -static int -int_rman_activate_resource(struct rman *rm, struct resource_i *r, - struct resource_i **whohas) -{ - struct resource_i *s; - int ok; - - /* - * If we are not timesharing, then there is nothing much to do. - * If we already have the resource, then there is nothing at all to do. - * If we are not on a sharing list with anybody else, then there is - * little to do. - */ - if ((r->r_flags & RF_TIMESHARE) == 0 - || (r->r_flags & RF_ACTIVE) != 0 - || r->r_sharehead == NULL) { - r->r_flags |= RF_ACTIVE; - return 0; - } - - ok = 1; - for (s = LIST_FIRST(r->r_sharehead); s && ok; - s = LIST_NEXT(s, r_sharelink)) { - if ((s->r_flags & RF_ACTIVE) != 0) { - ok = 0; - *whohas = s; - } - } - if (ok) { - r->r_flags |= RF_ACTIVE; - return 0; - } - return EBUSY; -} - int rman_activate_resource(struct resource *re) { - int rv; - struct resource_i *r, *whohas; + struct resource_i *r; struct rman *rm; r = re->__r_i; rm = r->r_rm; mtx_lock(rm->rm_mtx); - rv = int_rman_activate_resource(rm, r, &whohas); + r->r_flags |= RF_ACTIVE; mtx_unlock(rm->rm_mtx); - return rv; -} - -int -rman_await_resource(struct resource *re, int pri, int timo) -{ - int rv; - struct resource_i *r, *whohas; - struct rman *rm; - - r = re->__r_i; - rm = r->r_rm; - mtx_lock(rm->rm_mtx); - for (;;) { - rv = int_rman_activate_resource(rm, r, &whohas); - if (rv != EBUSY) - return (rv); /* returns with mutex held */ - - if (r->r_sharehead == NULL) - panic("rman_await_resource"); - whohas->r_flags |= RF_WANTED; - rv = msleep(r->r_sharehead, rm->rm_mtx, pri, "rmwait", timo); - if (rv) { - mtx_unlock(rm->rm_mtx); - return (rv); - } - } -} - -static int -int_rman_deactivate_resource(struct resource_i *r) -{ - - r->r_flags &= ~RF_ACTIVE; - if (r->r_flags & RF_WANTED) { - r->r_flags &= ~RF_WANTED; - wakeup(r->r_sharehead); - } return 0; } @@ -762,7 +670,7 @@ rm = r->__r_i->r_rm; mtx_lock(rm->rm_mtx); - int_rman_deactivate_resource(r->__r_i); + r->__r_i->r_flags &= ~RF_ACTIVE; mtx_unlock(rm->rm_mtx); return 0; } @@ -773,7 +681,7 @@ struct resource_i *s, *t; if (r->r_flags & RF_ACTIVE) - int_rman_deactivate_resource(r); + r->r_flags &= ~RF_ACTIVE; /* * Check for a sharing list first. If there is one, then we don't Index: sys/sys/rman.h =================================================================== --- sys/sys/rman.h (revision 268273) +++ sys/sys/rman.h (working copy) @@ -42,8 +42,8 @@ #define RF_ALLOCATED 0x0001 /* resource has been reserved */ #define RF_ACTIVE 0x0002 /* resource allocation has been activated */ #define RF_SHAREABLE 0x0004 /* resource permits contemporaneous sharing */ -#define RF_TIMESHARE 0x0008 /* resource permits time-division sharing */ -#define RF_WANTED 0x0010 /* somebody is waiting for this resource */ +#define RF_SPARE1 0x0008 +#define RF_SPARE2 0x0010 #define RF_FIRSTSHARE 0x0020 /* first in sharing list */ #define RF_PREFETCHABLE 0x0040 /* resource is prefetchable */ #define RF_OPTIONAL 0x0080 /* for bus_alloc_resources() */