On Mon, Oct 21, 2024 at 3:48 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > > On Fri, Oct 18, 2024 at 11:22:09AM -0700, Andrii Nakryiko wrote: > > > > So... after a few readings I think I'm mostly okay with this. But I got > > > annoyed by the whole HPROBE_STABLE with uprobe=NULL weirdness. Also, > > > that data_race() usage is weird, what is that about? > > > > People keep saying that evil KCSAN will come after me if I don't add > > data_race() for values that can change under me, so I add it to make > > it explicit that it's fine. But I can of course just drop data_race(), > > as it has no bearing on correctness. > > AFAICT this was READ_ONCE() vs xchg(), and that should work. Otherwise I > have to yell at KCSAN people again :-) > sounds good, READ_ONCE() it is :) > > > And then there's the case where we end up doing: > > > > > > try_get_uprobe() > > > put_uprobe() > > > try_get_uprobe() > > > > > > in the dup path. Yes, it's unlikely, but gah. > > > > > > > > > So how about something like this? > > > > Yep, it makes sense to start with HPROBE_GONE if it's already NULL, no > > problem. I'll roll those changes in. > > > > I'm fine with the `bool get` flag as well. Will incorporate all that > > into the next revision, thanks! > > > > The only problem I can see is in the assumption that `srcu_idx < 0` is > > never going to be returned by srcu_read_lock(). Paul says that it can > > only be 0 or 1, but it's not codified as part of a contract. > > Yeah, [0,1] is the current range. Fundamentally that thing is an array > index, so negative values are out and generally safe to use as 'error' > codes. Paul can't we simply document that the SRCU cookie is always a > positive integer (or zero) and the negative space shall not be used? > > > So until we change that, probably safer to pass an extra bool > > specifying whether srcu_idx is valid or not, is that OK? > > I think Changeing the SRCU documentation to provide us this guarantee > should be an achievable goal. agreed, I'll let Paul handle that, but will assume srcu_idx < 0 can't legally happen > > > (and I assume you want me to drop verbose comments for various states, right?) > > I axed the comments because I made them invalid and didn't care enough > to fix them up. If you like them feel free to amend them to reflect the > new state of things. got it, I'll update where necessary
