Re: assertion "_kernel_lock_held()" failed, uipc_socket2.c: ipsec

[Martin Pieuchot]

On 12/11/17(Sun) 22:10, Stuart Henderson wrote:
> On 2017/11/12 22:48, Martin Pieuchot wrote:
> > On 12/11/17(Sun) 21:30, Stuart Henderson wrote:
> > > iked box, GENERIC.MP + WITNESS, -current as of Friday 10th:
> > 
> > Weird, did you tweak "kern.splassert" on this box?   Otherwise is looks
> > like a major corruption.
> 
> It would have kern.splassert=2. (I know this can cause problems
> sometimes, though this would be the first time in 5+ years I've bumped
> into it, most of my routers where I have serial console have this set).

Well the panic below correspond to a value of 0 or > 3.

> > > login: panic: kernel diagnostic assertion "_kernel_lock_held()" failed: 
> > > file "/src/cvs-openbsd/sys/kern/uipc_socket2.c", line 310
> > ^^^
> > Looks like one CPU is triggering this.
> > 
> > > splassert: soassertlocked: want 1 have 256
> > > 
> > > panic: spl assertion failure in soassertlocked
> > ^^^
> > That can't be coming from the same CPU..
> > 
> > 
> > 
> > 
> > > Starting stack trace...
> > > Faulted in traceback, aborting...
> > > panic(splassert: if_down: want 1 have 256
> > > panic: spl assertion failure in if_down) at
> > > Faulted in traceback, aborting...
> > > panicsplassert: if_down: want 1 have 256
> > > +0x133panic: spl assertion failure in if_down
> > > Faulted in traceback, aborting...
> > > 
> > > 
> > > 
> > > It's stuck at this point, I can't enter ddb.
> > 
> > Are you running with WITNESS on purpose?  Can you reproduce such problem
> > without it?  I'm not saying it's WITNESS fault, but it's clear that
> > WITNESS kernels aren't ready for production yet.
> > 
> 
> I'm trying to get more information because it had either hanged or
> panicked previously (it didn't have serial connected at the time and
> the machine was needed so it had to be rebooted before I had chance
> to dig into it).

>From which snapshot was the kernel that hanged or panic'd?

Re: mbuf statistics, tracking of drops

[David Gwynne]

On Sun, Nov 12, 2017 at 11:23:31PM +0100, Gregor Best wrote:
> Hi Martin,
> 
> On Sun, Nov 12, 2017 at 03:40:59PM +0100, Martin Pieuchot wrote:
> > [...]
> > It does, some comments below.
> > [...]
> 
> Wonderful.
> 
> > [...]
> > This would be an approximation because it might happen that after
> > returning NULL the second pool_get(9) won't sleep.  However I think
> > it's the way to go because m_get(9) calls that can wait are generally
> > not performance critical.
> > [...]
> 
> I had not considered that the second pool_get might not block at all. On
> the other hand `netstat -m` says "requests for memory delayed" for that
> counter, so returning immediately on the second try not counting as a
> delay is OK, I think.
> 
> > [...]
> > Please do not expand the splx().  Only the counter need it.  Simply put
> > the NULL check after the splx().
> > [...]
> 
> I'm not sure I understand. If I move the NULL check after the splx(),
> counters_leave() will already have been called so increasing the counter
> value has no effect anymore. The only additional things running under
> splnet will be a few assignments, so I think moving the splx() a bit
> further down does not hurt too much.
> 
> Alternatively, I've attached a slightly different suggestion which
> doesn't expand the scope of the splx() but duplicates a bit of code.
> Does that make more sense?

pools maintain count of how many times they failed to provide an
allocation. you can watch this with vmstat -m or systat pool.
however, we could use that to populate mb_drops too.

the diff below moves populating the mbstat from kern_sysctl to
uipc_mbuf, and adds copying of pr_nfails in.

if we want to count the number of "delays", i could easily add that
to pools too and copy it out in sysctl_mbstat.

Index: sys/sysctl.h
===
RCS file: /cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.175
diff -u -p -r1.175 sysctl.h
--- sys/sysctl.h12 Oct 2017 09:14:16 -  1.175
+++ sys/sysctl.h13 Nov 2017 03:42:32 -
@@ -936,6 +936,7 @@ int sysctl_rdstruct(void *, size_t *, vo
 int sysctl_struct(void *, size_t *, void *, size_t, void *, size_t);
 int sysctl_file(int *, u_int, char *, size_t *, struct proc *);
 int sysctl_doproc(int *, u_int, char *, size_t *);
+int sysctl_mbstat(int *, u_int, void *, size_t *, void *, size_t);
 struct mbuf_queue;
 int sysctl_mq(int *, u_int, void *, size_t *, void *, size_t,
 struct mbuf_queue *);
Index: kern/kern_sysctl.c
===
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.330
diff -u -p -r1.330 kern_sysctl.c
--- kern/kern_sysctl.c  11 Aug 2017 21:24:19 -  1.330
+++ kern/kern_sysctl.c  13 Nov 2017 03:42:32 -
@@ -392,24 +392,9 @@ kern_sysctl(int *name, u_int namelen, vo
case KERN_FILE:
return (sysctl_file(name + 1, namelen - 1, oldp, oldlenp, p));
 #endif
-   case KERN_MBSTAT: {
-   extern struct cpumem *mbstat;
-   uint64_t counters[MBSTAT_COUNT];
-   struct mbstat mbs;
-   unsigned int i;
-
-   memset(, 0, sizeof(mbs));
-   counters_read(mbstat, counters, MBSTAT_COUNT);
-   for (i = 0; i < MBSTAT_TYPES; i++)
-   mbs.m_mtypes[i] = counters[i];
-
-   mbs.m_drops = counters[MBSTAT_DROPS];
-   mbs.m_wait = counters[MBSTAT_WAIT];
-   mbs.m_drain = counters[MBSTAT_DRAIN];
-
-   return (sysctl_rdstruct(oldp, oldlenp, newp,
-   , sizeof(mbs)));
-   }
+   case KERN_MBSTAT:
+   return (sysctl_mbstat(name + 1, namelen -1, oldp, oldlenp,
+   newp, newlen));
 #if defined(GPROF) || defined(DDBPROF)
case KERN_PROF:
return (sysctl_doprof(name + 1, namelen - 1, oldp, oldlenp,
Index: kern/uipc_mbuf.c
===
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.250
diff -u -p -r1.250 uipc_mbuf.c
--- kern/uipc_mbuf.c12 Oct 2017 09:14:16 -  1.250
+++ kern/uipc_mbuf.c13 Nov 2017 03:42:32 -
@@ -1421,6 +1421,30 @@ m_pool_init(struct pool *pp, u_int size,
pool_set_constraints(pp, _dma_contig);
 }
 
+int
+sysctl_mbstat(int *name, u_int namelen, void *oldp, size_t *oldlenp,
+void *newp, size_t newlen)
+{
+   uint64_t counters[MBSTAT_COUNT];
+   struct mbstat mbs;
+   unsigned int i;
+
+   if (namelen != 0)
+   return (ENOTDIR);
+
+   memset(, 0, sizeof(mbs));
+
+   counters_read(mbstat, counters, MBSTAT_COUNT);
+   for (i = 0; i < MBSTAT_TYPES; i++)
+   mbs.m_mtypes[i] = counters[i];
+
+   mbs.m_drops = counters[MBSTAT_DROPS] + mbpool.pr_nfail;
+   mbs.m_wait = counters[MBSTAT_WAIT];
+   mbs.m_drain = counters[MBSTAT_DRAIN]; /* pool gcs? */
+
+   return 

Re: try to bundle multiple packets on an ifq before sending

[Martin Pieuchot]

On 13/11/17(Mon) 10:56, David Gwynne wrote:
> On Sun, Nov 12, 2017 at 02:45:05PM +0100, Martin Pieuchot wrote:
> > [...]
> > We're currently using net_tq() to distribute load for incoming packets.
> > So I believe you should schedule the task on the current taskq or the
> > first one if coming from userland.
> 
> /em shrug.
> 
> i dont know how to tell what the current softnet tq is.

That's something we need to know.  This will allow to have per taskq
data structure.

> would it
> be enough to simply not mix if the ifq_idx into the argument to
> net_tq?

Maybe.  If a CPU processing packets from myx0 in softnet0 and want to
send forward via myx1, using net_tq() like you do it know will pick
softnet1.
My suggestion was to keep softnet1 busy with processing packets coming
from myx1 and enqueue the task on softnet0.  This way no inter CPU
communication is needed.
But without testing we can't tell if it's more efficient the other way.

> > >   because ifq work could now be pending in a softnet taskq,
> > > ifq_barrier also needs to put a barrier in the taskq. this is
> > > implemented using taskq_barrier, which i wrote ages ago but didn't
> > > have a use case for at the time.
> 
> both hrvoje and i hit a deadlock when downing an interface. if
> softnet (or softnets) are waiting on the netlock that and ioctl
> path holds, then an ifq_barrier in that ioctl path will sleep forever
> waiting for a task to run in softnets that are waiting for the
> netlock.
> 
> to mitigate this ive added code like what uvm has. thoughts?

That's only called in ioctl(2) path, right?  Which commands end up
there?

The problem with driver *_ioctl() routines is that pseudo-driver need
the NET_LOCK() while the others should not.  So I'd rather see the
NET_LOCK() released before calling ifp->if_ioctl().

> Index: share/man/man9/task_add.9
> ===
> RCS file: /cvs/src/share/man/man9/task_add.9,v
> retrieving revision 1.16
> diff -u -p -r1.16 task_add.9
> --- share/man/man9/task_add.9 14 Sep 2015 15:14:55 -  1.16
> +++ share/man/man9/task_add.9 13 Nov 2017 00:46:17 -
> @@ -20,6 +20,7 @@
>  .Sh NAME
>  .Nm taskq_create ,
>  .Nm taskq_destroy ,
> +.Nm taskq_barrier ,
>  .Nm task_set ,
>  .Nm task_add ,
>  .Nm task_del ,
> @@ -37,6 +38,8 @@
>  .Ft void
>  .Fn taskq_destroy "struct taskq *tq"
>  .Ft void
> +.Fn taskq_barrier "struct taskq *tq"
> +.Ft void
>  .Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
>  .Ft int
>  .Fn task_add "struct taskq *tq" "struct task *t"
> @@ -88,6 +91,15 @@ Calling
>  against the system taskq is an error and will lead to undefined
>  behaviour or a system fault.
>  .Pp
> +.Fn taskq_barrier
> +guarantees that any task that was running on the
> +.Fa tq
> +taskq when the barrier was called has finished by the time the barrier
> +returns.
> +.Fn taskq_barrier
> +is only supported on taskqs serviced by 1 thread,
> +and may not be called by a task running in the specified taskq.
> +.Pp
>  It is the responsibility of the caller to provide the
>  .Fn task_set ,
>  .Fn task_add ,
> @@ -163,6 +175,8 @@ argument given in
>  and
>  .Fn taskq_destroy
>  can be called during autoconf, or from process context.
> +.Fn taskq_barrier
> +can be called from process context.
>  .Fn task_set ,
>  .Fn task_add ,
>  and
> Index: sys/sys/task.h
> ===
> RCS file: /cvs/src/sys/sys/task.h,v
> retrieving revision 1.11
> diff -u -p -r1.11 task.h
> --- sys/sys/task.h7 Jun 2016 07:53:33 -   1.11
> +++ sys/sys/task.h13 Nov 2017 00:46:17 -
> @@ -43,6 +43,7 @@ extern struct taskq *const systqmp;
>  
>  struct taskq *taskq_create(const char *, unsigned int, int, unsigned int);
>  void  taskq_destroy(struct taskq *);
> +void  taskq_barrier(struct taskq *);
>  
>  void  task_set(struct task *, void (*)(void *), void *);
>  int   task_add(struct taskq *, struct task *);
> Index: sys/kern/kern_task.c
> ===
> RCS file: /cvs/src/sys/kern/kern_task.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 kern_task.c
> --- sys/kern/kern_task.c  30 Oct 2017 14:01:42 -  1.20
> +++ sys/kern/kern_task.c  13 Nov 2017 00:46:17 -
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define TASK_ONQUEUE 1
>  
> @@ -68,6 +69,7 @@ struct taskq *const systqmp = _sys
>  
>  void taskq_init(void); /* called in init_main.c */
>  void taskq_create_thread(void *);
> +void taskq_barrier_task(void *);
>  int  taskq_sleep(const volatile void *, struct mutex *, int,
>   const char *, int);
>  int  taskq_next_work(struct taskq *, struct task *, sleepfn);
> @@ -176,6 +178,30 @@ taskq_create_thread(void *arg)
>   } while (tq->tq_running < tq->tq_nthreads);
>  
>   mtx_leave(>tq_mtx);
> +}
> +
> 

Re: mbuf statistics, tracking of drops

[Martin Pieuchot]

On 13/11/17(Mon) 13:47, David Gwynne wrote:
> On Sun, Nov 12, 2017 at 11:23:31PM +0100, Gregor Best wrote:
> > Hi Martin,
> > 
> > On Sun, Nov 12, 2017 at 03:40:59PM +0100, Martin Pieuchot wrote:
> > > [...]
> > > It does, some comments below.
> > > [...]
> > 
> > Wonderful.
> > 
> > > [...]
> > > This would be an approximation because it might happen that after
> > > returning NULL the second pool_get(9) won't sleep.  However I think
> > > it's the way to go because m_get(9) calls that can wait are generally
> > > not performance critical.
> > > [...]
> > 
> > I had not considered that the second pool_get might not block at all. On
> > the other hand `netstat -m` says "requests for memory delayed" for that
> > counter, so returning immediately on the second try not counting as a
> > delay is OK, I think.
> > 
> > > [...]
> > > Please do not expand the splx().  Only the counter need it.  Simply put
> > > the NULL check after the splx().
> > > [...]
> > 
> > I'm not sure I understand. If I move the NULL check after the splx(),
> > counters_leave() will already have been called so increasing the counter
> > value has no effect anymore. The only additional things running under
> > splnet will be a few assignments, so I think moving the splx() a bit
> > further down does not hurt too much.
> > 
> > Alternatively, I've attached a slightly different suggestion which
> > doesn't expand the scope of the splx() but duplicates a bit of code.
> > Does that make more sense?
> 
> pools maintain count of how many times they failed to provide an
> allocation. you can watch this with vmstat -m or systat pool.
> however, we could use that to populate mb_drops too.
> 
> the diff below moves populating the mbstat from kern_sysctl to
> uipc_mbuf, and adds copying of pr_nfails in.

Makes sense.  So if we go this way, we can get rid of MBSTAT_DROPS,
MBSTAT_WAIT and MBSTAT_DRAIN that are currently unused.

> if we want to count the number of "delays", i could easily add that
> to pools too and copy it out in sysctl_mbstat.

That's be an interesting statistic.

> Index: sys/sysctl.h
> ===
> RCS file: /cvs/src/sys/sys/sysctl.h,v
> retrieving revision 1.175
> diff -u -p -r1.175 sysctl.h
> --- sys/sysctl.h  12 Oct 2017 09:14:16 -  1.175
> +++ sys/sysctl.h  13 Nov 2017 03:42:32 -
> @@ -936,6 +936,7 @@ int sysctl_rdstruct(void *, size_t *, vo
>  int sysctl_struct(void *, size_t *, void *, size_t, void *, size_t);
>  int sysctl_file(int *, u_int, char *, size_t *, struct proc *);
>  int sysctl_doproc(int *, u_int, char *, size_t *);
> +int sysctl_mbstat(int *, u_int, void *, size_t *, void *, size_t);
>  struct mbuf_queue;
>  int sysctl_mq(int *, u_int, void *, size_t *, void *, size_t,
>  struct mbuf_queue *);
> Index: kern/kern_sysctl.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.330
> diff -u -p -r1.330 kern_sysctl.c
> --- kern/kern_sysctl.c11 Aug 2017 21:24:19 -  1.330
> +++ kern/kern_sysctl.c13 Nov 2017 03:42:32 -
> @@ -392,24 +392,9 @@ kern_sysctl(int *name, u_int namelen, vo
>   case KERN_FILE:
>   return (sysctl_file(name + 1, namelen - 1, oldp, oldlenp, p));
>  #endif
> - case KERN_MBSTAT: {
> - extern struct cpumem *mbstat;
> - uint64_t counters[MBSTAT_COUNT];
> - struct mbstat mbs;
> - unsigned int i;
> -
> - memset(, 0, sizeof(mbs));
> - counters_read(mbstat, counters, MBSTAT_COUNT);
> - for (i = 0; i < MBSTAT_TYPES; i++)
> - mbs.m_mtypes[i] = counters[i];
> -
> - mbs.m_drops = counters[MBSTAT_DROPS];
> - mbs.m_wait = counters[MBSTAT_WAIT];
> - mbs.m_drain = counters[MBSTAT_DRAIN];
> -
> - return (sysctl_rdstruct(oldp, oldlenp, newp,
> - , sizeof(mbs)));
> - }
> + case KERN_MBSTAT:
> + return (sysctl_mbstat(name + 1, namelen -1, oldp, oldlenp,
> + newp, newlen));
>  #if defined(GPROF) || defined(DDBPROF)
>   case KERN_PROF:
>   return (sysctl_doprof(name + 1, namelen - 1, oldp, oldlenp,
> Index: kern/uipc_mbuf.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.250
> diff -u -p -r1.250 uipc_mbuf.c
> --- kern/uipc_mbuf.c  12 Oct 2017 09:14:16 -  1.250
> +++ kern/uipc_mbuf.c  13 Nov 2017 03:42:32 -
> @@ -1421,6 +1421,30 @@ m_pool_init(struct pool *pp, u_int size,
>   pool_set_constraints(pp, _dma_contig);
>  }
>  
> +int
> +sysctl_mbstat(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> +void *newp, size_t newlen)
> +{
> + uint64_t counters[MBSTAT_COUNT];
> + struct mbstat mbs;
> + unsigned int i;
> +
> + if (namelen != 0)
> + return 

Re: try to bundle multiple packets on an ifq before sending

[David Gwynne]

On Sun, Nov 12, 2017 at 02:45:05PM +0100, Martin Pieuchot wrote:
> On 10/11/17(Fri) 10:58, David Gwynne wrote:
> > this makes ifq_start try to wait for 4 packets before calling
> > if->if_qstart.
> 
> So you're adding back the IFXF_TXREADY mechanism that you removed in
> 2015 in r1.418.  At that time we couldn't clearly see how to make it
> MP-safe.

ha, id forgotten about that :)

> 
> > this is based on work sephe did in dragonflybsd, and described in
> > a comment in their sys/net/if.c. there's a link to it here:
> > https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/net/if.c#L2976-L2987
> > 
> > the tests we've done generally show a performance bump, but otherwise
> > no degradation in performance.
> > 
> > the mechanism for bundling packets is to but schedule a task to
> > service the queue later. if 4 packets get accumulated before the
> > task runs, it's cancelled and the code runs the start routine
> > directly.
> > 
> > the most significant difference this implementation has to the dfly
> > one is that our ifqs dont (currently) track the number of bytes on
> > the q. dfly sends if it can bundle 4 packets, or up to mtu bytes
> > worth of packets. this implementation only looks at the number of
> > packets.
> 
> IFXF_TXREADY had a different magic.  Instead of 4 packets it looked at:
> 
>   min(8, ifp->if_snd.ifq_maxlen)

8 was magic, i picked it because it was a nice round number. dfly
uses 4 because sephe tested a bunch of values and didnt see an
improvement beyond 4.

> It also checked for ifq_is_oactive()...  Why is this logic no longer
> relevant?

ifq_start_task called via ifq_run_start (which in is called via
ifq_start or ifq_bundle_task) checks ifq_is_oactive. doing it once
should be enough.

> 
> > the taskq the ifq uses is one of the softnets, which is assigned
> > when the ifq is initted and unconditionally used during the ifq's
> > lifetime.
> 
> We're currently using net_tq() to distribute load for incoming packets.
> So I believe you should schedule the task on the current taskq or the
> first one if coming from userland.

/em shrug.

i dont know how to tell what the current softnet tq is. would it
be enough to simply not mix if the ifq_idx into the argument to
net_tq?

> 
> >   because ifq work could now be pending in a softnet taskq,
> > ifq_barrier also needs to put a barrier in the taskq. this is
> > implemented using taskq_barrier, which i wrote ages ago but didn't
> > have a use case for at the time.

both hrvoje and i hit a deadlock when downing an interface. if
softnet (or softnets) are waiting on the netlock that and ioctl
path holds, then an ifq_barrier in that ioctl path will sleep forever
waiting for a task to run in softnets that are waiting for the
netlock.

to mitigate this ive added code like what uvm has. thoughts?

Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.16
diff -u -p -r1.16 task_add.9
--- share/man/man9/task_add.9   14 Sep 2015 15:14:55 -  1.16
+++ share/man/man9/task_add.9   13 Nov 2017 00:46:17 -
@@ -20,6 +20,7 @@
 .Sh NAME
 .Nm taskq_create ,
 .Nm taskq_destroy ,
+.Nm taskq_barrier ,
 .Nm task_set ,
 .Nm task_add ,
 .Nm task_del ,
@@ -37,6 +38,8 @@
 .Ft void
 .Fn taskq_destroy "struct taskq *tq"
 .Ft void
+.Fn taskq_barrier "struct taskq *tq"
+.Ft void
 .Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
 .Ft int
 .Fn task_add "struct taskq *tq" "struct task *t"
@@ -88,6 +91,15 @@ Calling
 against the system taskq is an error and will lead to undefined
 behaviour or a system fault.
 .Pp
+.Fn taskq_barrier
+guarantees that any task that was running on the
+.Fa tq
+taskq when the barrier was called has finished by the time the barrier
+returns.
+.Fn taskq_barrier
+is only supported on taskqs serviced by 1 thread,
+and may not be called by a task running in the specified taskq.
+.Pp
 It is the responsibility of the caller to provide the
 .Fn task_set ,
 .Fn task_add ,
@@ -163,6 +175,8 @@ argument given in
 and
 .Fn taskq_destroy
 can be called during autoconf, or from process context.
+.Fn taskq_barrier
+can be called from process context.
 .Fn task_set ,
 .Fn task_add ,
 and
Index: sys/sys/task.h
===
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.11
diff -u -p -r1.11 task.h
--- sys/sys/task.h  7 Jun 2016 07:53:33 -   1.11
+++ sys/sys/task.h  13 Nov 2017 00:46:17 -
@@ -43,6 +43,7 @@ extern struct taskq *const systqmp;
 
 struct taskq   *taskq_create(const char *, unsigned int, int, unsigned int);
 voidtaskq_destroy(struct taskq *);
+voidtaskq_barrier(struct taskq *);
 
 voidtask_set(struct task *, void (*)(void *), void *);
 int task_add(struct taskq *, struct task *);
Index: sys/kern/kern_task.c

Re: mbuf statistics, tracking of drops

[Martin Pieuchot]

On 11/11/17(Sat) 13:05, Gregor Best wrote:
> Hi people,
> 
> while reading around in /sys/kern/uipc_mbuf.c to try to track down a
> problem with my iwm(4) that seems to correlate with mbuf allocation
> failures, I noticed that the MBSTAT_DROPS counter and its friends
> MBSTAT_{WAIT,DRAIN} don't seem to get increased anywhere in /sys.
> 
> Does the patch below the signature make sense for counting MBSTAT_DROPS?

It does, some comments below.

> I've got a similar patch for MBSTAT_WAIT, but it's pretty ugly because
> as far as I can see, there's no real way to notice when pool_get sleeps
> except for "Try pool_get with M_NOWAIT first and if that returns NULL,
> try again with M_WAITOK".

This would be an approximation because it might happen that after
returning NULL the second pool_get(9) won't sleep.  However I think
it's the way to go because m_get(9) calls that can wait are generally
not performance critical.

> Index: /sys/kern/uipc_mbuf.c
> ===
> RCS file: /home/cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.250
> diff -u -p -r1.250 uipc_mbuf.c
> --- /sys/kern/uipc_mbuf.c 12 Oct 2017 09:14:16 -  1.250
> +++ /sys/kern/uipc_mbuf.c 11 Nov 2017 12:03:39 -
> @@ -233,14 +233,14 @@ m_get(int nowait, int type)
>   KDASSERT(type < MT_NTYPES);
>  
>   m = pool_get(, nowait == M_WAIT ? PR_WAITOK : PR_NOWAIT);
> - if (m == NULL)
> - return (NULL);
>  
>   s = splnet();
>   counters = counters_enter(, mbstat);
> + if (m == NULL) {
> + counters[MBSTAT_DROPS]++;
> + goto out;
> + }
>   counters[type]++;
> - counters_leave(, mbstat);
> - splx(s);

Please do not expand the splx().  Only the counter need it.  Simply put
the NULL check after the splx().

__warn_references: drop redundant "warning: " prefix

[Scott Cheloha]

Hi,

GNU ld has prefixed the contents of .gnu.warning.SYMBOL sections
with "warning: " since 2003, so the messages themselves need not
contain the prefix anymore.

If LLVM ld ever acknowledges .gnu.warning sections I imagine it
would emulate this behavior.

Thoughts?

--
Scott Cheloha

Index: lib/libc/arch/i386/string/strcat.S
===
RCS file: /cvs/src/lib/libc/arch/i386/string/strcat.S,v
retrieving revision 1.9
diff -u -p -r1.9 strcat.S
--- lib/libc/arch/i386/string/strcat.S  31 Aug 2015 02:53:56 -  1.9
+++ lib/libc/arch/i386/string/strcat.S  12 Nov 2017 04:21:37 -
@@ -9,7 +9,7 @@
 #if defined(APIWARN)
 #APP
.section .gnu.warning.strcat
-   .ascii "warning: strcat() is almost always misused, please use 
strlcat()"
+   .ascii "strcat() is almost always misused, please use strlcat()"
 #NO_APP
 #endif
 
Index: lib/libc/arch/i386/string/strcpy.S
===
RCS file: /cvs/src/lib/libc/arch/i386/string/strcpy.S,v
retrieving revision 1.9
diff -u -p -r1.9 strcpy.S
--- lib/libc/arch/i386/string/strcpy.S  31 Aug 2015 02:53:56 -  1.9
+++ lib/libc/arch/i386/string/strcpy.S  12 Nov 2017 04:21:37 -
@@ -9,7 +9,7 @@
 #if defined(APIWARN)
 #APP
.section .gnu.warning.strcpy
-   .ascii "warning: strcpy() is almost always misused, please use 
strlcpy()"
+   .ascii "strcpy() is almost always misused, please use strlcpy()"
 #NO_APP
 #endif
 
Index: lib/libc/compat-43/getwd.c
===
RCS file: /cvs/src/lib/libc/compat-43/getwd.c,v
retrieving revision 1.11
diff -u -p -r1.11 getwd.c
--- lib/libc/compat-43/getwd.c  30 Sep 2013 12:02:30 -  1.11
+++ lib/libc/compat-43/getwd.c  12 Nov 2017 04:21:37 -
@@ -46,4 +46,4 @@ getwd(char *buf)
 }
 
 __warn_references(getwd,
-"warning: getwd() possibly used unsafely; consider using getcwd()");
+"getwd() possibly used unsafely; consider using getcwd()");
Index: lib/libc/stdio/mktemp.c
===
RCS file: /cvs/src/lib/libc/stdio/mktemp.c,v
retrieving revision 1.38
diff -u -p -r1.38 mktemp.c
--- lib/libc/stdio/mktemp.c 13 Sep 2015 08:31:47 -  1.38
+++ lib/libc/stdio/mktemp.c 12 Nov 2017 04:21:37 -
@@ -119,7 +119,7 @@ _mktemp(char *path)
 }
 
 __warn_references(mktemp,
-"warning: mktemp() possibly used unsafely; consider using mkstemp()");
+"mktemp() possibly used unsafely; consider using mkstemp()");
 
 char *
 mktemp(char *path)
Index: lib/libc/stdio/sprintf.c
===
RCS file: /cvs/src/lib/libc/stdio/sprintf.c,v
retrieving revision 1.18
diff -u -p -r1.18 sprintf.c
--- lib/libc/stdio/sprintf.c1 Oct 2015 02:32:07 -   1.18
+++ lib/libc/stdio/sprintf.c12 Nov 2017 04:21:37 -
@@ -39,7 +39,7 @@
 
 #if defined(APIWARN)
 __warn_references(sprintf,
-"warning: sprintf() is often misused, please use snprintf()");
+"sprintf() is often misused, please use snprintf()");
 #endif
 
 int
Index: lib/libc/stdio/tempnam.c
===
RCS file: /cvs/src/lib/libc/stdio/tempnam.c,v
retrieving revision 1.19
diff -u -p -r1.19 tempnam.c
--- lib/libc/stdio/tempnam.c31 Aug 2015 02:53:57 -  1.19
+++ lib/libc/stdio/tempnam.c12 Nov 2017 04:21:37 -
@@ -37,7 +37,7 @@
 #include 
 
 __warn_references(tempnam,
-"warning: tempnam() possibly used unsafely; consider using mkstemp()");
+"tempnam() possibly used unsafely; consider using mkstemp()");
 
 char *
 tempnam(const char *dir, const char *pfx)
Index: lib/libc/stdio/tmpnam.c
===
RCS file: /cvs/src/lib/libc/stdio/tmpnam.c,v
retrieving revision 1.11
diff -u -p -r1.11 tmpnam.c
--- lib/libc/stdio/tmpnam.c 31 Aug 2015 02:53:57 -  1.11
+++ lib/libc/stdio/tmpnam.c 12 Nov 2017 04:21:37 -
@@ -37,7 +37,7 @@
 #include 
 
 __warn_references(tmpnam,
-"warning: tmpnam() possibly used unsafely; consider using mkstemp()");
+"tmpnam() possibly used unsafely; consider using mkstemp()");
 
 char *
 tmpnam(char *s)
Index: lib/libc/stdio/vsprintf.c
===
RCS file: /cvs/src/lib/libc/stdio/vsprintf.c,v
retrieving revision 1.16
diff -u -p -r1.16 vsprintf.c
--- lib/libc/stdio/vsprintf.c   9 Nov 2009 00:18:28 -   1.16
+++ lib/libc/stdio/vsprintf.c   12 Nov 2017 04:21:37 -
@@ -38,7 +38,7 @@
 
 #if defined(APIWARN)
 __warn_references(vsprintf,
-"warning: vsprintf() is often misused, please use vsnprintf()");
+"vsprintf() is often misused, please use vsnprintf()");
 #endif
 
 int
Index: lib/libc/stdlib/rand.c
===
RCS file: /cvs/src/lib/libc/stdlib/rand.c,v
retrieving 

Re: Remove commented out pledge in tsort

[Marc Espie]

On Sun, Nov 12, 2017 at 11:08:54AM +, George Brown wrote:
> Removes commented out pledge with whitelist, this is the only instance I
> found of this grep'ing through base.
> 
> diff --git usr.bin/tsort/tsort.c usr.bin/tsort/tsort.c
> index 5caa733f4..cc1cba164 100644
> --- usr.bin/tsort/tsort.c
> +++ usr.bin/tsort/tsort.c
> @@ -879,7 +879,6 @@ parse_args(int argc, char *argv[], struct ohash *pairs)
> 
>  files[i] = NULL;
> 
> -/*if (pledge("stdio rpath", files) == -1) */
>  if (pledge("stdio rpath", NULL) == -1)
>  err(1, "pledge");

I did this in an eager way a while back, was told in no uncertain terms
"not yet, we're still looking at things".

Well, see pledge(2). Whenever the dust settles, sure we can kill this line,
or activate it instead.

Dust is not yet settled.

Remove commented out pledge in tsort

[George Brown]

Removes commented out pledge with whitelist, this is the only instance I
found of this grep'ing through base.

diff --git usr.bin/tsort/tsort.c usr.bin/tsort/tsort.c
index 5caa733f4..cc1cba164 100644
--- usr.bin/tsort/tsort.c
+++ usr.bin/tsort/tsort.c
@@ -879,7 +879,6 @@ parse_args(int argc, char *argv[], struct ohash *pairs)

 files[i] = NULL;

-/*if (pledge("stdio rpath", files) == -1) */
 if (pledge("stdio rpath", NULL) == -1)
 err(1, "pledge");

/usr/share/calendar/calendar.christian - two entries for "First Sunday of Advent (4th Sunday before Christmas)"

[Raf Czlonka]

Hi all,

I've just noticed something strange in the
/usr/share/calendar/calendar.christian file, namely:

11/SunLast  First Sunday of Advent (4th Sunday before Christmas)
12/SunFirst First Sunday of Advent (4th Sunday before Christmas)

Obviously, in any given year either is true - not both.

I do understand the intent - the beginning of Advent will either
be on the last Sunday of November or the first Sunday of December
depending which day of the week Christmas Day falls on.

Calculating it isn't difficult - last Thursday of November + 3 days -
but I'm not sure whether adding any additional code to calendar(1)
is desirable.

I don't know what the best solution to the above is as currently,
as it stands, these entries are confusing - at first glance the
above looks like a bug and after figuring out it isn't one, I'm
sill none the wise which one it is without consulting another
calendar.

Adding code, modifying the above entries or an additional entry in
the BUGS section in the manual. What are your thought?

Best regards,

Raf

Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

[Martin Pieuchot]

On 12/11/17(Sun) 04:36, Helg Bredow wrote:
> On Fri, 10 Nov 2017 11:55:53 +
> Helg Bredow  wrote:
> 
> > On Fri, 10 Nov 2017 10:13:35 +0100
> > Anton Lindqvist  wrote:
> > 
> > > On Fri, Nov 10, 2017 at 09:36:25AM +0100, Martin Pieuchot wrote:
> > > > On 09/11/17(Thu) 09:02, Helg Bredow wrote:
> > > > > The current libfuse signal handling assumes that the file system will 
> > > > > always be unmounted by the child. One obvious case where this is not 
> > > > > true is if the file system is busy. To replicate:
> > > > > 
> > > > > 1. mount a fuse file system
> > > > > 2. cd anywhere on the file system
> > > > > 3. pkill -INT 
> > > > > 
> > > > > The result is a zombie child process and no more response to signals 
> > > > > even if the file system is no longer busy.
> > > > > 
> > > > > This patch ensures that the child always exits and that an error is 
> > > > > printed to stdout if the file system cannot be unmounted. Tested with 
> > > > > fuse-exfat and ntfs_3g. Suggestions for improvement are welcome.
> > > > 
> > > > Nice to see that you're fixing a bug.  However I'd suggest you to go
> > > > much further in your improvement.
> > > > 
> > > > Signal handlers are hard and instead of doing work inside the signal
> > > > handler you should toggle a global variable/flag and do this work
> > > > inside the main loop (fuse_loop()?).
> > > > 
> > > > For example your code below calls fprintf(3) in the signal handler.  
> > > > This
> > > > is incorrect, this functions is not signal handler safe.  What about 
> > > > fuse_unmount()?  Are you sure it can be called from a signal handler?
> > > 
> > > Some more info on making signal handlers asynchronous-safe:
> > > 
> > > https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
> > 
> > Thanks for the feedback and info guys. I wasn't too confident with this 
> > patch and you've given me some good pointers to improve it.
> > 
> > -- 
> > Helg 
> > 
> 
> I've completely rewritten the patch. Is this better?

Much better!  However I'd suggest using two variables instead of the
single `signum'.  Otherwise you might fail to handle the first signal
delivered.

> Index: fuse.c
> ===
> RCS file: /cvs/src/lib/libfuse/fuse.c,v
> retrieving revision 1.34
> diff -u -p -u -p -r1.34 fuse.c
> --- fuse.c4 Nov 2017 13:17:18 -   1.34
> +++ fuse.c12 Nov 2017 04:28:21 -
> @@ -31,7 +31,7 @@
>  #include "fuse_private.h"
>  #include "debug.h"
>  
> -static struct fuse_session *sigse;
> +static volatile sig_atomic_t signum = 0;
>  static struct fuse_context *ictx = NULL;
>  static int max_read = FUSEBUFMAXSIZE;
>  
> @@ -61,6 +61,48 @@ static struct fuse_opt fuse_core_opts[] 
>   FUSE_OPT_END
>  };
>  
> +static void
> +ifuse_get_signal(int num)
> +{
> + signum = num;
> +}
> +
> +static void
> +ifuse_child_exit(const struct fuse *f)
> +{
> + int status;
> +
> + signal(SIGCHLD, SIG_DFL);
> + if (waitpid(WAIT_ANY, , WNOHANG) == -1)
> + fprintf(stderr, "fuse: %s\n", strerror(errno));
> +
> + if (WIFEXITED(status) && (WEXITSTATUS(status) != 0))
> + fprintf(stderr, "fuse: %s: %s\n",
> + f->fc->dir, strerror(WEXITSTATUS(status)));
> +
> + return;
> +}
> +
> +static void
> +ifuse_try_unmount(const struct fuse *f)
> +{
> + pid_t child;
> +
> + signal(SIGCHLD, ifuse_get_signal);
> + child = fork();
> +
> + if (child < 0) {
> + DPERROR(__func__);
> + return;
> + }
> +
> + if (child == 0) {
> + errno = 0;
> + fuse_unmount(f->fc->dir, f->fc);
> + _exit(errno);
> + }
> +}
> +
>  int
>  fuse_loop(struct fuse *fuse)
>  {
> @@ -83,9 +125,24 @@ fuse_loop(struct fuse *fuse)
>  
>   while (!fuse->fc->dead) {
>   ret = kevent(fuse->fc->kq, >fc->event, 1, , 1, NULL);
> - if (ret == -1)
> - DPERROR(__func__);
> - else if (ret > 0) {
> + if (ret == -1) {
> + if (errno == EINTR) {
> + switch (signum) {
> + case SIGCHLD:
> + ifuse_child_exit(fuse);
> + break;
> + case SIGHUP:
> + case SIGINT:
> + case SIGTERM:
> + ifuse_try_unmount(fuse);
> + break;
> + default:
> + fprintf(stderr, "%s: %s\n", __func__,
> + strsignal(signum));
> + }
> + } else
> + DPERROR(__func__);
> + } else if (ret > 0) 

Re: try to bundle multiple packets on an ifq before sending

[Martin Pieuchot]

On 10/11/17(Fri) 10:58, David Gwynne wrote:
> this makes ifq_start try to wait for 4 packets before calling
> if->if_qstart.

So you're adding back the IFXF_TXREADY mechanism that you removed in
2015 in r1.418.  At that time we couldn't clearly see how to make it
MP-safe.

> this is based on work sephe did in dragonflybsd, and described in
> a comment in their sys/net/if.c. there's a link to it here:
> https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/net/if.c#L2976-L2987
> 
> the tests we've done generally show a performance bump, but otherwise
> no degradation in performance.
> 
> the mechanism for bundling packets is to but schedule a task to
> service the queue later. if 4 packets get accumulated before the
> task runs, it's cancelled and the code runs the start routine
> directly.
> 
> the most significant difference this implementation has to the dfly
> one is that our ifqs dont (currently) track the number of bytes on
> the q. dfly sends if it can bundle 4 packets, or up to mtu bytes
> worth of packets. this implementation only looks at the number of
> packets.

IFXF_TXREADY had a different magic.  Instead of 4 packets it looked at:

min(8, ifp->if_snd.ifq_maxlen)

It also checked for ifq_is_oactive()...  Why is this logic no longer
relevant?

> the taskq the ifq uses is one of the softnets, which is assigned
> when the ifq is initted and unconditionally used during the ifq's
> lifetime.

We're currently using net_tq() to distribute load for incoming packets.
So I believe you should schedule the task on the current taskq or the
first one if coming from userland.

>   because ifq work could now be pending in a softnet taskq,
> ifq_barrier also needs to put a barrier in the taskq. this is
> implemented using taskq_barrier, which i wrote ages ago but didn't
> have a use case for at the time.
> 
> tests? ok?
> 
> Index: share/man/man9/task_add.9
> ===
> RCS file: /cvs/src/share/man/man9/task_add.9,v
> retrieving revision 1.16
> diff -u -p -r1.16 task_add.9
> --- share/man/man9/task_add.9 14 Sep 2015 15:14:55 -  1.16
> +++ share/man/man9/task_add.9 10 Nov 2017 00:45:41 -
> @@ -20,6 +20,7 @@
>  .Sh NAME
>  .Nm taskq_create ,
>  .Nm taskq_destroy ,
> +.Nm taskq_barrier ,
>  .Nm task_set ,
>  .Nm task_add ,
>  .Nm task_del ,
> @@ -37,6 +38,8 @@
>  .Ft void
>  .Fn taskq_destroy "struct taskq *tq"
>  .Ft void
> +.Fn taskq_barrier "struct taskq *tq"
> +.Ft void
>  .Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
>  .Ft int
>  .Fn task_add "struct taskq *tq" "struct task *t"
> @@ -88,6 +91,15 @@ Calling
>  against the system taskq is an error and will lead to undefined
>  behaviour or a system fault.
>  .Pp
> +.Fn taskq_barrier
> +guarantees that any task that was running on the
> +.Fa tq
> +taskq when the barrier was called has finished by the time the barrier
> +returns.
> +.Fn taskq_barrier
> +is only supported on taskqs serviced by 1 thread,
> +and may not be called by a task running in the specified taskq.
> +.Pp
>  It is the responsibility of the caller to provide the
>  .Fn task_set ,
>  .Fn task_add ,
> @@ -163,6 +175,8 @@ argument given in
>  and
>  .Fn taskq_destroy
>  can be called during autoconf, or from process context.
> +.Fn taskq_barrier
> +can be called from process context.
>  .Fn task_set ,
>  .Fn task_add ,
>  and
> Index: sys/sys/task.h
> ===
> RCS file: /cvs/src/sys/sys/task.h,v
> retrieving revision 1.11
> diff -u -p -r1.11 task.h
> --- sys/sys/task.h7 Jun 2016 07:53:33 -   1.11
> +++ sys/sys/task.h10 Nov 2017 00:45:41 -
> @@ -43,6 +43,7 @@ extern struct taskq *const systqmp;
>  
>  struct taskq *taskq_create(const char *, unsigned int, int, unsigned int);
>  void  taskq_destroy(struct taskq *);
> +void  taskq_barrier(struct taskq *);
>  
>  void  task_set(struct task *, void (*)(void *), void *);
>  int   task_add(struct taskq *, struct task *);
> Index: sys/kern/kern_task.c
> ===
> RCS file: /cvs/src/sys/kern/kern_task.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 kern_task.c
> --- sys/kern/kern_task.c  30 Oct 2017 14:01:42 -  1.20
> +++ sys/kern/kern_task.c  10 Nov 2017 00:45:41 -
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define TASK_ONQUEUE 1
>  
> @@ -68,6 +69,7 @@ struct taskq *const systqmp = _sys
>  
>  void taskq_init(void); /* called in init_main.c */
>  void taskq_create_thread(void *);
> +void taskq_barrier_task(void *);
>  int  taskq_sleep(const volatile void *, struct mutex *, int,
>   const char *, int);
>  int  taskq_next_work(struct taskq *, struct task *, sleepfn);
> @@ -176,6 +178,30 @@ taskq_create_thread(void *arg)
>   } while (tq->tq_running < tq->tq_nthreads);
>  
>   mtx_leave(>tq_mtx);
> +}
> 

18 year old #if 0

[Martin Pieuchot]

Now that they can drink in many countries, let them go away... ok?

Index: netinet6/frag6.c
===
RCS file: /cvs/src/sys/netinet6/frag6.c,v
retrieving revision 1.79
diff -u -p -r1.79 frag6.c
--- netinet6/frag6.c7 Nov 2017 16:47:07 -   1.79
+++ netinet6/frag6.c8 Nov 2017 14:26:50 -
@@ -352,29 +352,13 @@ frag6_input(struct mbuf **mp, int *offp,
 */
if (paf6 != NULL) {
i = (paf6->ip6af_off + paf6->ip6af_frglen) - ip6af->ip6af_off;
-   if (i > 0) {
-#if 0  /* suppress the noisy log */
-   char ip[INET6_ADDRSTRLEN];
-   log(LOG_ERR, "%d bytes of a fragment from %s "
-   "overlaps the previous fragment\n",
-   i,
-   inet_ntop(AF_INET6, >ip6q_src, ip, sizeof(ip)));
-#endif
+   if (i > 0)
goto flushfrags;
-   }
}
if (af6 != NULL) {
i = (ip6af->ip6af_off + ip6af->ip6af_frglen) - af6->ip6af_off;
-   if (i > 0) {
-#if 0  /* suppress the noisy log */
-   char ip[INET6_ADDRSTRLEN];
-   log(LOG_ERR, "%d bytes of a fragment from %s "
-   "overlaps the succeeding fragment",
-   i,
-   inet_ntop(AF_INET6, >ip6q_src, ip, sizeof(ip)));
-#endif
+   if (i > 0)
goto flushfrags;
-   }
}
 
  insert:
@@ -390,12 +374,6 @@ frag6_input(struct mbuf **mp, int *offp,
LIST_INSERT_HEAD(>ip6q_asfrag, ip6af, ip6af_list);
frag6_nfrags++;
q6->ip6q_nfrag++;
-#if 0 /* xxx */
-   if (q6 != TAILQ_FIRST(_queue)) {
-   TAILQ_REMOVE(_queue, q6, ip6q_queue);
-   TAILQ_INSERT_HEAD(_queue, q6, ip6q_queue);
-   }
-#endif
next = 0;
for (paf6 = NULL, af6 = LIST_FIRST(>ip6q_asfrag);
af6 != NULL;

Re: armv7/sxie: less ierror

[Mark Kettenis]

> Date: Sun, 12 Nov 2017 04:58:23 +0200
> From: Artturi Alm 
> 
> Hi,
> 
> i'm likely responsible, for having sent the diff that introduced this.
> minimal fix taken w/diff -U10, to show the obvious dup++.
> 
> -Artturi

I think it makes more sense to fix it this way:

ok?


Index: arch/armv7/sunxi/sxie.c
===
RCS file: /cvs/src/sys/arch/armv7/sunxi/sxie.c,v
retrieving revision 1.25
diff -u -p -r1.25 sxie.c
--- arch/armv7/sunxi/sxie.c 22 Jan 2017 10:17:37 -  1.25
+++ arch/armv7/sunxi/sxie.c 12 Nov 2017 18:05:46 -
@@ -595,8 +595,8 @@ trynext:
SXISET4(sc, SXIE_RXCR, SXIE_RXFLUSH);
while (SXIREAD4(sc, SXIE_RXCR) & SXIE_RXFLUSH);
SXISET4(sc, SXIE_CR, SXIE_RX_ENABLE);
-
-   goto err_out;
+   ifp->if_ierrors++;
+   goto done;
}

reg = SXIREAD4(sc, SXIE_RXIO);
@@ -621,13 +621,11 @@ trynext:
m = m_devget([0], pktlen, ETHER_ALIGN);
if (m == NULL) {
ifp->if_ierrors++;
-   goto err_out;
+   goto done;
}
 
ml_enqueue(, m);
goto trynext;
-err_out:
-   ifp->if_ierrors++;
 done:
if_input(ifp, );
 }

assertion "_kernel_lock_held()" failed, uipc_socket2.c: ipsec

[Stuart Henderson]

iked box, GENERIC.MP + WITNESS, -current as of Friday 10th:

login: panic: kernel diagnostic assertion "_kernel_lock_held()" failed: file 
"/src/cvs-openbsd/sys/kern/uipc_socket2.c", line 310
splassert: soassertlocked: want 1 have 256

panic: spl assertion failure in soassertlocked
Starting stack trace...
Faulted in traceback, aborting...
panic(splassert: if_down: want 1 have 256
panic: spl assertion failure in if_down) at
Faulted in traceback, aborting...
panicsplassert: if_down: want 1 have 256
+0x133panic: spl assertion failure in if_down
Faulted in traceback, aborting...



It's stuck at this point, I can't enter ddb.


OpenBSD 6.2-current (WITNESS) #0: Fri Nov 10 07:54:54 GMT 2017

st...@symphytum.spacehopper.org:/src/cvs-openbsd/sys/arch/amd64/compile/WITNESS
real mem = 1996152832 (1903MB)
avail mem = 1907134464 (1818MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0x77fb7020 (7 entries)
bios0: vendor coreboot version "88a4f96" date 03/11/2016
bios0: PC Engines apu2
acpi0 at bios0: rev 2
acpi0: sleep states S0 S1 S2 S3 S4 S5
acpi0: tables DSDT FACP SSDT APIC HEST SSDT SSDT HPET
acpi0: wakeup devices PWRB(S4) PBR4(S4) PBR5(S4) PBR6(S4) PBR7(S4) PBR8(S4) 
UOH1(S3) UOH3(S3) UOH5(S3) XHC0(S4)
acpitimer0 at acpi0: 3579545 Hz, 32 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: AMD GX-412TC SOC, 998.27 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE
3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,M
ASSE,3DNOWP,OSVW,IBS,SKINIT,TOPEXT,ITSC,BMI1
cpu0: 32KB 64b/line 2-way I-cache, 32KB 64b/line 8-way D-cache, 2MB 64b/line 
16-way L2 cache
cpu0: ITLB 32 4KB entries fully associative, 8 4MB entries fully associative
cpu0: DTLB 40 4KB entries fully associative, 8 4MB entries fully associative
acpitimer0: recalibrated TSC frequency 998129941 Hz
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: AMD GX-412TC SOC, 998.25 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE
3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,M
ASSE,3DNOWP,OSVW,IBS,SKINIT,TOPEXT,ITSC,BMI1
cpu1: 32KB 64b/line 2-way I-cache, 32KB 64b/line 8-way D-cache, 2MB 64b/line 
16-way L2 cache
cpu1: ITLB 32 4KB entries fully associative, 8 4MB entries fully associative
cpu1: DTLB 40 4KB entries fully associative, 8 4MB entries fully associative
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 2 (application processor)
cpu2: AMD GX-412TC SOC, 998.13 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE
3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,M
ASSE,3DNOWP,OSVW,IBS,SKINIT,TOPEXT,ITSC,BMI1
cpu2: 32KB 64b/line 2-way I-cache, 32KB 64b/line 8-way D-cache, 2MB 64b/line 
16-way L2 cache
cpu2: ITLB 32 4KB entries fully associative, 8 4MB entries fully associative
cpu2: DTLB 40 4KB entries fully associative, 8 4MB entries fully associative
cpu2: smt 0, core 2, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: AMD GX-412TC SOC, 998.14 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE
3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,M
ASSE,3DNOWP,OSVW,IBS,SKINIT,TOPEXT,ITSC,BMI1
cpu3: 32KB 64b/line 2-way I-cache, 32KB 64b/line 8-way D-cache, 2MB 64b/line 
16-way L2 cache
cpu3: ITLB 32 4KB entries fully associative, 8 4MB entries fully associative
cpu3: DTLB 40 4KB entries fully associative, 8 4MB entries fully associative
cpu3: smt 0, core 3, package 0
ioapic0 at mainbus0: apid 4 pa 0xfec0, version 21, 24 pins
ioapic1 at mainbus0: apid 5 pa 0xfec2, version 21, 32 pins
, remapped to apid 5
acpihpet0 at acpi0: 14318180 Hz
acpihpet0: recalibrated TSC frequency 998133658 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (PBR4)
acpiprt2 at acpi0: bus 1 (PBR5)
acpiprt3 at acpi0: bus 2 (PBR6)
acpiprt4 at acpi0: bus 3 (PBR7)
acpiprt5 at acpi0: bus -1 (PBR8)
acpicpu0 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpicpu1 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpicpu2 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpicpu3 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpibtn0 at acpi0: PWRB
cpu0: 998 MHz: speeds: 1000 800 600 MHz
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "AMD AMD64 

Re: Add sizes for free() in the VIA PadLock driver

[Mike Belopuhov]

On Sun, Nov 12, 2017 at 21:53 +0100, Frederic Cambus wrote:
> Hi tech@,
> 
> Add sizes for free() in the VIA PadLock driver.
> 
> Comments? OK?
> 

OK mikeb.

Re: armv7/sxie: less ierror

[Artturi Alm]

On Sun, Nov 12, 2017 at 07:07:13PM +0100, Mark Kettenis wrote:
> > Date: Sun, 12 Nov 2017 04:58:23 +0200
> > From: Artturi Alm 
> > 
> > Hi,
> > 
> > i'm likely responsible, for having sent the diff that introduced this.
> > minimal fix taken w/diff -U10, to show the obvious dup++.
> > 
> > -Artturi
> 
> I think it makes more sense to fix it this way:
> 
> ok?
> 

yep, exactly what i had before minimalizing the diff.

-Artturi

> 
> Index: arch/armv7/sunxi/sxie.c
> ===
> RCS file: /cvs/src/sys/arch/armv7/sunxi/sxie.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 sxie.c
> --- arch/armv7/sunxi/sxie.c   22 Jan 2017 10:17:37 -  1.25
> +++ arch/armv7/sunxi/sxie.c   12 Nov 2017 18:05:46 -
> @@ -595,8 +595,8 @@ trynext:
>   SXISET4(sc, SXIE_RXCR, SXIE_RXFLUSH);
>   while (SXIREAD4(sc, SXIE_RXCR) & SXIE_RXFLUSH);
>   SXISET4(sc, SXIE_CR, SXIE_RX_ENABLE);
> -
> - goto err_out;
> + ifp->if_ierrors++;
> + goto done;
>   }
>   
>   reg = SXIREAD4(sc, SXIE_RXIO);
> @@ -621,13 +621,11 @@ trynext:
>   m = m_devget([0], pktlen, ETHER_ALIGN);
>   if (m == NULL) {
>   ifp->if_ierrors++;
> - goto err_out;
> + goto done;
>   }
>  
>   ml_enqueue(, m);
>   goto trynext;
> -err_out:
> - ifp->if_ierrors++;
>  done:
>   if_input(ifp, );
>  }

armv7/sxie: unused global

[Artturi Alm]

Hi,

one less useless leftover.

-Artturi


diff --git a/sys/arch/armv7/sunxi/sxie.c b/sys/arch/armv7/sunxi/sxie.c
index 0e062bac1fb..2ac4dda7c6b 100644
--- a/sys/arch/armv7/sunxi/sxie.c
+++ b/sys/arch/armv7/sunxi/sxie.c
@@ -168,8 +168,6 @@ struct sxie_softc {
uint32_ttxf_inuse;
 };
 
-struct sxie_softc *sxie_sc;
-
 intsxie_match(struct device *, void *, void *);
 void   sxie_attach(struct device *, struct device *, void *);
 void   sxie_setup_interface(struct sxie_softc *, struct device *);
@@ -271,8 +269,6 @@ sxie_attach(struct device *parent, struct device *self, 
void *aux)
if_attach(ifp);
ether_ifattach(ifp);
splx(s);
-
-   sxie_sc = sc;
 }
 
 void

Re: ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

[Klemens Nanni]

On Sun, Nov 12, 2017 at 09:04:22PM +, Robert Peichaer wrote:
> Hmm. I see.
> 
> The {add,del,no,pre}_path functions are in ksh.kshrc since when it was
> imported 21 years ago. If they would be used in ksh.kshrc or anywhere
> else, I'd say it might be worth "fixing" these functions. But they are
> not used anywhere in the tree and I would rather remove them alltogether.
> 
> In case someone uses them, ~/.kshrc seems to be a more appropriate place.
I personally prefer keeping them but won't object if the broader consent
is to remove them.

This makes me wonder who else actually uses those (on a regular basis).

Re: assertion "_kernel_lock_held()" failed, uipc_socket2.c: ipsec

[Martin Pieuchot]

On 12/11/17(Sun) 21:30, Stuart Henderson wrote:
> iked box, GENERIC.MP + WITNESS, -current as of Friday 10th:

Weird, did you tweak "kern.splassert" on this box?   Otherwise is looks
like a major corruption.

> login: panic: kernel diagnostic assertion "_kernel_lock_held()" failed: file 
> "/src/cvs-openbsd/sys/kern/uipc_socket2.c", line 310
^^^
Looks like one CPU is triggering this.

> splassert: soassertlocked: want 1 have 256
> 
> panic: spl assertion failure in soassertlocked
^^^
That can't be coming from the same CPU..




> Starting stack trace...
> Faulted in traceback, aborting...
> panic(splassert: if_down: want 1 have 256
> panic: spl assertion failure in if_down) at
> Faulted in traceback, aborting...
> panicsplassert: if_down: want 1 have 256
> +0x133panic: spl assertion failure in if_down
> Faulted in traceback, aborting...
> 
> 
> 
> It's stuck at this point, I can't enter ddb.

Are you running with WITNESS on purpose?  Can you reproduce such problem
without it?  I'm not saying it's WITNESS fault, but it's clear that
WITNESS kernels aren't ready for production yet.

Re: ofw_clock: reset_deassert and clock_enable order of use Qs

[Artturi Alm]

On Sat, Oct 07, 2017 at 10:16:05AM +0300, Artturi Alm wrote:
> Hi,
> 
> 
> what was the cause of these delays? i just spotted this, so untested on
> likely more affected HW(sunxi A64/H3 and rockchips), but just for discussion
> about the delays/ordering? are they really needed like that?
> 
> i don't remember having read anything about the order of these from
> devicetree-binding .txt's i have read so far from u-boot/linux.
> 
> "Make sure that the reset signal has been released
> before the release of module clock gating;"
> 
> above can be found from under the short CCU "x.3.6 Programming Guidelines"[0].
> maybe this ordering should be verified from somewhere else, as atleast
> for my sorry non-nativeNlazy grammar i could understand wrong, what is being
> meant w/"the release" above.. but even w/above discarded, my intuition does
> favor deasserting reset before gating sclki, hence the diff.
> 
> the diff below booted faster with no observable changes in dmesg on cubie2,
> and otherwise succesfully on panda and wandboard.
> 
> -Artturi
> 

Ping?

i think i've passed by sources in both fbsd since email above
suggesting the order should be like in the diff.
does anyone want me to provide more on my findings about this,
or anything else?

-Artturi

> [0] sources:
> A64 User Manual(Revision 1.0):
> "3.3.6.4. Gating and reset" Page 147
> H3 Datasheet(Revision1.2):
> "4.3.6.4. Gating and reset" Page 142
> 
> 
> 
> diff --git a/sys/dev/fdt/ehci_fdt.c b/sys/dev/fdt/ehci_fdt.c
> index 55fe75f1a9c..fae54ac11ee 100644
> --- a/sys/dev/fdt/ehci_fdt.c
> +++ b/sys/dev/fdt/ehci_fdt.c
> @@ -91,9 +91,10 @@ ehci_fdt_attach(struct device *parent, struct device 
> *self, void *aux)
>  
>   pinctrl_byname(sc->sc_node, "default");
>  
> - clock_enable_all(sc->sc_node);
>   reset_deassert_all(sc->sc_node);
>  
> + clock_enable_all(sc->sc_node);
> +
>   /* Disable interrupts, so we don't get any spurious ones. */
>   sc->sc.sc_offs = EREAD1(>sc, EHCI_CAPLENGTH);
>   EOWRITE2(>sc, EHCI_USBINTR, 0);
> diff --git a/sys/dev/fdt/if_dwge_fdt.c b/sys/dev/fdt/if_dwge_fdt.c
> index edfe5acb992..00668980f4a 100644
> --- a/sys/dev/fdt/if_dwge_fdt.c
> +++ b/sys/dev/fdt/if_dwge_fdt.c
> @@ -125,10 +125,10 @@ dwge_fdt_attach(struct device *parent, struct device 
> *self, void *aux)
>   else if (OF_is_compatible(faa->fa_node, "rockchip,rk3399-gmac"))
>   dwge_fdt_attach_rockchip(fsc);
>  
> + reset_deassert(faa->fa_node, "stmmaceth");
> +
>   /* Enable clock. */
>   clock_enable(faa->fa_node, "stmmaceth");
> - reset_deassert(faa->fa_node, "stmmaceth");
> - delay(5000);
>  
>   /* Power up PHY. */
>   phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> diff --git a/sys/dev/fdt/if_dwxe.c b/sys/dev/fdt/if_dwxe.c
> index 22a383c06ef..6e303745ec3 100644
> --- a/sys/dev/fdt/if_dwxe.c
> +++ b/sys/dev/fdt/if_dwxe.c
> @@ -376,10 +376,10 @@ dwxe_attach(struct device *parent, struct device *self, 
> void *aux)
>  
>   pinctrl_byname(faa->fa_node, "default");
>  
> + reset_deassert(faa->fa_node, "stmmaceth");
> +
>   /* Enable clock. */
>   clock_enable(faa->fa_node, "stmmaceth");
> - reset_deassert(faa->fa_node, "stmmaceth");
> - delay(5000);
>  
>   /* Power up PHY. */
>   phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> diff --git a/sys/dev/fdt/sximmc.c b/sys/dev/fdt/sximmc.c
> index 7acb8f55d56..9ed2ae09fe4 100644
> --- a/sys/dev/fdt/sximmc.c
> +++ b/sys/dev/fdt/sximmc.c
> @@ -366,11 +366,10 @@ sximmc_attach(struct device *parent, struct device 
> *self, void *aux)
>  
>   pinctrl_byname(faa->fa_node, "default");
>  
> + reset_deassert_all(faa->fa_node);
> +
>   /* enable clock */
>   clock_enable(faa->fa_node, NULL);
> - delay(5000);
> -
> - reset_deassert_all(faa->fa_node);
>  
>   /*
>* The FIFO register is in a different location on the
> diff --git a/sys/dev/fdt/sxitwi.c b/sys/dev/fdt/sxitwi.c
> index f53f2bfd594..c80bdc2ab06 100644
> --- a/sys/dev/fdt/sxitwi.c
> +++ b/sys/dev/fdt/sxitwi.c
> @@ -218,6 +218,8 @@ sxitwi_attach(struct device *parent, struct device *self, 
> void *aux)
>  
>   pinctrl_byname(faa->fa_node, "default");
>  
> + reset_deassert_all(faa->fa_node);
> +
>   /* Enable clock */
>   clock_enable(faa->fa_node, NULL);
>  

Re: assertion "_kernel_lock_held()" failed, uipc_socket2.c: ipsec

[Stuart Henderson]

On 2017/11/12 22:48, Martin Pieuchot wrote:
> On 12/11/17(Sun) 21:30, Stuart Henderson wrote:
> > iked box, GENERIC.MP + WITNESS, -current as of Friday 10th:
> 
> Weird, did you tweak "kern.splassert" on this box?   Otherwise is looks
> like a major corruption.

It would have kern.splassert=2. (I know this can cause problems
sometimes, though this would be the first time in 5+ years I've bumped
into it, most of my routers where I have serial console have this set).

> > login: panic: kernel diagnostic assertion "_kernel_lock_held()" failed: 
> > file "/src/cvs-openbsd/sys/kern/uipc_socket2.c", line 310
> ^^^
> Looks like one CPU is triggering this.
> 
> > splassert: soassertlocked: want 1 have 256
> > 
> > panic: spl assertion failure in soassertlocked
> ^^^
> That can't be coming from the same CPU..
> 
> 
> 
> 
> > Starting stack trace...
> > Faulted in traceback, aborting...
> > panic(splassert: if_down: want 1 have 256
> > panic: spl assertion failure in if_down) at
> > Faulted in traceback, aborting...
> > panicsplassert: if_down: want 1 have 256
> > +0x133panic: spl assertion failure in if_down
> > Faulted in traceback, aborting...
> > 
> > 
> > 
> > It's stuck at this point, I can't enter ddb.
> 
> Are you running with WITNESS on purpose?  Can you reproduce such problem
> without it?  I'm not saying it's WITNESS fault, but it's clear that
> WITNESS kernels aren't ready for production yet.
> 

I'm trying to get more information because it had either hanged or
panicked previously (it didn't have serial connected at the time and
the machine was needed so it had to be rebooted before I had chance
to dig into it).

Add sizes for free() in the VIA PadLock driver

[Frederic Cambus]

Hi tech@,

Add sizes for free() in the VIA PadLock driver.

Comments? OK?

Index: sys/arch/amd64/amd64/via.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/via.c,v
retrieving revision 1.24
diff -u -p -r1.24 via.c
--- sys/arch/amd64/amd64/via.c  14 Oct 2017 04:44:43 -  1.24
+++ sys/arch/amd64/amd64/via.c  12 Nov 2017 20:18:37 -
@@ -144,7 +144,7 @@ viac3_crypto_newsession(u_int32_t *sidp,
return (ENOMEM);
memcpy(ses, sc->sc_sessions, sesn * sizeof(*ses));
explicit_bzero(sc->sc_sessions, sesn * sizeof(*ses));
-   free(sc->sc_sessions, M_DEVBUF, 0);
+   free(sc->sc_sessions, M_DEVBUF, sesn * sizeof(*ses));
sc->sc_sessions = ses;
ses = >sc_sessions[sesn];
sc->sc_nsessions++;
@@ -284,13 +284,13 @@ viac3_crypto_freesession(u_int64_t tid)
 
if (swd->sw_ictx) {
explicit_bzero(swd->sw_ictx, axf->ctxsize);
-   free(swd->sw_ictx, M_CRYPTO_DATA, 0);
+   free(swd->sw_ictx, M_CRYPTO_DATA, axf->ctxsize);
}
if (swd->sw_octx) {
explicit_bzero(swd->sw_octx, axf->ctxsize);
-   free(swd->sw_octx, M_CRYPTO_DATA, 0);
+   free(swd->sw_octx, M_CRYPTO_DATA, axf->ctxsize);
}
-   free(swd, M_CRYPTO_DATA, 0);
+   free(swd, M_CRYPTO_DATA, sizeof(*swd));
}
 
explicit_bzero(>sc_sessions[sesn], sizeof(sc->sc_sessions[sesn]));
@@ -411,7 +411,7 @@ viac3_crypto_encdec(struct cryptop *crp,
 
if (sc->op_buf != NULL) {
explicit_bzero(sc->op_buf, crd->crd_len);
-   free(sc->op_buf, M_DEVBUF, 0);
+   free(sc->op_buf, M_DEVBUF, crd->crd_len);
sc->op_buf = NULL;
}
 

Re: ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

[Robert Peichaer]

On Sun, Nov 12, 2017 at 12:22:27AM +0100, Klemens Nanni wrote:
> On Sat, Nov 11, 2017 at 08:03:36PM +, Robert Peichaer wrote:
> > On Sat, Nov 11, 2017 at 08:11:25PM +0100, Klemens Nanni wrote:
> > > pre_path()ing directories with spaces is broken due to bad quoting.
> > > 
> > > This diff takes care of that by properly passing double quotes through
> > > eval and quoting the arguments for no_path() individually.
> > > 
> > > Feedback?
> > 
> > What is actually broken?
> > Can you give examples?
> Sure, pardon me.
> 
>   $ typeset -f add_path
>   function add_path {
>   [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
>   }
>   $ echo $PATH
>   /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
>   $ mkdir some\ bin
>   $ add_path some\ bin
>   add_path: bin: not found
>   $ echo $PATH
>   /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
>   $ PATH=$PATH:some\ bin
>   $ del_path some\ bin
>   $ echo $?
>   0
>   $ echo $PATH
>   /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:some bin/
> 
> Since the double quotes are not escaped and thus quote the string to be
> evaluated itself, `PATH=$PATH:some bin' is eventually being executed as
> seen above.
> 
> pre_path() behaves the same trying to execute `PATH=some bin:$PATH'.
> 
> del_path() silently fails to remove the directory.
> 
> Here's another example showing how to exploit this:
> 
>   $ ls foo
>   ls: foo: No such file or directory
>   $ mkdir '. touch foo'
>   $ add_path '. touch foo'
>   $ ls foo
>   foo
> 
> With my patch behaviour will be as expected.

Hmm. I see.

The {add,del,no,pre}_path functions are in ksh.kshrc since when it was
imported 21 years ago. If they would be used in ksh.kshrc or anywhere
else, I'd say it might be worth "fixing" these functions. But they are
not used anywhere in the tree and I would rather remove them alltogether.

In case someone uses them, ~/.kshrc seems to be a more appropriate place.

-- 
-=[rpe]=-

Re: mbuf statistics, tracking of drops

[Gregor Best]

Hi Martin,

On Sun, Nov 12, 2017 at 03:40:59PM +0100, Martin Pieuchot wrote:
> [...]
> It does, some comments below.
> [...]

Wonderful.

> [...]
> This would be an approximation because it might happen that after
> returning NULL the second pool_get(9) won't sleep.  However I think
> it's the way to go because m_get(9) calls that can wait are generally
> not performance critical.
> [...]

I had not considered that the second pool_get might not block at all. On
the other hand `netstat -m` says "requests for memory delayed" for that
counter, so returning immediately on the second try not counting as a
delay is OK, I think.

> [...]
> Please do not expand the splx().  Only the counter need it.  Simply put
> the NULL check after the splx().
> [...]

I'm not sure I understand. If I move the NULL check after the splx(),
counters_leave() will already have been called so increasing the counter
value has no effect anymore. The only additional things running under
splnet will be a few assignments, so I think moving the splx() a bit
further down does not hurt too much.

Alternatively, I've attached a slightly different suggestion which
doesn't expand the scope of the splx() but duplicates a bit of code.
Does that make more sense?

-- 
Gregor

Index: uipc_mbuf.c
===
RCS file: /home/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.250
diff -u -p -r1.250 uipc_mbuf.c
--- uipc_mbuf.c 12 Oct 2017 09:14:16 -  1.250
+++ uipc_mbuf.c 12 Nov 2017 22:09:00 -
@@ -233,11 +233,15 @@ m_get(int nowait, int type)
KDASSERT(type < MT_NTYPES);
 
m = pool_get(, nowait == M_WAIT ? PR_WAITOK : PR_NOWAIT);
-   if (m == NULL)
-   return (NULL);
 
s = splnet();
counters = counters_enter(, mbstat);
+   if (m == NULL) {
+   counters[MBSTAT_DROPS]++;
+   counters_leave(, mbstat);
+   splx(s);
+   return NULL;
+   }
counters[type]++;
counters_leave(, mbstat);
splx(s);
@@ -266,11 +270,15 @@ m_gethdr(int nowait, int type)
KDASSERT(type < MT_NTYPES);
 
m = pool_get(, nowait == M_WAIT ? PR_WAITOK : PR_NOWAIT);
-   if (m == NULL)
-   return (NULL);
 
s = splnet();
counters = counters_enter(, mbstat);
+   if (m == NULL) {
+   counters[MBSTAT_DROPS]++;
+   counters_leave(, mbstat);
+   splx(s);
+   return NULL;
+   }
counters[type]++;
counters_leave(, mbstat);
splx(s);
@@ -349,7 +357,10 @@ m_clget(struct mbuf *m, int how, u_int p
 {
struct mbuf *m0 = NULL;
struct pool *pp;
+   struct counters_ref cr;
+   uint64_t *counters;
caddr_t buf;
+   int s;
 
pp = m_clpool(pktlen);
 #ifdef DIAGNOSTIC
@@ -364,9 +375,16 @@ m_clget(struct mbuf *m, int how, u_int p
 
m = m0;
}
+
buf = pool_get(pp, how == M_WAIT ? PR_WAITOK : PR_NOWAIT);
+
if (buf == NULL) {
m_freem(m0);
+   s = splnet();
+   counters = counters_enter(, mbstat);
+   counters[MBSTAT_DROPS]++;
+   counters_leave(, mbstat);
+   splx(s);
return (NULL);
}