Re: CVS commit: src/sys/kern

2020-02-17 Thread Andrew Doran
Hi,

On Thu, Feb 06, 2020 at 06:33:55PM +0100, J. Hannken-Illjes wrote:

> > On 12. Jan 2020, at 18:49, Andrew Doran  wrote:
> > 
> > Module Name:src
> > Committed By:   ad
> > Date:   Sun Jan 12 17:49:17 UTC 2020
> > 
> > Modified Files:
> > src/sys/kern: vfs_vnode.c
> > 
> > Log Message:
> > vput(): don't drop the vnode lock, carry the hold over into vrelel() which
> > might need it anyway.
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.106 -r1.107 src/sys/kern/vfs_vnode.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> > 
> 
>  vput(vnode_t *vp)
>  {
> +   int lktype;
> 
> -   VOP_UNLOCK(vp);
> -   vrele(vp);
> +   if ((vp->v_vflag & VV_LOCKSWORK) == 0) {
> +   lktype = LK_EXCLUSIVE;
> +   } else {
> +   lktype = VOP_ISLOCKED(vp);
> +   KASSERT(lktype != LK_NONE);
> +   }
> +   mutex_enter(vp->v_interlock);
> +   vrelel(vp, 0, lktype);
>  }
> 
> This is quite wrong, from the manual:
> 
>  VOP_ISLOCKED(vp)
>   Test if the vnode vp is locked.  Possible return values are
>   LK_EXCLUSIVE, LK_SHARED or 0 for lock held exclusively by the
>   calling thread, shared lock held by anyone or unlocked,
>   respectively.
> 
>   This function must never be used to make locking decisions at
>   run time: it is provided only for diagnostic purposes.
> 
> I suppose you cannot determine if the current thread holds
> a shared lock.

The intention of that last sentence was to dissuade people from doing error
prone, complicated stuff with locks.  To my mind that idea of the locking
primitives is to take something difficult (concurrency) and package it up in
a way that's much easier to think about and work with - and yes it's still
complicated.

There are limited cases when I think it makes sense to infer lock ownership,
for example when known for sure that a RW lock is held but the type of hold
needs to be known - like this case.  The failure case there is the lock
being unheld, which would be caught by LOCKDEBUG in this case.  Consider
that a rw_exit() with the lock unheld by the caller will produce what you
might charitably call "undefined behaviour" in a non-LOCKDEBUG kernel.

Andrew


Re: CVS commit: src/sys/arch/powerpc/powerpc

2020-02-17 Thread Andrew Doran
On Wed, Feb 05, 2020 at 12:46:57PM +0900, Rin Okuyama wrote:

> Hi,
> 
> On 2019/12/06 5:55, Andrew Doran wrote:
> > Module Name:src
> > Committed By:   ad
> > Date:   Thu Dec  5 20:55:24 UTC 2019
> > 
> > Modified Files:
> > src/sys/arch/powerpc/powerpc: powerpc_machdep.c
> > 
> > Log Message:
> > Need to call userret() from cpu_ast().
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.74 -r1.75 src/sys/arch/powerpc/powerpc/powerpc_machdep.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> > 
> > 
> > Modified files:
> > 
> > Index: src/sys/arch/powerpc/powerpc/powerpc_machdep.c
> > diff -u src/sys/arch/powerpc/powerpc/powerpc_machdep.c:1.74 
> > src/sys/arch/powerpc/powerpc/powerpc_machdep.c:1.75
> > --- src/sys/arch/powerpc/powerpc/powerpc_machdep.c:1.74 Sat Nov 23 
> > 19:40:36 2019
> > +++ src/sys/arch/powerpc/powerpc/powerpc_machdep.c  Thu Dec  5 20:55:24 2019
> > @@ -1,4 +1,4 @@
> > -/* $NetBSD: powerpc_machdep.c,v 1.74 2019/11/23 19:40:36 ad Exp $  */
> > +/* $NetBSD: powerpc_machdep.c,v 1.75 2019/12/05 20:55:24 ad Exp $  */
> >   /*
> >* Copyright (C) 1995, 1996 Wolfgang Solfrank.
> > @@ -32,7 +32,7 @@
> >*/
> >   #include 
> > -__KERNEL_RCSID(0, "$NetBSD: powerpc_machdep.c,v 1.74 2019/11/23 19:40:36 
> > ad Exp $");
> > +__KERNEL_RCSID(0, "$NetBSD: powerpc_machdep.c,v 1.75 2019/12/05 20:55:24 
> > ad Exp $");
> >   #include "opt_altivec.h"
> >   #include "opt_ddb.h"
> > @@ -373,6 +373,8 @@ void
> >   cpu_ast(struct lwp *l, struct cpu_info *ci)
> >   {
> > l->l_md.md_astpending = 0;  /* we are about to do it */
> > +   __insn_barrier();
> > +   userret(l, l->l_md.md_utf);
> > if (l->l_pflag & LP_OWEUPC) {
> > l->l_pflag &= ~LP_OWEUPC;
> > @@ -400,7 +402,7 @@ cpu_need_resched(struct cpu_info *ci, st
> > cpu_send_ipi(cpu_index(ci), IPI_AST);
> >   #endif
> > } else {
> > -   l->l_md.md_astpending = 1;  /* force call to ast() 
> > */
> > +   l->l_md.md_astpending = 1;  /* force call to cpu_ast() */
> > }
> >   }
> > 
> 
> This commit makes userret() called twice with AST; cpu_ast() is
> invoked from
> 
> booke/trap.c,
> https://nxr.netbsd.org/xref/src/sys/arch/powerpc/booke/trap.c#815
> 
> ibm4xx/trap.c, and
> https://nxr.netbsd.org/xref/src/sys/arch/powerpc/ibm4xx/trap.c#276
> 
> powerpc/trap.c.
> https://nxr.netbsd.org/xref/src/sys/arch/powerpc/powerpc/trap.c#348
> 
> For all cases, userret() is called afterward. (Precisely speaking,
> for ibm4xx, mi_userret(9) is used instead. This should probably be
> replaced by userret().)
> 
> Also, other ports test (l->l_pflag & LP_OWEUPC) before mi_userret(9).

I corrected the cpu_ast() case.  Yes it's curious why ibm4xx calls
mi_userret() directly; that seems wrong (I have not changed it though).  I
think it definitely makes more sense to deal with OWEUPC before userret().

Andrew
 
> Thanks,
> rin