From owner-freebsd-arch@FreeBSD.ORG Mon Jul 7 00:41:41 2014 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5B959FE8; Mon, 7 Jul 2014 00:41:41 +0000 (UTC) Received: from mail-qg0-x229.google.com (mail-qg0-x229.google.com [IPv6:2607:f8b0:400d:c04::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id F0DB42664; Mon, 7 Jul 2014 00:41:40 +0000 (UTC) Received: by mail-qg0-f41.google.com with SMTP id i50so3140189qgf.14 for ; Sun, 06 Jul 2014 17:41:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=5QRNXdRj4vQhk0iJ2vZ5+HuLklZSVsjeEjAtbQyrqiU=; b=ADSxc0BkZKCCb7fTxA8D5+hKIPu0PsTLyc7AyfnkaGo9V9zONh6Mub1EHbOR31U2Zx CZpreZ6X0rwqfs9ktMnt4axXyiCdn9Q9KqXU1wxXVESCP7r8yGPuS6rWry/V9+xYC1SS zADoUuXxUa1My9DFmWSxEonvl2BS7sOE4L9TDIQsfezH/Jp+WmERiK0geddjkGGEN+iC 1iiKzaBdyDFW3RSRICtrvjMETGSwbXDaySPlWekC4YNORQUlEVs9PpP1+pkUw6XVqi9J bAJwB5q0HxiGemc2Ipf4vVe0SiYKjd9xXYtXofB2bqk7jQBpKv97fT+8kQQDkestKrlx U0rQ== MIME-Version: 1.0 X-Received: by 10.140.80.49 with SMTP id b46mr39156849qgd.102.1404693700004; Sun, 06 Jul 2014 17:41:40 -0700 (PDT) Sender: adrian.chadd@gmail.com Received: by 10.224.202.193 with HTTP; Sun, 6 Jul 2014 17:41:39 -0700 (PDT) In-Reply-To: <201407070038.s670cCkb022030@gw.catspoiler.org> References: <201407070038.s670cCkb022030@gw.catspoiler.org> Date: Sun, 6 Jul 2014 17:41:39 -0700 X-Google-Sender-Auth: D7X8H5FHezVqbGJfj0RRCKdij4M Message-ID: Subject: Re: [patch] axe RF_TIMESHARE? From: Adrian Chadd To: Don Lewis Content-Type: text/plain; charset=UTF-8 Cc: "freebsd-arch@freebsd.org" 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:41:41 -0000 Hi, What's it supposed to be used for? -a On 6 July 2014 17:38, Don Lewis wrote: > 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() */ > > _______________________________________________ > freebsd-arch@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"