Date: Wed, 18 Nov 2020 23:16:33 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r367695 - in head/sys: kern sys Message-ID: <CAGudoHGyyJRRmEUuoVkjowO5kRLpVz_VFha3MQwu%2B1P_4MU1kg@mail.gmail.com> In-Reply-To: <d3624785-1bc4-43d1-37b6-12a989444505@FreeBSD.org> References: <202011141922.0AEJM2ld055995@repo.freebsd.org> <d3624785-1bc4-43d1-37b6-12a989444505@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/17/20, John Baldwin <jhb@freebsd.org> wrote: > On 11/14/20 11:22 AM, Mateusz Guzik wrote: >> Author: mjg >> Date: Sat Nov 14 19:22:02 2020 >> New Revision: 367695 >> URL: https://svnweb.freebsd.org/changeset/base/367695 >> >> Log: >> thread: batch credential freeing >> >> Modified: >> head/sys/kern/kern_prot.c >> head/sys/kern/kern_thread.c >> head/sys/sys/ucred.h >> >> Modified: head/sys/kern/kern_prot.c >> ============================================================================== >> --- head/sys/kern/kern_prot.c Sat Nov 14 19:21:46 2020 (r367694) >> +++ head/sys/kern/kern_prot.c Sat Nov 14 19:22:02 2020 (r367695) >> @@ -2007,6 +2071,17 @@ crfree(struct ucred *cr) >> mtx_unlock(&cr->cr_mtx); >> return; >> } >> + crfree_final(cr); >> +} >> + >> +static void >> +crfree_final(struct ucred *cr) >> +{ >> + >> + KASSERT(cr->cr_users == 0, ("%s: users %d not == 0 on cred %p", >> + __func__, cr->cr_users, cr)); >> + KASSERT(cr->cr_ref == 0, ("%s: ref %d not == 0 on cred %p", >> + __func__, cr->cr_ref, cr)); >> /* > > Please add blank lines before comments. It's in style(9) and I've noticed > a pattern in your changes of not including them. > Ok. >> Modified: head/sys/sys/ucred.h >> ============================================================================== >> --- head/sys/sys/ucred.h Sat Nov 14 19:21:46 2020 (r367694) >> +++ head/sys/sys/ucred.h Sat Nov 14 19:22:02 2020 (r367695) >> @@ -114,6 +114,28 @@ struct xucred { >> struct proc; >> struct thread; >> >> +struct credbatch { >> + struct ucred *cred; >> + int users; >> + int ref; >> +}; >> + >> +static inline void >> +credbatch_prep(struct credbatch *crb) >> +{ >> + crb->cred = NULL; >> + crb->users = 0; >> + crb->ref = 0; >> +} >> +void credbatch_add(struct credbatch *crb, struct thread *td); >> +static inline void >> +credbatch_process(struct credbatch *crb) >> +{ >> + >> +} >> +void credbatch_add(struct credbatch *crb, struct thread *td); >> +void credbatch_final(struct credbatch *crb); >> + > > Do not mix prototypes and inlines, especially without spaces > around the prototype in the middle. Also, the kernel uses __inline > rather than inline (for better or for worse). The kernel has a huge mix of inline and __inline, to the point where my impression was that __inline is obsolete. > Better would be: > > static __inline void > credbatch_prep() > { > ... > } > > static __inline void > credbatch_process() > { > ... > } > > void credbatch_add(); > void credbatch_final(); > I disagree. The current header order follows how these routines are used. > It seems you just have a duplicate credbatch_add() in fact. > Indeed, I'll whack it. > Also, why have an empty inline function? > Interested parties can check the consumer (also seen in the diff) to see this is for consistency. I don't think any comments are warranted in the header. > These changes would benefit from review. > I don't think it's feasible to ask for review for everything lest it degardes to rubber stamping and I don't think this change warranted it, regardless of the cosmetic issues which can always show up. > -- > John Baldwin > -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHGyyJRRmEUuoVkjowO5kRLpVz_VFha3MQwu%2B1P_4MU1kg>