Re: CVS commit: src/sys

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Sat, May 11, 2019 at 10:49 AM Greg Troxel <gdt%lexort.com@localhost> wrote:
>
> Kamil Rytarowski <n54%gmx.com@localhost> writes:
>
> > On 08.05.2019 11:34, Ryota Ozaki wrote:
> >> On Sat, Apr 20, 2019 at 6:45 PM Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> >>>
> >>> On Fri, Apr 19, 2019 at 6:49 PM Kamil Rytarowski <n54%gmx.com@localhost> wrote:
> >>>>
> >>>> On 19.04.2019 11:41, J. Hannken-Illjes wrote:
> >>>>>> On 19. Apr 2019, at 03:52, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> >>>>>>
> >>>>>> Module Name: src
> >>>>>> Committed By:        ozaki-r
> >>>>>> Date:                Fri Apr 19 01:52:56 UTC 2019
> >>>>>>
> >>>>>> Modified Files:
> >>>>>>      src/sys/kern: kern_lwp.c kern_softint.c subr_psref.c
> >>>>>>      src/sys/rump/kern/lib/libsysproxy: sysproxy.c
> >>>>>>      src/sys/sys: lwp.h userret.h
> >>>>>>
> >>>>>> Log Message:
> >>>>>> Implement a simple psref leak detector
> >>>>>>
> >>>>>> It detects leaks by counting up the number of held psref by an LWP and checking
> >>>>>> its zeroness at the end of syscalls and softint handlers.  For the counter, a
> >>>>>> unused field of struct lwp is reused.
> >>>>>
> >>>>> For DIAGNOSTIC-only operations LWP specific data
> >>>>> (see kern/subr_lwp_specificdata.c) is a better choice.
> >>>>>
> >>>>
> >>>> I wanted to propose the same. An exampe of this is in KCOV.
> >>>
> >>> Thanks.  I'll try it.  (I'll be AFK for the next few days...)
> >>
> >> I'm sorry for delaying this task.  Finally I have benchmarked a revised patch
> >> (our benchmarking setups have been out of service for a couple of weeks...).
> >>
> >> Performance degradation of IP forwarding is 3%.  Is it acceptable as
> >> DIAGNOSTIC?
> >>
> >
> > For DIAGNOSTIC should be fine.
>
> I think 3% is too much for DIAGNOSTIC.   DEBUG, sure, and a specific
> option to turn it on seems fine.  DIAGNOSTIC historically has been only
> for things that check invariants and assert, such that if you don't mind
> the crashes when detecting things, there is no performance reason to
> avoid it.

So we have two options:
1. Keep the original (enabled on DIAGNOSTIC)
  - Its performance impact is negligible
2. Migrate to use lwp_specificadata and enable the feature on DEBUG
(and something)

I prefer to 1. because I want the feature to be enabled by many users
(I assume that users (of -current) tend to enable DIAGNOSTIC and not DEBUG).
If the detector is not enabled, a psref leak appears as a form that is
difficult to know
where the leak occurs;  a fatal page fault occurs on
psref_target_destroy that is
a completely different context.  If enabled, we can at least know a
suspect LWP or
softint handler and may find a cause in luck.

  ozaki-r