Re: [PATCH v3 00/17] Add static_call()

Re: [PATCH v3 00/17] Add static_call()

[Peter Zijlstra]

On Fri, Mar 20, 2020 at 10:38:44PM +0100, Peter Zijlstra wrote:
> static_call(), is the idea of static_branch() applied to indirect function
> calls. Remove a data load (indirection) by modifying the text.
> 
> The inline implementation still relies on objtool to generate the
> .static_call_sites section, mostly because this is a natural place for x86_64
> and works for both GCC and LLVM.  Other architectures can pick other means
> if/when they implement the inline patching. The out-of-line (aka. trampoline)
> variant doesn't require this.
> 
> Patches can also be found here:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/static_call
> 
> 
> Comments?

*groan*, I got the lkml address wrong (again!), this series is cursed.

Re: [PATCH v3 01/17] notifier: Fix broken error handling pattern

[Peter Zijlstra]

On Sat, Mar 21, 2020 at 01:24:19PM +0100, Borislav Petkov wrote:
> On Fri, Mar 20, 2020 at 10:38:45PM +0100, Peter Zijlstra wrote:
> > The current notifiers have the following error handling pattern all
> > over the place:
> > 
> > 	int err, nr;
> > 
> > 	err = __foo_notifier_call_chain(&chain, val_up, v, -1, &nr);
> > 	if (err & NOTIFIER_STOP_MASK)
> > 		__foo_notifier_call_chain(&chain, val_down, v, nr-1, NULL)
> > 
> > And aside from the endless repetition thereof, it is broken. Consider
> > blocking notifiers; both calls take and drop the rwsem, this means
> > that the notifier list can change in between the two calls, making @nr
> > meaningless.
> > 
> > Fix this by replacing all the __foo_notifier_call_chain() functions
> > with foo_notifier_call_chain_robust() that embeds the above pattern,
> > but ensures it is inside a single lock region.
> 
> "robust" huh? Sure reads funny :)

This has been around the bike-shed a few times already.

> So if the "normal" notifier_call_chain() usage is buggy, how about we
> prepend its name with "__" and call the new one with the rollback,
> notifier_call_chain() ?
> 
> Instead of adding the "robust" set of interfaces?

Well, it depends on the usecase. The robust one can deal with failure,
the other ones are fine (and preferred) if failure is not an option.

This robust variant ensures that all the notifiers that succeeded prior
to the one that failed get a second callback with another state. Some
notifier chains don't care, but a few clearly did and did it utterly
broken.

> Btw, the indentation in that notifier.* files is yuck.

Yeha, wasn't going to fix that.