Re: Mark setrtable(2) as NOLOCK

[Martin Pieuchot]

On 19/02/18(Mon) 16:31, Mark Kettenis wrote:
> > Date: Mon, 19 Feb 2018 16:22:30 +0100
> > From: Martin Pieuchot 
> > 
> > Now that suser() is no longer messing with a per-process field, we
> > can directly turn setrtable(2) as NOLOCK.
> > 
> > Apart from sanity checks this syscall writes an int-sized per-process
> > field.  Is a memory barrier enough?
> > 
> > ok?
> 
> I'm wondering if we need a memory barrier here at all.  What race are
> you trying to prevent here?

I just wanted to be sure the change is visible to other threads of the
same process when the current one is finished.  Syscalls that grab the
KERNEL_LOCK() or a rwlock have an implicit barrier when they release
their lock.  So I was wondering if I needed to add one here.  But I'd
be glade to hear if this is not needed and why :)

> > Index: kern/syscalls.master
> > ===
> > RCS file: /cvs/src/sys/kern/syscalls.master,v
> > retrieving revision 1.180
> > diff -u -p -r1.180 syscalls.master
> > --- kern/syscalls.master12 Dec 2017 01:12:34 -  1.180
> > +++ kern/syscalls.master19 Feb 2018 15:05:34 -
> > @@ -532,7 +532,7 @@
> >  307OBSOL   statfs53
> >  308OBSOL   fstatfs53
> >  309OBSOL   fhstatfs53
> > -310STD { int sys_setrtable(int rtableid); }
> > +310STD NOLOCK  { int sys_setrtable(int rtableid); }
> >  311STD NOLOCK  { int sys_getrtable(void); }
> >  312OBSOL   t32_getdirentries
> >  313STD { int sys_faccessat(int fd, const char *path, \
> > Index: kern/uipc_syscalls.c
> > ===
> > RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> > retrieving revision 1.165
> > diff -u -p -r1.165 uipc_syscalls.c
> > --- kern/uipc_syscalls.c19 Feb 2018 08:59:52 -  1.165
> > +++ kern/uipc_syscalls.c19 Feb 2018 15:19:35 -
> > @@ -1189,6 +1190,9 @@ sys_setrtable(struct proc *p, void *v, r
> > return (EINVAL);
> >  
> > p->p_p->ps_rtableid = (u_int)rtableid;
> > +   /* Force visibility */
> > +   membar_producer();
> > +
> > return (0);
> >  }
> >  
> > 
> > 
> 

Re: Mark setrtable(2) as NOLOCK

[Mark Kettenis]

> Date: Mon, 19 Feb 2018 16:22:30 +0100
> From: Martin Pieuchot 
> 
> Now that suser() is no longer messing with a per-process field, we
> can directly turn setrtable(2) as NOLOCK.
> 
> Apart from sanity checks this syscall writes an int-sized per-process
> field.  Is a memory barrier enough?
> 
> ok?

I'm wondering if we need a memory barrier here at all.  What race are
you trying to prevent here?

> Index: kern/syscalls.master
> ===
> RCS file: /cvs/src/sys/kern/syscalls.master,v
> retrieving revision 1.180
> diff -u -p -r1.180 syscalls.master
> --- kern/syscalls.master  12 Dec 2017 01:12:34 -  1.180
> +++ kern/syscalls.master  19 Feb 2018 15:05:34 -
> @@ -532,7 +532,7 @@
>  307  OBSOL   statfs53
>  308  OBSOL   fstatfs53
>  309  OBSOL   fhstatfs53
> -310  STD { int sys_setrtable(int rtableid); }
> +310  STD NOLOCK  { int sys_setrtable(int rtableid); }
>  311  STD NOLOCK  { int sys_getrtable(void); }
>  312  OBSOL   t32_getdirentries
>  313  STD { int sys_faccessat(int fd, const char *path, \
> Index: kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.165
> diff -u -p -r1.165 uipc_syscalls.c
> --- kern/uipc_syscalls.c  19 Feb 2018 08:59:52 -  1.165
> +++ kern/uipc_syscalls.c  19 Feb 2018 15:19:35 -
> @@ -1189,6 +1190,9 @@ sys_setrtable(struct proc *p, void *v, r
>   return (EINVAL);
>  
>   p->p_p->ps_rtableid = (u_int)rtableid;
> + /* Force visibility */
> + membar_producer();
> +
>   return (0);
>  }
>  
> 
> 

Mark setrtable(2) as NOLOCK

[Martin Pieuchot]

Now that suser() is no longer messing with a per-process field, we
can directly turn setrtable(2) as NOLOCK.

Apart from sanity checks this syscall writes an int-sized per-process
field.  Is a memory barrier enough?

ok?

Index: kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.180
diff -u -p -r1.180 syscalls.master
--- kern/syscalls.master12 Dec 2017 01:12:34 -  1.180
+++ kern/syscalls.master19 Feb 2018 15:05:34 -
@@ -532,7 +532,7 @@
 307OBSOL   statfs53
 308OBSOL   fstatfs53
 309OBSOL   fhstatfs53
-310STD { int sys_setrtable(int rtableid); }
+310STD NOLOCK  { int sys_setrtable(int rtableid); }
 311STD NOLOCK  { int sys_getrtable(void); }
 312OBSOL   t32_getdirentries
 313STD { int sys_faccessat(int fd, const char *path, \
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.165
diff -u -p -r1.165 uipc_syscalls.c
--- kern/uipc_syscalls.c19 Feb 2018 08:59:52 -  1.165
+++ kern/uipc_syscalls.c19 Feb 2018 15:19:35 -
@@ -1189,6 +1190,9 @@ sys_setrtable(struct proc *p, void *v, r
return (EINVAL);
 
p->p_p->ps_rtableid = (u_int)rtableid;
+   /* Force visibility */
+   membar_producer();
+
return (0);
 }