From owner-svn-src-all@freebsd.org Wed Nov 18 22:16:38 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 4005F473A33; Wed, 18 Nov 2020 22:16:38 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x435.google.com (mail-wr1-x435.google.com [IPv6:2a00:1450:4864:20::435]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Cbxx039W2z4l3X; Wed, 18 Nov 2020 22:16:36 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x435.google.com with SMTP id u12so4031881wrt.0; Wed, 18 Nov 2020 14:16:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=DolZsmvjrJyFalP6idbAlKkRI3OBSv0+f02qDDcVYkY=; b=mD9qOcZHrRgoPGE2Gv0rC83eB4M7ohpP+Cy41b6zZb5uL0fhbOayNr3sOaC8z95ZSV Rbiv+NZ1RosxXZlQZ6IUnrgF+m8A3lpMVntLHhp1ZwuuP7xYq0AxnOT7vyLDDptW47Xd YYcdQzr01yoL0e7ZXSdK5TFD4SiEaHVutkwelBw+7cKDtSfRTOdBAQiIDA8aaIPEX7jw gHT2IZ+gcTqJkAwwp6RR3vSiIGWO/uI7X6za1L1IwMHpdYekzZ/7i2W8sHifclRf7xAv lL2UxLnXR6id73usxRPj3W9rpMcEBmIece4SP3xj80himBgc6D8NRBlgszwVdWQnJPP2 Qn6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=DolZsmvjrJyFalP6idbAlKkRI3OBSv0+f02qDDcVYkY=; b=O7bFjyR1Z3MAgZI9Q3nCyjv9ZJs7kWJ7ncP0+aGbh8q62s7ahp1eanZI4kpOaBVyIX izOq7NGv1SBwnrzZJN4MFgIi/P5qEVGgh0R/0B9oElSwQM+fVFF6dc3EN+2UMCTZ1E59 xhlyHaSHulIdj39wcCQgvi3btvEXw48k5V/SZZx5XfkDBSLevew6JElqbJFkjsNF9KQD yt9Xq42mMarFCAPG0HgF1f2V9r/vZfOYu6JGIbFo91mCVSxTAZucks5wiNgxUxdVTezm taqRk6th1hzL97Uj9CIQ+Pp7ej/x8XeN2186O/FkMnWCiWpvqO5i4aTkihYdmVAuyQG3 AslQ== X-Gm-Message-State: AOAM533aYZzGuTnl57iPYm+WefGwq8Jc64J4pFYo9Q6Seqn86EQOoqUh YqRCN29DrJZJFrbhDyrJeIYO0wF1RzoZhFvr2dDno/SCcUE= X-Google-Smtp-Source: ABdhPJwGphJgKGPy18xsdzqq7EmBYhTEX+Bbqm/MVEsqUa8TaBLg0M8MaND3dI9KQQLDZ+2G12z0BviAJ42D8SvkCa0= X-Received: by 2002:a5d:5146:: with SMTP id u6mr7336885wrt.66.1605737794365; Wed, 18 Nov 2020 14:16:34 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:dec7:0:0:0:0:0 with HTTP; Wed, 18 Nov 2020 14:16:33 -0800 (PST) In-Reply-To: References: <202011141922.0AEJM2ld055995@repo.freebsd.org> From: Mateusz Guzik Date: Wed, 18 Nov 2020 23:16:33 +0100 Message-ID: Subject: Re: svn commit: r367695 - in head/sys: kern sys To: John Baldwin Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4Cbxx039W2z4l3X X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=mD9qOcZH; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::435 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-3.86 / 15.00]; ARC_NA(0.00)[]; RBL_DBL_DONT_QUERY_IPS(0.00)[2a00:1450:4864:20::435:from]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; RCVD_TLS_ALL(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36:c]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; TO_DN_SOME(0.00)[]; SPAMHAUS_ZRD(0.00)[2a00:1450:4864:20::435:from:127.0.2.255]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[2a00:1450:4864:20::435:from]; NEURAL_HAM_SHORT(-0.86)[-0.862]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; MAILMAN_DEST(0.00)[svn-src-all,svn-src-head]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2020 22:16:38 -0000 On 11/17/20, John Baldwin 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