From owner-freebsd-hackers  Thu Nov  7 18:46:37 1996
Return-Path: owner-hackers
Received: (from root@localhost)
          by freefall.freebsd.org (8.7.5/8.7.3) id SAA00683
          for hackers-outgoing; Thu, 7 Nov 1996 18:46:37 -0800 (PST)
Received: from parkplace.cet.co.jp (parkplace.cet.co.jp [202.32.64.1])
          by freefall.freebsd.org (8.7.5/8.7.3) with ESMTP id SAA00678;
          Thu, 7 Nov 1996 18:46:34 -0800 (PST)
Received: from localhost (michaelh@localhost) by parkplace.cet.co.jp (8.8.2/CET-v2.1) with SMTP id CAA01157; Fri, 8 Nov 1996 02:46:14 GMT
Date: Fri, 8 Nov 1996 11:46:13 +0900 (JST)
From: Michael Hancock <michaelh@cet.co.jp>
To: Terry Lambert <terry@lambert.org>
cc: dg@root.com, ponds!rivers@dg-rtp.dg.com, dyson@freefall.freebsd.org,
        freebsd-hackers@freefall.freebsd.org
Subject: Re: More info on the daily panics...
In-Reply-To: <199611071813.LAA10315@phaeton.artisoft.com>
Message-ID: <Pine.SV4.3.95.961108111353.760C-100000@parkplace.cet.co.jp>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-hackers@FreeBSD.ORG
X-Loop: FreeBSD.org
Precedence: bulk

On Thu, 7 Nov 1996, Terry Lambert wrote:

> If you are really concerned that this will mask a future multiple vrele()
> problem, I suggest you put the assert in vrele() and prevent the queue
> from ever getting corrupted that way in the first place.

We really need to engineer asserts into the kernel.  They're kind of there
with the #ifdef DIAGNOSTICS stuff, but this is ugly I hate looking at
#ifdef's.  I'm happy that we *have* a model.

In our code they are often in the wrong place.  In the case of vrele() 
they are too far into the function body, they should be the first things
checked for, near the parameter list.
 
The current model for parameter checking is this:

1) #ifdef DIAGNOSTICS ... #endif for parameter checks we assume the caller
does correctly, but want to check sometime.

2) if ( expr ) { ... } for checks we think are important enough to require
in it runtime code.

We also have stuff embedded, but I won't talk about that.

I like to improve upon the model like this:

Improved use of assertions
====================================================================
void
vrele(vp)
        register struct vnode *vp;
{

        ASSUME(vp != NULL, "vrele: vp shouldn't be null");

        REQUIRE(vp->v_usecount > 0, "vrele: ref count shouldn't already be
zero");

        vp->v_usecount--;

        if ((vp->v_usecount == 1) &&
                vp->v_object &&
                (vp->v_object->flags & OBJ_VFS_REF)) {
                vp->v_object->flags &= ~OBJ_VFS_REF;
                vm_object_deallocate(vp->v_object);
                return;
        }

        if (vp->v_usecount > 0)
                return;

[ .. ]


Current
====================================================================
void
vrele(vp)
        register struct vnode *vp;
{

#ifdef DIAGNOSTIC
        if (vp == NULL)
                panic("vrele: null vp");
#endif

        vp->v_usecount--;

        if ((vp->v_usecount == 1) &&
                vp->v_object &&
                (vp->v_object->flags & OBJ_VFS_REF)) {
                vp->v_object->flags &= ~OBJ_VFS_REF;
                vm_object_deallocate(vp->v_object);
                return;
        }

        if (vp->v_usecount > 0)
                return;

        if (vp->v_usecount < 0) {
#ifdef DIAGNOSTIC
                vprint("vrele: negative ref count", vp);
#endif
                panic("vrele: negative reference cnt");
        }

[ ... ]


Regards,



Mike Hancock