Date: Sun, 23 Jan 2011 13:31:38 GMT From: Edward Tomasz Napierala <trasz@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 188093 for review Message-ID: <201101231331.p0NDVco9071305@skunkworks.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@188093?ac=10 Change 188093 by trasz@trasz_victim on 2011/01/23 13:31:18 Stop trying to be clever in *_foreach() routines and just defer freeing rctl rules to a taskqueue. Affected files ... .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_container.c#59 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_jail.c#32 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_loginclass.c#27 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_rctl.c#14 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_resource.c#59 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/jail.h#19 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/loginclass.h#12 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/rctl.h#7 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/resourcevar.h#24 edit Differences ... ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_container.c#59 (text+ko) ==== @@ -739,7 +739,7 @@ } } -static int +static void container_dampen_callback(struct container *container, void *arg2, void *arg3) { int orig, diff, hz; @@ -751,7 +751,7 @@ KASSERT(orig >= 0, ("container_dampen_callback: orig < 0")); if (orig == 0) { mtx_unlock(&container_lock); - return (0); + return; } diff = orig / 10; if (diff == 0) @@ -760,8 +760,6 @@ KASSERT(container->c_resources[RUSAGE_PCTCPU] >= 0, ("container_dampen_callback: result < 0")); mtx_unlock(&container_lock); - - return (0); } static void ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_jail.c#32 (text+ko) ==== @@ -4262,25 +4262,16 @@ SYSCTL_JAIL_PARAM(_allow, socket_af, CTLTYPE_INT | CTLFLAG_RW, "B", "Jail may create sockets other than just UNIX/IPv4/IPv6/route"); -int -prison_container_foreach(int (*callback)(struct container *container, +void +prison_container_foreach(void (*callback)(struct container *container, void *arg2, void *arg3), void *arg2, void *arg3) { - int again; struct prison *pr; -again: sx_slock(&allprison_lock); - TAILQ_FOREACH(pr, &allprison, pr_list) { - again = (callback)(&pr->pr_container, arg2, arg3); - if (again != 0) { - sx_sunlock(&allprison_lock); - goto again; - } - } + TAILQ_FOREACH(pr, &allprison, pr_list) + (callback)(&pr->pr_container, arg2, arg3); sx_sunlock(&allprison_lock); - - return (0); } #ifdef DDB ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_loginclass.c#27 (text+ko) ==== @@ -214,33 +214,16 @@ return (0); } -int -loginclass_container_foreach(int (*callback)(struct container *container, +void +loginclass_container_foreach(void (*callback)(struct container *container, void *arg2, void *arg3), void *arg2, void *arg3) { - int again; struct loginclass *lc; -again: mtx_lock(&loginclasses_lock); - LIST_FOREACH(lc, &loginclasses, lc_next) { - /* - * Callback might free the rule, which in turn could - * result in freeing loginclass, which would cause - * recursion on loginclasses_lock. - */ - loginclass_acquire(lc); - again = (callback)(&lc->lc_container, arg2, arg3); - if (again != 0) { - mtx_unlock(&loginclasses_lock); - loginclass_release(lc); - goto again; - } - loginclass_release(lc); - } + LIST_FOREACH(lc, &loginclasses, lc_next) + (callback)(&lc->lc_container, arg2, arg3); mtx_unlock(&loginclasses_lock); - - return (0); } static void ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_rctl.c#14 (text+ko) ==== @@ -55,6 +55,7 @@ #include <sys/mutex.h> #include <sys/rwlock.h> #include <sys/sbuf.h> +#include <sys/taskqueue.h> #include <sys/tree.h> #include <vm/uma.h> @@ -689,6 +690,23 @@ refcount_acquire(&rule->rr_refcount); } +static void +rctl_rule_free(void *context, int pending) +{ + struct rctl_rule *rule; + + rule = (struct rctl_rule *)context; + + KASSERT(rule->rr_refcount == 0, ("rule->rr_refcount == 0")); + + /* + * We don't need locking here; rule is guaranteed to be inaccessible. + */ + + rctl_rule_release_subject(rule); + uma_zfree(rctl_rule_zone, rule); +} + void rctl_rule_release(struct rctl_rule *rule) { @@ -696,8 +714,15 @@ KASSERT(rule->rr_refcount > 0, ("rule->rr_refcount > 0")); if (refcount_release(&rule->rr_refcount)) { - rctl_rule_release_subject(rule); - uma_zfree(rctl_rule_zone, rule); + /* + * rctl_rule_release() is often called when iterating + * over all the uidinfo structures in the system, + * holding uihashtbl_lock. Since rctl_rule_free() + * might end up calling uifree(), this would lead + * to lock recursion. Use taskqueue to avoid this. + */ + TASK_INIT(&rule->rr_task, 0, rctl_rule_free, rule); + taskqueue_enqueue(taskqueue_thread, &rule->rr_task); } } @@ -959,7 +984,7 @@ return (0); } -static int +static void rctl_rule_remove_callback(struct container *container, void *arg2, void *arg3) { struct rctl_rule *filter = (struct rctl_rule *)arg2; @@ -970,8 +995,6 @@ rw_wunlock(&rctl_lock); *((int *)arg3) += found; - - return (found); } /* @@ -980,7 +1003,7 @@ int rctl_rule_remove(struct rctl_rule *filter) { - int error, found = 0; + int found = 0; struct proc *p; if (filter->rr_subject_type == RCTL_SUBJECT_TYPE_PROCESS && @@ -994,15 +1017,12 @@ return (ESRCH); } - error = loginclass_container_foreach(rctl_rule_remove_callback, filter, + loginclass_container_foreach(rctl_rule_remove_callback, filter, (void *)&found); - KASSERT(error == 0, ("loginclass_container_foreach failed")); - error = ui_container_foreach(rctl_rule_remove_callback, filter, + ui_container_foreach(rctl_rule_remove_callback, filter, (void *)&found); - KASSERT(error == 0, ("ui_container_foreach failed")); - error = prison_container_foreach(rctl_rule_remove_callback, filter, + prison_container_foreach(rctl_rule_remove_callback, filter, (void *)&found); - KASSERT(error == 0, ("prison_container_foreach failed")); sx_assert(&allproc_lock, SA_LOCKED); rw_wlock(&rctl_lock); @@ -1196,7 +1216,7 @@ return (error); } -static int +static void rctl_get_rules_callback(struct container *container, void *arg2, void *arg3) { struct rctl_rule *filter = (struct rctl_rule *)arg2; @@ -1211,8 +1231,6 @@ sbuf_printf(sb, ","); } rw_runlock(&rctl_lock); - - return (0); } int ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_resource.c#59 (text+ko) ==== @@ -1292,36 +1292,20 @@ rw_wunlock(&uihashtbl_lock); } -int -ui_container_foreach(int (*callback)(struct container *container, +void +ui_container_foreach(void (*callback)(struct container *container, void *arg2, void *arg3), void *arg2, void *arg3) { - int again; struct uidinfo *uip; struct uihashhead *uih; -again: rw_rlock(&uihashtbl_lock); for (uih = &uihashtbl[uihash]; uih >= uihashtbl; uih--) { LIST_FOREACH(uip, uih, ui_hash) { - /* - * Callback might free the rule, which in turn could - * result in freeing uidinfo, which would cause recursion - * on uihashtbl_lock. - */ - uihold(uip); - again = (callback)(&uip->ui_container, arg2, arg3); - if (again != 0) { - rw_runlock(&uihashtbl_lock); - uifree(uip); - goto again; - } - uifree(uip); + (callback)(&uip->ui_container, arg2, arg3); } } rw_runlock(&uihashtbl_lock); - - return (0); } /* ==== //depot/projects/soc2009/trasz_limits/sys/sys/jail.h#19 (text+ko) ==== @@ -384,7 +384,7 @@ char *prison_name(struct prison *, struct prison *); int prison_priv_check(struct ucred *cred, int priv); int sysctl_jail_param(struct sysctl_oid *, void *, int , struct sysctl_req *); -int prison_container_foreach(int (*callback)(struct container *container, +void prison_container_foreach(void (*callback)(struct container *container, void *arg2, void *arg3), void *arg2, void *arg3); #endif /* _KERNEL */ ==== //depot/projects/soc2009/trasz_limits/sys/sys/loginclass.h#12 (text+ko) ==== @@ -42,7 +42,7 @@ void loginclass_acquire(struct loginclass *lc); void loginclass_release(struct loginclass *lc); struct loginclass *loginclass_find(const char *name); -int loginclass_container_foreach(int (*callback)(struct container +void loginclass_container_foreach(void (*callback)(struct container *container, void *arg2, void *arg3), void *arg2, void *arg3); #endif /* !_SYS_LOGINCLASS_H_ */ ==== //depot/projects/soc2009/trasz_limits/sys/sys/rctl.h#7 (text+ko) ==== @@ -35,6 +35,7 @@ #include <sys/cdefs.h> #include <sys/queue.h> #include <sys/types.h> +#include <sys/_task.h> struct proc; struct uidinfo; @@ -65,7 +66,7 @@ * rule and remove the previous one. */ struct rctl_rule { - int rr_subject_type; + int rr_subject_type; #ifdef DIAGNOSTIC struct { #else @@ -76,11 +77,12 @@ struct loginclass *hr_loginclass; struct prison *rs_prison; } rr_subject; - int rr_per; - int rr_resource; - int rr_action; - int64_t rr_amount; - u_int rr_refcount; + int rr_per; + int rr_resource; + int rr_action; + int64_t rr_amount; + u_int rr_refcount; + struct task rr_task; }; #define RCTL_SUBJECT_TYPE_UNDEFINED -1 ==== //depot/projects/soc2009/trasz_limits/sys/sys/resourcevar.h#24 (text+ko) ==== @@ -142,7 +142,7 @@ void uifree(struct uidinfo *uip); void uihashinit(void); void uihold(struct uidinfo *uip); -int ui_container_foreach(int (*callback)(struct container *container, +void ui_container_foreach(void (*callback)(struct container *container, void *arg2, void *arg3), void *arg2, void *arg3); #endif /* _KERNEL */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201101231331.p0NDVco9071305>