| Message ID | 20240827110046.3209679-1-liaochang1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Return early when break handler is found on linked-list | expand |
On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote: > The search for breakpoint handlers iterate through the entire > linked list. Given that all registered hook has a valid fn field, and no > registered hooks share the same mask and imm. This commit optimize the > efficiency slightly by returning early as a matching handler is found. > > Signed-off-by: Liao Chang <liaochang1@huawei.com> > --- > arch/arm64/kernel/debug-monitors.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 024a7b245056..fc998956f44c 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook); > > void register_user_break_hook(struct break_hook *hook) > { > + WARN_ON(!hook->fn); > register_debug_hook(&hook->node, &user_break_hook); > } > > @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook) > > void register_kernel_break_hook(struct break_hook *hook) > { > + WARN_ON(!hook->fn); > register_debug_hook(&hook->node, &kernel_break_hook); > } I don't think we need these WARN_ON()s. This API is pretty limited and passing a NULL callback doesn't make sense. Rest of the patch looks fine. Will
On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote: > The search for breakpoint handlers iterate through the entire > linked list. Given that all registered hook has a valid fn field, and no > registered hooks share the same mask and imm. This commit optimize the > efficiency slightly by returning early as a matching handler is found. > > Signed-off-by: Liao Chang <liaochang1@huawei.com> This looks fine, though I'd love if we could clean this up to remove the linked list entirely by separating the user/kernel entrypoints and using a switch statement to decide the handler based on the immediate. That'd also remove the need for RCU protection. Last I looked that would require some largely mechanical restructuring, and the only painful bit was the hooks that KGDB uses, since those are the only ones that actually get unregistered. Mark. > --- > arch/arm64/kernel/debug-monitors.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 024a7b245056..fc998956f44c 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook); > > void register_user_break_hook(struct break_hook *hook) > { > + WARN_ON(!hook->fn); > register_debug_hook(&hook->node, &user_break_hook); > } > > @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook) > > void register_kernel_break_hook(struct break_hook *hook) > { > + WARN_ON(!hook->fn); > register_debug_hook(&hook->node, &kernel_break_hook); > } > > @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) > { > struct break_hook *hook; > struct list_head *list; > - int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL; > > list = user_mode(regs) ? &user_break_hook : &kernel_break_hook; > > @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) > */ > list_for_each_entry_rcu(hook, list, node) { > if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm) > - fn = hook->fn; > + return hook->fn(regs, esr); > } > > - return fn ? fn(regs, esr) : DBG_HOOK_ERROR; > + return DBG_HOOK_ERROR; > } > NOKPROBE_SYMBOL(call_break_hook); > > -- > 2.34.1 > >
在 2024/8/27 20:30, Will Deacon 写道: > On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote: >> The search for breakpoint handlers iterate through the entire >> linked list. Given that all registered hook has a valid fn field, and no >> registered hooks share the same mask and imm. This commit optimize the >> efficiency slightly by returning early as a matching handler is found. >> >> Signed-off-by: Liao Chang <liaochang1@huawei.com> >> --- >> arch/arm64/kernel/debug-monitors.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c >> index 024a7b245056..fc998956f44c 100644 >> --- a/arch/arm64/kernel/debug-monitors.c >> +++ b/arch/arm64/kernel/debug-monitors.c >> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook); >> >> void register_user_break_hook(struct break_hook *hook) >> { >> + WARN_ON(!hook->fn); >> register_debug_hook(&hook->node, &user_break_hook); >> } >> >> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook) >> >> void register_kernel_break_hook(struct break_hook *hook) >> { >> + WARN_ON(!hook->fn); >> register_debug_hook(&hook->node, &kernel_break_hook); >> } > > I don't think we need these WARN_ON()s. This API is pretty limited and > passing a NULL callback doesn't make sense. Them will be removed in next revision. > > Rest of the patch looks fine. > > Will > >
在 2024/8/27 20:50, Mark Rutland 写道: > On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote: >> The search for breakpoint handlers iterate through the entire >> linked list. Given that all registered hook has a valid fn field, and no >> registered hooks share the same mask and imm. This commit optimize the >> efficiency slightly by returning early as a matching handler is found. >> >> Signed-off-by: Liao Chang <liaochang1@huawei.com> > > This looks fine, though I'd love if we could clean this up to remove the > linked list entirely by separating the user/kernel entrypoints and using > a switch statement to decide the handler based on the immediate. That'd > also remove the need for RCU protection. Perhaps I could consider a similar approach to the bad addressing exception in the file arch/arm64/mm/fault.c. It involves defining an array of break hooks, including some default placeholder hooks. Kprobe, uprobe and KGDB could then reuse existing register API to replace atomically these placeholder with specific break hooks. While most break hooks use the default mask for immediate checking in ESR, with exception like KASAN and UBSAN. Then some hard-coded checks will be used in the default base of switch statement for KASAN and UBSAN. That might be a question. > > Last I looked that would require some largely mechanical restructuring, > and the only painful bit was the hooks that KGDB uses, since those are > the only ones that actually get unregistered. Unregistered hooks are repalced automically with the default placeholder hook that returns DGB_HOOK_ERROR. Chang. > > Mark. > >> --- >> arch/arm64/kernel/debug-monitors.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c >> index 024a7b245056..fc998956f44c 100644 >> --- a/arch/arm64/kernel/debug-monitors.c >> +++ b/arch/arm64/kernel/debug-monitors.c >> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook); >> >> void register_user_break_hook(struct break_hook *hook) >> { >> + WARN_ON(!hook->fn); >> register_debug_hook(&hook->node, &user_break_hook); >> } >> >> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook) >> >> void register_kernel_break_hook(struct break_hook *hook) >> { >> + WARN_ON(!hook->fn); >> register_debug_hook(&hook->node, &kernel_break_hook); >> } >> >> @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) >> { >> struct break_hook *hook; >> struct list_head *list; >> - int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL; >> >> list = user_mode(regs) ? &user_break_hook : &kernel_break_hook; >> >> @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) >> */ >> list_for_each_entry_rcu(hook, list, node) { >> if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm) >> - fn = hook->fn; >> + return hook->fn(regs, esr); >> } >> >> - return fn ? fn(regs, esr) : DBG_HOOK_ERROR; >> + return DBG_HOOK_ERROR; >> } >> NOKPROBE_SYMBOL(call_break_hook); >> >> -- >> 2.34.1 >> >> >
Hi, Mark and Will Kindly ping. I'd like to get your input on the next step. Should I send a v2 patch that only removes the WARN_ON() as Will suggested? Or should I push forward the refactoring of calling BRK hook [1]. Thanks. [1] https://lore.kernel.org/all/20240906031930.746118-1-liaochang1@huawei.com/ 在 2024/8/27 20:50, Mark Rutland 写道: > On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote: >> The search for breakpoint handlers iterate through the entire >> linked list. Given that all registered hook has a valid fn field, and no >> registered hooks share the same mask and imm. This commit optimize the >> efficiency slightly by returning early as a matching handler is found. >> >> Signed-off-by: Liao Chang <liaochang1@huawei.com> > > This looks fine, though I'd love if we could clean this up to remove the > linked list entirely by separating the user/kernel entrypoints and using > a switch statement to decide the handler based on the immediate. That'd > also remove the need for RCU protection. > > Last I looked that would require some largely mechanical restructuring, > and the only painful bit was the hooks that KGDB uses, since those are > the only ones that actually get unregistered. > > Mark. > >> --- >> arch/arm64/kernel/debug-monitors.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c >> index 024a7b245056..fc998956f44c 100644 >> --- a/arch/arm64/kernel/debug-monitors.c >> +++ b/arch/arm64/kernel/debug-monitors.c >> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook); >> >> void register_user_break_hook(struct break_hook *hook) >> { >> + WARN_ON(!hook->fn); >> register_debug_hook(&hook->node, &user_break_hook); >> } >> >> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook) >> >> void register_kernel_break_hook(struct break_hook *hook) >> { >> + WARN_ON(!hook->fn); >> register_debug_hook(&hook->node, &kernel_break_hook); >> } >> >> @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) >> { >> struct break_hook *hook; >> struct list_head *list; >> - int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL; >> >> list = user_mode(regs) ? &user_break_hook : &kernel_break_hook; >> >> @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) >> */ >> list_for_each_entry_rcu(hook, list, node) { >> if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm) >> - fn = hook->fn; >> + return hook->fn(regs, esr); >> } >> >> - return fn ? fn(regs, esr) : DBG_HOOK_ERROR; >> + return DBG_HOOK_ERROR; >> } >> NOKPROBE_SYMBOL(call_break_hook); >> >> -- >> 2.34.1 >> >> >
On Wed, Oct 23, 2024 at 09:52:27AM +0800, Liao, Chang wrote: > Kindly ping. > > I'd like to get your input on the next step. Should I send a v2 patch > that only removes the WARN_ON() as Will suggested? Or should I push > forward the refactoring of calling BRK hook [1]. If you have the bandwidth to tackle Mark's refactoring idea, then please feel free to send a series. Otherwise, I'm happy to merge the v2. We could even do both. Will
在 2024/10/24 0:40, Will Deacon 写道: > On Wed, Oct 23, 2024 at 09:52:27AM +0800, Liao, Chang wrote: >> Kindly ping. >> >> I'd like to get your input on the next step. Should I send a v2 patch >> that only removes the WARN_ON() as Will suggested? Or should I push >> forward the refactoring of calling BRK hook [1]. > > If you have the bandwidth to tackle Mark's refactoring idea, then please > feel free to send a series. Otherwise, I'm happy to merge the v2. We > could even do both. Will, please pick the v2 from here: https://lore.kernel.org/all/20241024034120.3814224-1-liaochang1@huawei.com/ Meanwhile, I will discuss the refactoring patch with Mark. Thanks for your suggestion. > > Will
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 024a7b245056..fc998956f44c 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook); void register_user_break_hook(struct break_hook *hook) { + WARN_ON(!hook->fn); register_debug_hook(&hook->node, &user_break_hook); } @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook) void register_kernel_break_hook(struct break_hook *hook) { + WARN_ON(!hook->fn); register_debug_hook(&hook->node, &kernel_break_hook); } @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) { struct break_hook *hook; struct list_head *list; - int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL; list = user_mode(regs) ? &user_break_hook : &kernel_break_hook; @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr) */ list_for_each_entry_rcu(hook, list, node) { if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm) - fn = hook->fn; + return hook->fn(regs, esr); } - return fn ? fn(regs, esr) : DBG_HOOK_ERROR; + return DBG_HOOK_ERROR; } NOKPROBE_SYMBOL(call_break_hook);
The search for breakpoint handlers iterate through the entire linked list. Given that all registered hook has a valid fn field, and no registered hooks share the same mask and imm. This commit optimize the efficiency slightly by returning early as a matching handler is found. Signed-off-by: Liao Chang <liaochang1@huawei.com> --- arch/arm64/kernel/debug-monitors.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)