Messages in this thread | | | Date | Tue, 25 Jun 2019 09:38:21 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 1/3] notifier: Fix broken error handling pattern |
| |
On Mon, Jun 24, 2019 at 05:21:07PM -0500, Josh Poimboeuf wrote: > On Mon, Jun 24, 2019 at 11:18:44AM +0200, Peter Zijlstra wrote: > > The current notifiers have the following error handling pattern all > > over the place: > > > > int nr; > > > > ret = __foo_notifier_call_chain(&chain, val_up, v, -1, &nr); > > if (err & NOTIFIER_STOP_MASK) > > s/err/ret/
-ETOOWARM :-)
> > __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_error() that embeds the above patter, but > > ensures it is inside a single lock region. > > The name "notifier_call_chain_error()" seems confusing, it almost sounds > like it's notifying an error code. Then again, I can't really think of > a more reasonably succinct name.
I;m not attached to the name; I very much ran out of ideas and just typed something.
> > @@ -25,8 +25,23 @@ static int cpu_pm_notify(enum cpu_pm_eve > > * RCU know this. > > */ > > rcu_irq_enter_irqson(); > > - ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL, > > - nr_to_call, nr_calls); > > + ret = atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL); > > + rcu_irq_exit_irqson(); > > + > > + return notifier_to_errno(ret); > > +} > > + > > +static int cpu_pm_notify_error(enum cpu_pm_event event_up, enum cpu_pm_event event_down) > > +{ > > + int ret; > > + > > + /* > > + * __atomic_notifier_call_chain has a RCU read critical section, which > > __atomic_notifier_call_chain() no longer exists. > > > + * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let > > "dysfunctional"
That's copy paste, I don't think I've read the comment, my bad.
> > @@ -156,43 +169,30 @@ int atomic_notifier_chain_unregister(str > > } > > EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister); > > > > -/** > > - * __atomic_notifier_call_chain - Call functions in an atomic notifier chain > > - * @nh: Pointer to head of the atomic notifier chain > > - * @val: Value passed unmodified to notifier function > > - * @v: Pointer passed unmodified to notifier function > > - * @nr_to_call: See the comment for notifier_call_chain. > > - * @nr_calls: See the comment for notifier_call_chain. > > - * > > - * Calls each function in a notifier chain in turn. The functions > > - * run in an atomic context, so they must not block. > > - * This routine uses RCU to synchronize with changes to the chain. > > - * > > - * If the return value of the notifier can be and'ed > > - * with %NOTIFY_STOP_MASK then atomic_notifier_call_chain() > > - * will return immediately, with the return value of > > - * the notifier function which halted execution. > > - * Otherwise the return value is the return value > > - * of the last notifier function called. > > - */ > > Why remove the useful comment?
Because I delete the whole function ?
| |