svn commit: r364944 - head/sys/kern

[Warner Losh]

On Tue, Sep 15, 2020 at 8:37 AM Mark Johnston <markj at freebsd.org> wrote:

> On Tue, Sep 15, 2020 at 10:22:09AM -0400, Mark Johnston wrote:
> > On Tue, Sep 15, 2020 at 05:15:30PM +0300, Konstantin Belousov wrote:
> > > On Sat, Aug 29, 2020 at 04:29:53AM +0000, Warner Losh wrote:
> > > > Author: imp
> > > > Date: Sat Aug 29 04:29:53 2020
> > > > New Revision: 364944
> > > > URL: https://svnweb.freebsd.org/changeset/base/364944
> > > >
> > > > Log:
> > > >   devctl: move to using a uma zone
> > > >
> > > >   Convert the memory management of devctl.  Rewrite if to make better
> > > >   use of memory. This eliminates several mallocs (5? worse case)
> needed
> > > >   to send a message. It's now possible to always send a message,
> though
> > > >   if things are really backed up the oldest message will be dropped
> to
> > > >   free up space for the newest.
> > > >
> > > >   Add a static bus_child_{location,pnpinfo}_sb to start migrating to
> > > >   sbuf instead of buffer + length. Use it in the new code.  Other
> code
> > > >   will be converted later (bus_child_*_str is only used inside of
> > > >   subr_bus.c, though implemented in ~100 places in the tree).
> > > >
> > > >   Reviewed by: markj@
> > > >   Differential Revision: https://reviews.freebsd.org/D26140
> > > >
> > > > Modified:
> > > >   head/sys/kern/subr_bus.c
> > > >
> > >
> > > > + } else {
> > > > +         /* dei can't be NULL -- we know we have at least one in
> the zone */
> > > > +         dei = uma_zalloc(devsoftc.zone, M_NOWAIT);
> > > > +         MPASS(dei != NULL);
> > > This does not work.  I believe you need to disable per-cpu cache for
> the
> > > zone, at least.  But I am not sure it is enough.
> >
> > From the report we have:
> >
> > db:0:pho>  show uma
> >               Zone   Size    Used    Free    Requests  Sleeps  Bucket
> Total Mem    XFree
> >             DEVCTL   1024 6415164       0     6416203       0      16
> 6569127936        0
> >
> > so it looks like the primary problem is a leak.
> >
> > > https://people.freebsd.org/~pho/stress/log/kostik1314.txt
>
> devctl_queue() does not maintain the queue length bound, so if devd goes
> away (due to an OOM kill in this case), we can end up with a large
> backlog.  This hack might be sufficient.
>
> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c
> index 19c056ab9974..91c57cdfae5b 100644
> --- a/sys/kern/subr_bus.c
> +++ b/sys/kern/subr_bus.c
> @@ -635,7 +635,14 @@ devctl_queue(struct dev_event_info *dei)
>  {
>         mtx_lock(&devsoftc.mtx);
>         STAILQ_INSERT_TAIL(&devsoftc.devq, dei, dei_link);
> -       devsoftc.queued++;
> +       if (devctl_queue_length != 0 &&
> +           devctl_queue_length == devsoftc.queued) {
> +               dei = STAILQ_FIRST(&devsoftc.devq);
> +               STAILQ_REMOVE_HEAD(&devsoftc.devq, dei_link);
> +               uma_zfree(devsoftc.zone, dei);
> +       } else {
> +               devsoftc.queued++;
> +       }
>         cv_broadcast(&devsoftc.cv);
>         KNOTE_LOCKED(&devsoftc.sel.si_note, 0);
>         mtx_unlock(&devsoftc.mtx);
>

I've come to a similar conclusion...

You can't queue w/o allocating... and that's where we're supposed to pop
off the oldest, free it so we can allocate.

So this isn't the right place for this fix, and the 'queued' name may be a
bad name since it's from before. It's now supposed to be closer to
'allocated'.

Warner