On Tue, Oct 13, 2020 at 11:52:32AM -0700, Dave Hansen wrote:
> On 10/9/20 12:42 PM, ira.weiny@xxxxxxxxx wrote:
> > @@ -341,6 +341,9 @@ noinstr void irqentry_enter(struct pt_regs *regs, irqentry_state_t *state)
> > /* Use the combo lockdep/tracing function */
> > trace_hardirqs_off();
> > instrumentation_end();
> > +
> > +done:
> > + irq_save_pkrs(state);
> > }
>
> One nit: This saves *and* sets PKRS. It's not obvious from the call
> here that PKRS is altered at this site. Seems like there could be a
> better name.
>
> Even if we did:
>
> irq_save_set_pkrs(state, INIT_VAL);
>
> It would probably compile down to the same thing, but be *really*
> obvious what's going on.
I suppose that is true. But I think it is odd having a parameter which is the
same for every call site.
But I'm not going to quibble over something like this.
Changed,
Ira
>
> > void irqentry_exit_cond_resched(void)
> > @@ -362,7 +365,12 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t *state)
> > /* Check whether this returns to user mode */
> > if (user_mode(regs)) {
> > irqentry_exit_to_user_mode(regs);
> > - } else if (!regs_irqs_disabled(regs)) {
> > + return;
> > + }
> > +
> > + irq_restore_pkrs(state);
> > +
> > + if (!regs_irqs_disabled(regs)) {
> > /*
> > * If RCU was not watching on entry this needs to be done
> > * carefully and needs the same ordering of lockdep/tracing
> >
>
