From owner-freebsd-arch Thu Sep 28 11:39:41 2000 Delivered-To: freebsd-arch@freebsd.org Received: from fw.wintelcom.net (ns1.wintelcom.net [209.1.153.20]) by hub.freebsd.org (Postfix) with ESMTP id E506D37B422; Thu, 28 Sep 2000 11:39:09 -0700 (PDT) Received: (from bright@localhost) by fw.wintelcom.net (8.10.0/8.10.0) id e8SId9D04813; Thu, 28 Sep 2000 11:39:09 -0700 (PDT) Date: Thu, 28 Sep 2000 11:39:09 -0700 From: Alfred Perlstein To: Mike Smith Cc: arch@freebsd.org Subject: Re: we need atomic_t Message-ID: <20000928113907.V7553@fw.wintelcom.net> References: <20000928110637.U7553@fw.wintelcom.net> <200009281816.e8SIGLA01632@mass.osd.bsdi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.4i In-Reply-To: <200009281816.e8SIGLA01632@mass.osd.bsdi.com>; from msmith@freebsd.org on Thu, Sep 28, 2000 at 11:16:21AM -0700 Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG * Mike Smith [000928 11:14] wrote: > > Linux has a datatype called "atomic_t", very useful for refcounts > > and struct counters like tcpstat. My impression is that it's the > > largest type an arch can support atomic ops on without weird > > gyrations and/or extremely expensive operations. > > sig_atomic_t. I'll look at that. > > > This would replace our atomic_op_type with just atomic_op and make > > code easier to read and get right. > > I strongly disagree. The atomic__ interface is useful and > necessary and should remain. I don't agree that this would make anything > easier. In particular, the explicit use of the atomic_* operations makes > the atomicity constraints very clear. I really hate it, it slows down my programming, most of these counters need to be the largest the platform will allow just avoid overflows, you remeber the struct file refcount problem we had with apache right? What's the point of having counters and refcounts that easily overflow? > > I'm already seeing a pretty good examples of where this can be > > applied: > > > > 1) struct ucred->cr_ref > > 2) struct uidinfo->ui_ref > > 3) tcpstats > > 4) other stats :) > > 5) mbuf external ref counts > > > > I don't have the gcc-assembler-foo to do this optimally without > > directly copying from Linux which isn't acceptable. > > In most cases, you're manipulating the reference count under a mutex > (since there's no other way to avoid the race where someone else frees > your structure while you're in the process of dereferencing it), so this > is largely unnecessary. It's not possible for this to happen, this is why struct ucred, mbuf and uidinfo lend themselves to mpsafeness pretty easily with atomic refcounts. You do need to own the parent structure lock (struct proc/socket/etc) so that two codepaths can not deref the same pointer at the same time, but you need to do that anyway. Basically you need to own a lock on whatever allows access to the pointer to the ucred/uidinfo/mbuf. The atomic ops are particularly useful when you have multiple different structures that point to a single object (ucred and mbuf particularly) When you instantiate a ucred, it has a refcount of 1 and you can be garanteed that no one else if referencing it, right before it's shallow copied (a point to it is in more than one place) the count is at 2 which prevents free() from other codepaths, you're expected to "own" the parent structure of be it a refcount of 1 or a lock on the parent. I'd much rather have: void crfree(cr) struct ucred *cr; { if (atomic_dec_test(&cr->cr_ref, 1) == 0) { /* * Some callers of crget(), such as nfs_statfs(), * allocate a temporary credential, but don't * allocate a uidinfo structure. */ if (cr->cr_uidinfo != NULL) uifree(cr->cr_uidinfo); FREE((caddr_t)cr, M_CRED); } } than: void crfree(cr) struct ucred *cr; { mtx_enter(&cr->cr_mtx, MTX_DEF); if (--cr->cr_ref == 0) { mtx_exit(&cr->cr_mtx, MTX_DEF); /* * Some callers of crget(), such as nfs_statfs(), * allocate a temporary credential, but don't * allocate a uidinfo structure. */ if (cr->cr_uidinfo != NULL) uifree(cr->cr_uidinfo); FREE((caddr_t)cr, M_CRED); } else { mtx_exit(&cr->cr_mtx, MTX_DEF); } } Note that there's a interesting cascade assertion going on: if (atomic_dec_test(&cr->cr_ref, 1) == 0) { /* * then i have exclusive ownership of this ucred * so I can free the uidinfo it references without a lock * because my parent is locked. */ Is there a problem with this scheme? > > Can anyone snap this up? I'd really appreciate it. > > Hold it right there, sunshine. 8) pfft! :) -- -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org] "I have the heart of a child; I keep it in a jar on my desk." To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message