Re: CVS commit: src/sys/netinet6

[Ryota Ozaki]

Hi,

I'm going to change to use callout_stop because it seems using it
is almost harmless in practical. See the below explanation (tl;dr).


I investigated how using callout_stop affects and figured out
it can be problematic but the probability is quite low.
(That's why NetBSD 7 and earlier using callout_stop have worked
without any problems until now.)

The issue of using callout_stop (not callout_halt) is that
callout_stop doesn't wait for the running(*) callout handler to
finish. In DAD cases, DAD-related data (struct dadq, dp) are
freed after callout_stop so the callout handler could
use-after-free it. However, the handler doesn't access dp where
callout_stop is called; the handler is passed ifa (not dp) and
it looks up dp by ifa (**). At the point that callout_stop is
called, the target dp is removed from the global list and the
handler fails to look up and returns with doing nothing.

(*) If the callout is scheduled but the handler isn't dispatched
  when calling callout_stop, it's just canceled.
(**) dp has a pointer to ifa (dp->dad_ifa)

One issue is on using a passed ifa. It can be a dangling pointer
during executing the callout handler because the handler doesn't
guarantee that the ifa isn't freed. Fortunately the handler uses
the ifa only as an address (to look up dp), which is harmless.
One possible problem of touching an ifa which may point a freed
memory area is when the area is reused as another ifa (ifa~).
In that case a wrong dp that points ifa~ can be looked up in the
callout handler, which causes unexpected behaviors.

I estimate that that happens in theory but is unlikely to happen
in practical. Am I wrong?


Note that of course I agree that using callout_halt is the way
to go (actually it's used in the NET_MPSAFE case) and using
callout_stop is just a temporal solution.

  ozaki-r

Re: CVS commit: src/sys/netinet

[Ryota Ozaki]

On Thu, Jan 11, 2018 at 3:51 AM, Christos Zoulas  wrote:
> Module Name:src
> Committed By:   christos
> Date:   Wed Jan 10 18:51:31 UTC 2018
>
> Modified Files:
> src/sys/netinet: ip_output.c
>
> Log Message:
> from ozaki-r: use the proper ifp.
> XXX: perhaps push the lock in in_delmulti()?

I rather prefer to pass ifp as an argument like in_addmulti.

  ozaki-r

Re: CVS commit: src/sys/arch

[Valery Ushakov]

On Wed, Jan 10, 2018 at 19:45:26 +0100, Maxime Villard wrote:

> Le 04/01/2018 ? 15:50, Valery Ushakov a ?crit :
> > On Thu, Jan 04, 2018 at 13:36:30 +, Maxime Villard wrote:
> > 
> > > Module Name:  src
> > > Committed By: maxv
> > > Date: Thu Jan  4 13:36:30 UTC 2018
> > > 
> > > Modified Files:
> > >   src/sys/arch/amd64/amd64: genassym.cf locore.S machdep.c
> > >   src/sys/arch/i386/i386: genassym.cf locore.S machdep.c
> > >   src/sys/arch/x86/include: cpu.h
> > >   src/sys/arch/x86/x86: intr.c pmap.c sys_machdep.c
> > > 
> > > Log Message:
> > > Allocate the TSS area dynamically. This way cpu_info and cpu_tss can be
> > > put in separate pages.
> > 
> > Splitting tss and cpu info speeds up NetBSD under virtualbox without
> > VT-x quite a bit as vbox traps all TSS accesses and so all the
> > same-page cpu_info accesses are also trapped, slowing things down.
> 
> Did you actually test? And if so, did my commit really change something?

I haven't tested this commit specifically, but I did test it with a
quick hack a few years ago

  https://mail-index.netbsd.org/netbsd-users/2013/06/21/msg013010.html

I don't remember the exact numbers, but it was a lot.  If memory
serves, compiling the kernel was 4x faster or something like that.


> I'm figuring out we might have had a great performance problem here: the
> cpu_info structure is allocated from kmem, which can borrow VA from the
> direct map. Since the direct map was mapped with 1GB superpages (or 2MB
> otherwise), it looks like you could get the whole gigabyte to fault all the
> time.

I don't know if that was true back in 2013 when I tested it, but as
you can see from that kludge I only pushed tss to a page of its own.

-uwe

Re: CVS commit: src/sys/arch

[Maxime Villard]

Le 04/01/2018 à 15:50, Valery Ushakov a écrit :

On Thu, Jan 04, 2018 at 13:36:30 +, Maxime Villard wrote:


Module Name:src
Committed By:   maxv
Date:   Thu Jan  4 13:36:30 UTC 2018

Modified Files:
src/sys/arch/amd64/amd64: genassym.cf locore.S machdep.c
src/sys/arch/i386/i386: genassym.cf locore.S machdep.c
src/sys/arch/x86/include: cpu.h
src/sys/arch/x86/x86: intr.c pmap.c sys_machdep.c

Log Message:
Allocate the TSS area dynamically. This way cpu_info and cpu_tss can be
put in separate pages.


Splitting tss and cpu info speeds up NetBSD under virtualbox without
VT-x quite a bit as vbox traps all TSS accesses and so all the
same-page cpu_info accesses are also trapped, slowing things down.


Did you actually test? And if so, did my commit really change something?

I'm figuring out we might have had a great performance problem here: the
cpu_info structure is allocated from kmem, which can borrow VA from the
direct map. Since the direct map was mapped with 1GB superpages (or 2MB
otherwise), it looks like you could get the whole gigabyte to fault all the
time.

I guess I should pull up this change [1] to NetBSD-8 and 7.

Maxime

[1] http://mail-index.netbsd.org/source-changes/2018/01/07/msg090941.html