Re: git: fa1d803c0f65 - main - epoch: replace hand coded assertion

From: Gleb Smirnoff <glebius_at_freebsd.org>
Date: Tue, 31 Jan 2023 18:38:15 UTC 
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