Re: CVS commit: src/sys/netinet6
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
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
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
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