svn commit: r367695 - in head/sys: kern sys

[Mateusz Guzik]

On 11/17/20, John Baldwin <jhb at 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>