Messages in this thread | | | Date | Thu, 9 Aug 2018 18:30:15 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path |
| |
On Thu, Aug 09, 2018 at 04:17:13PM +0200, Oleg Nesterov wrote: > On 08/09, Jiri Olsa wrote: > > > > @@ -523,8 +523,10 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att > > perf_event_disable(bp); > > > > err = modify_user_hw_breakpoint_check(bp, attr, false); > > - if (err) > > + if (err) { > > + bp->attr.disabled = 1; > > return err; > > Yes, but on the second thought... Can't we simply do > > int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr) > { > int err; > > /* > * modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it > * will not be possible to raise IPIs that invoke __perf_event_disable. > * So call the function directly after making sure we are targeting the > * current task. > */ > if (irqs_disabled() && bp->ctx && bp->ctx->task == current) > perf_event_disable_local(bp); > else > perf_event_disable(bp); > > err = modify_user_hw_breakpoint_check(bp, attr, false); > > if (!bp.attr->disabled) > perf_event_enable(bp); > > return err; > } > > instead of this and the next patch? > > We can do this because (as you pointed out) validate_hw_breakpoint() has already > gone in -tip tree, and (afaics) modify_user_hw_breakpoint_check() never changes > perf_event's state on failure, so we can safely "restart" this bp if it was enabled > before. > > 1. This is what we had before the recent f67b15037a7a50c57f72e69a6d59941ad90a0f0f > "perf/hwbp: Simplify the perf-hwbp code, fix documentation". > > Note that this commit was actually the bug fix which introduced another problem > fixed by your 2/5. > > But see above, perf_event_enable() is no longer unsafe after > modify_user_hw_breakpoint_check(), we can restore the previous semantics. > > 2. This matches perf_event_modify_breakpoint(). Which btw can be simplified a bit, > it too can simply do > > if (!bp->attr.disabled) > _perf_event_enable(bp); > > return err;
yep, seems good.. will send new version
thanks, jirka
| |