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,

Reply via email to