Date: Sun, 6 Jul 2014 17:41:39 -0700 From: Adrian Chadd <adrian@freebsd.org> To: Don Lewis <truckman@freebsd.org> Cc: "freebsd-arch@freebsd.org" <arch@freebsd.org> Subject: Re: [patch] axe RF_TIMESHARE? Message-ID: <CAJ-VmokLfZ1kwqCeCmbJmxBgZXCgdc=9BTD1Feyf0_%2BVPiZKmA@mail.gmail.com> In-Reply-To: <201407070038.s670cCkb022030@gw.catspoiler.org> References: <201407070038.s670cCkb022030@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
What's it supposed to be used for?
-a
On 6 July 2014 17:38, Don Lewis <truckman@freebsd.org> 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 ? "<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"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmokLfZ1kwqCeCmbJmxBgZXCgdc=9BTD1Feyf0_%2BVPiZKmA>