Re: [RFC PATCH 1/5] entry: Pass pt_regs to irqentry_exit_cond_resched()

[Ira Weiny]

On Mon, Aug 08, 2022 at 12:38:08PM +0200, Borislav Petkov wrote:
> On Fri, Aug 05, 2022 at 10:30:05AM -0700, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > 
> > Auxiliary pt_regs space needs to be manipulated by the generic
> > entry/exit code.
> > 
> > Ideally irqentry_exit() would take care of handling any auxiliary
> > pt_regs on exit.  Unfortunately, irqentry_exit() is not the only exit
> > from exception path.  The call to irqentry_exit_cond_resched() from
> > xen_pv_evtchn_do_upcall() bypasses irqentry_exit().
> > 
> > Make irqentry_exit_cond_resched() symmetrical with irqentry_enter() by
> > passing pt_regs to it.  This makes irqentry_exit_cond_resched() capable
> > of handling auxiliary pt_regs in future patches.
> > 
> > Cc: Rik van Riel <riel@xxxxxxxxxxx>
> > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > 
> > ---
> > Forward ported from PKS series:
> > 	https://lore.kernel.org/lkml/20220419170649.1022246-19-ira.weiny@xxxxxxxxx/
> > ---
> >  arch/arm64/include/asm/preempt.h |  2 +-
> >  arch/arm64/kernel/entry-common.c |  4 ++--
> >  arch/x86/entry/common.c          |  2 +-
> >  include/linux/entry-common.h     | 17 ++++++++------
> >  kernel/entry/common.c            | 13 +++++++----
> >  kernel/sched/core.c              | 40 ++++++++++++++++----------------
> >  6 files changed, 43 insertions(+), 35 deletions(-)
> 
> Why all this churn?
> 
> Why can't you add a parameter to irqentry_exit():
> 
>   noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state, bool cond_resched);

I thought about that but generally have been steered away from using bool
arguments like this.

> 
> and then have all callers except xen_pv_evtchn_do_upcall() pass in false
> and this way have all exit paths end up in irqentry_exit()?
> 
> And, ofc, move the true case which is the body of
> raw_irqentry_exit_cond_resched() to irqentry_exit() and then get rid of
> former.

Looking this over I think this may be a reasonable use of a flag like that.

I'm not sure if this series is going to land given the discussion of
performance.  Would this clean up be welcome on its own?  I'm willing to respin
this patch and submit if so.

Ira

> 
> xen_pv_evtchn_do_upcall() will, ofc, do:
> 
>         if (inhcall && !WARN_ON_ONCE(state.exit_rcu)) {
>                 irqentry_exit(regs, state, true);
>                 instrumentation_end();
>                 restore_inhcall(inhcall);
>         } else {
>                 instrumentation_end();
>                 irqentry_exit(regs, state, false);
> 
> Hmmm?
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette