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 -0000 1.175
> +++ sys/sysctl.h 13 Nov 2017 03:42:32 -0000
> @@ -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 -0000 1.330
> +++ kern/kern_sysctl.c 13 Nov 2017 03:42:32 -0000
> @@ -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(&mbs, 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,
> - &mbs, 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 -0000 1.250
> +++ kern/uipc_mbuf.c 13 Nov 2017 03:42:32 -0000
> @@ -1421,6 +1421,30 @@ m_pool_init(struct pool *pp, u_int size,
> pool_set_constraints(pp, &kp_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(&mbs, 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 (sysctl_rdstruct(oldp, oldlenp, newp, &mbs, sizeof(mbs)));
> +}
> +
> #ifdef DDB
> void
> m_print(void *v,
