Date: Tue, 31 Jan 2023 10:38:15 -0800 From: Gleb Smirnoff <glebius@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, Brooks Davis <brooks@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: fa1d803c0f65 - main - epoch: replace hand coded assertion Message-ID: <Y9lgF4qE90zDh9eN@FreeBSD.org> In-Reply-To: <e6cbc000-4bd8-f1cd-f600-4393dc371371@FreeBSD.org> References: <202301201805.30KI5Ht0099187@gitrepo.freebsd.org> <Y9gY3ZN5da3RxnvW@cell.glebi.us> <Y9hFJzrDePKSesCY@kib.kiev.ua> <e6cbc000-4bd8-f1cd-f600-4393dc371371@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jan 31, 2023 at 10:30:49AM -0800, John Baldwin wrote: J> >> @@ -420,7 +420,8 @@ _epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et EPOCH_FILE_LINE) J> >> MPASS(cold || epoch != NULL); J> >> MPASS(epoch->e_flags & EPOCH_PREEMPT); J> >> td = curthread; J> >> - MPASS((vm_offset_t)et >= td->td_kstack && J> >> + MPASS((vm_offset_t)et + sizeof(struct epoch_tracker) >= J> >> + (vm_offset_t)__builtin_frame_address(0) && J> >> (vm_offset_t)et + sizeof(struct epoch_tracker) <= J> >> td->td_kstack + td->td_kstack_pages * PAGE_SIZE); J> >> J> >> What do you guys think on legitimacy of the improved assertion that uses J> >> compiler built to get the current top of stack and makes the assertion even J> >> stricter? J> >> J> >> I think in FreeBSD we should at least add the MPASS(kstack_contains(td, ...) J> >> assertion to the epoch exit. J> > J> > The __builtin_frame_address() is somewhat weird thing, e.g. it assumes J> > that the local function frame is contiguous, and static. Also I do not J> > see a guarantee that any local variable address is higher that the value J> > returned from the builtin. J> J> Though in these cases the pointer value (et) is not a local variable on J> this frame, but should be in an earlier frame (usually the callers). Yes, this is exactly what my patch does. Limit the presence of epoch_tracker to the already used part of the stack. We've been running with this patch for couple years. -- Gleb Smirnoff
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Y9lgF4qE90zDh9eN>